Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Beta support for Package Runtime Config #4744

Merged
merged 25 commits into from Oct 17, 2023

Conversation

turkenh
Copy link
Member

@turkenh turkenh commented Oct 6, 2023

Description of your changes

This PR implements Package Runtime Config (Fixes #4699) as proposed in the design doc with the following exception:

Instead of initializing and maintaining our defaults like replicas: 1 or pod and runtime container securityContext in the DeploymentRuntimeConfig named default, it only initializes an empty DeploymentRuntimeConfig named as default only if it does not exist. It was discussed in the issue in details but this is primarily to avoid the following:

  • Creating a new DeploymentRuntimeConfig would require duplicating the values from the default one, assuming the user won't want to lose them. If you only intend to include additional labels, you would need to duplicate the entire default configuration and append labels to the newly created one.
  • From now on, the user would need to maintain the defaults in case Crossplane wants to change or extend them in the upcoming releases.
  • Since the user could change the default config in time, in the new releases, Crossplane would either overwrite the default DeploymentRuntimeConfig to reflect changes in defaulting or leave the default as is, which would mean Crossplane losing control over defaults.

Considering the above 3 being a degradation over User Experience and Crossplane controlling the defaulting behavior with new releases, I ended up changing it as follows:

  • Crossplane will create an empty default DeploymentRuntimeConfig if it does not exist.
  • Crossplane will set defaults only if DeploymentRuntimeConfig hasn't set them (i.e. unopinionated). For example, if the deployment replicas is not provided in the config, Crossplane will set the value as 1.
  • Crossplane is opinionated about some configurations and will override even if they were provided in the config.

This way, the default DeploymentRuntimeConfig could also be owned by the user without any maintenance burden.

This PR also:

  • Refactors existing hooks and manifest generation with a ManifestBuilder.
  • Introduces PackageWithRuntime and PackageRevisionWithRuntime interfaces.
  • Uses specific package types (Fixes Consider using specific package types #4690).
  • Does some clean up including removing the unused controllerRef from PackageRevisionStatus.

See the manifest in the e2e test to get a sense on how DeploymentRuntimeConfig could be customized but still operate properly.

I have:

@negz negz self-assigned this Oct 6, 2023
@turkenh turkenh force-pushed the package-runtime-config branch 6 times, most recently from bf99872 to 5d6b980 Compare October 13, 2023 13:46
@turkenh turkenh changed the title [WIP]: Beta support for Package Runtime Config Beta support for Package Runtime Config Oct 13, 2023
@turkenh turkenh marked this pull request as ready for review October 13, 2023 14:19
@turkenh turkenh requested review from a team and negz as code owners October 13, 2023 14:19
Copy link
Member

@negz negz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @turkenh!

WRT the design changes, I personally would still vote for what I wrote in #4699 (comment). I prefer moving the default values out of code and into a configuration file. I think this makes it more explicit and predictable, and I think that's worth folks potentially having to copy ~30 lines of YAML to make a new config.

That said, I can accept this as a compromise. It satisfies the primary goal of the design, which was to offer a full template for a Deployment (and potentially in future a Service, ServiceAccount, etc) and I think its cleaner than the other alternatives we discussed.

apis/pkg/v1/package_runtime_types.go Outdated Show resolved Hide resolved
apis/pkg/v1beta1/deployment_runtime_config_types.go Outdated Show resolved Hide resolved
cmd/crossplane/core/core.go Outdated Show resolved Hide resolved
internal/controller/pkg/revision/runtime_function.go Outdated Show resolved Hide resolved
internal/controller/pkg/revision/runtime_provider.go Outdated Show resolved Hide resolved
@turkenh turkenh force-pushed the package-runtime-config branch 2 times, most recently from 0a8c6ea to e71bd9e Compare October 17, 2023 10:17
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Also no RuntimeConfig (yet)

Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
@turkenh turkenh merged commit 6bdf688 into crossplane:master Oct 17, 2023
15 of 17 checks passed
@turkenh turkenh deleted the package-runtime-config branch October 20, 2023 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement beta support for RuntimeConfig Consider using specific package types
2 participants