Future of `Composition Environment`

[MisterMX]

What problem are you facing?

Support for EnvironmentConfigs was added in Crossplane v1.11.0.

How could Crossplane help solve your problem?

We need a plan to promote it to v1beta1, which may include accepting it as is, changing it, or dropping the feature. Community feedback would help here.

### Tracked issues
- [x] #3965
- [x] https://github.com/crossplane/docs/issues/305
- [ ] #4583
- [ ] #4275
- [ ] https://github.com/crossplane/crossplane/issues/3454: not strictly related, but causing confusion with this feature
- [ ] patch data back to EnvironmentConfigs and have it persistently stored?
- [x] https://github.com/crossplane/crossplane/pull/3981
- [ ] #3723
- [x] #3941
- [x] #3865
- [ ] #3967
- [x] #4080
- [x] #4070
- [ ] #3926
- [ ] #4274
- [ ] #4393
- [ ] https://github.com/crossplane/crossplane/issues/4591
- [ ] https://github.com/crossplane-contrib/function-environment-configs/issues/43

[blakebarnett]

We are using it successfully as-is.

Some clarity around the patch ordering in documentation and validation that a patch is just not going to work rather than having it fail silently would be great beta criteria. For example if attempting to patch a composed resource and then trying to use that same field elsewhere as if it were a composite field maybe a validatingAdmissionWebhook could deny it as invalid?

[ytsarev]

I agree, https://github.com/crossplane/crossplane/issues/3454 behavior should be documented.

Otherwise it is totally unclear why construction like https://github.com/ytsarev/composition-environment-example/blob/main/xvpc/composition.yaml#L36-L38 does not work.

It's not the best UX in general so if there is chance to change the order for ToEnvironment patches, we should try to do it.

@plumbis where are old doc snippets are living now?

E.g. I'm failing to find https://github.com/crossplane/crossplane/pull/3007/files#diff-57c8cd7167236fac0edefa6b72c183bb0fdbd21bdc8ec0a521407eaf436ebc0e in the current state of repos.

It is generally useful to have snippets close to the code as they represent the working samples of the various functionality right in the crossplane repo.

[ytsarev]

I've added working(aka not affected by ordering situation) examples of ToEnvironmentFieldPath patches to https://github.com/ytsarev/composition-environment-example/commit/2b14f43118905227625980f69c92f08f0524de53 .

[ytsarev]

https://github.com/crossplane/docs/issues/380 related concern

[plumbis]

@plumbis where are old doc snippets are living now?

@ytsarev all snippets are still under their versions (v1.10, v1.9). https://github.com/crossplane/docs/tree/master/content/v1.10/snippets

They are being removed in v1.11 and future versions. Docs that still reference snippets need to be updated.

(Happy to discuss why we're removing snippets but it's beyond the scope of this issue)

[P0t4T0o]

hi, is it intended that if we select env config by some label in composition and there are multiple configs with that label, only one config is selected per matchLabels list item:

 environment:
    environmentConfigs:
      - type: Selector
        selector:
          matchLabels:
            - key: environment-config/app123
              type: Value
              value: select-me

would be nice to be able to select and aggregate all configs with matching labels. these could be necessary changes to enable it: https://github.com/P0t4T0o/crossplane/commit/cf8ee8e1f97b658ca1669675d93d9794273e9839

[ytsarev]

@P0t4T0o that makes total sense to me and also looks matching original design by @MisterMX .

@P0t4T0o why don't you create PR from the changes you demonstrated? It already looks functional, just some unit test coverage to extend.

[mithie]

We are using it successfully as-is.

Some clarity around the patch ordering in documentation and validation that a patch is just not going to work rather than having it fail silently would be great beta criteria. For example if attempting to patch a composed resource and then trying to use that same field elsewhere as if it were a composite field maybe a validatingAdmissionWebhook could deny it as invalid?

I agree with that. Documentation should be adjusted accordingly. We were facing the same issue as mentioned above when we were trying to use the CombineFromComposite patch type with a value from a status field that was patched by the FromEnvironmentFieldPath before. There's a comment on this in the docs of FromEnvironmentFieldPath, but it's still confusing since it fails silently. So, I agree with the above. Having some sort of validation would be a good feature.

[bobh66]

I added the following comment to #3931 but I'll mention it here as well - it seems like it should be valid for an EnvironmentConfig Selector to return no results, just as it is valid for a CompositionSelector to return no results and fallback to the defaultComposition. In this case the environment may be constructed of a variety of named and selected EnvironmentConfigs, as well as patches from the Composite, and the absence of one or more selectors may not be a problem, and may even be expected.

