Discontinue kube-rbac-proxy and disable metrics service #721

black-dragon74 · 2024-12-04T13:58:08Z

This patch drops support for kube-rbac-proxy
and uses controller manager's
WithAuthenticationAndAuthorization.

Closes: #643

For more details please refer here

In addition to migration, some of the resources have been re-categorized for better placement and the metrics service is now disabled by default as it did not contain any significant information specific to csi-addons. It can be re-enabled by following the steps outlined in #721 (comment)

black-dragon74 · 2024-12-04T14:04:55Z

Testing

1. Access `/metrics` endpoint as controller-manager's SA

❯ oc auth can-i get /metrics --as=system:serviceaccount:csi-addons-system:csi-addons-controller-manager
yes

2. Get the metrics using valid SA token

❯ oc exec -it po/metrics-consumer -- sh
$ curl -k -H "Authorization: Bearer $(cat /var/run/secrets/kubernetes.io/serviceaccount/token)" https://csi-addons-controller-manager-metrics-service.csi-addons-system.svc.cluster.local:8443/metrics
# HELP certwatcher_read_certificate_errors_total Total number of certificate read errors
# TYPE certwatcher_read_certificate_errors_total counter
certwatcher_read_certificate_errors_total 0
# HELP certwatcher_read_certificate_total Total number of certificate reads
...
...
...
stripped....

3. Get the metrics using invalid SA token

$ curl -k -H "Authorization: Bearer $(cat /var/run/secrets/kubernetes.io/serviceaccount/INVALID)" https://csi-addons-controller-manager-metrics-service.csi-addons-system.svc.cluster.local:8443/metrics
Unauthorized

4. Check POD's containers (no kube-rbac-proxy-container)

❯ oc get po/csi-addons-controller-manager-7886c6d5c9-f2vwh -o jsonpath='{.spec.containers[*].name}'
manager

Regards

Madhu-1 · 2024-12-04T15:55:46Z

cmd/manager/main.go

