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

Use latest kubeVersion as helm parameter when rendering charts in mirror-images #12513

Merged

Conversation

SimonTheLeg
Copy link
Member

What this PR does / why we need it:

This PR fixes the issue where the mirror-images command is going to fail when using it with charts that specify a Kubernetes version greater than 1.20. This is currently the case for the consul chart.

Tested with the current installer from the latest KKP Github release:

./kubermatic-installer mirror-images registry.example.com --config examples/kubermatic.example.yaml --dry-run
INFO[0003] 🚀 Collecting images…
INFO[0147] 🚀 Rendering Helm charts…                      charts-directory=charts
ERRO[0168] ❌ Operation failed: failed to get images: failed to get images for Helm chart: failed to render Helm chart "consul": Error: chart requires kubeVersion: >=1.21.0-0 which is incompatible with Kubernetes v1.20.0

The reason for this is a little bit entertaining. Helm actually uses a kubeVersion flag to specify how a chart should be rendered. This is by default hardcoded to 1.20 (see https://github.com/helm/helm/blob/main/pkg/chartutil/capabilities.go#L32-L48 & https://github.com/helm/helm/blob/main/pkg/chartutil/capabilities_test.go#L45) and therefore can collide with charts that specify a kubeVersion greater than 1.20.

What type of PR is this?

/kind bug

Special notes for your reviewer:

Two changes in this PR should be reviewed closer:

  1. This introduces the ability to pass flags into the RenderChart func. This is done in accordance with other funcs in the interface which allow similar features (e.g InstallChart)
  2. The mirror-images command now uses the latest Kubernetes version we support in KKP as the basis for the render. I have written a longer comment in the the func to explain this. PTAL if that behaviour makes sense to you.

Does this PR introduce a user-facing change? Then add your Release Note here:

Fix an issue in the kubermatic-installer mirror-images command, which led to failure on the mla-consul chart

Documentation:

NONE

…ror-images command

Signed-off-by: Simon Bein <simontheleg@gmail.com>
@kubermatic-bot kubermatic-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. docs/none Denotes a PR that doesn't need documentation (changes). kind/bug Categorizes issue or PR as related to a bug. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management. labels Jul 27, 2023
@SimonTheLeg SimonTheLeg requested review from embik and xrstf July 27, 2023 12:12
@kubermatic-bot kubermatic-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 27, 2023
@xrstf
Copy link
Contributor

xrstf commented Jul 27, 2023

/approve
/lgtm

@kubermatic-bot kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2023
@kubermatic-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 7b444dc42cd888992b1c9a1f927829a61ee39c86

@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SimonTheLeg, xrstf

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubermatic-bot kubermatic-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 27, 2023
@wurbanski
Copy link
Contributor

@SimonTheLeg would it make sense to use the oldest supported version instead?

@kubermatic-bot kubermatic-bot merged commit 09f075e into kubermatic:main Jul 27, 2023
17 checks passed
@kubermatic-bot kubermatic-bot added this to the KKP 2.24 milestone Jul 27, 2023
@SimonTheLeg
Copy link
Member Author

@wurbanski I have been contemplating this topic and came to the conclusion that using the oldest supported version is more brittle than using the latest one. My logic behind this is based on that I think it is more likely that the author of a chart will have a version constraint eliminating old versions, than they will have a constraint limiting new versions.

@wurbanski
Copy link
Contributor

I forgot we're not using it for anything other than listing images at the moment, nevermind. :)

@xrstf
Copy link
Contributor

xrstf commented Jul 28, 2023

Should we backport this?

@SimonTheLeg
Copy link
Member Author

yes I think this makes sense. Otherwise the KKP 1.23 installer will not be usable

/cherrypick release/v2.23

@kubermatic-bot
Copy link
Contributor

@SimonTheLeg: new pull request created: #12517

In response to this:

yes I think this makes sense. Otherwise the KKP 1.23 installer will not be usable

/cherrypick release/v2.23

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@pkprzekwas pkprzekwas added the backport-needed Denotes a PR or issue that has not been fully backported. label Jul 31, 2023
@embik embik added backport-complete Denotes a PR or issue which has been fully backported to all required release branches. and removed backport-needed Denotes a PR or issue that has not been fully backported. labels Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. backport-complete Denotes a PR or issue which has been fully backported to all required release branches. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. docs/none Denotes a PR that doesn't need documentation (changes). kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants