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.

Sign up for GitHub

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

Jump to bottom

Discontinue kube-rbac-proxy #721

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Discontinue kube-rbac-proxy #721

wants to merge 2 commits into from
+189,404 −180

Conversation

black-dragon74
Copy link
Member

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.

@black-dragon74
Discontinue kube-rbac-proxy
4bfc2be 
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]>
@black-dragon74
Update vendor artifacts
16fb3dd 
Signed-off-by: Niraj Yadav <[email protected]>
@mergify mergify bot added the vendor Pull requests that update vendored dependencies label Dec 4, 2024
@black-dragon74
Copy link
Member Author

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
Madhu-1 reviewed Dec 4, 2024
View reviewed changes
cmd/manager/main.go
TLSOpts: tlsOpts,
}

if secureMetrics {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

disableHTTP2 := func(config *tls.Config) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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
nixpanic requested changes Dec 9, 2024
View reviewed changes
Copy link
Collaborator

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

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

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

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

disableHTTP2 := func(config *tls.Config) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

cmd/manager/main.go
TLSOpts: tlsOpts,
}

if secureMetrics {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Madhu-1
Madhu-1 reviewed Dec 11, 2024
View reviewed changes
Copy link
Member

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

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

@black-dragon74
Copy link
Member Author

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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Madhu-1 Madhu-1 Madhu-1 left review comments

@nixpanic nixpanic nixpanic requested changes

@iPraveenParihar iPraveenParihar Awaiting requested review from iPraveenParihar

@Rakshith-R Rakshith-R Awaiting requested review from Rakshith-R

@yati1998 yati1998 Awaiting requested review from yati1998

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
vendor Pull requests that update vendored dependencies
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider using NetworkPolicies to allow/deny access to the CSI-driver sidecar
3 participants
@black-dragon74 @nixpanic @Madhu-1