-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
✨clusterctl: improve support for air-gapped environments all image overrides #2586
✨clusterctl: improve support for air-gapped environments all image overrides #2586
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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:
Approvers can indicate their approval by writing |
@@ -199,6 +199,14 @@ func NewComponents(provider config.Provider, version string, rawyaml []byte, con | |||
return nil, errors.Wrap(err, "failed to parse yaml") | |||
} | |||
|
|||
// apply image overrides, if defined | |||
objs, err = util.FixImages(objs, func(image string) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For deployments that have multiple images, such as capa-controller-manager
:
- gcr.io/kubebuilder/kube-rbac-proxy:v0.4.1
- us.gcr.io/k8s-artifacts-prod/cluster-api-aws/cluster-api-aws-controller:v0.5.0-rc.1
I think FixImages()
would potentially set the same tag for both images, if you specified a tag in the image meta in the config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ncdc the PR is not designed to prevent for user misconfigurations, but for making it really easier to address the main use case to change the image repository for all the images.
Then, if the need arises, we can introduce fine-grained configuration at image level of more sophisticated config validation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. I just wanted to confirm this is the behavior. Let's make sure the image config section is very well documented. Basically users can't set the tag on anything other than cert-manager; otherwise, they'll mess up their cluster. Right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that's right (assuming all the providers have more than 1 cimage)
I'm starting to work on a PR for documenting air-gapped support/the config, II will take care of documenting this properly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xref #2595
LGTM. @vincepri PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/milestone v0.3.0
failures seem unrelated to this PR |
What this PR does / why we need it:
This PR is a follow up of #2558 designed to improve clusterctl support for air-gapped environments by supporting the following image override config:
Thus allowing to use upstream manifest for air-gapped environments without requiring manual modifications to components yaml for all the providers.
The real change is really simple, but unfortunately, it requires refactoring the signature of a method that is used in many places.
For helping reviewers to better understand the above point, the refactor and the real change are in separate commits.
/area clusterctl
/assign @vincepri
/assign @ncdc