fix: replace non-expiring metrics monitor SA token with TokenRequest#1215
Open
tzprograms wants to merge 1 commit into
Open
fix: replace non-expiring metrics monitor SA token with TokenRequest#1215tzprograms wants to merge 1 commit into
tzprograms wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @tzprograms. Thanks for your PR. I'm waiting for a redhat-developer member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change introduces a dedicated controller for renewable metrics bearer tokens, updates ServiceMonitor authentication, removes the previous metrics reconciliation path, wires the controller into runtime and e2e managers, and adds unit and end-to-end validation. ChangesMetrics token authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ServiceMonitor
participant OperatorMetricsTokenReconciler
participant TokenRequest
participant Secret
ServiceMonitor->>OperatorMetricsTokenReconciler: reconcile target monitor
OperatorMetricsTokenReconciler->>ServiceMonitor: configure Bearer authorization
OperatorMetricsTokenReconciler->>TokenRequest: request service-account token
TokenRequest-->>OperatorMetricsTokenReconciler: return token and expiry
OperatorMetricsTokenReconciler->>Secret: persist token and expiry
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Migrate ServiceMonitor from deprecated bearerTokenSecret to authorization and manage a short-lived Opaque bearer token Secret via TokenRequest. Signed-off-by: Tejas Soham <tejassoham05@gmail.com>
tzprograms
force-pushed
the
fix/non-expiring-metrics-token-pr
branch
from
4625e6c to
b36fc1e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controllers/operator_metrics_controller.go`:
- Around line 266-275: Update the legacySAToken branch to replace the existing
Secret in a single API operation: copy the desiredSecret type and data onto
secret, then persist it with the client update method. Remove the separate
Delete and Create calls while preserving the existing error handling, logging,
and requeue behavior.
- Around line 97-105: Update SetupWithManager to watch the bearer-token Secret
in addition to the filtered ServiceMonitor, mapping events for the specific
managed Secret to the operatorMetricsMonitorName ServiceMonitor reconcile key.
Preserve the existing ServiceMonitor predicate and reconciliation target while
adding the Secret-to-ServiceMonitor event mapping.
- Around line 225-239: The bearer-token validation branch must verify the stored
token before treating its expiry as valid. In the logic around
parseBearerTokenExpiry, require secret.Type to be SecretTypeOpaque and the token
data to be non-empty before returning requeueAfter; otherwise set needsRefresh
so renewal occurs.
- Line 107: The TokenRequest RBAC restriction declared by the kubebuilder marker
is not preserved in the shipped manifests. Update the generated
config/rbac/role.yaml and
bundle/manifests/gitops-operator.clusterserviceversion.yaml outputs so the
serviceaccounts/token rule includes resourceNames limited to
openshift-gitops-operator-controller-manager, matching the marker in
controllers/operator_metrics_controller.go.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 58e0aba7-cf79-41c3-a047-25cb8e2dc45c
📒 Files selected for processing (12)
bundle/manifests/gitops-operator.clusterserviceversion.yamlbundle/manifests/openshift-gitops-operator-metrics-monitor-bearer-token_v1_secret.yamlbundle/manifests/openshift-gitops-operator-metrics-monitor_monitoring.coreos.com_v1_servicemonitor.yamlcmd/main.goconfig/prometheus/monitor.yamlcontrollers/argocd_controller.gocontrollers/argocd_metrics_controller.gocontrollers/operator_metrics_controller.gocontrollers/operator_metrics_controller_test.gotest/e2e/suite_test.gotest/nondefaulte2e/suite_test.gotest/openshift/e2e/ginkgo/parallel/1-104_validate_prometheus_alert_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
argoproj-labs/argocd-operator(manual)
💤 Files with no reviewable changes (1)
- bundle/manifests/openshift-gitops-operator-metrics-monitor-bearer-token_v1_secret.yaml
Comment on lines
+97
to
+105
| func (r *OperatorMetricsTokenReconciler) SetupWithManager(mgr ctrl.Manager) error { | ||
| return ctrl.NewControllerManagedBy(mgr). | ||
| Named("operator-metrics-token"). | ||
| For(&monitoringv1.ServiceMonitor{}). | ||
| WithEventFilter(predicate.NewPredicateFuncs(func(obj client.Object) bool { | ||
| return obj.GetName() == operatorMetricsMonitorName | ||
| })). | ||
| Complete(r) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Watch the managed Secret as well as the ServiceMonitor.
Deleting or corrupting the bearer-token Secret does not enqueue reconciliation; metrics authentication can remain broken until the existing renewal timer fires. Map events for this specific Secret to the ServiceMonitor reconcile key.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controllers/operator_metrics_controller.go` around lines 97 - 105, Update
SetupWithManager to watch the bearer-token Secret in addition to the filtered
ServiceMonitor, mapping events for the specific managed Secret to the
operatorMetricsMonitorName ServiceMonitor reconcile key. Preserve the existing
ServiceMonitor predicate and reconciliation target while adding the
Secret-to-ServiceMonitor event mapping.
Comment on lines
+225
to
+239
| } else if secret.Type == corev1.SecretTypeServiceAccountToken { | ||
| // Keep the legacy Secret until TokenRequest succeeds so scrape auth | ||
| // is not interrupted if minting fails. | ||
| legacySAToken = true | ||
| needsRefresh = true | ||
| } else { | ||
| expiry, parseErr := parseBearerTokenExpiry(secret.Data[operatorMetricsBearerTokenExpiryKey]) | ||
| if parseErr != nil || !time.Now().Before(expiry) { | ||
| needsRefresh = true | ||
| } else { | ||
| requeueAfter := bearerTokenRequeueDuration(expiry) | ||
| if requeueAfter <= 0 { | ||
| needsRefresh = true | ||
| } else { | ||
| return requeueAfter, nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate the stored token before accepting its expiry.
An Opaque Secret with a future expiry but a missing or empty token is treated as valid, leaving Prometheus unable to authenticate until renewal. Require SecretTypeOpaque and a non-empty token before returning requeueAfter.
Proposed fix
} else {
+ token := secret.Data[operatorMetricsBearerTokenKey]
expiry, parseErr := parseBearerTokenExpiry(secret.Data[operatorMetricsBearerTokenExpiryKey])
- if parseErr != nil || !time.Now().Before(expiry) {
+ if secret.Type != corev1.SecretTypeOpaque ||
+ len(token) == 0 ||
+ parseErr != nil ||
+ !time.Now().Before(expiry) {
needsRefresh = true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else if secret.Type == corev1.SecretTypeServiceAccountToken { | |
| // Keep the legacy Secret until TokenRequest succeeds so scrape auth | |
| // is not interrupted if minting fails. | |
| legacySAToken = true | |
| needsRefresh = true | |
| } else { | |
| expiry, parseErr := parseBearerTokenExpiry(secret.Data[operatorMetricsBearerTokenExpiryKey]) | |
| if parseErr != nil || !time.Now().Before(expiry) { | |
| needsRefresh = true | |
| } else { | |
| requeueAfter := bearerTokenRequeueDuration(expiry) | |
| if requeueAfter <= 0 { | |
| needsRefresh = true | |
| } else { | |
| return requeueAfter, nil | |
| } else if secret.Type == corev1.SecretTypeServiceAccountToken { | |
| // Keep the legacy Secret until TokenRequest succeeds so scrape auth | |
| // is not interrupted if minting fails. | |
| legacySAToken = true | |
| needsRefresh = true | |
| } else { | |
| token := secret.Data[operatorMetricsBearerTokenKey] | |
| expiry, parseErr := parseBearerTokenExpiry(secret.Data[operatorMetricsBearerTokenExpiryKey]) | |
| if secret.Type != corev1.SecretTypeOpaque || | |
| len(token) == 0 || | |
| parseErr != nil || | |
| !time.Now().Before(expiry) { | |
| needsRefresh = true | |
| } else { | |
| requeueAfter := bearerTokenRequeueDuration(expiry) | |
| if requeueAfter <= 0 { | |
| needsRefresh = true | |
| } else { | |
| return requeueAfter, nil |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controllers/operator_metrics_controller.go` around lines 225 - 239, The
bearer-token validation branch must verify the stored token before treating its
expiry as valid. In the logic around parseBearerTokenExpiry, require secret.Type
to be SecretTypeOpaque and the token data to be non-empty before returning
requeueAfter; otherwise set needsRefresh so renewal occurs.
Comment on lines
+266
to
+275
| if legacySAToken { | ||
| reqLogger.Info("Replacing legacy non-expiring service account token Secret", | ||
| "Namespace", namespace, "Name", operatorMetricsBearerTokenSecretName) | ||
| if err := r.Client.Delete(ctx, secret); err != nil && !errors.IsNotFound(err) { | ||
| return 0, err | ||
| } | ||
| if err := r.Client.Create(ctx, desiredSecret); err != nil { | ||
| return 0, err | ||
| } | ||
| return bearerTokenRequeueDuration(expiry), nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Replace the legacy Secret atomically.
Deleting the working Secret before creating its replacement creates a scrape outage and can leave the Secret absent if creation fails. Update the existing object’s type and data in one API operation instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controllers/operator_metrics_controller.go` around lines 266 - 275, Update
the legacySAToken branch to replace the existing Secret in a single API
operation: copy the desiredSecret type and data onto secret, then persist it
with the client update method. Remove the separate Delete and Create calls while
preserving the existing error handling, logging, and requeue behavior.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
controllers/operator_metrics_controller.go (3)
225-242: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStored token still isn't validated before trusting its expiry.
An Opaque-typed Secret with a future
expirybut empty/missingtokendata is accepted as valid, leaving Prometheus unable to authenticate until the next renewal. Require a non-empty token (andSecretTypeOpaque) before returningrequeueAfter.🔧 Proposed fix
} else { + token := secret.Data[operatorMetricsBearerTokenKey] expiry, parseErr := parseBearerTokenExpiry(secret.Data[operatorMetricsBearerTokenExpiryKey]) - if parseErr != nil || !time.Now().Before(expiry) { + if len(token) == 0 || parseErr != nil || !time.Now().Before(expiry) { needsRefresh = true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/operator_metrics_controller.go` around lines 225 - 242, Update the stored-token validation in the Secret handling branch before returning requeueAfter: only treat the token as valid when secret.Type is SecretTypeOpaque and the token data is non-empty, in addition to a parseable future expiry and positive requeue duration. Otherwise set needsRefresh and continue the renewal path.
266-276: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winLegacy Secret replacement isn't atomic.
Deleting the working Secret before creating its replacement causes a scrape outage and can leave the Secret missing entirely if
Createfails afterDeletesucceeds. Mutatesecret's type/data in place andUpdateit instead of Delete+Create.🔧 Proposed fix
- if legacySAToken { - reqLogger.Info("Replacing legacy non-expiring service account token Secret", - "Namespace", namespace, "Name", operatorMetricsBearerTokenSecretName) - if err := r.Client.Delete(ctx, secret); err != nil && !errors.IsNotFound(err) { - return 0, err - } - if err := r.Client.Create(ctx, desiredSecret); err != nil { - return 0, err - } - return bearerTokenRequeueDuration(expiry), nil - } + if legacySAToken { + reqLogger.Info("Replacing legacy non-expiring service account token Secret", + "Namespace", namespace, "Name", operatorMetricsBearerTokenSecretName) + secret.Type = desiredSecret.Type + secret.Data = desiredSecret.Data + if err := r.Client.Update(ctx, secret); err != nil { + return 0, err + } + return bearerTokenRequeueDuration(expiry), nil + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/operator_metrics_controller.go` around lines 266 - 276, Update the legacySAToken branch in the reconciler to preserve the existing Secret during replacement: copy the desired Secret type and data onto the fetched secret, then persist the mutation with r.Client.Update instead of deleting and creating separate objects. Keep the existing logging, error propagation, and requeue behavior unchanged.
97-105: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSecret changes still aren't watched.
Deleting/corrupting the bearer-token Secret doesn't enqueue reconciliation; auth stays broken until the renewal timer or an unrelated ServiceMonitor event fires. Map events for the managed Secret to the ServiceMonitor reconcile key (e.g. via
Watches+handler.EnqueueRequestsFromMapFunc).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/operator_metrics_controller.go` around lines 97 - 105, The SetupWithManager controller currently watches only the named ServiceMonitor, so managed bearer-token Secret changes do not trigger reconciliation. Add a Watches mapping for the managed Secret using handler.EnqueueRequestsFromMapFunc to enqueue the corresponding ServiceMonitor reconcile request, while preserving the existing ServiceMonitor filter and controller setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controllers/operator_metrics_controller_test.go`:
- Around line 140-176: Add a targeted SA1019 nolint directive to the assertion
accessing updatedSM.Spec.Endpoints[0].BearerTokenSecret in
TestOperatorMetricsTokenReconciler_migratesServiceMonitorAuth, preserving the
intentional deprecated-field verification while keeping the remaining assertions
unchanged.
- Around line 76-107: Add a scoped //nolint:staticcheck directive to the
deprecated BearerTokenSecret assignment inside newOperatorMetricsServiceMonitor,
limiting suppression to the intentional legacy-auth fixture while leaving the
surrounding ServiceMonitor construction unchanged.
In `@controllers/operator_metrics_controller.go`:
- Around line 170-196: Add a scoped //nolint:staticcheck directive with a brief
migration justification at the intentional endpoint.BearerTokenSecret read/clear
in the surrounding reconciliation logic, suppressing only SA1019 while
preserving the legacy-field migration behavior.
---
Duplicate comments:
In `@controllers/operator_metrics_controller.go`:
- Around line 225-242: Update the stored-token validation in the Secret handling
branch before returning requeueAfter: only treat the token as valid when
secret.Type is SecretTypeOpaque and the token data is non-empty, in addition to
a parseable future expiry and positive requeue duration. Otherwise set
needsRefresh and continue the renewal path.
- Around line 266-276: Update the legacySAToken branch in the reconciler to
preserve the existing Secret during replacement: copy the desired Secret type
and data onto the fetched secret, then persist the mutation with r.Client.Update
instead of deleting and creating separate objects. Keep the existing logging,
error propagation, and requeue behavior unchanged.
- Around line 97-105: The SetupWithManager controller currently watches only the
named ServiceMonitor, so managed bearer-token Secret changes do not trigger
reconciliation. Add a Watches mapping for the managed Secret using
handler.EnqueueRequestsFromMapFunc to enqueue the corresponding ServiceMonitor
reconcile request, while preserving the existing ServiceMonitor filter and
controller setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 381f507f-6f6c-4c21-8da6-7c78f4aaf7cc
📒 Files selected for processing (12)
bundle/manifests/gitops-operator.clusterserviceversion.yamlbundle/manifests/openshift-gitops-operator-metrics-monitor-bearer-token_v1_secret.yamlbundle/manifests/openshift-gitops-operator-metrics-monitor_monitoring.coreos.com_v1_servicemonitor.yamlcmd/main.goconfig/prometheus/monitor.yamlcontrollers/argocd_controller.gocontrollers/argocd_metrics_controller.gocontrollers/operator_metrics_controller.gocontrollers/operator_metrics_controller_test.gotest/e2e/suite_test.gotest/nondefaulte2e/suite_test.gotest/openshift/e2e/ginkgo/parallel/1-104_validate_prometheus_alert_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
argoproj-labs/argocd-operator(manual)
💤 Files with no reviewable changes (1)
- bundle/manifests/openshift-gitops-operator-metrics-monitor-bearer-token_v1_secret.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- test/e2e/suite_test.go
- bundle/manifests/openshift-gitops-operator-metrics-monitor_monitoring.coreos.com_v1_servicemonitor.yaml
- bundle/manifests/gitops-operator.clusterserviceversion.yaml
- controllers/argocd_controller.go
- test/nondefaulte2e/suite_test.go
- config/prometheus/monitor.yaml
- controllers/argocd_metrics_controller.go
- cmd/main.go
Comment on lines
+76
to
+107
| func newOperatorMetricsServiceMonitor(namespace string, useLegacyAuth bool) *monitoringv1.ServiceMonitor { | ||
| sm := &monitoringv1.ServiceMonitor{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: operatorMetricsMonitorName, | ||
| Namespace: namespace, | ||
| }, | ||
| Spec: monitoringv1.ServiceMonitorSpec{ | ||
| Endpoints: []monitoringv1.Endpoint{ | ||
| { | ||
| Interval: monitoringv1.Duration("30s"), | ||
| Path: "/metrics", | ||
| Port: "metrics", | ||
| Scheme: "https", | ||
| TLSConfig: &monitoringv1.TLSConfig{ | ||
| SafeTLSConfig: monitoringv1.SafeTLSConfig{ | ||
| ServerName: ptr.To("old-server-name"), | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
| if useLegacyAuth { | ||
| sm.Spec.Endpoints[0].BearerTokenSecret = &corev1.SecretKeySelector{ | ||
| LocalObjectReference: corev1.LocalObjectReference{ | ||
| Name: operatorMetricsBearerTokenSecretName, | ||
| }, | ||
| Key: operatorMetricsBearerTokenKey, | ||
| } | ||
| } | ||
| return sm | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Same SA1019 lint failure here - add nolint.
Building the legacy-auth fixture intentionally sets the deprecated BearerTokenSecret field, which fails the same staticcheck rule flagged in operator_metrics_controller.go. Add a scoped //nolint:staticcheck here too.
🔧 Proposed fix
if useLegacyAuth {
- sm.Spec.Endpoints[0].BearerTokenSecret = &corev1.SecretKeySelector{
+ sm.Spec.Endpoints[0].BearerTokenSecret = &corev1.SecretKeySelector{ //nolint:staticcheck // building legacy-auth fixture for migration test
LocalObjectReference: corev1.LocalObjectReference{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func newOperatorMetricsServiceMonitor(namespace string, useLegacyAuth bool) *monitoringv1.ServiceMonitor { | |
| sm := &monitoringv1.ServiceMonitor{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: operatorMetricsMonitorName, | |
| Namespace: namespace, | |
| }, | |
| Spec: monitoringv1.ServiceMonitorSpec{ | |
| Endpoints: []monitoringv1.Endpoint{ | |
| { | |
| Interval: monitoringv1.Duration("30s"), | |
| Path: "/metrics", | |
| Port: "metrics", | |
| Scheme: "https", | |
| TLSConfig: &monitoringv1.TLSConfig{ | |
| SafeTLSConfig: monitoringv1.SafeTLSConfig{ | |
| ServerName: ptr.To("old-server-name"), | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| } | |
| if useLegacyAuth { | |
| sm.Spec.Endpoints[0].BearerTokenSecret = &corev1.SecretKeySelector{ | |
| LocalObjectReference: corev1.LocalObjectReference{ | |
| Name: operatorMetricsBearerTokenSecretName, | |
| }, | |
| Key: operatorMetricsBearerTokenKey, | |
| } | |
| } | |
| return sm | |
| } | |
| func newOperatorMetricsServiceMonitor(namespace string, useLegacyAuth bool) *monitoringv1.ServiceMonitor { | |
| sm := &monitoringv1.ServiceMonitor{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: operatorMetricsMonitorName, | |
| Namespace: namespace, | |
| }, | |
| Spec: monitoringv1.ServiceMonitorSpec{ | |
| Endpoints: []monitoringv1.Endpoint{ | |
| { | |
| Interval: monitoringv1.Duration("30s"), | |
| Path: "/metrics", | |
| Port: "metrics", | |
| Scheme: "https", | |
| TLSConfig: &monitoringv1.TLSConfig{ | |
| SafeTLSConfig: monitoringv1.SafeTLSConfig{ | |
| ServerName: ptr.To("old-server-name"), | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| } | |
| if useLegacyAuth { | |
| sm.Spec.Endpoints[0].BearerTokenSecret = &corev1.SecretKeySelector{ //nolint:staticcheck // building legacy-auth fixture for migration test | |
| LocalObjectReference: corev1.LocalObjectReference{ | |
| Name: operatorMetricsBearerTokenSecretName, | |
| }, | |
| Key: operatorMetricsBearerTokenKey, | |
| } | |
| } | |
| return sm | |
| } |
🧰 Tools
🪛 GitHub Check: Ensure that code passes gosec and golint
[failure] 99-99:
SA1019: sm.Spec.Endpoints[0].BearerTokenSecret is deprecated: use authorization instead. (staticcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controllers/operator_metrics_controller_test.go` around lines 76 - 107, Add a
scoped //nolint:staticcheck directive to the deprecated BearerTokenSecret
assignment inside newOperatorMetricsServiceMonitor, limiting suppression to the
intentional legacy-auth fixture while leaving the surrounding ServiceMonitor
construction unchanged.
Source: Linters/SAST tools
Comment on lines
+140
to
+176
| func TestOperatorMetricsTokenReconciler_migratesServiceMonitorAuth(t *testing.T) { | ||
| writeOperatorNamespaceFile(t, testOperatorNamespace) | ||
|
|
||
| s := newOperatorMetricsTokenScheme() | ||
| serviceMonitor := newOperatorMetricsServiceMonitor(testOperatorNamespace, true) | ||
| c := fake.NewClientBuilder().WithScheme(s).WithObjects(serviceMonitor).Build() | ||
|
|
||
| r := &OperatorMetricsTokenReconciler{ | ||
| Client: c, | ||
| Scheme: s, | ||
| TokenRequester: &fakeTokenRequester{ | ||
| token: "test-token", | ||
| expiry: time.Now().Add(operatorMetricsTokenExpiry), | ||
| }, | ||
| } | ||
|
|
||
| result, err := r.Reconcile(context.Background(), reconcile.Request{ | ||
| NamespacedName: types.NamespacedName{ | ||
| Name: operatorMetricsMonitorName, | ||
| Namespace: testOperatorNamespace, | ||
| }, | ||
| }) | ||
| assert.NilError(t, err) | ||
| assert.Assert(t, result.RequeueAfter > 0) | ||
|
|
||
| updatedSM := &monitoringv1.ServiceMonitor{} | ||
| err = c.Get(context.Background(), types.NamespacedName{ | ||
| Name: operatorMetricsMonitorName, | ||
| Namespace: testOperatorNamespace, | ||
| }, updatedSM) | ||
| assert.NilError(t, err) | ||
| assert.Assert(t, is.Nil(updatedSM.Spec.Endpoints[0].BearerTokenSecret)) | ||
| assert.Assert(t, updatedSM.Spec.Endpoints[0].Authorization != nil) | ||
| assert.Equal(t, updatedSM.Spec.Endpoints[0].Authorization.Type, "Bearer") | ||
| assert.Equal(t, updatedSM.Spec.Endpoints[0].Authorization.Credentials.Name, operatorMetricsBearerTokenSecretName) | ||
| assert.Equal(t, *updatedSM.Spec.Endpoints[0].TLSConfig.ServerName, operatorMetricsServiceName+"."+testOperatorNamespace+".svc") | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Same SA1019 lint failure - add nolint to the assertion.
Asserting BearerTokenSecret was cleared intentionally touches the deprecated field, tripping the same staticcheck rule.
🔧 Proposed fix
- assert.Assert(t, is.Nil(updatedSM.Spec.Endpoints[0].BearerTokenSecret))
+ assert.Assert(t, is.Nil(updatedSM.Spec.Endpoints[0].BearerTokenSecret)) //nolint:staticcheck // verifying deprecated field was cleared during migration📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestOperatorMetricsTokenReconciler_migratesServiceMonitorAuth(t *testing.T) { | |
| writeOperatorNamespaceFile(t, testOperatorNamespace) | |
| s := newOperatorMetricsTokenScheme() | |
| serviceMonitor := newOperatorMetricsServiceMonitor(testOperatorNamespace, true) | |
| c := fake.NewClientBuilder().WithScheme(s).WithObjects(serviceMonitor).Build() | |
| r := &OperatorMetricsTokenReconciler{ | |
| Client: c, | |
| Scheme: s, | |
| TokenRequester: &fakeTokenRequester{ | |
| token: "test-token", | |
| expiry: time.Now().Add(operatorMetricsTokenExpiry), | |
| }, | |
| } | |
| result, err := r.Reconcile(context.Background(), reconcile.Request{ | |
| NamespacedName: types.NamespacedName{ | |
| Name: operatorMetricsMonitorName, | |
| Namespace: testOperatorNamespace, | |
| }, | |
| }) | |
| assert.NilError(t, err) | |
| assert.Assert(t, result.RequeueAfter > 0) | |
| updatedSM := &monitoringv1.ServiceMonitor{} | |
| err = c.Get(context.Background(), types.NamespacedName{ | |
| Name: operatorMetricsMonitorName, | |
| Namespace: testOperatorNamespace, | |
| }, updatedSM) | |
| assert.NilError(t, err) | |
| assert.Assert(t, is.Nil(updatedSM.Spec.Endpoints[0].BearerTokenSecret)) | |
| assert.Assert(t, updatedSM.Spec.Endpoints[0].Authorization != nil) | |
| assert.Equal(t, updatedSM.Spec.Endpoints[0].Authorization.Type, "Bearer") | |
| assert.Equal(t, updatedSM.Spec.Endpoints[0].Authorization.Credentials.Name, operatorMetricsBearerTokenSecretName) | |
| assert.Equal(t, *updatedSM.Spec.Endpoints[0].TLSConfig.ServerName, operatorMetricsServiceName+"."+testOperatorNamespace+".svc") | |
| } | |
| func TestOperatorMetricsTokenReconciler_migratesServiceMonitorAuth(t *testing.T) { | |
| writeOperatorNamespaceFile(t, testOperatorNamespace) | |
| s := newOperatorMetricsTokenScheme() | |
| serviceMonitor := newOperatorMetricsServiceMonitor(testOperatorNamespace, true) | |
| c := fake.NewClientBuilder().WithScheme(s).WithObjects(serviceMonitor).Build() | |
| r := &OperatorMetricsTokenReconciler{ | |
| Client: c, | |
| Scheme: s, | |
| TokenRequester: &fakeTokenRequester{ | |
| token: "test-token", | |
| expiry: time.Now().Add(operatorMetricsTokenExpiry), | |
| }, | |
| } | |
| result, err := r.Reconcile(context.Background(), reconcile.Request{ | |
| NamespacedName: types.NamespacedName{ | |
| Name: operatorMetricsMonitorName, | |
| Namespace: testOperatorNamespace, | |
| }, | |
| }) | |
| assert.NilError(t, err) | |
| assert.Assert(t, result.RequeueAfter > 0) | |
| updatedSM := &monitoringv1.ServiceMonitor{} | |
| err = c.Get(context.Background(), types.NamespacedName{ | |
| Name: operatorMetricsMonitorName, | |
| Namespace: testOperatorNamespace, | |
| }, updatedSM) | |
| assert.NilError(t, err) | |
| assert.Assert(t, is.Nil(updatedSM.Spec.Endpoints[0].BearerTokenSecret)) //nolint:staticcheck // verifying deprecated field was cleared during migration | |
| assert.Assert(t, updatedSM.Spec.Endpoints[0].Authorization != nil) | |
| assert.Equal(t, updatedSM.Spec.Endpoints[0].Authorization.Type, "Bearer") | |
| assert.Equal(t, updatedSM.Spec.Endpoints[0].Authorization.Credentials.Name, operatorMetricsBearerTokenSecretName) | |
| assert.Equal(t, *updatedSM.Spec.Endpoints[0].TLSConfig.ServerName, operatorMetricsServiceName+"."+testOperatorNamespace+".svc") | |
| } |
🧰 Tools
🪛 GitHub Check: Ensure that code passes gosec and golint
[failure] 171-171:
SA1019: updatedSM.Spec.Endpoints[0].BearerTokenSecret is deprecated: use authorization instead. (staticcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controllers/operator_metrics_controller_test.go` around lines 140 - 176, Add
a targeted SA1019 nolint directive to the assertion accessing
updatedSM.Spec.Endpoints[0].BearerTokenSecret in
TestOperatorMetricsTokenReconciler_migratesServiceMonitorAuth, preserving the
intentional deprecated-field verification while keeping the remaining assertions
unchanged.
Source: Linters/SAST tools
Comment on lines
+170
to
+196
| if endpoint.BearerTokenSecret != nil { | ||
| endpoint.BearerTokenSecret = nil | ||
| endpoint.Authorization = &monitoringv1.SafeAuthorization{ | ||
| Type: "Bearer", | ||
| Credentials: &corev1.SecretKeySelector{ | ||
| LocalObjectReference: corev1.LocalObjectReference{ | ||
| Name: operatorMetricsBearerTokenSecretName, | ||
| }, | ||
| Key: operatorMetricsBearerTokenKey, | ||
| }, | ||
| } | ||
| updated = true | ||
| } else if endpoint.Authorization == nil || | ||
| endpoint.Authorization.Credentials == nil || | ||
| endpoint.Authorization.Credentials.Name != operatorMetricsBearerTokenSecretName || | ||
| endpoint.Authorization.Credentials.Key != operatorMetricsBearerTokenKey { | ||
| endpoint.Authorization = &monitoringv1.SafeAuthorization{ | ||
| Type: "Bearer", | ||
| Credentials: &corev1.SecretKeySelector{ | ||
| LocalObjectReference: corev1.LocalObjectReference{ | ||
| Name: operatorMetricsBearerTokenSecretName, | ||
| }, | ||
| Key: operatorMetricsBearerTokenKey, | ||
| }, | ||
| } | ||
| updated = true | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Suppress the deprecated-field lint failure with a justified nolint.
CI is failing on SA1019: endpoint.BearerTokenSecret is deprecated at Lines 170-171. The read/clear is intentional (migrating legacy config), so silence staticcheck with a scoped //nolint:staticcheck and a short justification instead of leaving the build red.
🔧 Proposed fix
updated := false
- if endpoint.BearerTokenSecret != nil {
- endpoint.BearerTokenSecret = nil
+ if endpoint.BearerTokenSecret != nil { //nolint:staticcheck // detect legacy bearerTokenSecret for migration
+ endpoint.BearerTokenSecret = nil //nolint:staticcheck // clear deprecated field after migrating to authorization
endpoint.Authorization = &monitoringv1.SafeAuthorization{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if endpoint.BearerTokenSecret != nil { | |
| endpoint.BearerTokenSecret = nil | |
| endpoint.Authorization = &monitoringv1.SafeAuthorization{ | |
| Type: "Bearer", | |
| Credentials: &corev1.SecretKeySelector{ | |
| LocalObjectReference: corev1.LocalObjectReference{ | |
| Name: operatorMetricsBearerTokenSecretName, | |
| }, | |
| Key: operatorMetricsBearerTokenKey, | |
| }, | |
| } | |
| updated = true | |
| } else if endpoint.Authorization == nil || | |
| endpoint.Authorization.Credentials == nil || | |
| endpoint.Authorization.Credentials.Name != operatorMetricsBearerTokenSecretName || | |
| endpoint.Authorization.Credentials.Key != operatorMetricsBearerTokenKey { | |
| endpoint.Authorization = &monitoringv1.SafeAuthorization{ | |
| Type: "Bearer", | |
| Credentials: &corev1.SecretKeySelector{ | |
| LocalObjectReference: corev1.LocalObjectReference{ | |
| Name: operatorMetricsBearerTokenSecretName, | |
| }, | |
| Key: operatorMetricsBearerTokenKey, | |
| }, | |
| } | |
| updated = true | |
| } | |
| if endpoint.BearerTokenSecret != nil { //nolint:staticcheck // detect legacy bearerTokenSecret for migration | |
| endpoint.BearerTokenSecret = nil //nolint:staticcheck // clear deprecated field after migrating to authorization | |
| endpoint.Authorization = &monitoringv1.SafeAuthorization{ | |
| Type: "Bearer", | |
| Credentials: &corev1.SecretKeySelector{ | |
| LocalObjectReference: corev1.LocalObjectReference{ | |
| Name: operatorMetricsBearerTokenSecretName, | |
| }, | |
| Key: operatorMetricsBearerTokenKey, | |
| }, | |
| } | |
| updated = true | |
| } else if endpoint.Authorization == nil || | |
| endpoint.Authorization.Credentials == nil || | |
| endpoint.Authorization.Credentials.Name != operatorMetricsBearerTokenSecretName || | |
| endpoint.Authorization.Credentials.Key != operatorMetricsBearerTokenKey { | |
| endpoint.Authorization = &monitoringv1.SafeAuthorization{ | |
| Type: "Bearer", | |
| Credentials: &corev1.SecretKeySelector{ | |
| LocalObjectReference: corev1.LocalObjectReference{ | |
| Name: operatorMetricsBearerTokenSecretName, | |
| }, | |
| Key: operatorMetricsBearerTokenKey, | |
| }, | |
| } | |
| updated = true | |
| } |
🧰 Tools
🪛 GitHub Actions: Verify generated code is up to date / 2_Ensure that code passes gosec and golint.txt
[error] 170-170: golangci-lint (staticcheck) reported SA1019: endpoint.BearerTokenSecret is deprecated; use authorization instead.
🪛 GitHub Actions: Verify generated code is up to date / Ensure that code passes gosec and golint
[error] 170-170: golangci-lint (staticcheck) SA1019: endpoint.BearerTokenSecret is deprecated; use authorization instead.
🪛 GitHub Check: Ensure that code passes gosec and golint
[failure] 171-171:
SA1019: endpoint.BearerTokenSecret is deprecated: use authorization instead. (staticcheck)
[failure] 170-170:
SA1019: endpoint.BearerTokenSecret is deprecated: use authorization instead. (staticcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controllers/operator_metrics_controller.go` around lines 170 - 196, Add a
scoped //nolint:staticcheck directive with a brief migration justification at
the intentional endpoint.BearerTokenSecret read/clear in the surrounding
reconciliation logic, suppressing only SA1019 while preserving the legacy-field
migration behavior.
Sources: Linters/SAST tools, Pipeline failures
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Migrate ServiceMonitor from deprecated bearerTokenSecret to authorization and manage a short-lived Opaque bearer token Secret via TokenRequest.
What type of PR is this?
/kind bug
What does this PR do / why we need it:
The operator metrics ServiceMonitor (
openshift-gitops-operator-metrics-monitor) previously relied on a non-expiringkubernetes.io/service-account-tokenSecret and the deprecatedbearerTokenSecretfield.This PR:
authorization(Bearer + credentials).OperatorMetricsTokenReconcilerto mint a short lived token via the Kubernetes TokenRequest API, store it in an Opaque Secret (token+expiry), renew before expiry, and replace the legacy SA token Secret on upgrade.ArgoCDMetricsReconcilerinto the dedicated controller.Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes #GITOPS-9795
Test acceptance criteria:
How to test changes / Special notes to the reviewer:
Unit: