feat(helm chart): Add metrics-server hardening options #1288

mkilchhofer · 2023-07-14T13:28:00Z

What this PR does / why we need it:

It adds options on howto secure the connection between metrics-server and the kube-apiserver.

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

related to:

Resolves #1230

Additional info:
The accepted FR #1230 is implemented also as I need this to test the hardening feature.

/cc: @serathius

linux-foundation-easycla · 2023-07-14T13:28:04Z

The committers listed above are authorized under a signed CLA.

✅ login: mkilchhofer / name: Marco Kilchhofer (0f4d76f)

k8s-ci-robot · 2023-07-14T13:28:08Z

Welcome @mkilchhofer!

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 · 2023-07-14T13:28:09Z

Hi @mkilchhofer. 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.

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/test-infra repository.

charts/metrics-server/ci/tls-custom-values.yaml

serathius · 2023-07-14T21:29:13Z

/assign @stevehipwell

k8s-ci-robot · 2023-07-17T06:50:53Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mkilchhofer
Once this PR has been reviewed and has the lgtm label, please assign serathius for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

stevehipwell

Thanks for the PR @mkilchhofer, I've got a couple of high level review point which it would be good if you could address before I review this in more details

There is no need to run functions for the APIService if it's not enabled
The helm mode would be a better default (see below)
You don't need to recreate the cert on each Helm install, you can look up the existing secret
I don't think the chart should support creating a secret, the secret option should only be existing
Please remove the extraObjects value, this isn't a "good" pattern

mkilchhofer · 2023-07-17T11:18:27Z

There is no need to run functions for the APIService if it's not enabled

How do you mean that? The genSelfSignedCert function?

The helm mode would be a better default (see below)

Sure, will do

You don't need to recreate the cert on each Helm install, you can look up the existing secret

I think we should not rely on the lookup function. There are GitOps solutions in the wild which only use helm for rendering (helm template subcommand). The lookup function of helm returns nothing during template as it cannot talk to the kubeapi by design. Only upgrade or install talks to the apiserver.

I don't think the chart should support creating a secret, the secret option should only be existing

Hmm why not? It is pretty common that helm charts support this.

Please remove the extraObjects value, this isn't a "good" pattern

How can I test the existing secret method then? I think it is an approved feature request, so contributors can file PRs for this, ref:

[FR] Allow the Chart to create extra manifest #1230
-> See label triage/accepted

stevehipwell · 2023-07-17T11:26:30Z

How do you mean that? The genSelfSignedCert function?

You moved the test for if the APIService was enabled below the new code.

I think we should not rely on the lookup function. There are GitOps solutions in the wild which only use helm for rendering (helm template subcommand). The lookup function of helm returns nothing during template as it cannot talk to the kubeapi by design. Only upgrade or install talks to the apiserver.

In that case they can use one of the other patterns, GitOps shouldn't define architecture it should support best practice.

Hmm why not? It is pretty common that helm charts support this.

Because it's unnecessary and leaks data into the chart values.

How can I test the existing secret method then? I think it is an approved feature request, so contributors can file PRs for this, ref:

We could add a test secret as part or the CI initialisation or it could be generated through the chart as a testing resource.

mkilchhofer · 2023-07-17T11:39:33Z

Because it's unnecessary and leaks data into the chart values.

Well, why is that an issue? This is how helm is designed for. These values are stored as a K8s secret.

stevehipwell · 2023-07-17T12:51:52Z

Well, why is that an issue? This is how helm is designed for. These values are stored as a K8s secret.

It's the unnecessary part which is the primary concern here; but the Helm implementation and increased surface area for secret information shouldn't be discounted as unimportant.

But as I've said in my other replies you can easily extend this chart by creating a wrapper chart which depends on it and at which point you can make your own architectural decisions.

stevehipwell

Thanks for the changes @mkilchhofer, I've added a couple of structural change requests.

charts/metrics-server/templates/apiservice.yaml

stevehipwell · 2023-07-19T15:04:34Z

charts/metrics-server/templates/apiservice.yaml

+{{- $altNames = append $altNames (printf "%s.%s" $commonName $ns) }}
+{{- $altNames = append $altNames (printf "%s.%s.svc" $commonName $ns) }}
+{{- $altNames = append $altNames (printf "%s.%s.svc.%s" $commonName $ns .Values.tls.clusterDomain) }}
+{{- $certs = genSelfSignedCert $commonName nil $altNames 36500 }}


Please look up the existing secret instead of generating a new certificate each time the chart is deployed.

We can easily recreate the Secrets every helm upgrade. It's even more secure to change it on a regular basis IMHO.
Using the lookup function isn't safe against helm tooling as already mentioned somewhere in this PR.

People who are using

Terraform template datasource

Argo CD

Kustomize with helm support

