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

feat(helm): Add kubeVersionOverride for Helm chart #14434

Merged
merged 4 commits into from
Oct 21, 2024

Conversation

mcastellin
Copy link
Contributor

What this PR does / why we need it:

With this change is now possible to override the
.Capabilities.KubeVersion.Version parameter with a fixed Kubernetes version using kubeVersionOverride in values.yaml file.

Overriding the kubeVersion allows chart's user to generate templates from the chart that can be deployed to cluster that are not directly accessible from the user's CLI and run a different Kubernetes version.

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@mcastellin mcastellin requested a review from a team as a code owner October 9, 2024 10:25
@CLAassistant
Copy link

CLAassistant commented Oct 9, 2024

CLA assistant check
All committers have signed the CLA.

This comment has been minimized.

With this change is now possible to override the
`.Capabilities.KubeVersion.Version` parameter with a fixed Kubernetes
version using `kubeVersionOverride` in values.yaml file.

Overriding the kubeVersion allows chart's user to generate templates
from the chart that can be deployed to cluster that are not directly
accessible from the user's CLI and run a different Kubernetes version.

Signed-off-by: Manuel Castellin <[email protected]>
@mcastellin mcastellin force-pushed the feat/helm-kubeversion-override branch from a6f539a to 0a1a2e0 Compare October 9, 2024 10:45

This comment has been minimized.

This comment has been minimized.

Copy link
Collaborator

@trevorwhitney trevorwhitney left a comment

Choose a reason for hiding this comment

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

This feels like a case for post-processing (ie. modifying rendered templates for different clusters). What becomes the default behavior with this change? When kubeVersionOverride is null, does it fall back to .Capabilities.KubeVersion.Version? I'm not seeing where that is happening (but I also only took a very quick glance at this).

@mcastellin
Copy link
Contributor Author

mcastellin commented Oct 18, 2024

This feels like a case for post-processing (ie. modifying rendered templates for different clusters). What becomes the default behavior with this change? When kubeVersionOverride is null, does it fall back to .Capabilities.KubeVersion.Version? I'm not seeing where that is happening (but I also only took a very quick glance at this).

Hello @trevorwhitney, yes, you are correct indeed, when the kubeVersionOverride is null the value of loki.kubeVersion falls back to .Capabilities.KubeVersion.Version and it's implemented here. This approach is already implemented for other grafana products like Mimir (see here for instance). The null default value ensures backward compatibility with existing values files.

This is a very important feature to have for anyone that manages infrastructure with ArgoCD and needs to deploy helm charts. Charts are not applied directly by Argo, they're first inflated using helm template, hence pseudo-parameters such as .Capabilities.KubeVersion.Version are not resolved against the target k8s cluster and will default to, I think, v1.16.

Post-processing the templates as you suggested is an option, though resources are generated by the chart based on many conditions (kube version of the target cluster, variables, ..) so even the tiniest change to the values.yaml could add new resources in the rendered template and can become a nightmare to maintain.

Copy link
Collaborator

@trevorwhitney trevorwhitney left a comment

Choose a reason for hiding this comment

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

sorry for the cursory initial review, thanks for pointing out the default and similar behavior with mimir. I'm good with this as long as it doesn't affect the default, which is apparently the case. thanks!

@trevorwhitney trevorwhitney changed the title Feat: Add kubeVersionOverride for Helm chart feat(helm): Add kubeVersionOverride for Helm chart Oct 18, 2024

This comment has been minimized.

@github-actions github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Oct 21, 2024
Copy link
Contributor

Kubernetes Manifest Diff Summary

Scenario: default-single-binary-values (Added: 0, Modified: 0, Removed: 0)

Summary:

  • Added: 0

  • Modified: 0

  • Removed: 0

Added Files

No added files

Modified Files

No modified files

Removed Files

No removed files

Scenario: default-values (Added: 0, Modified: 0, Removed: 0)

Summary:

  • Added: 0

  • Modified: 0

  • Removed: 0

Added Files

No added files

Modified Files

No modified files

Removed Files

No removed files

Scenario: ingress-values (Added: 0, Modified: 0, Removed: 0)

Summary:

  • Added: 0

  • Modified: 0

  • Removed: 0

Added Files

No added files

Modified Files

No modified files

Removed Files

No removed files

Scenario: legacy-monitoring-values (Added: 0, Modified: 0, Removed: 0)

Summary:

  • Added: 0

  • Modified: 0

  • Removed: 0

Added Files

No added files

Modified Files

No modified files

Removed Files

No removed files

Scenario: simple-scalable-aws-kube-irsa-values (Added: 0, Modified: 0, Removed: 0)

Summary:

  • Added: 0

  • Modified: 0

  • Removed: 0

Added Files

No added files

Modified Files

No modified files

Removed Files

No removed files

@mcastellin
Copy link
Contributor Author

sorry for the cursory initial review, thanks for pointing out the default and similar behavior with mimir. I'm good with this as long as it doesn't affect the default, which is apparently the case. thanks!

No worries @trevorwhitney, I can see you have a lot on your plate 🙂
I've updated the PR with the generated docs.

@trevorwhitney trevorwhitney merged commit 0935d77 into grafana:main Oct 21, 2024
64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm size/M type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants