[opentelemetry-operator] Remove kube-rbac-proxy and use built-in metrics auth#2004
[opentelemetry-operator] Remove kube-rbac-proxy and use built-in metrics auth#2004YASHMAHAKAL wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the kube-rbac-proxy sidecar dependency and migrates to controller-runtime's built-in metrics authentication, aligning with upstream OpenTelemetry Operator v0.142.0+ changes.
Key Changes
- Removed kube-rbac-proxy sidecar container from the deployment
- Replaced
kubeRBACProxyconfiguration section withmetricsServerconfiguration - Added support for
--metrics-secureflag and TLS configuration options for built-in authentication
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| charts/opentelemetry-operator/values.yaml | Replaced kubeRBACProxy config with metricsServer config (secure, port, tls options) |
| charts/opentelemetry-operator/values.schema.json | Updated JSON schema to reflect metricsServer structure and validation rules |
| charts/opentelemetry-operator/templates/deployment.yaml | Removed kube-rbac-proxy container, added metrics-secure and TLS flags to manager container |
| charts/opentelemetry-operator/templates/service.yaml | Simplified service to use single metrics port based on metricsServer.secure setting |
| charts/opentelemetry-operator/templates/clusterrole.yaml | Changed RBAC condition from kubeRBACProxy.enabled to metricsServer.secure, removed proxy-specific ClusterRole |
| charts/opentelemetry-operator/templates/clusterrolebinding.yaml | Removed proxy-specific ClusterRoleBinding |
| charts/opentelemetry-operator/templates/tests/test-service-connection.yaml | Updated test to conditionally use metricsServer.port or manager.ports.metricsPort based on secure mode |
| charts/opentelemetry-operator/examples//rendered/.yaml | Regenerated examples reflecting removal of kube-rbac-proxy container and RBAC resources |
| charts/opentelemetry-operator/UPGRADING.md | Added migration guide from kubeRBACProxy to metricsServer configuration |
| charts/opentelemetry-operator/Chart.yaml | Bumped chart version to 0.103.0 and appVersion to 0.142.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Switching to draft until CoPilot is done reviewing. |
|
@TylerHelmuth Copilot review is complete. I've already addressed the port default issue (commit d9931e6). The remaining comments are style suggestions. Should I address them all, or would you like to review first? because i'm not much sure as first two are regarding UPGRADING.md... |
|
@YASHMAHAKAL close any copilot reviews you're ignoring and then mark the PR ready for review when you want us to review it. I dont want to read through copilot comments. |
|
@TylerHelmuth , i apologize for inconvinience, i noticed same and have turned off specific settings !! : ) |
|
Greetings @TylerHelmuth, Sir, could you please review mine changes, i'm waiting for our feedback : ) |
|
@YASHMAHAKAL please fix CI failures. |
@TylerHelmuth, Thanks for review, i'm working on it ! |
jaronoff97
left a comment
There was a problem hiding this comment.
Should be good @pavolloffay mind taking a look?
|
Thanks @pavolloffay, @jaronoff97 !! |
rogercoll
left a comment
There was a problem hiding this comment.
Should we bump opentelemetry-collector-k8s image to 0.142.0 as well? https://github.com/open-telemetry/opentelemetry-helm-charts/pull/2004/files#diff-56c52957e9b80d7dcb9b9771e329d7dc4f1a3ee8fb22acdcb3a75c1c92e75c18R53
|
@rogercoll yes i think we should @YASHMAHAKAL apologies one more change |
Thanks @jaronoff97, it's absolutely okay ! i'll be updating the PR with necessary changes : ) |
|
@jaronoff97, @rogercoll, i've made desired changes, please take a look : ) |
|
@YASHMAHAKAL, @swiatekm are going to merge the PRs to bump the release numbers after which you can rebase and we can have a focused diff on just removing the rbac proxy. Please bear with us while we get these through, thank you for your patience! |
|
@jaronoff97, sorry again for delayed responses, yeah I will be fixing this today itself 🙂 |
|
@YASHMAHAKAL you should be good to rebase now, we have bumped the chart all the way up to latest |
|
@jaronoff97 one question, what about port missmatch ? should i change to 8443 or just a rebase ? |
Signed-off-by: YASHMAHAKAL <yvsst01@gmail.com>
Signed-off-by: YASHMAHAKAL <yvsst01@gmail.com>
37b1536 to
b286583
Compare
Signed-off-by: YASHMAHAKAL <yvsst01@gmail.com>
b286583 to
4e36599
Compare
|
@jaronoff97 @eenchevlp Rebased onto main and updated configuration to use port 8443 with secure: true, examples regenerated to reflect correct metrics configuration and added a clusterrole too.. change to 8443 will fix the failing tests, let me know if anything, Thanks ! |
jaronoff97
left a comment
There was a problem hiding this comment.
@YASHMAHAKAL PR looks good, but youll need to bump the Chart version. Given this is technically a breaking change, please bump the minor version instead of the patch version
okay @jaronoff97, Thanks ! |
|
@YASHMAHAKAL you'll need to revert the changes from 6c98bba to get the e2e tests to pass. |
Remove invalid kubeRBACProxy configuration from workflow. Regenerate examples with new version labels. Signed-off-by: YASHMAHAKAL <yvsst01@gmail.com>
| --set "manager.extraEnvs[0].valueFrom.fieldRef.fieldPath=metadata.namespace" \ | ||
| --set "kubeRBACProxy.enabled=false" \ | ||
| --set "manager.ports.metricsPort=8443" | ||
| --set "manager.extraEnvs[0].valueFrom.fieldRef.fieldPath=metadata.namespace" |
There was a problem hiding this comment.
@swiatekm, i have reverted the changes as per your recommendation.....
| namespace: default | ||
| labels: | ||
| helm.sh/chart: opentelemetry-operator-0.105.0 | ||
| helm.sh/chart: opentelemetry-operator-0.106.0 |
There was a problem hiding this comment.
@jaronoff97 minor version bumped to 0.106.0 as per your recommendation, Thanks !
|
Nice to see greenery in checks section 😃 |
swiatekm
left a comment
There was a problem hiding this comment.
I'd prefer if we put the new config section under manager and only had one way of setting the metrics port. I've left some comments with suggestions.
Notably, if we're enabling HTTPS by default, we also need to deal with #2063 by configuring the ServiceMonitor appropriately.
| ## Metrics server configuration | ||
| ## The operator uses controller-runtime built-in auth for the metrics server | ||
| ## Authentication is disabled by default for backward compatibility | ||
| metricsServer: |
There was a problem hiding this comment.
Is there a reason we're putting this in a new section? Intuitively, this should be under a path like manager.metrics, since it's really just configuring metrics for the operator binary. @jaronoff97 WDYT?
| name: manager | ||
| imagePullPolicy: {{ .Values.manager.image.imagePullPolicy }} | ||
| ports: | ||
| - containerPort: {{ .Values.manager.ports.metricsPort }} |
There was a problem hiding this comment.
This port should be conditional on the settings as well.
|
|
||
| # Port for the metrics server | ||
| # Use 8443 when secure is true, 8080 when false | ||
| port: 8443 |
There was a problem hiding this comment.
Can we not use manager.ports.metrics instead? That would remove a lot of the conditional logic we have around ports and we wouldn't have two sources of truth for them. The HTTPS settings could go into a separate section, like manager.metrics.
| - --metrics-addr=0.0.0.0:{{ .Values.manager.ports.metricsPort }} | ||
| - --metrics-addr=0.0.0.0:{{ if .Values.metricsServer.secure }}{{ .Values.metricsServer.port }}{{ else }}{{ .Values.manager.ports.metricsPort }}{{ end }} | ||
| {{- if .Values.metricsServer.secure }} | ||
| - --metrics-secure=true |
There was a problem hiding this comment.
Sorry for butting in but I've stumbled upon this PR as I'm interested in it.
I'm wondering whether this should be passed always as such:
`
- --metrics-secure={{ .Values.metricsServer.secure }}
`
The reason being this seems to be enabled by default in the operator (https://github.com/open-telemetry/opentelemetry-operator/blob/v0.144.0/internal/config/config.go#L179). Though I'm no go developer so I may be mistaken this is the default value and isn't overridden.
There was a problem hiding this comment.
Is that a problem? We're always explicitly passing the value we want, without relying on defaults.
There was a problem hiding this comment.
Correct me if I'm wrong but if currently the default within operator itself is 'enabled' and the current state of the Helm chart code in this PR conditionally enables then there's no way to disable secure metrics endpoint with Helm chart with the implementation this PR proposes.
There was a problem hiding this comment.
Ah, I misread your comment. You're right, we should set it unconditionally.
|
Hi @swiatekm @YASHMAHAKAL , do you think anything can be done re this PR? Essentially, the chart has no proper way to configure secure metrics collection on the newer versions of operator. So there is discrepency between the chart configuration and the actual operator until this is merged. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Description
Removes kube-rbac-proxy dependency and migrates to controller-runtime's built-in metrics authentication, aligning with upstream operator v0.142.0+ changes.
Changes
kubeRBACProxyconfig withmetricsServerconfig--metrics-secureflag and TLS optionsMigration
Default behavior unchanged (metrics on port 8080). For secure metrics, set
metricsServer.secure: true, as described in UPGRADING.md.Testing
Helm lint passes
Templates render correctly
Examples regenerated
Tested on local cluster (Minikube)
Fixes #1999