feat(helm) add namespaceOverride

[TheRealNoob]

What this PR does / why we need it:
adds support for namespaceOverride in values file, useful if this chart is subcharted. For example, many people include myself probably subchart this as part of their "standard monitoring deployment" and want to direct metrics-server off to namespace=kube-system.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
none

[k8s-ci-robot]

Welcome @TheRealNoob!

It looks like this is your first PR to kubernetes-sigs/metrics-server 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/metrics-server has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @TheRealNoob. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[rexagod]

/ok-to-test

[TheRealNoob]

so i see that one test failed, but i'm scratching my head wondering how that could be caused by my change here.....any pointers would be much appreciated

[stevehipwell]

@TheRealNoob I'm not sure we want to go down this avenue, but I'm open to being convinced. My main concern is that Helm doesn't have sub-charts, it has dependencies but these charts are just flattened to create a single "chart". Could you please explain what benefit you get from this pattern and why it's worth the implicit downsides? It seems to me that you're attempting to solve an issue in the layers above the chart by modifying the chart causing a leaking abstraction.

[TheRealNoob]

One of the most popular use-cases for namespaceOverride is creating an umbrella chart and then selectively changing the namespace for specific subcharts. For example, creating a chart that rolls together all of your monitoring tools into one release. You would probably pull together kube-prometheus-stack, prometheus-operator-admission-webhook, prometheus-operator-crds, metrics-server, another example exporter, etc. helm install monitoring-stack umbrella/chart -n monitoring would force all of these subcharts to that namespace, but metrics-server typically goes to namespace=kube-system, which could be overridden with this PR.

I can understand your hesitation to think I'm setting up some unique scenario but it is quite common

• prometheus-community/kube-prometheus-stack
• prometheus-community/kube-state-metrics
• basically every chart in prometheus-community
• grafana/grafana
• grafana/loki (recently added)
• dex/dex
• argoproj/argo-helm

I see metrics-server as a staple of every monitoring stack, so I'm a bit surprised this hasn't been added before now to be honest

[TheRealNoob]

@stevehipwell could i get a re-review please

[stevehipwell]

@TheRealNoob I'm familiar with the umbrella chart principal and why you might want to do this, but as a general rule of thumb it's not the kind of pattern that you want to be using for something as important as Metrics Server.

[TheRealNoob]

but as a general rule of thumb it's not the kind of pattern that you want to be using

It sounds like this is the crux of where we're not seeing eye to eye. Could I ask you to elaborate on why you see this as an unappealing implementation/solution?

[TheRealNoob]

@stevehipwell I would really like to keep moving this PR forward with your support. I have looked into each SIG-instrumentation managed project and found that all of them which support helm charts already implement namespaceOverride, leaving metrics-server as the outlier which this PR addresses:

• kube-state-metrics
• prometheus-adapter

In addition to the above projects which already implement namespaceOverride, I have found several other kubernetes-sig projects that do:

• descheduler
• node-feature-discovery
• node-feature-discovery-operator
• headlamp
• aws-ebs-csi-driver

Because metrics-server is a staple of every monitoring stack, it should support the standard patterns operators use to deploy them. This is a purely opt-in feature with zero impact when unset -- the rendered output is identical to today's behavior. Lacking namespaceOverride doesn't make metrics-server safer -- it just forces users to manage it outside their unified deployment or deploy it to the wrong namespace.

I'd welcome any specific technical concerns about this approach.

[stevehipwell]

@TheRealNoob do the charts you've linked to above have a governance process or are they free contribution? Free contribution charts generally tend towards exposing the the whole KRM API via the Helm chart values. This chart is explicitly opinionated with the aim to be the "correct" way to deploy Metrics Server with Helm. This change is not aligned with the aim of this Helm chart.

[aw-talos]

@stevehipwell Where is the aim of the Helm chart documented?

[TheRealNoob]

I understand your concern about exposing too much of the KRM API. However in this case, the user already has the ability to control the namespace via built-in Helm capabilities helm install -n NAMESPACE. This PR only offers an alternative way to control it. It adds no security risk, doesn't change anything for existing users (unless they opt-in), and doesn't encroach across the KRM API at all. I have also demonstrated that this feature is an accepted standard by many organizations including prometheus-community, kubernetes (sig-scheduling, sig-node, sig-ui, sig-cloud-provider), and others.

I'd also like to understand what you mean by this chart being "explicitly opinionated". If there's a documented design philosophy for this chart, I'd like to review it so I can align this PR accordingly. If it's an informal position, I think it's worth reconsidering given how broadly this pattern is accepted across the Kubernetes ecosystem. Ultimately, the other projects I've listed implemented namespaceOverride because their users asked for it -- the same is true here. An opinionated chart should still align with the expectations of the ecosystem it serves.

[rexagod]

/assign @stevehipwell
/triage accepted

[rexagod]

/cc @skl

[TheRealNoob]

@stevehipwell If you could spare some time to review please? I really don't want to see this sit several months again with no progress.

[stevehipwell]

@TheRealNoob just a minor change.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: TheRealNoob
Once this PR has been reviewed and has the lgtm label, please ask for approval from stevehipwell. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• charts/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[skl]

/retest

[skl]

@TheRealNoob ci failed a couple of times, I wonder if you need another rebase?

[TheRealNoob]

I merged main. I had a look at the failing test and I suspect it's just flaky. at the very least it's not related to this change since the test case is metrics-server's exposing CPU metrics; a e2e test. i saw this same error when i first opened the PR then later on it went away. perhaps run the test a few times until it passes? or I can add blank commits to trigger it? I'll wait for your thoughts.

[TheRealNoob]

I see that the helm installed succeeded. log lines 243 - 260 show it installed to namespace=kube-system

[skl]

/retest-required

[skl]

Also suggest adding namespaceOverride to README.md.

[skl]

Suggested change

	- Add `namespaceOverride` which allows deploying to a namespace other than `.Release.namespace`. ([#1710](https://github.com/kubernetes-sigs/metrics-server/pull/1710)) @TheRealNoob
	- Add `namespaceOverride` which allows deploying to a namespace other than `.Release.namespace`. ([#1711](https://github.com/kubernetes-sigs/metrics-server/pull/1711)) @TheRealNoob