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.

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

Remove usage of kube-rbac-proxy and switch to the built-in WithAuthenticationAndAuthorization filter instead #338

Conversation

rm3l
Copy link
Member

@rm3l rm3l commented Oct 15, 2024

Description

This removes all usage of the kube-rbac-proxy sidecar container, which was used to protect the /metrics endpoint exposed by the controller. This ensured that only authorized users and service accounts could access potentially sensitive metrics data.
Instead, it is now recommended to leverage the built-in WithAuthenticationAndAuthorization filter which provides a similar authn/authz protection of this endpoint but without the need for an extra container.
More context in kubernetes-sigs/controller-runtime#2073 and https://book.kubebuilder.io/reference/metrics#how-the-metrics-endpoint-can-be-protected-

Which issue(s) does this PR fix or relate to

PR acceptance criteria

How to test changes / Special notes to the reviewer

From this PR:

# Download Ginkgo locally if needed
$ make ginkgo
$ IMG=quay.io/rhdh-community/operator:0.4.0-pr-338 \
  ./bin/ginkgo -v -p -focus 'operator metrics endpoint' tests/e2e

[...]
Ran 3 of 9 Specs in 48.442 seconds
SUCCESS! -- 3 Passed | 0 Failed | 0 Pending | 6 Skipped

Ginkgo ran 1 suite in 50.800777439s
Test Suite Passed

@rm3l rm3l force-pushed the RHIDP-4236--remove-kube-rbac-proxy-and-use-WithAuthenticationAndAuthorization-instead branch from 5f9d5fd to bed266f Compare October 15, 2024 12:52
@rm3l rm3l force-pushed the RHIDP-4236--remove-kube-rbac-proxy-and-use-WithAuthenticationAndAuthorization-instead branch from bed266f to 378f723 Compare October 15, 2024 13:37
@rm3l rm3l changed the title [WIP] Remove the use of kube-rbac-proxy and switch to the built-in WithAuthenticationAndAuthorization filter instead Remove the use of kube-rbac-proxy and switch to the built-in WithAuthenticationAndAuthorization filter instead Oct 16, 2024
@rm3l rm3l force-pushed the RHIDP-4236--remove-kube-rbac-proxy-and-use-WithAuthenticationAndAuthorization-instead branch from 2f3c58d to 8195a4e Compare October 16, 2024 13:21
@rm3l rm3l force-pushed the RHIDP-4236--remove-kube-rbac-proxy-and-use-WithAuthenticationAndAuthorization-instead branch from 8195a4e to d4fdab7 Compare October 16, 2024 13:31
@rm3l rm3l marked this pull request as ready for review October 16, 2024 14:04
@openshift-ci openshift-ci bot requested a review from gazarenkov October 16, 2024 14:04
@openshift-ci openshift-ci bot requested a review from kim-tsao October 16, 2024 14:04
@rm3l
Copy link
Member Author

rm3l commented Oct 16, 2024

@openshift-ci openshift-ci bot requested a review from Fortune-Ndlovu October 16, 2024 14:05
@openshift-ci openshift-ci bot requested a review from nickboldt October 16, 2024 14:05
@rm3l rm3l changed the title Remove the use of kube-rbac-proxy and switch to the built-in WithAuthenticationAndAuthorization filter instead Remove usage of kube-rbac-proxy and switch to the built-in WithAuthenticationAndAuthorization filter instead Oct 16, 2024
Copy link
Member

@nickboldt nickboldt left a comment

Choose a reason for hiding this comment

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

seems legit though did not test (or fully grok the changes).

rm3l added 4 commits October 16, 2024 22:53
…dAuthorization` filter

This is enabled only if `--metrics-over-https` is enabled,
which is done to avoid passing service account tokens over HTTP.
Metrics serving over HTTPS is disabled by default (when running the operator locally),
but enabled when deployed either with or without OLM.

Ref: https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/metrics/filters#WithAuthenticationAndAuthorization
@rm3l rm3l force-pushed the RHIDP-4236--remove-kube-rbac-proxy-and-use-WithAuthenticationAndAuthorization-instead branch from d4fdab7 to 430941d Compare October 16, 2024 20:58
@openshift-ci openshift-ci bot removed the lgtm label Oct 16, 2024
Copy link

openshift-ci bot commented Oct 16, 2024

New changes are detected. LGTM label has been removed.

@rm3l
Copy link
Member Author

rm3l commented Oct 16, 2024

Rebased and forced-push to fix Git conflicts reported.

@gazarenkov gazarenkov self-requested a review October 17, 2024 14:28
Copy link

openshift-ci bot commented Oct 17, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gazarenkov, nickboldt

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 [gazarenkov,nickboldt]

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

@rm3l rm3l merged commit 823ae0d into redhat-developer:main Oct 17, 2024
6 of 7 checks passed
@rm3l rm3l deleted the RHIDP-4236--remove-kube-rbac-proxy-and-use-WithAuthenticationAndAuthorization-instead branch October 17, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants