-
Notifications
You must be signed in to change notification settings - Fork 141
feat: expose and secure controller-runtime metrics #1369
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
base: main
Are you sure you want to change the base?
feat: expose and secure controller-runtime metrics #1369
Conversation
6e81c7e to
6058c96
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR secures the controller-runtime metrics endpoint by enabling built-in HTTPS and RBAC authentication/authorization, eliminating the need for kube-rbac-proxy. The metrics endpoint is changed from port 8080 to 8443 with TLS enabled.
- Changed metrics endpoint from HTTP on port 8080 to HTTPS on port 8443 with built-in authentication and authorization
- Added RBAC permissions for TokenReviews and SubjectAccessReviews to enable metrics endpoint authentication
- Added comprehensive E2E tests to verify secured metrics endpoint functionality
Reviewed Changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| operator/main.go | Enables secure serving on metrics endpoint (port 8443) with authentication and authorization filters |
| config/operator/deployment.yaml | Updates metrics container port from 8080 to 8443 |
| config/operator/role.yaml | Adds permissions for authentication.k8s.io and authorization.k8s.io APIs required for metrics authentication |
| config/operator/role_binding.yaml | Adds system:auth-delegator ClusterRoleBinding and updates service account references |
| config/operator/metrics_service.yaml | Creates new Service resource to expose secured metrics endpoint |
| config/operator/metrics_reader_role.yaml | Defines ClusterRole for reading metrics via non-resource URLs |
| config/operator/kustomization.yaml | Includes new metrics service and reader role resources |
| tests/checks/operator_metrics/operator_metrics_test.go | Adds E2E tests to verify HTTPS metrics endpoint and RBAC authorization |
| tests/utils/setup_test.go | Fixes missing selector labels in opentelemetry-collector deployment |
| go.mod | Adds new dependencies for authentication/authorization features |
| go.sum | Updates checksums for new and updated dependencies |
| CHANGELOG.md | Documents removal of kube-rbac-proxy in v0.12.0 |
| .gitignore | Adds *.test to ignore compiled test binaries |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
| Status | Scanner | Total (0) | ||||
|---|---|---|---|---|---|---|
| Open Source Security | 0 | 0 | 0 | 0 | See details |
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.
dbf3112 to
2c85d48
Compare
|
@wozniakjan : Could you please help with this Snyk security check? I can't see any report. The end2end tests on my local system are working. |
operator/main.go
Outdated
| SecureServing: true, | ||
| FilterProvider: filters.WithAuthenticationAndAuthorization, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a user perspective I would like this to be configurable. I definitely understand that there can be a requirement for a secure and authenticated metrics endpoint but think that simple setups and developer environments would prefer a plain HTTP endpoint.
KEDA also still has an HTTP metric endpoints.
How about two separate flags like --metrics-secure and --metrics-auth as these are separate concerns?
I think it would also make sense to allow configuring the certificate directory, e.g. with --metrics-cert-dir?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old kube-proxy endpoint is 8443 by default. Therefore I thought to also add this to the addon itself. But your points are reasonable to me and I will add the proposed changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@linkvt What do you think regarding the default behavior, secure==true and auth==true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the proposed changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I would prefer a metrics endpoint that is using HTTP by default as basically all the tools I used in the last few years do it like that.
This would also be the way keda itself does it by default and we would also avoid a breaking change for users using it.
I'm not a maintainer or regular contributor though (yet) but I think the argument is still valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will have a look on how it's done on keda.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8080 is default now
7d87dcd to
589f823
Compare
Signed-off-by: Karsten Ludwig Hauser <KHauser@intershop.com>
…orization - Updated operator/main.go to configure metrics server with SecureServing on port 8443 - Added WithAuthenticationAndAuthorization filter for metrics endpoint - Updated deployment to use port 8443 for metrics - Created metrics service for operator - Added RBAC permissions for TokenReviews and SubjectAccessReviews - Created ClusterRole for metrics reader access - Added e2e test for operator metrics endpoint - Updated go.mod and go.sum with required dependencies Co-authored-by: khauser <1460475+khauser@users.noreply.github.com> Signed-off-by: Karsten Ludwig Hauser <KHauser@intershop.com>
…ments Co-authored-by: khauser <1460475+khauser@users.noreply.github.com> Signed-off-by: Karsten Ludwig Hauser <KHauser@intershop.com>
Added release notes for version 0.12.0, including improvements. Signed-off-by: Karsten Ludwig Hauser <khauser@users.noreply.github.com> Signed-off-by: Karsten Ludwig Hauser <KHauser@intershop.com>
Signed-off-by: Karsten Ludwig Hauser <KHauser@intershop.com>
Signed-off-by: Karsten Ludwig Hauser <KHauser@intershop.com>
Signed-off-by: Karsten Ludwig Hauser <KHauser@intershop.com>
Signed-off-by: Karsten Ludwig Hauser <KHauser@intershop.com>
589f823 to
7a07b38
Compare
Signed-off-by: Karsten Ludwig Hauser <KHauser@intershop.com>
7a07b38 to
1ba7fa4
Compare
According to https://book.kubebuilder.io/reference/metrics.html the metrics endpoint could be secured by the controller-runtime itself without utilizing kube-rbac-proxy.
Checklist
Fixes #1123