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

HIP for dependency overrides #176

Closed
wants to merge 2 commits into from
Closed

HIP for dependency overrides #176

wants to merge 2 commits into from

Conversation

mladedav
Copy link

I was told this feature should be written up as a HIP. It's based on helm/helm#2205.

Signed-off-by: David Mládek <[email protected]>
@papanito
Copy link

Looks good.

Copy link
Member

@bacongobbler bacongobbler left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to write a HIP. I have a few questions about the proposal that may be best addressed as follow-up material in the HIP.

There are a few other security implications that would probably need addressing with this proposal. For example, how do we address chart provenance with the --set-dependency flag? How do we verify a chart's integrity?

How would this proposal handle cases such as the one described in HIP 13?

The following questions are partially referenced in the "Open Issues" section, but I'll call it out here as well:

  • How can a user write dependency updates to the lockfile?
  • What happens when there are version conflicts? As in, the Chart.yaml requests for ^1.0.0 but --set-dependency requests v2.0.0. What about transitive dependencies?
  • What happens when the user requests a package that is NOT declared in Chart.yaml? Is that a failure case? Why or why not?
  • If the above case is supported, how is a chart developer expected to handle that case?

Finally, I would suggest that under the "How to Teach This" section, I would place an example demonstrating a compelling use case that clearly illustrates why this proposal is required to solve the issue. That way we can observe its practicality and compare it to existing solutions or alternative HIPs in the future.

Thanks!

@mladedav
Copy link
Author

@bacongobbler Thank you for the review.

There are a few other security implications that would probably need addressing with this proposal. For example, how do we address chart provenance with the --set-dependency flag? How do we verify a chart's integrity?

As I see it this flag would not change the actual process of how the packages are downloaded and checked for integrity, it would only change which versions (and possibly packages) are downloaded. The actual installation would be identical to a scenario where the user changes the dependencies in the Chart.yaml file and called helm dependency update with the only difference being whether the lock file is to be created or updated.

Therefore provenance would be checked in the same way.

How would this proposal handle cases such as the one described in HIP 13?

I don't see any concrete cases in the HIP on caches, but this would also be used in the same way as other dependency commands would use it.

  • How can a user write dependency updates to the lockfile?

It seems I have accidentally omitted this in the HIP. As I see it and from what I've understood from the proposals in the original issue, this would be used mainly for development and interoperation in cases where the user wants to use different dependencies one time and therefore I would propose to not change the lock file. This is open to discussion though.

  • What happens when there are version conflicts? As in, the Chart.yaml requests for ^1.0.0 but --set-dependency requests v2.0.0. What about transitive dependencies?

The --set-dependency flag overrides any dependencies in Chart.yaml. I have honestly not worked with clashes of transitive dependencies, but from what I've seen in several issues like this it's an open problem in Helm. For this feature the result would have been the same as if there were transitive clashes between Chart.yaml and its transitive dependencies.

  • What happens when the user requests a package that is NOT declared in Chart.yaml? Is that a failure case? Why or why not?

I have not thought of that, but I would allow it if they specify the version and repository. Here I'm assuming that even with cache both of these must be specified to correctly locate the digest and chart. If only one is specified that would be failure because there is not enough information to retrieve the dependency.

  • If the above case is supported, how is a chart developer expected to handle that case?

I don't understand this question.

Finally, I would suggest that under the "How to Teach This" section, I would place an example demonstrating a compelling use case that clearly illustrates why this proposal is required to solve the issue. That way we can observe its practicality and compare it to existing solutions or alternative HIPs in the future.

Sure.

@Boojapho
Copy link

In HIP 15 there is a proposal to add images lists to Chart.yaml. And, there have been requests in the past to allow overrides of appversion in Chart.yaml. My concern is that if we address this using the command line options, we end up with only a partial, uncoordinated solution.

Maybe an alternative is to provide an overlay flag for Chart.yaml (similar to -f) where we can override the Chart.yaml settings, regardless of what they are. This would help resolve dependency overrides, image overrides (if implemented), appversion overrides, etc. The same code/rules for values.yaml overlays would apply.

@mladedav
Copy link
Author

This seems to stagnate for a year now, while several people want this feature there is no champion to really push this through. Furthermore there seems to be desire to solve this in more general way.

I will close this now, if anyone wishes to take over I would be delighted.

@mladedav mladedav closed this Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants