Remove kube-rbac-proxy #1730

CecileRobertMichon · 2021-09-24T17:49:40Z

What type of PR is this?
/kind deprecation

What this PR does / why we need it: This should have been done as part of v1alpha4 per https://cluster-api.sigs.k8s.io/developer/providers/v1alpha3-to-v1alpha4, fixing now.

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

Special notes for your reviewer: I verified that this does not affect our telemetry in tilt.

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Remove kube-rbac-proxy

CecileRobertMichon · 2021-09-24T23:03:51Z

/retest

CecileRobertMichon · 2021-09-27T16:04:37Z

/retest

CecileRobertMichon · 2021-09-27T16:34:19Z

/assign @mboersma @shysank

CecileRobertMichon · 2021-09-27T17:53:46Z

/retest

mboersma · 2021-09-27T18:35:48Z

I tested this locally, and it seems to break prometheus metrics, at least as viewed locally via the "metrics" link in Tilt. Switching back to the main branch fixes it. I hope I didn't give you bad advice--I didn't think there was a dependency there...

sbueringer · 2021-09-27T18:37:24Z

config/manager/manager.yaml

        - args:
            - --leader-elect
-            - "--metrics-bind-addr=127.0.0.1:8080"
            - "--feature-gates=MachinePool=${EXP_MACHINE_POOL:=false},AKS=${EXP_AKS:=false}"


just fyi: In CAPI we kept that one (but with localhost:8080) so we can replace it in our e2e test (https://github.com/kubernetes-sigs/cluster-api/blob/89df9472d1678ed8ff59f4e124d48ad51ffd85c6/test/e2e/config/docker.yaml#L53-L54) and the e2e test framework is able to retrieve the metrics via proxy (https://github.com/kubernetes-sigs/cluster-api/blob/c55b5013fcb84841cc3705dfb250cdfd8b140447/test/framework/deployment_helpers.go#L200-L206)

Not sure if that's also relevant for CAPZ

added back with localhost

If you want to collect metrics during e2e tests you also have to add the replacements to azure-dev.yaml (for all providers).

Otherwise you'll get: https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-azure/1730/pull-cluster-api-provider-azure-e2e/1442611507668979712/artifacts/clusters/bootstrap/controllers/capi-controller-manager/capi-controller-manager-7dc56b9799-mk5tf/metrics-error.txt

sbueringer · 2021-09-27T18:39:20Z

I tested this locally, and it seems to break prometheus metrics, at least as viewed locally via the "metrics" link in Tilt. Switching back to the main branch fixes it. I hope I didn't give you bad advice--I didn't think there was a dependency there...

Sounds like what I described here: #1707 (comment)

CecileRobertMichon · 2021-09-27T21:47:09Z

I tested this locally, and it seems to break prometheus metrics, at least as viewed locally via the "metrics" link in Tilt. Switching back to the main branch fixes it. I hope I didn't give you bad advice--I didn't think there was a dependency there...

I'm surprised that is the case, we already have a patch for adding the metrid-bind-addr arg to the CAPZ manager: https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/main/hack/observability/opentelemetry/controller-manager-patch.yaml#L12 and that's what we use in tilt:

https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/main/Tiltfile#L154

Will test again and report back.

CecileRobertMichon · 2021-09-27T22:03:57Z

hack/observability/opentelemetry/controller-manager-patch.yaml

        - name: manager
          args:
-            - "--metrics-bind-addr=127.0.0.1:8080"
+            - "--metrics-bind-addr=:8080"


@mboersma @sbueringer this fixes Prometheus metrics in tilt

I think the problem is that the ServiceMonitor still points to the https (8443) port.

One solution would be to adjust the auth_proxy_service.yaml (8443=>8080, scheme:https => scheme:http, port https => metrics (and also add the corresponding metrics port (8080) to the deployment)

CecileRobertMichon · 2021-09-27T23:03:46Z

/retest

CecileRobertMichon · 2021-09-28T00:15:51Z

/hold

@mboersma reported that metrics are still not getting collected

CecileRobertMichon · 2021-09-28T22:50:53Z

@devigned @mboersma @sbueringer I think this time I fixed it for real... I realized from our testing earlier that I hadn't re-added the auth_proxy_service.yaml to kustomization.yaml so the changes I made in there had no impact 🤦‍♀️

sbueringer · 2021-09-29T06:25:57Z

config/rbac/auth_proxy_service.yaml

 kind: Service
 metadata:
  annotations:
    prometheus.io/scrape: "true"


Currently this service only works in combination with the controller-manager-patch.yaml from the observability folder.

I think it would be good to either:

move the file to the observability folder

make it functional by changing the bind addr in config/manager/manager.yaml (As mentioned: that's something which was rejected for security reasons in the main repo, but I'll stay out of this discussion :))

@sbueringer thank you so much for your insights!

You're welcome

mboersma · 2021-09-29T15:21:14Z

I can confirm metrics are flowing again in Tilt.

I like @sbueringer's suggestion of moving auth_proxy_service.yaml into the hack/observability folder, to make it more clear that the metrics aren't intended for use in production.

devigned · 2021-09-29T15:36:34Z

/assign

With @CecileRobertMichon out of office, I'll make the changes and update the PR.

devigned · 2021-09-29T19:37:59Z

@mboersma please give this a look when you have a minute. I've updated the PR by moving the service exposure to the observability folder.

mboersma · 2021-09-29T20:30:36Z

/retest
(failed AKS cluster creation)

mboersma

/lgtm

I tested this locally and prometheus was seeing metrics.

sbueringer · 2021-09-30T06:28:40Z

One nit: I don't think you need auth_proxy_client_clusterrole.yaml anymore. It was only required to make it possible to scrape metrics through the kube-rbac-proxy.

devigned · 2021-09-30T14:58:31Z

One nit: I don't think you need auth_proxy_client_clusterrole.yaml anymore. It was only required to make it possible to scrape metrics through the kube-rbac-proxy.

I was thinking that. I could probably rip out some of the prom config, like the service account token, etc.

sbueringer · 2021-09-30T15:19:51Z

One nit: I don't think you need auth_proxy_client_clusterrole.yaml anymore. It was only required to make it possible to scrape metrics through the kube-rbac-proxy.

I was thinking that. I could probably rip out some of the prom config, like the service account token, etc.

Yes, sgtm

devigned · 2021-09-30T18:56:28Z

@mboersma I've ripped out some of the prom auth config and slimmed down the PR to only the required resources. If you could give this a quick check, I'd appreciate it.

mboersma · 2021-09-30T22:45:49Z

/lgtm

I retested, and prometheus looks good.

devigned · 2021-09-30T23:09:48Z

Thank you, @mboersma!

/approve

k8s-ci-robot · 2021-09-30T23:10:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: devigned

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [devigned]

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

sbueringer · 2021-10-01T08:43:44Z

/lgtm

devigned · 2021-10-01T13:13:18Z

/hold cancel

CecileRobertMichon · 2021-10-04T17:07:33Z

thanks for carrying this to the finish line @devigned!

k8s-ci-robot requested review from juan-lee and shysank September 24, 2021 17:50

k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Sep 24, 2021

k8s-ci-robot assigned mboersma and shysank Sep 27, 2021

sbueringer reviewed Sep 27, 2021

View reviewed changes

CecileRobertMichon force-pushed the remove-kube-rbac-proxy branch from b751518 to 32b7e61 Compare September 27, 2021 22:02

CecileRobertMichon commented Sep 27, 2021

View reviewed changes

CecileRobertMichon force-pushed the remove-kube-rbac-proxy branch from 32b7e61 to ccef607 Compare September 27, 2021 22:05

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2021

CecileRobertMichon force-pushed the remove-kube-rbac-proxy branch from ccef607 to 63ae4ab Compare September 28, 2021 22:48

sbueringer reviewed Sep 29, 2021

View reviewed changes

k8s-ci-robot assigned devigned Sep 29, 2021

mboersma approved these changes Sep 29, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2021

Cecile Robert-Michon and others added 2 commits September 30, 2021 14:54

Remove kube-rbac-proxy

15b5d93

stop exposing the metric service outside of tilt deploy

eb881cd

devigned force-pushed the remove-kube-rbac-proxy branch from 9c069a0 to eb881cd Compare September 30, 2021 18:54

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 30, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 30, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 30, 2021

k8s-ci-robot assigned sbueringer Oct 1, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 1, 2021

k8s-ci-robot merged commit 62a3b4f into kubernetes-sigs:main Oct 1, 2021

k8s-ci-robot added this to the v0.5 milestone Oct 1, 2021

mboersma mentioned this pull request Oct 1, 2021

Remove kube-rbac-proxy #1751

Closed

3 tasks

CecileRobertMichon deleted the remove-kube-rbac-proxy branch February 17, 2023 23:24

Remove kube-rbac-proxy #1730

Remove kube-rbac-proxy #1730

Uh oh!

Conversation

CecileRobertMichon commented Sep 24, 2021

Uh oh!

CecileRobertMichon commented Sep 24, 2021

Uh oh!

CecileRobertMichon commented Sep 27, 2021

Uh oh!

CecileRobertMichon commented Sep 27, 2021

Uh oh!

CecileRobertMichon commented Sep 27, 2021

Uh oh!

mboersma commented Sep 27, 2021

Uh oh!

sbueringer Sep 27, 2021

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon Sep 27, 2021

Choose a reason for hiding this comment

Uh oh!

sbueringer Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

sbueringer commented Sep 27, 2021

Uh oh!

CecileRobertMichon commented Sep 27, 2021

Uh oh!

CecileRobertMichon Sep 27, 2021

Choose a reason for hiding this comment

Uh oh!

sbueringer Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon commented Sep 27, 2021

Uh oh!

CecileRobertMichon commented Sep 28, 2021

Uh oh!

CecileRobertMichon commented Sep 28, 2021

Uh oh!

sbueringer Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devigned Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

sbueringer Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

mboersma commented Sep 29, 2021

Uh oh!

devigned commented Sep 29, 2021

Uh oh!

devigned commented Sep 29, 2021

Uh oh!

mboersma commented Sep 29, 2021

Uh oh!

mboersma left a comment

Choose a reason for hiding this comment

Uh oh!

sbueringer commented Sep 30, 2021

Uh oh!

devigned commented Sep 30, 2021

Uh oh!

sbueringer commented Sep 30, 2021

Uh oh!

devigned commented Sep 30, 2021

Uh oh!

mboersma commented Sep 30, 2021

Uh oh!

devigned commented Sep 30, 2021

Uh oh!

k8s-ci-robot commented Sep 30, 2021

Uh oh!

sbueringer commented Oct 1, 2021

Uh oh!

devigned commented Oct 1, 2021

Uh oh!

sbueringer Sep 29, 2021 •

edited

Loading