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

Preview Give feedback

1. feat(crank): convert supporting function-environment-configs #6016
   +0
   
2. update composition environment docs to function-environment-configs docs#824
   +0
3. Promote EnvironmentConfig to v1beta1, but remove native support #5938
   release/api-deprecation release/breaking-api-change release/breaking-behavior-change +3
4. docs: migration from native composition environment crossplane-contrib/function-environment-configs#44
   +0
5. EnvironmentPatch missing FromEnvironmentFieldPath patch #3965
   bug environment +2
   
6. User guide for EnvironmentConfig docs#305
   concepts/composition content request +2
7. Add Data from Arbitrary K8s Object to Composition Environment #4583
   composition enhancement environment proposal +4
   
8. Add support for Composition environment schema definition/validation #4275
   composition enhancement environment stale +4
9. ToComposite Patches are executed out-of-order #3454: not strictly related, but causing confusion with this feature
10. patch data back to EnvironmentConfigs and have it persistently stored?
11. Support selecting multiple environment configs #3981
    composition environment +2
    
12. Allow ignoring environment config if it does not exists #3723
    composition enhancement environment +3
13. Additionally added EnvironmentConfigs in Composition are not propagated to XR #3941
    bug +1
14. Support EnvironmentConfig in crossplane packages #3865
    enhancement +1
15. Debugging computed environment #3967
    composition enhancement environment stale +4
16. Ignored FromEnvironmentFieldPath patch type in spec.environment.patches #4080
    bug composition environment +3
    
17. Non-deterministic EnvironmentConfig choice with label selector #4070
    bug composition environment +3
18. Combine from parameters, status and environmentConfig values #3926
    composition enhancement environment stale +4
19. Add support for default environment in Composition #4274
    composition enhancement environment +3
20. Patching from environment to Composite resource status not working #4393
    bug composition environment +3
21. Rename environment.environmentConfigs to environment.configs #4591
    api environment stale +3
22. Clearly document migration steps from in-tree composition environment crossplane-contrib/function-environment-configs#43
    documentation enhancement +2

[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, #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 ytsarev/composition-environment-example@2b14f43 .

[ytsarev]

crossplane/docs#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: P0t4T0o@cf8ee8e

[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.

[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.

[bobh66]

@phisco @negz I'd like to get the EnvironmentConfig promoted to beta in 1.18, or sooner if we move it to function-environment-configs. My concerns with moving it out-of-tree are that you have to install the function in order to get the resource, which introduces another dependency, and it calls into question the ability to include EnvironmentConfigs in crossplane Configuration packages as described in #3865

I think the thing that pushes me toward keeping it in-tree is the desire to be able to include it in a Configuration package. That seems to be more Crossplane-ish and I don't know that we would want to support including out-of-tree resources in Configuration packages, although including things like ConfigMaps might be useful in both Configuration and Function packages.

Thoughts? Do we need a new issue for promoting the resource? Or can we treat the promotion/deprecation as one thing and deal with it here?

[phisco]

@bobh66, I'm fine keeping this issue.

I'm fine just promoting EnvironmentConfig and leaving the environment machinery to be deprecated with the rest of the native patch and transform composition. I think it would add a lot of useless pain for users to move it out of Crossplane.

[bobh66]

Thanks @phisco - is there any additional effort required to deprecate the machinery, or does deprecating the native P&T cover it?

[negz]

I feel okay leaving the EnvironmentConfig type in-tree. I'd like to actively remove the in-tree code paths though.

By that I mean:

• The environment field on the Composition type
• The controller code that selects and fetches environment config
• All the native FromEnvironment and ToEnvironment patch types

I think it would add a lot of useless pain for users to move it out of Crossplane.

I'm assuming by "it" here you mean the things I listed above - it's unclear if you meant that or the EnvironmentConfig type.

If so, I empathize, but the flipside is that we must maintain the code for this alpha feature indefinitely. We're committed to maintain most of the in-tree Composition code because it shipped as a GA feature, but EnvironmentConfigs never left alpha.

I know EnvironmentConfigs are popular and widely used, but everyone using them has opted into an alpha feature with plenty of warning that it might be dropped or changed in future. I think it's reasonable to ask them to migrate to the beta iteration of the feature (i.e. using functions).

[phisco]

I meant dropping the EnvironmentConfig resource would be painful.

Sounds good to me dropping the machinery around them though, the migration path is as easy as pushing the configuration down to function-environment-config/function-patch-and-transform. If we start making people aware now, it should be fine.

[phisco]

As discussed we'll have to:

• Merge Promote EnvironmentConfig to v1beta1, but remove native support #5938, removing the in-tree code and promoting EnvironmentConfigs.
• update crossplane beta convert pipeline-composition to handle function-environment-configs too: feat(crank): convert supporting function-environment-configs #6016
• Document the migration in function-environment-configs: docs: migration from native composition environment crossplane-contrib/function-environment-configs#44
• Update crossplane docs to reference function-environment-configs in https://docs.crossplane.io/latest/concepts/environment-configs/

[jbw976]

@phisco i've taken your task list in the comment above and merged them into the main task list for this epic, along with current status. Thanks for driving this in v1.18! 🙇‍♂️

[phisco]

Ok, we finally managed to execute on the plan above, so we can close this!

TL;DR: We've promoted the EnvironmentConfig resource to Beta, but dropped all "in-tree" Composition Environment capabilities, i.e. spec.environment from Compositions. There is clear documentation and automated tooling (crossplane beta convert pipeline-composition/composition-environment) to migrate from that to function-environment-configs. We have almost complete feature-parity, except for spec.environment.policy.resolve: IfNotPresent, which only supports Always now, but we don't think this will affect users, see here for more details.