Skip to content

Conversation

@komish
Copy link
Member

@komish komish commented Apr 10, 2025

fixes #501

Short Version:

This is a bit tricky, but it boils down to two things.

  1. When an error is thrown while trying to parse image references from rendered templates, we need to update the check result and return it with the errored (i.e. failing) state. This is a one-line change.
  2. When rendering templates to extract image references, we need to make sure that we have sane defaults for mocked kube-version values. In addition, we should allow users to set this value to meet their constraints.

Long Version:

To add some detail, mostly to item (2):

Extracting images from the chart's templates requires rendering them, and chart-verifier does this in the same way Helm does (more or less). In doing this, we have to fake/mock a server's capabilities. We don't really need a server, but the clients we (and helm) use expect a server capability (read: the server's version) to be defined in order to avoid code duplication.

That server's capability is used to check against a Chart's kubeVersion constraint as defined in Chart.yaml. It's a bit arbitrary, but again - it lets helm maintainers avoid duplication of code.

The problem is in how that constraint is set. We would set the server's capability using chartutil.DefaultCapabilities from Helm's code. In Helm's code, that default capability is set to kubernetes version 1.20. From what I'm seeing, it doesn't actually matter what we set here, because the Helm codebase will throw it out as soon as it sees that we're running in "client-only" mode (e.g. the same mode that helm template users), and puts in place the default. https://github.com/helm/helm/blob/386523bdbc6f5e5f289ade7d9d4cf4c935354450/pkg/action/install.go#L277-L292

That logic give us a way to override the KubeVersion when operating in this "client-only" mode, so I use that path instead (setting the value via action.Install{}.KubeVersion).


What's interesting is that Helm doesn't exhibit this problem. Or rather, in my testing, the Helm CLI downloaded from GitHub doesn't run into the same problem, but building from source did. This is because Helm dynamically sets the default contraint at build time.

https://github.com/helm/helm/blob/386523bdbc6f5e5f289ade7d9d4cf4c935354450/Makefile#L61-L69

It's not my favorite bit of magic, mostly because it required a decent amount of time to figure out why the Helm CLI doesn't exhibit the same problem, while library callers and building from source did. Also, library callers get what effectively is a non-useful default (1.20). I digress.

We can technically set Helm's build flags here, and things would align, but that would force me to have to track their repo structure over time. Instead, I did two things.

  1. In repo, we use a high version constraint (v99.99). It should serve most people who don't have an upper bound, and we really don't see any side effects. If you're a library caller, then you should have a somewhat reasonable default.

  2. For CLI builds, we'll set this to align with out client-go version, just like Helm. We just track this against our own variables (instead of theirs). See Makefile changes in this PR for how this is working.

I don't have a good way of tracking client-go's supported version for item 1 here because of what I mentioned before: you don't run build flags when importing a library (and I don't know of a way to do so). I don't want to force people to set build flags for themselves, so I just set a high value and now we don't have to update this over time.


PTAL @mgoerens @jsm84
Thanks @xiongzubiao for the detailed issue, and @mgoerens for thorough research

@komish komish requested review from jsm84 and mgoerens April 10, 2025 02:13
@komish
Copy link
Member Author

komish commented Apr 10, 2025

I’ll look into test and linting errors in the morning but the core logic should be ready for review.

@komish komish force-pushed the support-faked-kube-version branch from cd96daa to af4777e Compare April 10, 2025 16:49
@komish
Copy link
Member Author

komish commented Apr 10, 2025

I've rebased off of the depguard PR (#503). When that merges, I can manually (if GitHub doesn't do it automatically) rebase off of main again.

@komish komish force-pushed the support-faked-kube-version branch from af4777e to 332335e Compare April 10, 2025 16:59
@komish komish force-pushed the support-faked-kube-version branch from 332335e to 2d379be Compare April 10, 2025 17:03
@komish
Copy link
Member Author

komish commented Apr 10, 2025

I've rebased off of the depguard PR (#503). When that merges, I can manually (if GitHub doesn't do it automatically) rebase off of main again.

This is done.

Copy link
Contributor

@mgoerens mgoerens left a comment

Choose a reason for hiding this comment

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

Thank you @komish ! LGTM

@komish komish merged commit c6275d0 into redhat-certification:main Apr 14, 2025
6 of 7 checks 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.

images-are-certified always passes with reason "No images to certify"

2 participants