kubernetes-sigs / cluster-api-operator

Home for Cluster API Operator, a subproject of sig-cluster-lifecycle
https://cluster-api-operator.sigs.k8s.io
Apache License 2.0
162 stars 77 forks source link

✨ Add support for customizing additional provider deployments #538

Closed willie-yao closed 2 months ago

willie-yao commented 4 months ago

What this PR does / why we need it: This PR adds support for customizing additional provider deployments via the additionalDeployments field in the provider spec. additionalDeployments is a map of the name of the provider to both the deployment and manager spec which is used for customizing the deployment/management container.

This PR also adds allows CRDPattern to be configurable in deployment customization. This is to allow for installing additional CRDs through the ASO deployment.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged): Fixes #

netlify[bot] commented 4 months ago

Deploy Preview for kubernetes-sigs-cluster-api-operator ready!

Name Link
Latest commit d1112d3aa72527cdf031bf254aeebfef4f8e38bc
Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-operator/deploys/667df1d13ef2ce0008206184
Deploy Preview https://deploy-preview-538--kubernetes-sigs-cluster-api-operator.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

willie-yao commented 4 months ago

I'm a bit confused on how to add a conversion function for CRDPattern. @Fedosin @alexander-demicev is there an example on how to do this? I tried adding something like this to the conversion funcs which didn't seem to work: https://github.com/willie-yao/cluster-api-operator/blob/upstream-install-aso-crds/api/v1alpha1/provider_conversion.go#L48

alexander-demicev commented 3 months ago

@willie-yao You can find an example here https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/4228/files, this PR added a new field to API and conversion to older versions

willie-yao commented 3 months ago

Thanks @alexander-demicev! I fixed up the conversion and added a unit test!

alexander-demicev commented 3 months ago

@willie-yao This change is too specific to capz. What do you think about making it more generic? Can we introduce a new field to customize additional deployments that come with providers? I think something like AdditionalDeployments might work, where we can store the deployment names to customize and DeploymentSpec structure to apply these customizations. Do you think it makes sense? I can see that in the future there might be more providers like CAPZ with more than one Deployment so it might be better to try to fix the problem in a bigger scope.

willie-yao commented 3 months ago

@alexander-demicev That is a great idea. I did notice that this may be too specific to CAPZ to be implemented like this. I'll work on adding a field like AdditionalDeployments. Thanks for the suggestion!

willie-yao commented 3 months ago

/retest

alexander-demicev commented 3 months ago

PR looks good, just one small comment

furkatgofurov7 commented 3 months ago

/retest

willie-yao commented 3 months ago

Looks like there's a problem with the E2E test output. Will be looking into this.

willie-yao commented 3 months ago

The hyphen in --crd-pattern messed up the test helm command, and since we're just testing if something can be set in the additionalDeployments rather than a specific field, I removed that field.

willie-yao commented 3 months ago

Actually it looks like the error is coming from the recursivePrinter: https://github.com/willie-yao/cluster-api-operator/blob/25328eb918edd7eb4b2e79cf10ab73cf0ca4f9e1/hack/charts/cluster-api-operator/templates/infra.yaml#L1

I'm not sure how to adjust that function to allow array values to be passed inline. When specifying container customizations for additionalDeployments, using a values file works. Do we have to add support for setting this inline?

willie-yao commented 3 months ago

I went ahead and removed the test case from helm_test.go due to the complicated nature of setting additionalDeployments via inline --set. If this is something we should support, I'll look into another way to scaffold out the additional customizations.

alexander-demicev commented 2 months ago

@willie-yao We can look into setting additionalDeployments in tests in a follow-up PR

k8s-ci-robot commented 2 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexander-demicev

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubernetes-sigs/cluster-api-operator/blob/main/OWNERS)~~ [alexander-demicev] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
k8s-ci-robot commented 2 months ago

LGTM label has been added.

Git tree hash: 35e1c27d5d4bc9aba5487fb18b15f2309573f2ff