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

✨ Add protection to metrics endpoint using authn/authz via controller-runtime feature #4003

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented Jul 4, 2024

This PR re-introduces authentication and authorization protection for the metrics endpoint using WithAuthenticationAndAuthorization provided by controller-runtime.

We are re-introducing this protection, which is similar to what was previously done via kube-rbac-proxy, whose usage was discontinued in the project.

PS.:: Address item 3 mapped in : #3871

Author: Joe Lanford joe.lanford@gmail.com
Co-Author: Camila Macedo

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 4, 2024
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 4, 2024
…untime feature

This PR re-introduce authn/authz protection for the endpoint but without
use the kube-rbac-proxy project and image.

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
@camilamacedo86
Copy link
Member Author

/skip APIDiff

/override APIDiff

@k8s-ci-robot
Copy link
Contributor

@camilamacedo86: /override requires failed status contexts, check run or a prowjob name to operate on.
The following unknown contexts/checkruns were given:

  • APIDiff

Only the following failed contexts/checkruns were expected:

  • EasyCLA
  • netlify/kubebuilder/deploy-preview
  • pull-kubebuilder-e2e-k8s-1-28-7
  • pull-kubebuilder-e2e-k8s-1-29-2
  • pull-kubebuilder-e2e-k8s-1-30-0
  • pull-kubebuilder-test
  • tide

If you are trying to override a checkrun that has a space in it, you must put a double quote on the context.

In response to this:

/skip APIDiff

/override APIDiff

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-sigs/prow repository.

Copy link
Contributor

@Kavinjsir Kavinjsir left a comment

Choose a reason for hiding this comment

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

lgtm 👍🏼

// These configurations ensure that only authorized users and service accounts
// can access the metrics endpoint. The RBAC are configured in 'config/rbac/kustomization.yaml'. More info:
// https://pkg.go.dev/sigs.k8s.io/controller-runtime@{{ .ControllerRuntimeVersion }}/pkg/metrics/filters#WithAuthenticationAndAuthorization
FilterProvider: filters.WithAuthenticationAndAuthorization,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nits:
Similar to the concern in the previous PR, would it be necessary/good to remind user of assigning the cluster role to the clients with the permission to scrape the metrics?
See the ref:

To scrape metrics e.g. via Prometheus the client needs a ClusterRole with the following rule: * nonResourceURLs: "/metrics", verbs: get

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that the problem is that depends on how the Prometheus is configured.
We might could say something like Prometheus requires access to the metrics endpoint.
But we are either linking the metrics doc so users can check that, there we are clarifying it right.

Anyway, lets to do one thing:
Since it is a nit, lets move forward with as it is, however, if you see any good way to improve the comments/docs etc, please feel free to push a PR with your idea and we can shape things around.

Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

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

I did a quick check by checking out the branch and running the example locally. It does work from my end.

Thank you for the contribution!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 5, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, Kavinjsir, varshaprasad96

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 [Kavinjsir,camilamacedo86,varshaprasad96]

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

@camilamacedo86 camilamacedo86 merged commit de1cc60 into kubernetes-sigs:master Jul 5, 2024
19 of 21 checks passed
// These configurations ensure that only authorized users and service accounts
// can access the metrics endpoint. The RBAC are configured in 'config/rbac/kustomization.yaml'. More info:
// https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.18.4/pkg/metrics/filters#WithAuthenticationAndAuthorization
FilterProvider: filters.WithAuthenticationAndAuthorization,
Copy link
Member

Choose a reason for hiding this comment

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

I think this should only be set if secureMetrics is true.

By always enabling this folks have to always provide a ServiceAccount token. But these tokens should never be send via HTTP

Copy link
Member Author

Choose a reason for hiding this comment

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

we can address this one good catcher.

aleskandro added a commit to aleskandro/multiarch-manager-operator that referenced this pull request Sep 6, 2024
Images provided under gcr.io/kubebuilder/ will be unavailable from March 18, 2025.
Projects initialized with Kubebuilder versions v3.14 or lower utilize gcr.io/kubebuilder/kube-rbac-proxy to protect the metrics endpoint.

Following the work in kubernetes-sigs/kubebuilder#4003, this commit removes the kube-rbac-proxy container and let the main container of the controller expose the metrics via HTTPS and by using the WithAuthenticatoinAndAuthorization filter.

This also includes a minor fix in BuildService escaped during the resolution of some conflicts during a rebase.

Related to kubernetes-sigs/kubebuilder#3871
aleskandro added a commit to aleskandro/multiarch-manager-operator that referenced this pull request Sep 6, 2024
Images provided under gcr.io/kubebuilder/ will be unavailable from March 18, 2025.
Projects initialized with Kubebuilder versions v3.14 or lower utilize gcr.io/kubebuilder/kube-rbac-proxy to protect the metrics endpoint.

Following the work in kubernetes-sigs/kubebuilder#4003, this commit removes the kube-rbac-proxy container and let the main container of the controller expose the metrics via HTTPS and by using the WithAuthenticatoinAndAuthorization filter.

This also includes a minor fix in BuildService escaped during the resolution of some conflicts during a rebase.

Related to kubernetes-sigs/kubebuilder#3871
aleskandro added a commit to aleskandro/multiarch-manager-operator that referenced this pull request Sep 6, 2024
Images provided under gcr.io/kubebuilder/ will be unavailable from March 18, 2025.
Projects initialized with Kubebuilder versions v3.14 or lower utilize gcr.io/kubebuilder/kube-rbac-proxy to protect the metrics endpoint.

Following the work in kubernetes-sigs/kubebuilder#4003, this commit removes the kube-rbac-proxy container and let the main container of the controller expose the metrics via HTTPS and by using the WithAuthenticatoinAndAuthorization filter.

This also includes a minor fix in BuildService escaped during the resolution of some conflicts during a rebase.

Related to kubernetes-sigs/kubebuilder#3871
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants