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.

Sign up for GitHub

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

Jump to bottom

✨ Add support for customizing additional provider deployments #538

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

✨ Add support for customizing additional provider deployments #538

wants to merge 1 commit into from

Conversation

willie-yao
Copy link
Contributor

@willie-yao willie-yao commented May 31, 2024 •
edited
Loading

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 #

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 31, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign neolit123 for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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 k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 31, 2024
@netlify Netlify
Copy link

netlify bot commented May 31, 2024 •
edited
Loading

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

Name Link
🔨 Latest commit d1112d3
🔍 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
Copy link
Contributor Author

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
Copy link
Contributor

@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 willie-yao force-pushed the upstream-install-aso-crds branch 3 times, most recently from 7888168 to 7c0f00d Compare June 3, 2024 20:58
@willie-yao
Copy link
Contributor Author

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

@alexander-demicev
Copy link
Contributor

@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
Copy link
Contributor Author

@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 willie-yao changed the title ✨ Add configurable CRDPattern field for manager ✨ WIP: Add configurable CRDPattern field for manager Jun 4, 2024
@willie-yao willie-yao changed the title ✨ WIP: Add configurable CRDPattern field for manager WIP: ✨ Add configurable CRDPattern field for manager Jun 4, 2024
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 4, 2024
@willie-yao willie-yao force-pushed the upstream-install-aso-crds branch 2 times, most recently from de8275a to a40a7c8 Compare June 5, 2024 22:02
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 5, 2024
willie-yao
willie-yao commented Jun 5, 2024
View reviewed changes
api/v1alpha2/provider_types.go

// Deployment defines the properties that can be enabled on the deployment for the provider.
// +optional
Deployment *DeploymentSpec `json:"deployment,omitempty"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexander-demicev not sure if this is the best way to do it, but I refactored the code to include an AdditionalDeployments field that maps the name of the deployment to the Manager and Deployment spec, where the additional deployments can be customized. Is this a good way to go about it?

@willie-yao willie-yao force-pushed the upstream-install-aso-crds branch 4 times, most recently from 9a75480 to 1306800 Compare June 6, 2024 22:15
@willie-yao
Copy link
Contributor Author

/retest

@willie-yao willie-yao changed the title WIP: ✨ Add configurable CRDPattern field for manager ✨ Add configurable CRDPattern field for manager Jun 6, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 6, 2024
@willie-yao willie-yao changed the title ✨ Add configurable CRDPattern field for manager ✨ Add support for customizing additional provider deployments Jun 6, 2024
willie-yao
willie-yao commented Jun 7, 2024
View reviewed changes
hack/charts/cluster-api-operator/templates/infra.yaml
{{- include "recursivePrinter" $value | indent 2 }}
{{- else }}
{{ $key }}: {{ $value }}
{{- end }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function allows the chart to print out additionalDeployments correctly in yaml format as both deployments and manager has nested fields.

willie-yao
willie-yao commented Jun 7, 2024
View reviewed changes
internal/controller/component_customizer.go
@@ -73,25 +73,41 @@ func customizeObjectsFn(provider operatorv1.GenericProvider) func(objs []unstruc
}))
}

//nolint:nestif
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if a nolint is allowed here. I'll have to create a few helper functions if not. lmk which approach is better!

alexander-demicev
alexander-demicev reviewed Jun 7, 2024
View reviewed changes
Copy link
Contributor

@alexander-demicev alexander-demicev left a comment

Choose a reason for hiding this comment

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

looks great, couple of minor questions

api/v1alpha2/provider_types.go Outdated Show resolved Hide resolved
api/v1alpha2/provider_types.go Outdated
@@ -123,6 +141,11 @@ type ManagerSpec struct {
// in as container args to the provider's controller manager.
// Controller Manager flag is --feature-gates.
FeatureGates map[string]bool `json:"featureGates,omitempty"`

// CRDPattern is a regular expression that matches the CRDs that the provider
Copy link
Contributor

Choose a reason for hiding this comment

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

do we still need to keep this field? can it be replaced with the container arguments in the deployment spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, confirmed it works via container arguments in the deployment spec!

@willie-yao willie-yao force-pushed the upstream-install-aso-crds branch 2 times, most recently from 0cec2c0 to b6684d7 Compare June 10, 2024 18:48
alexander-demicev
alexander-demicev reviewed Jun 21, 2024
View reviewed changes
test/e2e/resources/only-infra-with-additional-deployments.yaml Outdated
additionalDeployments:
fake-controller-manager:
manager:
crdPattern: ".*"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should crdPattern be removed since we can achieve the same with container arguments?

@alexander-demicev
Copy link
Contributor

PR looks good, just one small comment

@willie-yao willie-yao force-pushed the upstream-install-aso-crds branch 2 times, most recently from 151f8ee to 9e2c3ed Compare June 21, 2024 22:43
@furkatgofurov7
Copy link
Member

/retest

@willie-yao
Copy link
Contributor Author

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

@willie-yao
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexander-demicev alexander-demicev alexander-demicev left review comments

@Fedosin Fedosin Awaiting requested review from Fedosin

@furkatgofurov7 furkatgofurov7 Awaiting requested review from furkatgofurov7

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@willie-yao @k8s-ci-robot @alexander-demicev @furkatgofurov7