I view it as creating a runtime context from a number of sources, where different sources may exist in different scenarios and the absence of a given source is not necessarily an error condition.

I'm also thinking it might be helpful to have a "default" or "init" section under spec.environment which can contain a hard-coded set of "defaults" for the environment, which may or may not be overridden by selected/named environments or patches. This would also help with the FromValue patch type scenario that has been discussed in #3270

If there is interest in that I can open a separate issue and see about implementation for 1.13

[blakebarnett]

I'm also thinking it might be helpful to have a "default" or "init" section under spec.environment which can contain a hard-coded set of "defaults" for the environment, which may or may not be overridden by selected/named environments or patches. This would also help with the FromValue patch type scenario that has been discussed in #3270

If there is interest in that I can open a separate issue and see about implementation for 1.13

I have wanted exactly this functionality many times. I've ended up creating "cluster-wide" defaults, and then creating free-form status fields to do CombineToComposite to make use of the fields that aren't exact fits for each composition. It would be much nicer to be able to set them per-composition in this way.

[phisco]

Firstly, I think this issue should be renamed to Promote Composition Environment to beta as EnvironmentConfigs are just a part of the larger functionality introduced in the original one-pager.

That said, the widespread adoption of this functionality clearly demonstrates that it addresses a real need for users and does so effectively. For these reasons, we aim to promote it to Beta in v1.15. This is a good opportunity to incorporate any necessary breaking changes to the current API based on the feedbacks we have received since this feature was introduced in v1.11.

To facilitate this process, I will create a one-pager to collect and start discussing the known pain points and missing functionalities of the current implementation. This will hopefully help us decide the changes required to confidently promote this feature to Beta. Here a related comment.

[phisco]

FYI: https://github.com/crossplane/crossplane/pull/5061

[negz]

@MisterMX @phisco do you think we should reframe this issue to reflect the decision taken in #5061?

This document proposes not promoting the Composition Environment feature to beta in v1.15, nor setting a timeline for its promotion to beta, investing in enabling Composition Functions to request extra resources allowing them to reimplement the same functionality while exploring other possible approaches.

Here's my understanding of the state of the feature:

• We've made a decision that we don't want to promote it to beta as-is.
• We want to avoid leaving it as alpha as-is, because the longer it exists as-is the harder it is to change.
• We plan to remove spec.environment and the environment patch types from native P&T (mode: Resources).
• We plan to replace it with function-environment-configs combined with function-patch-and-transform.
• We want to keep the EnvironmentConfig type but we're not sure where it should live. Possibly a provider?

Is that correct?

[negz]

We need to make a decision about the future of this feature.

I don't think it's going to be promoted to beta in its current form. @phisco @jbw976 could we start by removing the EnvConfig specific fields from the Composition type in v1.17?

[phisco]

I'd rather deprecate them first for one release, at least. Otherwise we are forcing people using this alpha, but really widely used,feature to also migrate to functions, while native compositions are not deprecated yet. Given that the migration is not seamless, I'd prefer not rushing it.

[negz]

Deprecation sounds great to me, as long as we're making forward progress in some direction. 😄

Do we have an example of deprecating a field (as opposed to a whole type)? How would we deprecate it? Probably:

• // Deprecated comment on the field
• Release notes
• Docs updates

Anything else?

[phisco]

We can emit a deprecation warning from the webhook too if marking the field deprecated is not doing that already, I don't think it does, I have to check 🤔

[bobh66]

@phisco @negz any thoughts on promoting EnvironmentConfig to v1beta1 so it can be supported when building Configuration packages? Regardless of the composition environment plumbing, the EnvironmentConfig resource itself is extremely useful.

[phisco]

I'm fine promoting the EnvironmentConfig resource to beta as is, the other option would be to move it out of tree, e.g. provider-environment-configs. WDYT @negz?

[negz]

I maybe have a slight preference for moving it out of tree? Not sure.

My thinking is that it'd be unusual to have a type installed by core that wasn't actually used by core - only by functions. On the other hand it's also a bit unusual to have a "provider" that installs a type but doesn't actually reconcile it, so I don't feel super strongly.

If/when we bump it to beta can we make sure the "old" env config machinery is explicitly marked deprecated. I want to make it clear that we're only promoting the type (for use with functions) and that we still plan to remove the "built in" env config support.