probably Helmfile (cannot verify my statement here as I don't use this)

probably Tanka (cannot verify my statement here as I don't use this)

uses the helm chart only for templating. And template cannot use lookup

The second point is moot as this is a Helm chart and optimised for idiomatic Helm use, alternative usage patterns can use this if it works but if it doesn't there are alternatives. Also this should behave exactly the same as if lookup wasn't used if called by helm template.

For the first point have you tested the behaviour of changing the certificate during standard usage of MS for HPAs?

I did not test the upgrade procedure. But it should be fine:

We update both the Secret and the APIserver resource

Kubernetes spin up a new Pod and performs a rolling update

The existing Pod should have an updated Secret mounted as we don't use subPath mounting options. MS uses generic k8s.io/apiserver/pkg/server to implement the API, this one uses github.com/fsnotify/fsnotify to watch if the certificate file changes:

https://github.com/kubernetes/apiserver/blob/00a0da2cb88bcb3c971bc419510c50ca8b1b7abe/pkg/server/dynamiccertificates/dynamic_serving_content.go#L148

...

Do you need some test results here? :-)

We probably need to test that MS is reloading the cert when it changes and then what the behaviour during the cert change is. If there is no interruption then I think we could not lookup the cert but add a comment in the code to say it could be done.

Okay I now learned that Secrets/ConfigMaps are not immediately updated via kubelet. When there are no events happening on the Pod, it can take up to 1 minute.

Screen.Recording.2023-07-20.at.17.17.25_compressed.mov

I found the technical background inside a Kyverno policy 😄 :

Although ConfigMaps and Secrets mounted as volumes to a Pod, when the contents change, will eventually propagate to the Pods mounting them, this process may take between 60-90 seconds. In order to reduce that time, a modification made to downstream Pods will cause the changes to take effect almost instantly. This policy watches for changes to ConfigMaps which have been marked for this quick reloading process which contain the label `kyverno.io/watch=true` and will write an annotation to any Pods which mount them as volumes causing a fast refresh in their contents. See the related policy entitled "Refresh Environment Variables in Pods" for a similar reloading process when ConfigMaps and Secrets are consumed as environment variables instead. Use of this policy may require providing the Kyverno ServiceAccount with permission to update Pods.

Source: https://kyverno.io/policies/other/refresh-volumes-in-pods/refresh-volumes-in-pods/

Will investigate what I can do here..

This has been my experience with optimising Fluent Bit for hot-reload.

If the Helm pattern used lookup it could be documented as interruption safe which run directly from Helm, if interruptions are an issue and you're using an alternative apply method then use Cert Manager (or an external secret).

Another option would be to use an init container or a helm job hook and something like ingress-nginx from the kubernetes org:
https://github.com/kubernetes/ingress-nginx/tree/main/images/kube-webhook-certgen
Seems to be a fork of https://github.com/jet/kube-webhook-certgen

This seems like overengineering to me. For the Helm implementation we should use lookup and document why you might not want to do so.

Alright. Lookup is already implemented 👍

charts/metrics-server/templates/certificate.yaml

charts/metrics-server/templates/apiservice.yaml

dashpole · 2023-07-27T16:56:55Z

/triage accepted

stevehipwell · 2023-07-28T14:59:36Z

@mkilchhofer could you please rebase and then I'll review again?

mkilchhofer · 2023-07-28T18:06:41Z

@stevehipwell rebase done

stevehipwell · 2023-08-02T11:07:09Z

charts/metrics-server/values.yaml

+    # Use helm lookup function to reuse Secret created in previous helm install
+    lookup: true
+    # Cert validity duration in days
+    certDurationDays: 90


I think this expiry should probably be longer by default and we should provide a cert replacement mechanism. This could be automated based on an expiry field in the secret or based on a generation chart input to rotate the certificate. Either way the Helm TLS pattern should probably be called out as not suitable for production without additional engineering overhead.

k8s-ci-robot · 2023-10-28T03:30:10Z

PR needs rebase.

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/test-infra repository.

stevehipwell · 2023-12-01T12:14:33Z

@mkilchhofer could you rebase this PR?

k8s-triage-robot · 2024-02-29T12:33:17Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

mkilchhofer · 2024-02-29T13:44:25Z

Will try to rebase the PR as soon as I have time

yangjunmyfm192085 · 2024-02-29T13:55:46Z

/remove-lifecycle stale

k8s-triage-robot · 2024-05-29T14:21:01Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 14, 2023

k8s-ci-robot requested review from stevehipwell and yangjunmyfm192085 July 14, 2023 13:28

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 14, 2023

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 14, 2023

mkilchhofer force-pushed the feature/helm-chart_hardening branch from 1a6b939 to f0b5051 Compare July 14, 2023 14:06

mkilchhofer commented Jul 14, 2023

View reviewed changes

charts/metrics-server/ci/tls-custom-values.yaml Outdated Show resolved Hide resolved

mkilchhofer force-pushed the feature/helm-chart_hardening branch from 4ca13e0 to 167cf2d Compare July 14, 2023 15:26

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 14, 2023

mkilchhofer force-pushed the feature/helm-chart_hardening branch from 167cf2d to 7baff6e Compare July 14, 2023 15:30

k8s-ci-robot assigned stevehipwell Jul 14, 2023

mkilchhofer force-pushed the feature/helm-chart_hardening branch from 7baff6e to 25e9877 Compare July 14, 2023 22:44

stevehipwell suggested changes Jul 17, 2023

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 18, 2023

mkilchhofer force-pushed the feature/helm-chart_hardening branch 2 times, most recently from cddec52 to 81f71cd Compare July 18, 2023 06:28

mkilchhofer force-pushed the feature/helm-chart_hardening branch from 81f71cd to ee37851 Compare July 18, 2023 06:36

stevehipwell suggested changes Jul 19, 2023

View reviewed changes

stevehipwell reviewed Jul 21, 2023

View reviewed changes

charts/metrics-server/templates/apiservice.yaml Outdated Show resolved Hide resolved

mkilchhofer force-pushed the feature/helm-chart_hardening branch from b0326dc to 0d99387 Compare July 24, 2023 20:59

mkilchhofer requested a review from stevehipwell July 24, 2023 20:59

mkilchhofer force-pushed the feature/helm-chart_hardening branch 3 times, most recently from 2e903c2 to 93f1c41 Compare July 24, 2023 21:36

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 27, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2023

feat(helm chart): Add metrics-server hardening options

0f4d76f

mkilchhofer force-pushed the feature/helm-chart_hardening branch from 93f1c41 to 0f4d76f Compare July 28, 2023 18:06

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2023

stevehipwell reviewed Aug 2, 2023

View reviewed changes

stevehipwell mentioned this pull request Sep 21, 2023

Support for certManager in helmChart and CLI #1326

Open

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2023

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 29, 2024

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 29, 2024

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(helm chart): Add metrics-server hardening options #1288

feat(helm chart): Add metrics-server hardening options #1288

mkilchhofer commented Jul 14, 2023 •

edited

linux-foundation-easycla bot commented Jul 14, 2023 •

edited

k8s-ci-robot commented Jul 14, 2023

k8s-ci-robot commented Jul 14, 2023

serathius commented Jul 14, 2023

k8s-ci-robot commented Jul 17, 2023

stevehipwell left a comment

mkilchhofer commented Jul 17, 2023

stevehipwell commented Jul 17, 2023

mkilchhofer commented Jul 17, 2023 •

edited

stevehipwell commented Jul 17, 2023

stevehipwell left a comment

stevehipwell Jul 19, 2023

mkilchhofer Jul 19, 2023 •

edited

stevehipwell Jul 19, 2023

mkilchhofer Jul 19, 2023 •

edited

stevehipwell Jul 20, 2023

mkilchhofer Jul 20, 2023

stevehipwell Jul 21, 2023

mkilchhofer Jul 24, 2023

stevehipwell Jul 26, 2023

mkilchhofer Jul 26, 2023

dashpole commented Jul 27, 2023

stevehipwell commented Jul 28, 2023

mkilchhofer commented Jul 28, 2023

stevehipwell Aug 2, 2023

k8s-ci-robot commented Oct 28, 2023

stevehipwell commented Dec 1, 2023

k8s-triage-robot commented Feb 29, 2024

mkilchhofer commented Feb 29, 2024

yangjunmyfm192085 commented Feb 29, 2024

k8s-triage-robot commented May 29, 2024

feat(helm chart): Add metrics-server hardening options #1288

Are you sure you want to change the base?

feat(helm chart): Add metrics-server hardening options #1288

Conversation

mkilchhofer commented Jul 14, 2023 • edited

linux-foundation-easycla bot commented Jul 14, 2023 • edited

k8s-ci-robot commented Jul 14, 2023

k8s-ci-robot commented Jul 14, 2023

serathius commented Jul 14, 2023

k8s-ci-robot commented Jul 17, 2023

stevehipwell left a comment

Choose a reason for hiding this comment

mkilchhofer commented Jul 17, 2023

stevehipwell commented Jul 17, 2023

mkilchhofer commented Jul 17, 2023 • edited

stevehipwell commented Jul 17, 2023

stevehipwell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkilchhofer Jul 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkilchhofer Jul 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dashpole commented Jul 27, 2023

stevehipwell commented Jul 28, 2023

mkilchhofer commented Jul 28, 2023

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 28, 2023

stevehipwell commented Dec 1, 2023

k8s-triage-robot commented Feb 29, 2024

mkilchhofer commented Feb 29, 2024

yangjunmyfm192085 commented Feb 29, 2024

k8s-triage-robot commented May 29, 2024

mkilchhofer commented Jul 14, 2023 •

edited

linux-foundation-easycla bot commented Jul 14, 2023 •

edited

mkilchhofer commented Jul 17, 2023 •

edited

mkilchhofer Jul 19, 2023 •

edited

mkilchhofer Jul 19, 2023 •

edited