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

Add chart validation tests #4615

Merged
merged 2 commits into from
Nov 13, 2023

Conversation

thomasferrandiz
Copy link
Contributor

Proposed Changes

validate-charts runs as part of 'make validate' step and checks that all images used in packaged charts:

  • use systemGlobalRegistry
  • are present in script/build-images

Types of Changes

Tests

Verification

Testing

Linked Issues

#4485

User-Facing Change


Further Comments

At the moment, the new test fails because some charts don't use systemGlobalRegistry at all.
I'm not sure what would be the best way to move forward:

  • make the test informative only for now
  • fix the problematic charts in another PR before merging this one

Also I don;t know if the busybox image should be an exception or not.

@thomasferrandiz thomasferrandiz requested a review from a team as a code owner August 10, 2023 09:22
@brandond
Copy link
Member

brandond commented Aug 14, 2023

fix the problematic charts in another PR before merging this one

I would probably fix the charts in rke2-charts, then have two commits in this PR:

  • first commit to add the test, and show that they are failing.
  • second commit to update the charts and make the test pass.

Dockerfile Outdated Show resolved Hide resolved
chart_versions.csv Outdated Show resolved Hide resolved
@thomasferrandiz thomasferrandiz force-pushed the add-chart-validation branch 2 times, most recently from da325f4 to 9379420 Compare September 27, 2023 14:12
Dockerfile Outdated Show resolved Hide resolved
scripts/build-images Outdated Show resolved Hide resolved
@thomasferrandiz thomasferrandiz force-pushed the add-chart-validation branch 2 times, most recently from 41a4531 to 2da819b Compare September 29, 2023 12:11
Dockerfile Outdated Show resolved Hide resolved
@thomasferrandiz thomasferrandiz force-pushed the add-chart-validation branch 2 times, most recently from 3cd624e to a05c1bc Compare October 2, 2023 09:39
charts/build-charts.sh Outdated Show resolved Hide resolved
@thomasferrandiz thomasferrandiz force-pushed the add-chart-validation branch 2 times, most recently from 4c1ba88 to 132decd Compare October 3, 2023 08:40
Copy link
Contributor

@manuelbuil manuelbuil left a comment

Choose a reason for hiding this comment

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

validate-charts is run as part of the CI for PRs, right?

@thomasferrandiz
Copy link
Contributor Author

validate-charts is run as part of the CI for PRs, right?

yes as part of make validate

scripts/validate-charts Outdated Show resolved Hide resolved
scripts/validate-charts Outdated Show resolved Hide resolved
chart_name=$2
chart_tmp=$3

if [[ $chart_name == 'rancher-vsphere-cpi' ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

is this chart untestable? if so, why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the chart doesn't work well with helm template because of the versionOverrides in its values.yaml.
Since there is no actual k8s cluster to determine the version of the image to use, helm template uses the .Capabilities.APIVersion of the version of k8s that helm was built against so it chooses an image that doesn't match the one we need in the airgap file.

To workaround this we would need manually install the right version of Helm which would be different for the main branch and each release branch and would need to be updated for every new k8s version. Since it's only for this chart, I found it simpler to skip the test.

See https://github.com/rancher/vsphere-charts/blob/7c1f2d4ee52548bcc8f7aee0e986b338a229ac8a/charts/rancher-vsphere-cpi/values.yaml#L28

Copy link
Member

@brandond brandond Oct 10, 2023

Choose a reason for hiding this comment

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

You don't need to install different versions of Helm, you just need to use the --kube-version flag to specify the Kubernetes version you want it to use when templating. You could probably just pass all the charts the $VERSION environment variable set by version.sh to make sure they handle it properly.

See: https://helm.sh/docs/helm/helm_template/#:~:text=Capabilities.KubeVersion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip!
Unfortunately now it shows that the vsphere charts aren't updated for k8s 1.28 so the test is still broken.

I started to update them by adding the missing images to image-mirror:
rancher/image-mirror#482

@thomasferrandiz thomasferrandiz force-pushed the add-chart-validation branch 3 times, most recently from 55b26bc to 01d35a1 Compare October 5, 2023 12:00
@brandond
Copy link
Member

brandond commented Oct 11, 2023

I believe the test failure is valid, and that chart doesn't support 1.28 yet.

https://github.com/rancher/rke2-charts/blob/main/charts/rancher-vsphere-csi/rancher-vsphere-csi/3.0.1-rancher101/values.yaml#L142

This will be an ongoing problem whenever upstream support lags on a new minor.

@thomasferrandiz
Copy link
Contributor Author

I believe the test failure is valid, and that chart doesn't support 1.28 yet.

https://github.com/rancher/rke2-charts/blob/main/charts/rancher-vsphere-csi/rancher-vsphere-csi/3.0.1-rancher101/values.yaml#L142

This will be an ongoing problem whenever upstream support lags on a new minor.

I made a PR to update the upstream chart: rancher/vsphere-charts#62
but I have no idea of how to test it as I don't know the charts.

In any case, the problem will happen for every new minor release so I guess it would still be better if the test doesn't fail because of this chart.

@brandond
Copy link
Member

brandond commented Oct 20, 2023

Yeah, but we don't want to ignore problems on minors that ARE supported.

Some of the charts have an annotation that we could probably use to decide whether to fail or warn, based on whether or not the chart is expected to support the current Kubernetes version: catalog.cattle.io/kube-version: '>= 1.20.0-0 < 1.28.0-0'

https://github.com/rancher/rke2-charts/blob/main/charts/rancher-vsphere-csi/rancher-vsphere-csi/3.0.1-rancher101/Chart.yaml#L4

Comment on lines +94 to +100
if [ "$lower_bound_major" -le "$kube_version_major" ] && \
[ "$kube_version_major" -le "$upper_bound_major" ] && \
[ "$lower_bound_minor" -le "$kube_version_minor" ] && \
[ "$kube_version_minor" -le "$upper_bound_minor" ] && \
[ "$lower_bound_patch" -le "$kube_version_patch" ] && \
[ "$kube_version_patch" -le "$upper_bound_patch" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

this is a little gnarly but I think it'll work...

scripts/validate-charts Outdated Show resolved Hide resolved
validate-charts runs as part of 'make validate' step and checks
that all images used in packaged charts:
- use systemGlobalRegistry
- are present in script/build-images

Signed-off-by: Thomas Ferrandiz <[email protected]>
@thomasferrandiz thomasferrandiz merged commit 2d149fd into rancher:master Nov 13, 2023
1 check passed
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.

4 participants