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.

Sign up for GitHub

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

Jump to bottom

Support explicitly passing --kube-version to helm #2563

Closed
henry-fn opened this issue Sep 13, 2023 · 9 comments · Fixed by henry-fn/pulumi-kubernetes#1 or #2593
Closed

Support explicitly passing --kube-version to helm #2563

henry-fn opened this issue Sep 13, 2023 · 9 comments · Fixed by henry-fn/pulumi-kubernetes#1 or #2593
Assignees
@henry-fn
Labels
area/helm kind/enhancement Improvements or new features resolution/fixed This issue was fixed

Comments

@henry-fn
Copy link
Contributor

Hello!

  • Vote on this issue by adding a 👍 reaction
  • If you want to implement this feature, comment to let us know (we'll work with you on design, scheduling, etc.)

Issue details

#753 explained this issue well, but the resolution does not cover one edge case that we cared about. We have a workflow where we would like to run previews without access to the actual K8s API server (just based on pulumi state which IIUC in the case of helm charts should only use helm template).

#753 (comment) asked for a way to explicitly set kubeVersion in the helm args which would have solved our issue as we can explicitly pass this value when running in this semi-offline mode, however the resolution to the previous issue was to auto-derive it from the server which doesn't work in our case. Could a kubeVersion argument still be added to the helm release / chart options to allow explicit override in these cases?

Affected area/feature

helm
sdk
@henry-fn henry-fn added kind/enhancement Improvements or new features needs-triage Needs attention from the triage team labels Sep 13, 2023
@mikhailshilkov mikhailshilkov added area/helm and removed needs-triage Needs attention from the triage team labels Sep 13, 2023
@henry-fn henry-fn mentioned this issue Oct 3, 2023
Merged
henry-fn added a commit to henry-fn/pulumi-kubernetes that referenced this issue Oct 3, 2023
@henry-fn
pulumi#2563: Expose Helm --kube-version argument for contextless diffs
844d8c0 
<!--Thanks for your contribution. See [CONTRIBUTING](CONTRIBUTING.md)
    for Pulumi's contribution guidelines.

    Help us merge your changes more quickly by adding more details such
    as labels, milestones, and reviewers.-->

### Proposed changes

This PR exposes the helm template `--kube-version` argument so it can be manually specified in situations when there is no access to the Kubernetes API server (e.g running a diff in CSA mode with no connection to the cluster).

Prior to this merge:
* If there were zero k8s contexts --> this field was unset meaning helm would default to some old version (1.20.0 at the time of writing) if it couldn't connect to the k8s context
* 1+ k8s contexts --> would always try to contact the API server to get the version leading to an error if the API server was unreachable.

This variable will override the behavior in either case, but is mostly useful in the first case or when the context in the second case is malformed / unreachable. 

### Related issues (optional)

<!--Refer to related PRs or issues: pulumi#1234, or 'Fixes pulumi#1234' or 'Closes pulumi#1234'.
    Or link to full URLs to issues or pull requests in other GitHub repositories. -->

Closes pulumi#2563
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Oct 3, 2023
@henry-fn henry-fn mentioned this issue Oct 3, 2023
Merged
@henry-fn
Copy link
Contributor Author

henry-fn commented Oct 3, 2023 •
edited
Loading

Mistakenly put a closes comment in my fork's MR. Still needs #2593 to be merged (merging fork into master)

@henry-fn henry-fn reopened this Oct 3, 2023
@henry-fn henry-fn removed their assignment Oct 3, 2023
@rquitales rquitales removed the resolution/fixed This issue was fixed label Oct 3, 2023
rquitales added a commit that referenced this issue Oct 19, 2023
@henry-fn @rquitales
Expose Helm --kube-version argument for contextless diffs (#2593)
753a164 
This commit exposes the helm template `--kube-version` argument so it can be
manually specified in situations when there is no access to the
Kubernetes API server (e.g running a diff in CSA mode with no connection
to the cluster).

Prior to this merge:
* If there were zero k8s contexts --> this field was unset meaning helm
would default to some old version (1.20.0 at the time of writing) if it
couldn't connect to the k8s context
* 1+ k8s contexts --> would always try to contact the API server to get
the version leading to an error if the API server was unreachable.

This variable will override the behavior in either case, but is mostly
useful in the first case or when the context in the second case is
malformed / unreachable.

Closes #2563

---------

Co-authored-by: Ramon Quitales <[email protected]>
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Oct 19, 2023
@hypesystem
Copy link

hypesystem commented Oct 24, 2023 •
edited
Loading

Did this make it into Releases to, or just Chart? I'm seeing the same issue using an imported Release, and am not familiar enough with the code here to understand what it affects 😄

I am using 4.5.0 which looks like it should have the change 😄

@EronWright
Copy link
Contributor

I believe the fix was developed for Chart only. @rquitales I am re-opening this to consider a fix for Release.

@EronWright EronWright reopened this Oct 24, 2023
@henry-fn henry-fn removed their assignment Oct 24, 2023
@hypesystem
Copy link

If there is anything I can help with (including writing code), please let me know, and I'll be happy to contribute in any way I can to help move it forward 😄

@hypesystem
Copy link

I've been looking into the code, and adding KubeVersion seems like it would be pretty straight-forward. I've noticed that the automatic discovery of api versions and kubeversion is also only implemented for chart, and not release.

This has lead to my confusing situation of importing a release from a running cluster, that now cannot be previewed or upped because the validation assumes a wrong kubeVersion (from local helm install).

I think it would make sense to try and align the two, maybe even reuse some common code for the templating in the two cases, to minimize the risk of them diverging in the future?

@pulumi-bot pulumi-bot added the needs-triage Needs attention from the triage team label Oct 25, 2023
@mikhailshilkov mikhailshilkov removed the needs-triage Needs attention from the triage team label Oct 26, 2023
@omidraha

This comment was marked as resolved.

Sign in to view
@EronWright

This comment was marked as resolved.

Sign in to view
@EronWright EronWright mentioned this issue Nov 4, 2023
Closed
@EronWright
Copy link
Contributor

As of pulumi-kubernetes v4.5.4 (#2653), any problem with "chart requires kubeVersion" should be fixed, except in the special case that there's a server connectivity problem, e.g. due to a bad provider configuration.

I see no way to pass an explicit kubeVersion to Helm during the create or upgrade operation; Helm always uses the server version. Such a feature seems to make sense only for offline templating, i.e. using Chart resource. For that reason, I am closing this issue as complete.

@hypesystem
Copy link

@EronWright Really nice work! Thank you 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@henry-fn henry-fn

Labels
area/helm kind/enhancement Improvements or new features resolution/fixed This issue was fixed
Projects
None yet
Milestone
No milestone
7 participants
@omidraha @hypesystem @mikhailshilkov @EronWright @rquitales @pulumi-bot @henry-fn