+		TLSOpts:       tlsOpts,
+	}
+
+	if secureMetrics {


why someone will use insecure metrics, cant we always use secureMetircs?

It is enabled by default. The reason why it is controlled by a flag of its own is, in prod env it is recommended to setup proper certs and keys before opting in for secure metrics.

If someone does not want to do that and just wants to check something, they may disable it.

can you document the steps to provide certificates, so that production deployments may replace automatic generated ones?

Madhu-1 · 2024-12-04T15:56:41Z

cmd/manager/main.go

@@ -128,8 +135,27 @@ func main() {
 		os.Exit(1)
 	}

+	disableHTTP2 := func(config *tls.Config) {


Is this one for test clusters or means for production clusters also?

Production clusters as well. As of now, curl misses support for HTTP2 on some distros. We can enable it by default once we are sure that all the clients support HTTP2.

Kubebuilder's default scaffolding also follows the same procedure as of now.

nixpanic

looks good overall, just a few nits regarding comments and documentation

nixpanic · 2024-12-09T16:44:33Z

cmd/manager/main.go

@@ -128,8 +135,27 @@ func main() {
 		os.Exit(1)
 	}

+	disableHTTP2 := func(config *tls.Config) {


please leave a comment in the code why it is disabled by default

nixpanic · 2024-12-09T16:48:33Z

cmd/manager/main.go

+		TLSOpts:       tlsOpts,
+	}
+
+	if secureMetrics {


can you document the steps to provide certificates, so that production deployments may replace automatic generated ones?

Madhu-1

on other thought, Do we need the metrics? if not lets disable it?

black-dragon74 · 2024-12-11T12:13:43Z

on other thought, Do we need the metrics? if not lets disable it?

It is enabled in all of the other projects by default. Still, we could disable it if not needed. Thoughts @nixpanic ?

nixpanic · 2024-12-11T15:03:32Z

on other thought, Do we need the metrics? if not lets disable it?

It is enabled in all of the other projects by default. Still, we could disable it if not needed. Thoughts @nixpanic ?

Metrics enabled by default is fine, I think.

However, the metrics should be useful though. Not sure what the metrics contain, and if there is anything related to the CSI-Addons procedures. If it is only minor things, calling it "metrics for kubernetes-csi-addons" would not really be correct. In that case, the usefulness for the project is little, and disabling sets more appropriate expectations for users.

black-dragon74 · 2024-12-11T15:14:41Z

However, the metrics should be useful though. Not sure what the metrics contain, and if there is anything related to the CSI-Addons procedures.

It includes these metrics and also the metrics for all the CRs that are reconciled by the controllers.

nixpanic · 2024-12-12T07:48:27Z

However, the metrics should be useful though. Not sure what the metrics contain, and if there is anything related to the CSI-Addons procedures.

It includes these metrics and also the metrics for all the CRs that are reconciled by the controllers.

In that case, disable it by default. These are not metrics about CSI-Addons procedures, it is too incomplete for that. Once we add metrics about the different CSI-Addons procedures, it can be enabled by default.

black-dragon74 · 2024-12-17T10:55:40Z

The metrics service is now disabled by default (by setting its bind address to 0). In order to enable it,

Set metrics-bind-address=:8443 flag for csi-addons controller pod.
Modify config/manager/kustomization.yaml and uncomment the lines 13-18:

#- metrics_service.yaml

#patches:
#- path: manager_metrics_patch.yaml
#  target:
#    kind: Deployment

For necessary RBACs uncomment the lines 27-30 in config/rbac/kustomization.yaml

  # The following RBAC configurations are used to protect
  # the metrics endpoint with authn/authz. These configurations
  # ensure that only authorized users and service accounts
  # can access the metrics endpoint.
  # - metrics_auth_role.yaml
  # - metrics_auth_role_binding.yaml
  # - metrics_reader_role.yaml
  # - metrics_reader_role_binding.yaml

cc @nixpanic @Madhu-1

Madhu-1 · 2024-12-17T13:43:32Z

cmd/manager/main.go

@@ -138,7 +174,8 @@ func main() {
 		RenewDeadline:          &leaderElectionRenewDeadline,
 		RetryPeriod:            &leaderElectionRetryPeriod,
 		WebhookServer: webhook.NewServer(webhook.Options{
-			Port: 9443,
+			Port:    9443,


we might not need webhook server as well, lets check and address this in followup as well.

This patch drops support for kube-rbac-proxy and uses controller manager's WithAuthenticationAndAuthorization. Closes: csi-addons#643 Signed-off-by: Niraj Yadav <[email protected]>

Signed-off-by: Niraj Yadav <[email protected]>

mergify bot added the vendor Pull requests that update vendored dependencies label Dec 4, 2024

mergify bot requested review from iPraveenParihar, Madhu-1, nixpanic, Rakshith-R and yati1998 December 4, 2024 14:00

Madhu-1 reviewed Dec 4, 2024

View reviewed changes

nixpanic requested changes Dec 9, 2024

View reviewed changes

Madhu-1 reviewed Dec 11, 2024

View reviewed changes

black-dragon74 force-pushed the migrate-kube-proxy branch 3 times, most recently from 343da07 to 65189b6 Compare December 17, 2024 10:52

black-dragon74 changed the title ~~Discontinue kube-rbac-proxy~~ Discontinue kube-rbac-proxy and disable metrics service Dec 17, 2024

nixpanic approved these changes Dec 17, 2024

View reviewed changes

nixpanic requested a review from Madhu-1 December 17, 2024 13:34

Madhu-1 approved these changes Dec 17, 2024

View reviewed changes

black-dragon74 added 2 commits December 17, 2024 13:45

Discontinue kube-rbac-proxy

08d9c4f

This patch drops support for kube-rbac-proxy and uses controller manager's WithAuthenticationAndAuthorization. Closes: csi-addons#643 Signed-off-by: Niraj Yadav <[email protected]>

Update vendor artifacts

2142a14

Signed-off-by: Niraj Yadav <[email protected]>

black-dragon74 force-pushed the migrate-kube-proxy branch from 65189b6 to 2142a14 Compare December 17, 2024 13:45

mergify bot merged commit 1e99ab9 into csi-addons:main Dec 17, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Discontinue kube-rbac-proxy and disable metrics service #721

Discontinue kube-rbac-proxy and disable metrics service #721

black-dragon74 commented Dec 4, 2024 •

edited

Loading

black-dragon74 commented Dec 4, 2024

Madhu-1 Dec 4, 2024

black-dragon74 Dec 9, 2024

nixpanic Dec 9, 2024

black-dragon74 Dec 17, 2024

Madhu-1 Dec 4, 2024

black-dragon74 Dec 9, 2024

nixpanic left a comment

nixpanic Dec 9, 2024

black-dragon74 Dec 17, 2024

nixpanic Dec 9, 2024

Madhu-1 left a comment

black-dragon74 commented Dec 11, 2024

nixpanic commented Dec 11, 2024

black-dragon74 commented Dec 11, 2024

nixpanic commented Dec 12, 2024

black-dragon74 commented Dec 17, 2024 •

edited

Loading

Madhu-1 Dec 17, 2024

Discontinue kube-rbac-proxy and disable metrics service #721

Discontinue kube-rbac-proxy and disable metrics service #721

Conversation

black-dragon74 commented Dec 4, 2024 • edited Loading

black-dragon74 commented Dec 4, 2024

Testing

1. Access /metrics endpoint as controller-manager's SA

2. Get the metrics using valid SA token

3. Get the metrics using invalid SA token

4. Check POD's containers (no kube-rbac-proxy-container)

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

nixpanic left a 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

Madhu-1 left a comment

Choose a reason for hiding this comment

black-dragon74 commented Dec 11, 2024

nixpanic commented Dec 11, 2024

black-dragon74 commented Dec 11, 2024

nixpanic commented Dec 12, 2024

black-dragon74 commented Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

black-dragon74 commented Dec 4, 2024 •

edited

Loading

1. Access `/metrics` endpoint as controller-manager's SA

black-dragon74 commented Dec 17, 2024 •

edited

Loading