From b36fc1e4d189442d9bde21bc05cb2125bdd29dc8 Mon Sep 17 00:00:00 2001 From: Tejas Soham Date: Mon, 13 Jul 2026 15:01:58 +0530 Subject: [PATCH 1/9] fix: replace non-expiring metrics monitor SA token with TokenRequest Migrate ServiceMonitor from deprecated bearerTokenSecret to authorization and manage a short-lived Opaque bearer token Secret via TokenRequest. Signed-off-by: Tejas Soham --- ...gitops-operator.clusterserviceversion.yaml | 2 +- ...etrics-monitor-bearer-token_v1_secret.yaml | 7 - ...nitoring.coreos.com_v1_servicemonitor.yaml | 8 +- cmd/main.go | 8 + config/prometheus/monitor.yaml | 17 +- controllers/argocd_controller.go | 14 +- controllers/argocd_metrics_controller.go | 55 +-- controllers/operator_metrics_controller.go | 319 +++++++++++++++ .../operator_metrics_controller_test.go | 365 ++++++++++++++++++ test/e2e/suite_test.go | 6 + test/nondefaulte2e/suite_test.go | 6 + .../1-104_validate_prometheus_alert_test.go | 28 +- 12 files changed, 751 insertions(+), 84 deletions(-) delete mode 100644 bundle/manifests/openshift-gitops-operator-metrics-monitor-bearer-token_v1_secret.yaml create mode 100644 controllers/operator_metrics_controller.go create mode 100644 controllers/operator_metrics_controller_test.go diff --git a/bundle/manifests/gitops-operator.clusterserviceversion.yaml b/bundle/manifests/gitops-operator.clusterserviceversion.yaml index cb42eba81eb..0832ad66c38 100644 --- a/bundle/manifests/gitops-operator.clusterserviceversion.yaml +++ b/bundle/manifests/gitops-operator.clusterserviceversion.yaml @@ -203,7 +203,7 @@ metadata: operatorframework.io/cluster-monitoring: "true" operatorframework.io/suggested-namespace: openshift-gitops-operator operators.openshift.io/infrastructure-features: '["disconnected"]' - operators.operatorframework.io/builder: operator-sdk-v1.35.0 + operators.operatorframework.io/builder: operator-sdk-v1.42.2 operators.operatorframework.io/project_layout: go.kubebuilder.io/v4 repository: https://github.com/redhat-developer/gitops-operator support: Red Hat diff --git a/bundle/manifests/openshift-gitops-operator-metrics-monitor-bearer-token_v1_secret.yaml b/bundle/manifests/openshift-gitops-operator-metrics-monitor-bearer-token_v1_secret.yaml deleted file mode 100644 index ab422c66cfb..00000000000 --- a/bundle/manifests/openshift-gitops-operator-metrics-monitor-bearer-token_v1_secret.yaml +++ /dev/null @@ -1,7 +0,0 @@ -apiVersion: v1 -kind: Secret -metadata: - annotations: - kubernetes.io/service-account.name: openshift-gitops-operator-controller-manager - name: openshift-gitops-operator-metrics-monitor-bearer-token -type: kubernetes.io/service-account-token diff --git a/bundle/manifests/openshift-gitops-operator-metrics-monitor_monitoring.coreos.com_v1_servicemonitor.yaml b/bundle/manifests/openshift-gitops-operator-metrics-monitor_monitoring.coreos.com_v1_servicemonitor.yaml index 3b4e719bc5e..87d125c6904 100644 --- a/bundle/manifests/openshift-gitops-operator-metrics-monitor_monitoring.coreos.com_v1_servicemonitor.yaml +++ b/bundle/manifests/openshift-gitops-operator-metrics-monitor_monitoring.coreos.com_v1_servicemonitor.yaml @@ -6,9 +6,11 @@ metadata: name: openshift-gitops-operator-metrics-monitor spec: endpoints: - - bearerTokenSecret: - key: token - name: openshift-gitops-operator-metrics-monitor-bearer-token + - authorization: + credentials: + key: token + name: openshift-gitops-operator-metrics-monitor-bearer-token + type: Bearer interval: 30s path: /metrics port: metrics diff --git a/cmd/main.go b/cmd/main.go index c3dd5dd4471..0d0c7f137fa 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -340,6 +340,14 @@ func main() { setupLog.Error(err, "unable to create controller", "controller", "Argo CD metrics") os.Exit(1) } + + if err = (&controllers.OperatorMetricsTokenReconciler{ + Client: client, + Scheme: mgr.GetScheme(), + }).SetupWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create controller", "controller", "Operator metrics token") + os.Exit(1) + } } else { setupLog.Info("Monitoring API not found, skipping Argo CD metrics controller setup") } diff --git a/config/prometheus/monitor.yaml b/config/prometheus/monitor.yaml index c601f41a47c..76af779b912 100644 --- a/config/prometheus/monitor.yaml +++ b/config/prometheus/monitor.yaml @@ -1,14 +1,5 @@ --- apiVersion: v1 -kind: Secret -metadata: - name: metrics-monitor-bearer-token - namespace: openshift-gitops-operator - annotations: - kubernetes.io/service-account.name: openshift-gitops-operator-controller-manager -type: kubernetes.io/service-account-token ---- -apiVersion: v1 kind: ConfigMap metadata: annotations: @@ -31,9 +22,11 @@ spec: matchLabels: control-plane: gitops-operator endpoints: - - bearerTokenSecret: - name: openshift-gitops-operator-metrics-monitor-bearer-token - key: token + - authorization: + type: Bearer + credentials: + name: openshift-gitops-operator-metrics-monitor-bearer-token + key: token interval: 30s path: /metrics port: metrics diff --git a/controllers/argocd_controller.go b/controllers/argocd_controller.go index 0f82aae61e7..a07a4e62ce4 100644 --- a/controllers/argocd_controller.go +++ b/controllers/argocd_controller.go @@ -45,16 +45,16 @@ import ( ) const ( - argocdNS = "openshift-gitops" - depracatedArgoCDNS = "openshift-pipelines-app-delivery" - consoleLinkName = "argocd" - argocdRouteName = "openshift-gitops-server" - iconFilePath = "/argo.png" - operatorPodNamespacePath = "/var/run/secrets/kubernetes.io/serviceaccount/namespace" + argocdNS = "openshift-gitops" + depracatedArgoCDNS = "openshift-pipelines-app-delivery" + consoleLinkName = "argocd" + argocdRouteName = "openshift-gitops-server" + iconFilePath = "/argo.png" ) var ( - encodedArgoImage string + operatorPodNamespacePath = "/var/run/secrets/kubernetes.io/serviceaccount/namespace" + encodedArgoImage string //go:embed argocd/img/argo.png argoImage []byte diff --git a/controllers/argocd_metrics_controller.go b/controllers/argocd_metrics_controller.go index 0abe9a1ced8..80221f178f0 100644 --- a/controllers/argocd_metrics_controller.go +++ b/controllers/argocd_metrics_controller.go @@ -20,7 +20,6 @@ import ( "context" "embed" "fmt" - "os" "path/filepath" "strings" @@ -44,13 +43,11 @@ import ( ) const ( - readRoleNameFormat = "%s-read" - readRoleBindingNameFormat = "%s-prometheus-k8s-read-binding" - alertRuleName = "gitops-operator-argocd-alerts" - dashboardNamespace = "openshift-config-managed" - dashboardFolder = "dashboards" - operatorMetricsServiceName = "openshift-gitops-operator-metrics-service" - operatorMetricsMonitorName = "openshift-gitops-operator-metrics-monitor" + readRoleNameFormat = "%s-read" + readRoleBindingNameFormat = "%s-prometheus-k8s-read-binding" + alertRuleName = "gitops-operator-argocd-alerts" + dashboardNamespace = "openshift-config-managed" + dashboardFolder = "dashboards" ) type ArgoCDMetricsReconciler struct { @@ -182,11 +179,6 @@ func (r *ArgoCDMetricsReconciler) Reconcile(ctx context.Context, request reconci if err != nil { return reconcile.Result{}, err } - - err = r.reconcileOperatorMetricsServiceMonitor(reqLogger) - if err != nil { - return reconcile.Result{}, err - } } else { if exists { namespace.Labels[monitoringLabel] = "false" @@ -369,43 +361,6 @@ func (r *ArgoCDMetricsReconciler) deleteServiceMonitor(name string, namespace st } -func (r *ArgoCDMetricsReconciler) reconcileOperatorMetricsServiceMonitor(reqLogger logr.Logger) error { - - data, err := os.ReadFile(operatorPodNamespacePath) - if err != nil { - if os.IsNotExist(err) { - reqLogger.Info(fmt.Sprintf("Unable to retrieve the operator's running namespace via '%s': you should only see this message when running within unit tests, otherwise it is an error.", operatorPodNamespacePath)) - return nil - } - reqLogger.Error(err, "Error retrieving operator's running namespace") - return err - } - - operatorNS := string(data) - desiredMetricsServerName := operatorMetricsServiceName + "." + operatorNS + ".svc" - - existingServiceMonitor := &monitoringv1.ServiceMonitor{} - err = r.Client.Get(context.TODO(), types.NamespacedName{Name: operatorMetricsMonitorName, Namespace: operatorNS}, existingServiceMonitor) - - if err != nil { - if !errors.IsNotFound(err) { - reqLogger.Error(err, "Error querying for ServiceMonitor", "Namespace", operatorNS, "Name", operatorMetricsMonitorName) - return err - } - - // no svc monitor found, nothing to do - return nil - } - - currentServerName := existingServiceMonitor.Spec.Endpoints[0].TLSConfig.ServerName - if currentServerName == nil || *currentServerName != desiredMetricsServerName { - existingServiceMonitor.Spec.Endpoints[0].TLSConfig.ServerName = &desiredMetricsServerName - return r.Client.Update(context.TODO(), existingServiceMonitor) - } - - return nil -} - func (r *ArgoCDMetricsReconciler) createPrometheusRuleIfAbsent(namespace string, argocd *argoapp.ArgoCD, reqLogger logr.Logger) error { alertRule := newPrometheusRule(namespace) existingAlertRule := &monitoringv1.PrometheusRule{} diff --git a/controllers/operator_metrics_controller.go b/controllers/operator_metrics_controller.go new file mode 100644 index 00000000000..b257c79fc44 --- /dev/null +++ b/controllers/operator_metrics_controller.go @@ -0,0 +1,319 @@ +/* +Copyright 2021. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controllers + +import ( + "context" + "fmt" + "os" + "strings" + "time" + + "github.com/go-logr/logr" + monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" + authenticationv1 "k8s.io/api/authentication/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +const ( + operatorMetricsServiceName = "openshift-gitops-operator-metrics-service" + operatorMetricsMonitorName = "openshift-gitops-operator-metrics-monitor" + operatorMetricsBearerTokenSecretName = "openshift-gitops-operator-metrics-monitor-bearer-token" + operatorControllerSAName = "openshift-gitops-operator-controller-manager" + operatorMetricsTokenExpirySecs = int64(3600) + operatorMetricsTokenExpiry = time.Duration(operatorMetricsTokenExpirySecs) * time.Second + operatorMetricsBearerTokenKey = "token" + operatorMetricsBearerTokenExpiryKey = "expiry" +) + +type serviceAccountTokenRequester interface { + RequestToken(ctx context.Context, namespace, serviceAccountName string, expirationSeconds int64) (token string, expiry time.Time, err error) +} + +type clientServiceAccountTokenRequester struct { + client client.Client +} + +func (r *clientServiceAccountTokenRequester) RequestToken(ctx context.Context, namespace, serviceAccountName string, expirationSeconds int64) (string, time.Time, error) { + sa := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: serviceAccountName, + Namespace: namespace, + }, + } + tr := &authenticationv1.TokenRequest{ + Spec: authenticationv1.TokenRequestSpec{ + ExpirationSeconds: ptr.To(expirationSeconds), + }, + } + if err := r.client.SubResource("token").Create(ctx, sa, tr); err != nil { + return "", time.Time{}, err + } + return tr.Status.Token, tr.Status.ExpirationTimestamp.Time, nil +} + +// OperatorMetricsTokenReconciler manages the short-lived bearer token Secret used by the +// operator's ServiceMonitor for Prometheus metrics scraping. +type OperatorMetricsTokenReconciler struct { + Client client.Client + Scheme *runtime.Scheme + TokenRequester serviceAccountTokenRequester +} + +var _ reconcile.Reconciler = &OperatorMetricsTokenReconciler{} + +func (r *OperatorMetricsTokenReconciler) tokenRequester() serviceAccountTokenRequester { + if r.TokenRequester != nil { + return r.TokenRequester + } + return &clientServiceAccountTokenRequester{client: r.Client} +} + +// SetupWithManager sets up the controller with the Manager. +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) +} + +//+kubebuilder:rbac:groups="",resources=serviceaccounts/token,resourceNames=openshift-gitops-operator-controller-manager,verbs=create +//+kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;create;update;patch;delete +//+kubebuilder:rbac:groups=monitoring.coreos.com,resources=servicemonitors,verbs=get;list;watch;update;patch + +func (r *OperatorMetricsTokenReconciler) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) { + reqLogger := logf.Log.WithName("controller_operator_metrics_token"). + WithValues("Request.Namespace", request.Namespace, "Request.Name", request.Name) + + operatorNS, err := getOperatorNamespace() + if err != nil { + if os.IsNotExist(err) { + reqLogger.Info(fmt.Sprintf("Unable to retrieve the operator's running namespace via '%s': you should only see this message when running within unit tests, otherwise it is an error.", operatorPodNamespacePath)) + return reconcile.Result{}, nil + } + reqLogger.Error(err, "Error retrieving operator's running namespace") + return reconcile.Result{}, err + } + + if request.Namespace != operatorNS || request.Name != operatorMetricsMonitorName { + return reconcile.Result{}, nil + } + + serviceMonitor := &monitoringv1.ServiceMonitor{} + if err := r.Client.Get(ctx, request.NamespacedName, serviceMonitor); err != nil { + if errors.IsNotFound(err) { + return reconcile.Result{}, nil + } + reqLogger.Error(err, "Error querying for ServiceMonitor") + return reconcile.Result{}, err + } + + if err := r.reconcileServiceMonitor(ctx, serviceMonitor, operatorNS, reqLogger); err != nil { + return reconcile.Result{}, err + } + + requeueAfter, err := r.reconcileBearerTokenSecret(ctx, operatorNS, reqLogger) + if err != nil { + return reconcile.Result{}, err + } + + if requeueAfter > 0 { + reqLogger.Info("Scheduling bearer token renewal", "after", requeueAfter.String()) + } + return reconcile.Result{RequeueAfter: requeueAfter}, nil +} + +func getOperatorNamespace() (string, error) { + data, err := os.ReadFile(operatorPodNamespacePath) + if err != nil { + return "", err + } + return strings.TrimSpace(string(data)), nil +} + +func (r *OperatorMetricsTokenReconciler) reconcileServiceMonitor(ctx context.Context, serviceMonitor *monitoringv1.ServiceMonitor, operatorNS string, reqLogger logr.Logger) error { + if len(serviceMonitor.Spec.Endpoints) == 0 { + return fmt.Errorf("ServiceMonitor %s has no endpoints", serviceMonitor.Name) + } + + desiredMetricsServerName := operatorMetricsServiceName + "." + operatorNS + ".svc" + endpoint := &serviceMonitor.Spec.Endpoints[0] + + updated := false + 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.TLSConfig != nil && (endpoint.TLSConfig.ServerName == nil || *endpoint.TLSConfig.ServerName != desiredMetricsServerName) { + endpoint.TLSConfig.ServerName = &desiredMetricsServerName + updated = true + } + + if !updated { + return nil + } + + reqLogger.Info("Updating operator metrics ServiceMonitor", + "Namespace", serviceMonitor.Namespace, "Name", serviceMonitor.Name) + return r.Client.Update(ctx, serviceMonitor) +} + +func (r *OperatorMetricsTokenReconciler) reconcileBearerTokenSecret(ctx context.Context, namespace string, reqLogger logr.Logger) (time.Duration, error) { + secret := &corev1.Secret{} + err := r.Client.Get(ctx, types.NamespacedName{ + Name: operatorMetricsBearerTokenSecretName, + Namespace: namespace, + }, secret) + + needsRefresh := false + legacySAToken := false + if errors.IsNotFound(err) { + needsRefresh = true + } else if err != nil { + return 0, err + } 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 + } + } + } + + if !needsRefresh { + return 0, nil + } + + token, expiry, err := r.tokenRequester().RequestToken(ctx, namespace, operatorControllerSAName, operatorMetricsTokenExpirySecs) + if err != nil { + reqLogger.Error(err, "Failed to request service account token") + return 0, err + } + + desiredSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: operatorMetricsBearerTokenSecretName, + Namespace: namespace, + }, + Type: corev1.SecretTypeOpaque, + Data: map[string][]byte{ + operatorMetricsBearerTokenKey: []byte(token), + operatorMetricsBearerTokenExpiryKey: []byte(expiry.UTC().Format(time.RFC3339)), + }, + } + + 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 + } + + existingSecret := &corev1.Secret{} + getErr := r.Client.Get(ctx, types.NamespacedName{ + Name: operatorMetricsBearerTokenSecretName, + Namespace: namespace, + }, existingSecret) + if getErr != nil { + if errors.IsNotFound(getErr) { + reqLogger.Info("Creating metrics monitor bearer token Secret", + "Namespace", namespace, "Name", operatorMetricsBearerTokenSecretName) + if err := r.Client.Create(ctx, desiredSecret); err != nil { + return 0, err + } + return bearerTokenRequeueDuration(expiry), nil + } + return 0, getErr + } + + existingSecret.Type = corev1.SecretTypeOpaque + existingSecret.Data = desiredSecret.Data + reqLogger.Info("Updating metrics monitor bearer token Secret", + "Namespace", namespace, "Name", operatorMetricsBearerTokenSecretName) + if err := r.Client.Update(ctx, existingSecret); err != nil { + return 0, err + } + + return bearerTokenRequeueDuration(expiry), nil +} + +func parseBearerTokenExpiry(expiryData []byte) (time.Time, error) { + return time.Parse(time.RFC3339, string(expiryData)) +} + +// bearerTokenRequeueDuration returns how long to wait before renewing the token. +// Renewal is scheduled after one third of the *actual* remaining lifetime has +// elapsed, so cluster-capped TokenRequest lifetimes still get a renewal. +func bearerTokenRequeueDuration(expiry time.Time) time.Duration { + remaining := time.Until(expiry) + if remaining <= 0 { + return 0 + } + return remaining / 3 +} diff --git a/controllers/operator_metrics_controller_test.go b/controllers/operator_metrics_controller_test.go new file mode 100644 index 00000000000..7ead6afa433 --- /dev/null +++ b/controllers/operator_metrics_controller_test.go @@ -0,0 +1,365 @@ +/* +Copyright 2021. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controllers + +import ( + "context" + "fmt" + "os" + "path/filepath" + "testing" + "time" + + monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" + "gotest.tools/assert" + is "gotest.tools/assert/cmp" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes/scheme" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +const testOperatorNamespace = "openshift-gitops-operator" + +type fakeTokenRequester struct { + token string + expiry time.Time + err error +} + +func (f *fakeTokenRequester) RequestToken(_ context.Context, _, _ string, _ int64) (string, time.Time, error) { + if f.err != nil { + return "", time.Time{}, f.err + } + return f.token, f.expiry, nil +} + +func writeOperatorNamespaceFile(t *testing.T, namespace string) { + t.Helper() + dir := t.TempDir() + namespaceFile := filepath.Join(dir, "namespace") + if err := os.WriteFile(namespaceFile, []byte(namespace), 0o644); err != nil { + t.Fatal(err) + } + oldPath := operatorPodNamespacePath + operatorPodNamespacePath = namespaceFile + t.Cleanup(func() { + operatorPodNamespacePath = oldPath + }) +} + +func newOperatorMetricsTokenScheme() *runtime.Scheme { + s := scheme.Scheme + s.AddKnownTypes(monitoringv1.SchemeGroupVersion, &monitoringv1.ServiceMonitor{}) + return s +} + +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 TestGetOperatorNamespace_trimsNewline(t *testing.T) { + dir := t.TempDir() + namespaceFile := filepath.Join(dir, "namespace") + if err := os.WriteFile(namespaceFile, []byte("openshift-gitops-operator\n"), 0o644); err != nil { + t.Fatal(err) + } + oldPath := operatorPodNamespacePath + operatorPodNamespacePath = namespaceFile + t.Cleanup(func() { + operatorPodNamespacePath = oldPath + }) + + ns, err := getOperatorNamespace() + assert.NilError(t, err) + assert.Equal(t, ns, "openshift-gitops-operator") +} + +func TestBearerTokenRequeueDuration(t *testing.T) { + expiry := time.Now().Add(operatorMetricsTokenExpiry) + requeue := bearerTokenRequeueDuration(expiry) + assert.Assert(t, requeue > 19*time.Minute && requeue <= 20*time.Minute) + + // Cluster-capped shorter lifetime must still schedule renewal. + shortExpiry := time.Now().Add(30 * time.Minute) + shortRequeue := bearerTokenRequeueDuration(shortExpiry) + assert.Assert(t, shortRequeue > 9*time.Minute && shortRequeue <= 10*time.Minute) + + expiredExpiry := time.Now().Add(-time.Minute) + assert.Equal(t, bearerTokenRequeueDuration(expiredExpiry), time.Duration(0)) +} + +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_replacesLegacySecret(t *testing.T) { + writeOperatorNamespaceFile(t, testOperatorNamespace) + + s := newOperatorMetricsTokenScheme() + serviceMonitor := newOperatorMetricsServiceMonitor(testOperatorNamespace, false) + serviceMonitor.Spec.Endpoints[0].Authorization = &monitoringv1.SafeAuthorization{ + Type: "Bearer", + Credentials: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: operatorMetricsBearerTokenSecretName, + }, + Key: operatorMetricsBearerTokenKey, + }, + } + legacySecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: operatorMetricsBearerTokenSecretName, + Namespace: testOperatorNamespace, + }, + Type: corev1.SecretTypeServiceAccountToken, + } + c := fake.NewClientBuilder().WithScheme(s).WithObjects(serviceMonitor, legacySecret).Build() + + expiry := time.Now().Add(operatorMetricsTokenExpiry) + r := &OperatorMetricsTokenReconciler{ + Client: c, + Scheme: s, + TokenRequester: &fakeTokenRequester{ + token: "new-token", + expiry: expiry, + }, + } + + _, err := r.Reconcile(context.Background(), reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: operatorMetricsMonitorName, + Namespace: testOperatorNamespace, + }, + }) + assert.NilError(t, err) + + secret := &corev1.Secret{} + err = c.Get(context.Background(), types.NamespacedName{ + Name: operatorMetricsBearerTokenSecretName, + Namespace: testOperatorNamespace, + }, secret) + assert.NilError(t, err) + assert.Equal(t, secret.Type, corev1.SecretTypeOpaque) + assert.Equal(t, string(secret.Data[operatorMetricsBearerTokenKey]), "new-token") + parsedExpiry, err := parseBearerTokenExpiry(secret.Data[operatorMetricsBearerTokenExpiryKey]) + assert.NilError(t, err) + assert.Equal(t, parsedExpiry.UTC().Format(time.RFC3339), expiry.UTC().Format(time.RFC3339)) +} + +func TestOperatorMetricsTokenReconciler_keepsLegacySecretIfTokenRequestFails(t *testing.T) { + writeOperatorNamespaceFile(t, testOperatorNamespace) + + s := newOperatorMetricsTokenScheme() + serviceMonitor := newOperatorMetricsServiceMonitor(testOperatorNamespace, false) + serviceMonitor.Spec.Endpoints[0].Authorization = &monitoringv1.SafeAuthorization{ + Type: "Bearer", + Credentials: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: operatorMetricsBearerTokenSecretName, + }, + Key: operatorMetricsBearerTokenKey, + }, + } + legacySecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: operatorMetricsBearerTokenSecretName, + Namespace: testOperatorNamespace, + }, + Type: corev1.SecretTypeServiceAccountToken, + Data: map[string][]byte{ + operatorMetricsBearerTokenKey: []byte("legacy-token"), + }, + } + c := fake.NewClientBuilder().WithScheme(s).WithObjects(serviceMonitor, legacySecret).Build() + + r := &OperatorMetricsTokenReconciler{ + Client: c, + Scheme: s, + TokenRequester: &fakeTokenRequester{ + err: fmt.Errorf("token request failed"), + }, + } + + _, err := r.Reconcile(context.Background(), reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: operatorMetricsMonitorName, + Namespace: testOperatorNamespace, + }, + }) + assert.ErrorContains(t, err, "token request failed") + + secret := &corev1.Secret{} + err = c.Get(context.Background(), types.NamespacedName{ + Name: operatorMetricsBearerTokenSecretName, + Namespace: testOperatorNamespace, + }, secret) + assert.NilError(t, err) + assert.Equal(t, secret.Type, corev1.SecretTypeServiceAccountToken) + assert.Equal(t, string(secret.Data[operatorMetricsBearerTokenKey]), "legacy-token") +} + +func TestOperatorMetricsTokenReconciler_refreshesExpiredToken(t *testing.T) { + writeOperatorNamespaceFile(t, testOperatorNamespace) + + s := newOperatorMetricsTokenScheme() + serviceMonitor := newOperatorMetricsServiceMonitor(testOperatorNamespace, false) + serviceMonitor.Spec.Endpoints[0].Authorization = &monitoringv1.SafeAuthorization{ + Type: "Bearer", + Credentials: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: operatorMetricsBearerTokenSecretName, + }, + Key: operatorMetricsBearerTokenKey, + }, + } + expiredSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: operatorMetricsBearerTokenSecretName, + Namespace: testOperatorNamespace, + }, + Type: corev1.SecretTypeOpaque, + Data: map[string][]byte{ + operatorMetricsBearerTokenKey: []byte("old-token"), + operatorMetricsBearerTokenExpiryKey: []byte(time.Now().Add(-time.Hour).UTC().Format(time.RFC3339)), + }, + } + c := fake.NewClientBuilder().WithScheme(s).WithObjects(serviceMonitor, expiredSecret).Build() + + newExpiry := time.Now().Add(operatorMetricsTokenExpiry) + r := &OperatorMetricsTokenReconciler{ + Client: c, + Scheme: s, + TokenRequester: &fakeTokenRequester{ + token: "refreshed-token", + expiry: newExpiry, + }, + } + + _, err := r.Reconcile(context.Background(), reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: operatorMetricsMonitorName, + Namespace: testOperatorNamespace, + }, + }) + assert.NilError(t, err) + + secret := &corev1.Secret{} + err = c.Get(context.Background(), types.NamespacedName{ + Name: operatorMetricsBearerTokenSecretName, + Namespace: testOperatorNamespace, + }, secret) + assert.NilError(t, err) + assert.Equal(t, string(secret.Data[operatorMetricsBearerTokenKey]), "refreshed-token") +} + +func TestOperatorMetricsTokenReconciler_skipsOtherNamespaces(t *testing.T) { + writeOperatorNamespaceFile(t, testOperatorNamespace) + + s := newOperatorMetricsTokenScheme() + c := fake.NewClientBuilder().WithScheme(s).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: "other-namespace", + }, + }) + assert.NilError(t, err) + assert.Equal(t, result.RequeueAfter, time.Duration(0)) + + var secrets corev1.SecretList + err = c.List(context.Background(), &secrets, client.InNamespace("other-namespace")) + assert.NilError(t, err) + assert.Equal(t, len(secrets.Items), 0) +} diff --git a/test/e2e/suite_test.go b/test/e2e/suite_test.go index b39fcfb2d89..2ab324bb0a5 100644 --- a/test/e2e/suite_test.go +++ b/test/e2e/suite_test.go @@ -164,6 +164,12 @@ var _ = BeforeSuite(func() { }).SetupWithManager(mgr) Expect(err).NotTo(HaveOccurred()) + err = (&controllers.OperatorMetricsTokenReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + }).SetupWithManager(mgr) + Expect(err).NotTo(HaveOccurred()) + k8sClient, err := initK8sClient() Expect(err).ToNot(HaveOccurred()) diff --git a/test/nondefaulte2e/suite_test.go b/test/nondefaulte2e/suite_test.go index a0a4c14b473..46b3e7b8958 100644 --- a/test/nondefaulte2e/suite_test.go +++ b/test/nondefaulte2e/suite_test.go @@ -151,6 +151,12 @@ var _ = BeforeSuite(func() { }).SetupWithManager(mgr) Expect(err).NotTo(HaveOccurred()) + err = (&controllers.OperatorMetricsTokenReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + }).SetupWithManager(mgr) + Expect(err).NotTo(HaveOccurred()) + k8sClient, err := initK8sClient() Expect(err).ToNot(HaveOccurred()) diff --git a/test/openshift/e2e/ginkgo/parallel/1-104_validate_prometheus_alert_test.go b/test/openshift/e2e/ginkgo/parallel/1-104_validate_prometheus_alert_test.go index f2931d2b37a..2650f370f6a 100644 --- a/test/openshift/e2e/ginkgo/parallel/1-104_validate_prometheus_alert_test.go +++ b/test/openshift/e2e/ginkgo/parallel/1-104_validate_prometheus_alert_test.go @@ -1,6 +1,8 @@ package parallel import ( + "time" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -37,11 +39,14 @@ var _ = Describe("GitOps Operator Parallel E2E Tests", func() { Eventually(sm).Should(k8sFixture.ExistByName()) serverName := "openshift-gitops-operator-metrics-service.openshift-gitops-operator.svc" Expect(sm.Spec.Endpoints).To(Equal([]monitoringv1.Endpoint{{ - BearerTokenSecret: &corev1.SecretKeySelector{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: "openshift-gitops-operator-metrics-monitor-bearer-token", + Authorization: &monitoringv1.SafeAuthorization{ + Type: "Bearer", + Credentials: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "openshift-gitops-operator-metrics-monitor-bearer-token", + }, + Key: "token", }, - Key: "token", }, Interval: monitoringv1.Duration("30s"), Path: "/metrics", @@ -68,6 +73,21 @@ var _ = Describe("GitOps Operator Parallel E2E Tests", func() { "control-plane": "gitops-operator", }, })) + + bearerTokenSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "openshift-gitops-operator-metrics-monitor-bearer-token", + Namespace: "openshift-gitops-operator", + }, + } + Eventually(bearerTokenSecret).Should(k8sFixture.ExistByName()) + Expect(bearerTokenSecret.Type).To(Equal(corev1.SecretTypeOpaque)) + Expect(bearerTokenSecret.Data).To(HaveKey("token")) + Expect(bearerTokenSecret.Data).To(HaveKey("expiry")) + + expiry, err := time.Parse(time.RFC3339, string(bearerTokenSecret.Data["expiry"])) + Expect(err).NotTo(HaveOccurred()) + Expect(expiry.After(time.Now())).To(BeTrue()) }) }) }) From 406b8ed06c03061888661d9c258ead6e1b18d433 Mon Sep 17 00:00:00 2001 From: Alka Kumari Date: Sun, 16 Aug 2026 02:19:11 +0530 Subject: [PATCH 2/9] fix: improve metrics bearer token Secret reconciliation Signed-off-by: Alka Kumari --- controllers/operator_metrics_controller.go | 57 +++++++++++-------- .../operator_metrics_controller_test.go | 48 +++++++++++++++- 2 files changed, 78 insertions(+), 27 deletions(-) diff --git a/controllers/operator_metrics_controller.go b/controllers/operator_metrics_controller.go index b257c79fc44..fd4b40ad42d 100644 --- a/controllers/operator_metrics_controller.go +++ b/controllers/operator_metrics_controller.go @@ -33,16 +33,18 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/handler" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) const ( - operatorMetricsServiceName = "openshift-gitops-operator-metrics-service" - operatorMetricsMonitorName = "openshift-gitops-operator-metrics-monitor" - operatorMetricsBearerTokenSecretName = "openshift-gitops-operator-metrics-monitor-bearer-token" + operatorMetricsServiceName = "openshift-gitops-operator-metrics-service" + operatorMetricsMonitorName = "openshift-gitops-operator-metrics-monitor" + operatorMetricsBearerTokenSecretName = "openshift-gitops-operator-metrics-monitor-bearer-token" operatorControllerSAName = "openshift-gitops-operator-controller-manager" operatorMetricsTokenExpirySecs = int64(3600) operatorMetricsTokenExpiry = time.Duration(operatorMetricsTokenExpirySecs) * time.Second @@ -95,16 +97,33 @@ func (r *OperatorMetricsTokenReconciler) tokenRequester() serviceAccountTokenReq // SetupWithManager sets up the controller with the Manager. func (r *OperatorMetricsTokenReconciler) SetupWithManager(mgr ctrl.Manager) error { + serviceMonitorPredicate := predicate.NewPredicateFuncs(func(obj client.Object) bool { + return obj.GetName() == operatorMetricsMonitorName + }) + bearerTokenSecretPredicate := predicate.NewPredicateFuncs(func(obj client.Object) bool { + return obj.GetName() == operatorMetricsBearerTokenSecretName + }) + return ctrl.NewControllerManagedBy(mgr). Named("operator-metrics-token"). - For(&monitoringv1.ServiceMonitor{}). - WithEventFilter(predicate.NewPredicateFuncs(func(obj client.Object) bool { - return obj.GetName() == operatorMetricsMonitorName - })). + For(&monitoringv1.ServiceMonitor{}, builder.WithPredicates(serviceMonitorPredicate)). + Watches( + &corev1.Secret{}, + handler.EnqueueRequestsFromMapFunc(mapBearerTokenSecretToServiceMonitor), + builder.WithPredicates(bearerTokenSecretPredicate), + ). Complete(r) } -//+kubebuilder:rbac:groups="",resources=serviceaccounts/token,resourceNames=openshift-gitops-operator-controller-manager,verbs=create +func mapBearerTokenSecretToServiceMonitor(_ context.Context, obj client.Object) []reconcile.Request { + return []reconcile.Request{{ + NamespacedName: types.NamespacedName{ + Namespace: obj.GetNamespace(), + Name: operatorMetricsMonitorName, + }, + }} +} + //+kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;create;update;patch;delete //+kubebuilder:rbac:groups=monitoring.coreos.com,resources=servicemonitors,verbs=get;list;watch;update;patch @@ -167,8 +186,8 @@ func (r *OperatorMetricsTokenReconciler) reconcileServiceMonitor(ctx context.Con endpoint := &serviceMonitor.Spec.Endpoints[0] updated := false - if endpoint.BearerTokenSecret != nil { - endpoint.BearerTokenSecret = nil + if endpoint.BearerTokenSecret != nil { //nolint:staticcheck // SA1019: migrate deprecated bearerTokenSecret to authorization + endpoint.BearerTokenSecret = nil //nolint:staticcheck // SA1019: migrate deprecated bearerTokenSecret to authorization endpoint.Authorization = &monitoringv1.SafeAuthorization{ Type: "Bearer", Credentials: &corev1.SecretKeySelector{ @@ -217,15 +236,15 @@ func (r *OperatorMetricsTokenReconciler) reconcileBearerTokenSecret(ctx context. }, secret) needsRefresh := false - legacySAToken := false if errors.IsNotFound(err) { needsRefresh = true } else if err != nil { return 0, err } else if secret.Type == corev1.SecretTypeServiceAccountToken { - // Keep the legacy Secret until TokenRequest succeeds so scrape auth + // Migrate legacy Secret in place after TokenRequest succeeds so scrape auth // is not interrupted if minting fails. - legacySAToken = true + needsRefresh = true + } else if secret.Type != corev1.SecretTypeOpaque || len(secret.Data[operatorMetricsBearerTokenKey]) == 0 { needsRefresh = true } else { expiry, parseErr := parseBearerTokenExpiry(secret.Data[operatorMetricsBearerTokenExpiryKey]) @@ -263,18 +282,6 @@ func (r *OperatorMetricsTokenReconciler) reconcileBearerTokenSecret(ctx context. }, } - 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 - } - existingSecret := &corev1.Secret{} getErr := r.Client.Get(ctx, types.NamespacedName{ Name: operatorMetricsBearerTokenSecretName, diff --git a/controllers/operator_metrics_controller_test.go b/controllers/operator_metrics_controller_test.go index 7ead6afa433..0bbdaa0702e 100644 --- a/controllers/operator_metrics_controller_test.go +++ b/controllers/operator_metrics_controller_test.go @@ -96,7 +96,7 @@ func newOperatorMetricsServiceMonitor(namespace string, useLegacyAuth bool) *mon }, } if useLegacyAuth { - sm.Spec.Endpoints[0].BearerTokenSecret = &corev1.SecretKeySelector{ + sm.Spec.Endpoints[0].BearerTokenSecret = &corev1.SecretKeySelector{ //nolint:staticcheck // SA1019: migrate deprecated bearerTokenSecret to authorization LocalObjectReference: corev1.LocalObjectReference{ Name: operatorMetricsBearerTokenSecretName, }, @@ -168,7 +168,7 @@ func TestOperatorMetricsTokenReconciler_migratesServiceMonitorAuth(t *testing.T) Namespace: testOperatorNamespace, }, updatedSM) assert.NilError(t, err) - assert.Assert(t, is.Nil(updatedSM.Spec.Endpoints[0].BearerTokenSecret)) + assert.Assert(t, is.Nil(updatedSM.Spec.Endpoints[0].BearerTokenSecret)) //nolint:staticcheck // SA1019: migrate deprecated bearerTokenSecret to authorization//nolint:staticcheck // SA1019: migrate deprecated bearerTokenSecret to authorization 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) @@ -335,6 +335,50 @@ func TestOperatorMetricsTokenReconciler_refreshesExpiredToken(t *testing.T) { assert.Equal(t, string(secret.Data[operatorMetricsBearerTokenKey]), "refreshed-token") } +func TestOperatorMetricsTokenReconciler_refreshesSecretWithMissingToken(t *testing.T) { + writeOperatorNamespaceFile(t, testOperatorNamespace) + + s := newOperatorMetricsTokenScheme() + serviceMonitor := newOperatorMetricsServiceMonitor(testOperatorNamespace, false) + invalidSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: operatorMetricsBearerTokenSecretName, + Namespace: testOperatorNamespace, + }, + Type: corev1.SecretTypeOpaque, + Data: map[string][]byte{ + operatorMetricsBearerTokenExpiryKey: []byte(time.Now().Add(operatorMetricsTokenExpiry).UTC().Format(time.RFC3339)), + }, + } + c := fake.NewClientBuilder().WithScheme(s).WithObjects(serviceMonitor, invalidSecret).Build() + + newExpiry := time.Now().Add(operatorMetricsTokenExpiry) + r := &OperatorMetricsTokenReconciler{ + Client: c, + Scheme: s, + TokenRequester: &fakeTokenRequester{ + token: "minted-token", + expiry: newExpiry, + }, + } + + _, err := r.Reconcile(context.Background(), reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: operatorMetricsMonitorName, + Namespace: testOperatorNamespace, + }, + }) + assert.NilError(t, err) + + secret := &corev1.Secret{} + err = c.Get(context.Background(), types.NamespacedName{ + Name: operatorMetricsBearerTokenSecretName, + Namespace: testOperatorNamespace, + }, secret) + assert.NilError(t, err) + assert.Equal(t, string(secret.Data[operatorMetricsBearerTokenKey]), "minted-token") +} + func TestOperatorMetricsTokenReconciler_skipsOtherNamespaces(t *testing.T) { writeOperatorNamespaceFile(t, testOperatorNamespace) From 3f7a01a317fc31a1f5d5acb90ee5274749e2614d Mon Sep 17 00:00:00 2001 From: Alka Kumari Date: Mon, 17 Aug 2026 15:07:43 +0530 Subject: [PATCH 3/9] fix: keep CSV operator-sdk builder annotation at v1.35.0 Signed-off-by: Alka Kumari --- bundle/manifests/gitops-operator.clusterserviceversion.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/manifests/gitops-operator.clusterserviceversion.yaml b/bundle/manifests/gitops-operator.clusterserviceversion.yaml index 0832ad66c38..d0944546d02 100644 --- a/bundle/manifests/gitops-operator.clusterserviceversion.yaml +++ b/bundle/manifests/gitops-operator.clusterserviceversion.yaml @@ -190,7 +190,7 @@ metadata: capabilities: Deep Insights console.openshift.io/plugins: '["gitops-plugin"]' containerImage: quay.io/redhat-developer/gitops-operator - createdAt: "2026-07-02T15:48:02Z" + createdAt: "2026-08-17T09:26:28Z" description: Enables teams to adopt GitOps principles for managing cluster configurations and application delivery across hybrid multi-cluster Kubernetes environments. features.operators.openshift.io/disconnected: "true" @@ -203,7 +203,7 @@ metadata: operatorframework.io/cluster-monitoring: "true" operatorframework.io/suggested-namespace: openshift-gitops-operator operators.openshift.io/infrastructure-features: '["disconnected"]' - operators.operatorframework.io/builder: operator-sdk-v1.42.2 + operators.operatorframework.io/builder: operator-sdk-v1.35.0 operators.operatorframework.io/project_layout: go.kubebuilder.io/v4 repository: https://github.com/redhat-developer/gitops-operator support: Red Hat From c56b3075e5bfaaefbc931455f62f746f9f305449 Mon Sep 17 00:00:00 2001 From: Alka Kumari Date: Tue, 18 Aug 2026 20:46:59 +0530 Subject: [PATCH 4/9] minimise requeue calls assisted-by: claude-code Signed-off-by: Alka Kumari --- controllers/operator_metrics_controller.go | 52 ++++--- .../operator_metrics_controller_test.go | 145 ++++++++++++++++-- 2 files changed, 167 insertions(+), 30 deletions(-) diff --git a/controllers/operator_metrics_controller.go b/controllers/operator_metrics_controller.go index fd4b40ad42d..83de39ea7d6 100644 --- a/controllers/operator_metrics_controller.go +++ b/controllers/operator_metrics_controller.go @@ -50,6 +50,7 @@ const ( operatorMetricsTokenExpiry = time.Duration(operatorMetricsTokenExpirySecs) * time.Second operatorMetricsBearerTokenKey = "token" operatorMetricsBearerTokenExpiryKey = "expiry" + operatorMetricsTokenRenewalPercent = 20 ) type serviceAccountTokenRequester interface { @@ -247,16 +248,16 @@ func (r *OperatorMetricsTokenReconciler) reconcileBearerTokenSecret(ctx context. } else if secret.Type != corev1.SecretTypeOpaque || len(secret.Data[operatorMetricsBearerTokenKey]) == 0 { needsRefresh = true } else { - expiry, parseErr := parseBearerTokenExpiry(secret.Data[operatorMetricsBearerTokenExpiryKey]) - if parseErr != nil || !time.Now().Before(expiry) { + expiry, parseErr := parseBearerTokenTimestamp(secret.Data[operatorMetricsBearerTokenExpiryKey]) + if parseErr != nil { + reqLogger.Error(parseErr, "bearer token secret has unparseable expiry, renewal needed", + "Namespace", namespace, "Name", operatorMetricsBearerTokenSecretName, + "expiry", string(secret.Data[operatorMetricsBearerTokenExpiryKey])) + needsRefresh = true + } else if requeueAfter, refresh := evaluateBearerTokenRenewal(expiry, time.Now()); refresh { needsRefresh = true } else { - requeueAfter := bearerTokenRequeueDuration(expiry) - if requeueAfter <= 0 { - needsRefresh = true - } else { - return requeueAfter, nil - } + return requeueAfter, nil } } @@ -281,6 +282,7 @@ func (r *OperatorMetricsTokenReconciler) reconcileBearerTokenSecret(ctx context. operatorMetricsBearerTokenExpiryKey: []byte(expiry.UTC().Format(time.RFC3339)), }, } + requeueAfter, _ := evaluateBearerTokenRenewal(expiry, time.Now()) existingSecret := &corev1.Secret{} getErr := r.Client.Get(ctx, types.NamespacedName{ @@ -294,7 +296,7 @@ func (r *OperatorMetricsTokenReconciler) reconcileBearerTokenSecret(ctx context. if err := r.Client.Create(ctx, desiredSecret); err != nil { return 0, err } - return bearerTokenRequeueDuration(expiry), nil + return requeueAfter, nil } return 0, getErr } @@ -307,20 +309,32 @@ func (r *OperatorMetricsTokenReconciler) reconcileBearerTokenSecret(ctx context. return 0, err } - return bearerTokenRequeueDuration(expiry), nil + return requeueAfter, nil } -func parseBearerTokenExpiry(expiryData []byte) (time.Time, error) { - return time.Parse(time.RFC3339, string(expiryData)) +func parseBearerTokenTimestamp(value []byte) (time.Time, error) { + return time.Parse(time.RFC3339, string(value)) } -// bearerTokenRequeueDuration returns how long to wait before renewing the token. -// Renewal is scheduled after one third of the *actual* remaining lifetime has -// elapsed, so cluster-capped TokenRequest lifetimes still get a renewal. -func bearerTokenRequeueDuration(expiry time.Time) time.Duration { - remaining := time.Until(expiry) +// bearerTokenRenewalLead is how long before expiry renewal should happen. It +// matches one third of the requested token lifetime: renew once two thirds of +// the nominal TTL remain (equivalent to renewing after one third has elapsed on +// a fully granted token). +func bearerTokenRenewalLead() time.Duration { + return operatorMetricsTokenExpiry * operatorMetricsTokenRenewalPercent / 100 +} + +// evaluateBearerTokenRenewal decides whether to refresh now and, if not, when +// to requeue. Only expiry is stored in the Secret; the renewal boundary is +// expiry minus a fixed lead derived from the requested TTL. +func evaluateBearerTokenRenewal(expiry, now time.Time) (time.Duration, bool) { + remaining := expiry.Sub(now) if remaining <= 0 { - return 0 + return 0, true + } + lead := bearerTokenRenewalLead() + if remaining <= lead { + return 0, true } - return remaining / 3 + return remaining - lead, false } diff --git a/controllers/operator_metrics_controller_test.go b/controllers/operator_metrics_controller_test.go index 0bbdaa0702e..5d8bc31f589 100644 --- a/controllers/operator_metrics_controller_test.go +++ b/controllers/operator_metrics_controller_test.go @@ -123,18 +123,25 @@ func TestGetOperatorNamespace_trimsNewline(t *testing.T) { assert.Equal(t, ns, "openshift-gitops-operator") } -func TestBearerTokenRequeueDuration(t *testing.T) { - expiry := time.Now().Add(operatorMetricsTokenExpiry) - requeue := bearerTokenRequeueDuration(expiry) - assert.Assert(t, requeue > 19*time.Minute && requeue <= 20*time.Minute) +func TestBearerTokenRenewalLead(t *testing.T) { + lead := bearerTokenRenewalLead() + assert.Assert(t, lead > 11*time.Minute && lead <= 12*time.Minute) +} + +func TestEvaluateBearerTokenRenewal(t *testing.T) { + now := time.Now() + expiry := now.Add(operatorMetricsTokenExpiry) + + requeueAfter, needsRefresh := evaluateBearerTokenRenewal(expiry, now) + assert.Assert(t, !needsRefresh) + assert.Assert(t, requeueAfter > 47*time.Minute && requeueAfter <= 48*time.Minute) - // Cluster-capped shorter lifetime must still schedule renewal. - shortExpiry := time.Now().Add(30 * time.Minute) - shortRequeue := bearerTokenRequeueDuration(shortExpiry) - assert.Assert(t, shortRequeue > 9*time.Minute && shortRequeue <= 10*time.Minute) + lead := bearerTokenRenewalLead() + _, needsRefresh = evaluateBearerTokenRenewal(now.Add(lead), now) + assert.Assert(t, needsRefresh) - expiredExpiry := time.Now().Add(-time.Minute) - assert.Equal(t, bearerTokenRequeueDuration(expiredExpiry), time.Duration(0)) + _, needsRefresh = evaluateBearerTokenRenewal(now.Add(-time.Minute), now) + assert.Assert(t, needsRefresh) } func TestOperatorMetricsTokenReconciler_migratesServiceMonitorAuth(t *testing.T) { @@ -224,9 +231,11 @@ func TestOperatorMetricsTokenReconciler_replacesLegacySecret(t *testing.T) { assert.NilError(t, err) assert.Equal(t, secret.Type, corev1.SecretTypeOpaque) assert.Equal(t, string(secret.Data[operatorMetricsBearerTokenKey]), "new-token") - parsedExpiry, err := parseBearerTokenExpiry(secret.Data[operatorMetricsBearerTokenExpiryKey]) + parsedExpiry, err := parseBearerTokenTimestamp(secret.Data[operatorMetricsBearerTokenExpiryKey]) assert.NilError(t, err) assert.Equal(t, parsedExpiry.UTC().Format(time.RFC3339), expiry.UTC().Format(time.RFC3339)) + _, hasIssuedAt := secret.Data["issuedAt"] + assert.Assert(t, !hasIssuedAt) } func TestOperatorMetricsTokenReconciler_keepsLegacySecretIfTokenRequestFails(t *testing.T) { @@ -379,6 +388,120 @@ func TestOperatorMetricsTokenReconciler_refreshesSecretWithMissingToken(t *testi assert.Equal(t, string(secret.Data[operatorMetricsBearerTokenKey]), "minted-token") } +func TestOperatorMetricsTokenReconciler_schedulesRenewalFromExpiry(t *testing.T) { + writeOperatorNamespaceFile(t, testOperatorNamespace) + + s := newOperatorMetricsTokenScheme() + serviceMonitor := newOperatorMetricsServiceMonitor(testOperatorNamespace, false) + expiry := time.Now().Add(operatorMetricsTokenExpiry) + validSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: operatorMetricsBearerTokenSecretName, + Namespace: testOperatorNamespace, + }, + Type: corev1.SecretTypeOpaque, + Data: map[string][]byte{ + operatorMetricsBearerTokenKey: []byte("current-token"), + operatorMetricsBearerTokenExpiryKey: []byte(expiry.UTC().Format(time.RFC3339)), + }, + } + c := fake.NewClientBuilder().WithScheme(s).WithObjects(serviceMonitor, validSecret).Build() + + r := &OperatorMetricsTokenReconciler{ + Client: c, + Scheme: s, + TokenRequester: &fakeTokenRequester{ + token: "unexpected-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 > 47*time.Minute && result.RequeueAfter <= 48*time.Minute) + + secret := &corev1.Secret{} + err = c.Get(context.Background(), types.NamespacedName{ + Name: operatorMetricsBearerTokenSecretName, + Namespace: testOperatorNamespace, + }, secret) + assert.NilError(t, err) + assert.Equal(t, string(secret.Data[operatorMetricsBearerTokenKey]), "current-token") + + result, err = r.Reconcile(context.Background(), reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: operatorMetricsMonitorName, + Namespace: testOperatorNamespace, + }, + }) + assert.NilError(t, err) + assert.Assert(t, result.RequeueAfter > 47*time.Minute && result.RequeueAfter <= 48*time.Minute) +} + +func TestOperatorMetricsTokenReconciler_refreshesTokenAtRenewalDeadline(t *testing.T) { + writeOperatorNamespaceFile(t, testOperatorNamespace) + + s := newOperatorMetricsTokenScheme() + serviceMonitor := newOperatorMetricsServiceMonitor(testOperatorNamespace, false) + serviceMonitor.Spec.Endpoints[0].Authorization = &monitoringv1.SafeAuthorization{ + Type: "Bearer", + Credentials: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: operatorMetricsBearerTokenSecretName, + }, + Key: operatorMetricsBearerTokenKey, + }, + } + now := time.Now() + dueSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: operatorMetricsBearerTokenSecretName, + Namespace: testOperatorNamespace, + }, + Type: corev1.SecretTypeOpaque, + Data: map[string][]byte{ + operatorMetricsBearerTokenKey: []byte("old-token"), + operatorMetricsBearerTokenExpiryKey: []byte(now.Add(bearerTokenRenewalLead()).UTC().Format(time.RFC3339)), + }, + } + c := fake.NewClientBuilder().WithScheme(s).WithObjects(serviceMonitor, dueSecret).Build() + + newExpiry := time.Now().Add(operatorMetricsTokenExpiry) + r := &OperatorMetricsTokenReconciler{ + Client: c, + Scheme: s, + TokenRequester: &fakeTokenRequester{ + token: "renewed-token", + expiry: newExpiry, + }, + } + + _, err := r.Reconcile(context.Background(), reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: operatorMetricsMonitorName, + Namespace: testOperatorNamespace, + }, + }) + assert.NilError(t, err) + + secret := &corev1.Secret{} + err = c.Get(context.Background(), types.NamespacedName{ + Name: operatorMetricsBearerTokenSecretName, + Namespace: testOperatorNamespace, + }, secret) + assert.NilError(t, err) + assert.Equal(t, string(secret.Data[operatorMetricsBearerTokenKey]), "renewed-token") + _, hasIssuedAt := secret.Data["issuedAt"] + assert.Assert(t, !hasIssuedAt) + _, hasRenewAt := secret.Data["renewAt"] + assert.Assert(t, !hasRenewAt) +} + func TestOperatorMetricsTokenReconciler_skipsOtherNamespaces(t *testing.T) { writeOperatorNamespaceFile(t, testOperatorNamespace) From 544324e2f7f50c8921b9a3cd7a754d5bcc1e0c4f Mon Sep 17 00:00:00 2001 From: Alka Kumari Date: Wed, 19 Aug 2026 14:03:55 +0530 Subject: [PATCH 5/9] fix: delete and recreate legacy metrics bearer token Secret on migration assisted-by: claude-code Signed-off-by: Alka Kumari --- controllers/operator_metrics_controller.go | 21 +++++++++++++--- .../operator_metrics_controller_test.go | 24 +++++++++++++++++-- 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/controllers/operator_metrics_controller.go b/controllers/operator_metrics_controller.go index 83de39ea7d6..df157dcb4a3 100644 --- a/controllers/operator_metrics_controller.go +++ b/controllers/operator_metrics_controller.go @@ -237,13 +237,16 @@ func (r *OperatorMetricsTokenReconciler) reconcileBearerTokenSecret(ctx context. }, secret) needsRefresh := false + legacySAToken := false if errors.IsNotFound(err) { needsRefresh = true } else if err != nil { return 0, err } else if secret.Type == corev1.SecretTypeServiceAccountToken { - // Migrate legacy Secret in place after TokenRequest succeeds so scrape auth - // is not interrupted if minting fails. + // Keep the legacy Secret until TokenRequest succeeds so scrape auth + // is not interrupted if minting fails. Replace it via delete/create + // because Secret type is immutable in the API. + legacySAToken = true needsRefresh = true } else if secret.Type != corev1.SecretTypeOpaque || len(secret.Data[operatorMetricsBearerTokenKey]) == 0 { needsRefresh = true @@ -284,6 +287,18 @@ func (r *OperatorMetricsTokenReconciler) reconcileBearerTokenSecret(ctx context. } requeueAfter, _ := evaluateBearerTokenRenewal(expiry, time.Now()) + 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 requeueAfter, nil + } + existingSecret := &corev1.Secret{} getErr := r.Client.Get(ctx, types.NamespacedName{ Name: operatorMetricsBearerTokenSecretName, @@ -317,7 +332,7 @@ func parseBearerTokenTimestamp(value []byte) (time.Time, error) { } // bearerTokenRenewalLead is how long before expiry renewal should happen. It -// matches one third of the requested token lifetime: renew once two thirds of +// matches 20% of the requested token lifetime: renew once 80% of // the nominal TTL remain (equivalent to renewing after one third has elapsed on // a fully granted token). func bearerTokenRenewalLead() time.Duration { diff --git a/controllers/operator_metrics_controller_test.go b/controllers/operator_metrics_controller_test.go index 5d8bc31f589..fb0b3c8a5ea 100644 --- a/controllers/operator_metrics_controller_test.go +++ b/controllers/operator_metrics_controller_test.go @@ -35,6 +35,7 @@ import ( "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) @@ -73,6 +74,25 @@ func newOperatorMetricsTokenScheme() *runtime.Scheme { return s } +func newOperatorMetricsClientBuilder(s *runtime.Scheme) *fake.ClientBuilder { + return fake.NewClientBuilder().WithScheme(s).WithInterceptorFuncs(interceptor.Funcs{ + Update: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.UpdateOption) error { + secret, ok := obj.(*corev1.Secret) + if !ok { + return c.Update(ctx, obj, opts...) + } + existing := &corev1.Secret{} + if err := c.Get(ctx, client.ObjectKeyFromObject(secret), existing); err != nil { + return c.Update(ctx, obj, opts...) + } + if existing.Type == corev1.SecretTypeServiceAccountToken && secret.Type != existing.Type { + return fmt.Errorf("secret type is immutable") + } + return c.Update(ctx, obj, opts...) + }, + }) +} + func newOperatorMetricsServiceMonitor(namespace string, useLegacyAuth bool) *monitoringv1.ServiceMonitor { sm := &monitoringv1.ServiceMonitor{ ObjectMeta: metav1.ObjectMeta{ @@ -175,7 +195,7 @@ func TestOperatorMetricsTokenReconciler_migratesServiceMonitorAuth(t *testing.T) Namespace: testOperatorNamespace, }, updatedSM) assert.NilError(t, err) - assert.Assert(t, is.Nil(updatedSM.Spec.Endpoints[0].BearerTokenSecret)) //nolint:staticcheck // SA1019: migrate deprecated bearerTokenSecret to authorization//nolint:staticcheck // SA1019: migrate deprecated bearerTokenSecret to authorization + assert.Assert(t, is.Nil(updatedSM.Spec.Endpoints[0].BearerTokenSecret)) //nolint:staticcheck // SA1019: migrate deprecated bearerTokenSecret to authorization 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) @@ -203,7 +223,7 @@ func TestOperatorMetricsTokenReconciler_replacesLegacySecret(t *testing.T) { }, Type: corev1.SecretTypeServiceAccountToken, } - c := fake.NewClientBuilder().WithScheme(s).WithObjects(serviceMonitor, legacySecret).Build() + c := newOperatorMetricsClientBuilder(s).WithObjects(serviceMonitor, legacySecret).Build() expiry := time.Now().Add(operatorMetricsTokenExpiry) r := &OperatorMetricsTokenReconciler{ From 135b367912d1a1b5f678be7982e976c8c64a8810 Mon Sep 17 00:00:00 2001 From: Alka Kumari Date: Thu, 27 Aug 2026 14:31:29 +0530 Subject: [PATCH 6/9] fix: handover creation of operator metrics ServiceMonitor to controllerr after secret creation. Assisted-by: Cursor Signed-off-by: Alka Kumari --- ...gitops-operator.clusterserviceversion.yaml | 2 +- ...nitoring.coreos.com_v1_servicemonitor.yaml | 26 -- config/prometheus/monitor.yaml | 30 +- controllers/operator_metrics_controller.go | 370 +++++++++++++----- .../operator_metrics_controller_test.go | 126 +++++- ...date_operator_metrics_bearer_token_test.go | 343 ++++++++++++++++ 6 files changed, 724 insertions(+), 173 deletions(-) delete mode 100644 bundle/manifests/openshift-gitops-operator-metrics-monitor_monitoring.coreos.com_v1_servicemonitor.yaml create mode 100644 test/openshift/e2e/ginkgo/sequential/1-133_validate_operator_metrics_bearer_token_test.go diff --git a/bundle/manifests/gitops-operator.clusterserviceversion.yaml b/bundle/manifests/gitops-operator.clusterserviceversion.yaml index d0944546d02..a057770809c 100644 --- a/bundle/manifests/gitops-operator.clusterserviceversion.yaml +++ b/bundle/manifests/gitops-operator.clusterserviceversion.yaml @@ -190,7 +190,7 @@ metadata: capabilities: Deep Insights console.openshift.io/plugins: '["gitops-plugin"]' containerImage: quay.io/redhat-developer/gitops-operator - createdAt: "2026-08-17T09:26:28Z" + createdAt: "2026-08-27T08:58:36Z" description: Enables teams to adopt GitOps principles for managing cluster configurations and application delivery across hybrid multi-cluster Kubernetes environments. features.operators.openshift.io/disconnected: "true" diff --git a/bundle/manifests/openshift-gitops-operator-metrics-monitor_monitoring.coreos.com_v1_servicemonitor.yaml b/bundle/manifests/openshift-gitops-operator-metrics-monitor_monitoring.coreos.com_v1_servicemonitor.yaml deleted file mode 100644 index 87d125c6904..00000000000 --- a/bundle/manifests/openshift-gitops-operator-metrics-monitor_monitoring.coreos.com_v1_servicemonitor.yaml +++ /dev/null @@ -1,26 +0,0 @@ -apiVersion: monitoring.coreos.com/v1 -kind: ServiceMonitor -metadata: - labels: - control-plane: gitops-operator - name: openshift-gitops-operator-metrics-monitor -spec: - endpoints: - - authorization: - credentials: - key: token - name: openshift-gitops-operator-metrics-monitor-bearer-token - type: Bearer - interval: 30s - path: /metrics - port: metrics - scheme: https - tlsConfig: - ca: - configMap: - key: service-ca.crt - name: openshift-gitops-operator-metrics-monitor-ca-bundle - serverName: openshift-gitops-operator-metrics-service.openshift-gitops-operator.svc - selector: - matchLabels: - control-plane: gitops-operator diff --git a/config/prometheus/monitor.yaml b/config/prometheus/monitor.yaml index 76af779b912..24657916cba 100644 --- a/config/prometheus/monitor.yaml +++ b/config/prometheus/monitor.yaml @@ -1,4 +1,6 @@ --- +# CA bundle for the operator metrics ServiceMonitor TLS config. The ServiceMonitor +# itself is created by OperatorMetricsTokenReconciler after the bearer token Secret. apiVersion: v1 kind: ConfigMap metadata: @@ -9,31 +11,3 @@ metadata: service.beta.openshift.io/inject-cabundle: "true" name: metrics-monitor-ca-bundle namespace: openshift-gitops-operator ---- -apiVersion: monitoring.coreos.com/v1 -kind: ServiceMonitor -metadata: - name: metrics-monitor - namespace: openshift-gitops-operator - labels: - control-plane: gitops-operator -spec: - selector: - matchLabels: - control-plane: gitops-operator - endpoints: - - authorization: - type: Bearer - credentials: - name: openshift-gitops-operator-metrics-monitor-bearer-token - key: token - interval: 30s - path: /metrics - port: metrics - scheme: https - tlsConfig: - ca: - configMap: - name: openshift-gitops-operator-metrics-monitor-ca-bundle - key: service-ca.crt - serverName: openshift-gitops-operator-metrics-service.openshift-gitops-operator.svc diff --git a/controllers/operator_metrics_controller.go b/controllers/operator_metrics_controller.go index df157dcb4a3..48768426819 100644 --- a/controllers/operator_metrics_controller.go +++ b/controllers/operator_metrics_controller.go @@ -42,15 +42,22 @@ import ( ) const ( + // Metrics resources owned by this reconciler. The metrics Service and CA + // bundle ConfigMap are still shipped in the operator bundle; only the + // ServiceMonitor and bearer token Secret are managed here so the Secret + // always exists before Prometheus starts scraping. operatorMetricsServiceName = "openshift-gitops-operator-metrics-service" operatorMetricsMonitorName = "openshift-gitops-operator-metrics-monitor" - operatorMetricsBearerTokenSecretName = "openshift-gitops-operator-metrics-monitor-bearer-token" + operatorMetricsCABundleConfigMapName = operatorMetricsMonitorName + "-ca-bundle" + operatorMetricsBearerTokenSecretName = operatorMetricsMonitorName + "-bearer-token" operatorControllerSAName = "openshift-gitops-operator-controller-manager" operatorMetricsTokenExpirySecs = int64(3600) operatorMetricsTokenExpiry = time.Duration(operatorMetricsTokenExpirySecs) * time.Second operatorMetricsBearerTokenKey = "token" operatorMetricsBearerTokenExpiryKey = "expiry" operatorMetricsTokenRenewalPercent = 20 + operatorMetricsControlPlaneLabel = "control-plane" + operatorMetricsControlPlaneLabelVal = "gitops-operator" ) type serviceAccountTokenRequester interface { @@ -79,8 +86,12 @@ func (r *clientServiceAccountTokenRequester) RequestToken(ctx context.Context, n return tr.Status.Token, tr.Status.ExpirationTimestamp.Time, nil } -// OperatorMetricsTokenReconciler manages the short-lived bearer token Secret used by the -// operator's ServiceMonitor for Prometheus metrics scraping. +// OperatorMetricsTokenReconciler manages the short-lived bearer token Secret and +// the ServiceMonitor that references it for operator Prometheus metrics scraping. +// +// The ServiceMonitor is intentionally created by this controller (not the bundle) +// so the bearer token Secret is minted first. That avoids a window where the +// ServiceMonitor exists but the referenced Secret does not yet. type OperatorMetricsTokenReconciler struct { Client client.Client Scheme *runtime.Scheme @@ -96,8 +107,13 @@ func (r *OperatorMetricsTokenReconciler) tokenRequester() serviceAccountTokenReq return &clientServiceAccountTokenRequester{client: r.Client} } -// SetupWithManager sets up the controller with the Manager. +// SetupWithManager wires watches for the metrics Service (always present from the +// bundle), the bearer token Secret, and the ServiceMonitor. All events reconcile +// the same operator-metrics resources in the operator namespace. func (r *OperatorMetricsTokenReconciler) SetupWithManager(mgr ctrl.Manager) error { + metricsServicePredicate := predicate.NewPredicateFuncs(func(obj client.Object) bool { + return obj.GetName() == operatorMetricsServiceName + }) serviceMonitorPredicate := predicate.NewPredicateFuncs(func(obj client.Object) bool { return obj.GetName() == operatorMetricsMonitorName }) @@ -107,16 +123,27 @@ func (r *OperatorMetricsTokenReconciler) SetupWithManager(mgr ctrl.Manager) erro return ctrl.NewControllerManagedBy(mgr). Named("operator-metrics-token"). - For(&monitoringv1.ServiceMonitor{}, builder.WithPredicates(serviceMonitorPredicate)). + // The metrics Service is installed with the operator and provides a stable + // initial reconcile trigger even before the ServiceMonitor exists. + For(&corev1.Service{}, builder.WithPredicates(metricsServicePredicate)). Watches( &corev1.Secret{}, - handler.EnqueueRequestsFromMapFunc(mapBearerTokenSecretToServiceMonitor), + handler.EnqueueRequestsFromMapFunc(mapOperatorMetricsResourceToReconcile), builder.WithPredicates(bearerTokenSecretPredicate), ). + // Watch ServiceMonitor updates (for example legacy auth migration) and + // token renewals that only touch the Secret. + Watches( + &monitoringv1.ServiceMonitor{}, + handler.EnqueueRequestsFromMapFunc(mapOperatorMetricsResourceToReconcile), + builder.WithPredicates(serviceMonitorPredicate), + ). Complete(r) } -func mapBearerTokenSecretToServiceMonitor(_ context.Context, obj client.Object) []reconcile.Request { +// mapOperatorMetricsResourceToReconcile maps Secret and ServiceMonitor events to +// the fixed reconcile key used for operator metrics resources. +func mapOperatorMetricsResourceToReconcile(_ context.Context, obj client.Object) []reconcile.Request { return []reconcile.Request{{ NamespacedName: types.NamespacedName{ Namespace: obj.GetNamespace(), @@ -126,7 +153,7 @@ func mapBearerTokenSecretToServiceMonitor(_ context.Context, obj client.Object) } //+kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;create;update;patch;delete -//+kubebuilder:rbac:groups=monitoring.coreos.com,resources=servicemonitors,verbs=get;list;watch;update;patch +//+kubebuilder:rbac:groups=monitoring.coreos.com,resources=servicemonitors,verbs=get;list;watch;create;update;patch func (r *OperatorMetricsTokenReconciler) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) { reqLogger := logf.Log.WithName("controller_operator_metrics_token"). @@ -142,25 +169,28 @@ func (r *OperatorMetricsTokenReconciler) Reconcile(ctx context.Context, request return reconcile.Result{}, err } - if request.Namespace != operatorNS || request.Name != operatorMetricsMonitorName { + if request.Namespace != operatorNS { return reconcile.Result{}, nil } - - serviceMonitor := &monitoringv1.ServiceMonitor{} - if err := r.Client.Get(ctx, request.NamespacedName, serviceMonitor); err != nil { - if errors.IsNotFound(err) { - return reconcile.Result{}, nil - } - reqLogger.Error(err, "Error querying for ServiceMonitor") - return reconcile.Result{}, err + if request.Name != operatorMetricsMonitorName && request.Name != operatorMetricsServiceName { + return reconcile.Result{}, nil } - if err := r.reconcileServiceMonitor(ctx, serviceMonitor, operatorNS, reqLogger); err != nil { + // Ensure the bearer token Secret exists and is current. The + // ServiceMonitor must not be created until this succeeds. + requeueAfter, secretReady, err := r.reconcileBearerTokenSecret(ctx, operatorNS, reqLogger) + if err != nil { return reconcile.Result{}, err } + if !secretReady { + if requeueAfter > 0 { + reqLogger.Info("Waiting for metrics bearer token Secret before creating ServiceMonitor", "after", requeueAfter.String()) + } + return reconcile.Result{RequeueAfter: requeueAfter}, nil + } - requeueAfter, err := r.reconcileBearerTokenSecret(ctx, operatorNS, reqLogger) - if err != nil { + // Create or update the ServiceMonitor now that scrape auth exists. + if err := r.reconcileServiceMonitor(ctx, operatorNS, reqLogger); err != nil { return reconcile.Result{}, err } @@ -178,45 +208,146 @@ func getOperatorNamespace() (string, error) { return strings.TrimSpace(string(data)), nil } -func (r *OperatorMetricsTokenReconciler) reconcileServiceMonitor(ctx context.Context, serviceMonitor *monitoringv1.ServiceMonitor, operatorNS string, reqLogger logr.Logger) error { - if len(serviceMonitor.Spec.Endpoints) == 0 { - return fmt.Errorf("ServiceMonitor %s has no endpoints", serviceMonitor.Name) +// desiredOperatorMetricsServiceMonitor returns the ServiceMonitor spec written by +// this controller. Keep this aligned with the historical bundle manifest. +func desiredOperatorMetricsServiceMonitor(operatorNS string) *monitoringv1.ServiceMonitor { + serverName := operatorMetricsServiceName + "." + operatorNS + ".svc" + return &monitoringv1.ServiceMonitor{ + ObjectMeta: metav1.ObjectMeta{ + Name: operatorMetricsMonitorName, + Namespace: operatorNS, + Labels: map[string]string{ + operatorMetricsControlPlaneLabel: operatorMetricsControlPlaneLabelVal, + }, + }, + Spec: monitoringv1.ServiceMonitorSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + operatorMetricsControlPlaneLabel: operatorMetricsControlPlaneLabelVal, + }, + }, + Endpoints: []monitoringv1.Endpoint{ + { + Authorization: &monitoringv1.SafeAuthorization{ + Type: "Bearer", + Credentials: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: operatorMetricsBearerTokenSecretName, + }, + Key: operatorMetricsBearerTokenKey, + }, + }, + Interval: monitoringv1.Duration("30s"), + Path: "/metrics", + Port: "metrics", + Scheme: "https", + TLSConfig: &monitoringv1.TLSConfig{ + SafeTLSConfig: monitoringv1.SafeTLSConfig{ + CA: monitoringv1.SecretOrConfigMap{ + ConfigMap: &corev1.ConfigMapKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: operatorMetricsCABundleConfigMapName, + }, + Key: "service-ca.crt", + }, + }, + ServerName: &serverName, + }, + }, + }, + }, + }, } +} - desiredMetricsServerName := operatorMetricsServiceName + "." + operatorNS + ".svc" - endpoint := &serviceMonitor.Spec.Endpoints[0] +// reconcileServiceMonitor creates or updates the operator metrics ServiceMonitor. +// It is only called after reconcileBearerTokenSecret reports the bearer token +// Secret is ready, so Prometheus never observes a missing credentials Secret. +func (r *OperatorMetricsTokenReconciler) reconcileServiceMonitor(ctx context.Context, operatorNS string, reqLogger logr.Logger) error { + desired := desiredOperatorMetricsServiceMonitor(operatorNS) + + serviceMonitor := &monitoringv1.ServiceMonitor{} + err := r.Client.Get(ctx, types.NamespacedName{ + Name: operatorMetricsMonitorName, + Namespace: operatorNS, + }, serviceMonitor) + if errors.IsNotFound(err) { + reqLogger.Info("Creating operator metrics ServiceMonitor", + "Namespace", operatorNS, "Name", operatorMetricsMonitorName) + return r.Client.Create(ctx, desired) + } + if err != nil { + return err + } updated := false - if endpoint.BearerTokenSecret != nil { //nolint:staticcheck // SA1019: migrate deprecated bearerTokenSecret to authorization - endpoint.BearerTokenSecret = nil //nolint:staticcheck // SA1019: migrate deprecated bearerTokenSecret to authorization - endpoint.Authorization = &monitoringv1.SafeAuthorization{ - Type: "Bearer", - Credentials: &corev1.SecretKeySelector{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: operatorMetricsBearerTokenSecretName, - }, - Key: operatorMetricsBearerTokenKey, - }, - } + if serviceMonitor.Labels == nil { + serviceMonitor.Labels = map[string]string{} + } + if serviceMonitor.Labels[operatorMetricsControlPlaneLabel] != operatorMetricsControlPlaneLabelVal { + serviceMonitor.Labels[operatorMetricsControlPlaneLabel] = operatorMetricsControlPlaneLabelVal 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, - }, - } + } + + if len(serviceMonitor.Spec.Endpoints) == 0 { + serviceMonitor.Spec.Endpoints = desired.Spec.Endpoints updated = true + } else { + endpoint := &serviceMonitor.Spec.Endpoints[0] + desiredEndpoint := desired.Spec.Endpoints[0] + + if endpoint.BearerTokenSecret != nil { //nolint:staticcheck // SA1019: migrate deprecated bearerTokenSecret to authorization + // Upgrades may still carry the deprecated field from older bundle installs. + endpoint.BearerTokenSecret = nil //nolint:staticcheck // SA1019: migrate deprecated bearerTokenSecret to authorization + updated = true + } + if endpoint.Authorization == nil || + endpoint.Authorization.Type != desiredEndpoint.Authorization.Type || + endpoint.Authorization.Credentials == nil || + endpoint.Authorization.Credentials.Name != operatorMetricsBearerTokenSecretName || + endpoint.Authorization.Credentials.Key != operatorMetricsBearerTokenKey { + endpoint.Authorization = desiredEndpoint.Authorization + updated = true + } + if endpoint.Interval != desiredEndpoint.Interval || + endpoint.Path != desiredEndpoint.Path || + endpoint.Port != desiredEndpoint.Port || + endpoint.Scheme != desiredEndpoint.Scheme { + endpoint.Interval = desiredEndpoint.Interval + endpoint.Path = desiredEndpoint.Path + endpoint.Port = desiredEndpoint.Port + endpoint.Scheme = desiredEndpoint.Scheme + updated = true + } + + desiredServerName := desiredEndpoint.TLSConfig.SafeTLSConfig.ServerName + if endpoint.TLSConfig == nil || + endpoint.TLSConfig.ServerName == nil || + desiredServerName == nil || + *endpoint.TLSConfig.ServerName != *desiredServerName { + if endpoint.TLSConfig == nil { + endpoint.TLSConfig = &monitoringv1.TLSConfig{} + } + endpoint.TLSConfig.ServerName = desiredServerName + updated = true + } + + desiredCA := desiredEndpoint.TLSConfig.SafeTLSConfig.CA.ConfigMap + if endpoint.TLSConfig == nil || + endpoint.TLSConfig.CA.ConfigMap == nil || + desiredCA == nil || + endpoint.TLSConfig.CA.ConfigMap.Name != desiredCA.Name || + endpoint.TLSConfig.CA.ConfigMap.Key != desiredCA.Key { + if endpoint.TLSConfig == nil { + endpoint.TLSConfig = &monitoringv1.TLSConfig{} + } + endpoint.TLSConfig.CA = desiredEndpoint.TLSConfig.CA + updated = true + } } - if endpoint.TLSConfig != nil && (endpoint.TLSConfig.ServerName == nil || *endpoint.TLSConfig.ServerName != desiredMetricsServerName) { - endpoint.TLSConfig.ServerName = &desiredMetricsServerName + if !mapsEqual(serviceMonitor.Spec.Selector.MatchLabels, desired.Spec.Selector.MatchLabels) { + serviceMonitor.Spec.Selector = desired.Spec.Selector updated = true } @@ -229,49 +360,111 @@ func (r *OperatorMetricsTokenReconciler) reconcileServiceMonitor(ctx context.Con return r.Client.Update(ctx, serviceMonitor) } -func (r *OperatorMetricsTokenReconciler) reconcileBearerTokenSecret(ctx context.Context, namespace string, reqLogger logr.Logger) (time.Duration, error) { - secret := &corev1.Secret{} +func mapsEqual(a, b map[string]string) bool { + if len(a) != len(b) { + return false + } + for key, value := range a { + if b[key] != value { + return false + } + } + return true +} + +// isLegacyMetricsBearerTokenSecret reports whether the Secret is a deprecated +// kubernetes.io/service-account-token entry that must be replaced via delete/create. +func isLegacyMetricsBearerTokenSecret(secret *corev1.Secret) bool { + return secret.Type == corev1.SecretTypeServiceAccountToken +} + +// metricsBearerTokenSecretMustReplace reports whether the Secret cannot be +// updated in place. Secret type is immutable, so any non-Opaque Secret (legacy +// or otherwise) must be deleted and recreated. +func metricsBearerTokenSecretMustReplace(existing *corev1.Secret) bool { + return existing.Type != corev1.SecretTypeOpaque +} + +func (r *OperatorMetricsTokenReconciler) applyMetricsBearerTokenSecret( + ctx context.Context, + namespace string, + desired *corev1.Secret, + existing *corev1.Secret, + reqLogger logr.Logger, +) error { + if existing == nil { + reqLogger.Info("Creating metrics monitor bearer token Secret", + "Namespace", namespace, "Name", operatorMetricsBearerTokenSecretName) + return r.Client.Create(ctx, desired) + } + + if metricsBearerTokenSecretMustReplace(existing) { + if isLegacyMetricsBearerTokenSecret(existing) { + reqLogger.Info("Replacing legacy non-expiring service account token Secret", + "Namespace", namespace, "Name", operatorMetricsBearerTokenSecretName) + } else { + reqLogger.Info("Replacing metrics bearer token Secret with incompatible type", + "Namespace", namespace, "Name", operatorMetricsBearerTokenSecretName, "Type", existing.Type) + } + if err := r.Client.Delete(ctx, existing); err != nil && !errors.IsNotFound(err) { + return err + } + return r.Client.Create(ctx, desired) + } + + existing.Data = desired.Data + reqLogger.Info("Updating metrics monitor bearer token Secret", + "Namespace", namespace, "Name", operatorMetricsBearerTokenSecretName) + return r.Client.Update(ctx, existing) +} + +// reconcileBearerTokenSecret mints or refreshes the short-lived bearer token +// Secret used for metrics scraping. +// +// The returned ready flag is true when an Opaque Secret with a usable token is +// present. Reconcile must not create the ServiceMonitor until ready is true. +func (r *OperatorMetricsTokenReconciler) reconcileBearerTokenSecret(ctx context.Context, namespace string, reqLogger logr.Logger) (time.Duration, bool, error) { + existing := &corev1.Secret{} err := r.Client.Get(ctx, types.NamespacedName{ Name: operatorMetricsBearerTokenSecretName, Namespace: namespace, - }, secret) + }, existing) needsRefresh := false - legacySAToken := false if errors.IsNotFound(err) { + existing = nil needsRefresh = true } else if err != nil { - return 0, err - } else if secret.Type == corev1.SecretTypeServiceAccountToken { - // Keep the legacy Secret until TokenRequest succeeds so scrape auth - // is not interrupted if minting fails. Replace it via delete/create - // because Secret type is immutable in the API. - legacySAToken = true + return 0, false, err + } else if metricsBearerTokenSecretMustReplace(existing) { + // Keep legacy Secrets until TokenRequest succeeds so scrape auth is not + // interrupted if minting fails. All non-Opaque Secrets are replaced via + // delete/create because Secret type is immutable in the API. needsRefresh = true - } else if secret.Type != corev1.SecretTypeOpaque || len(secret.Data[operatorMetricsBearerTokenKey]) == 0 { + } else if len(existing.Data[operatorMetricsBearerTokenKey]) == 0 { needsRefresh = true } else { - expiry, parseErr := parseBearerTokenTimestamp(secret.Data[operatorMetricsBearerTokenExpiryKey]) + expiry, parseErr := parseBearerTokenTimestamp(existing.Data[operatorMetricsBearerTokenExpiryKey]) if parseErr != nil { reqLogger.Error(parseErr, "bearer token secret has unparseable expiry, renewal needed", "Namespace", namespace, "Name", operatorMetricsBearerTokenSecretName, - "expiry", string(secret.Data[operatorMetricsBearerTokenExpiryKey])) + "expiry", string(existing.Data[operatorMetricsBearerTokenExpiryKey])) needsRefresh = true } else if requeueAfter, refresh := evaluateBearerTokenRenewal(expiry, time.Now()); refresh { needsRefresh = true } else { - return requeueAfter, nil + return requeueAfter, true, nil } } if !needsRefresh { - return 0, nil + return 0, true, nil } token, expiry, err := r.tokenRequester().RequestToken(ctx, namespace, operatorControllerSAName, operatorMetricsTokenExpirySecs) if err != nil { reqLogger.Error(err, "Failed to request service account token") - return 0, err + return 0, false, err } desiredSecret := &corev1.Secret{ @@ -287,44 +480,11 @@ func (r *OperatorMetricsTokenReconciler) reconcileBearerTokenSecret(ctx context. } requeueAfter, _ := evaluateBearerTokenRenewal(expiry, time.Now()) - 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 requeueAfter, nil - } - - existingSecret := &corev1.Secret{} - getErr := r.Client.Get(ctx, types.NamespacedName{ - Name: operatorMetricsBearerTokenSecretName, - Namespace: namespace, - }, existingSecret) - if getErr != nil { - if errors.IsNotFound(getErr) { - reqLogger.Info("Creating metrics monitor bearer token Secret", - "Namespace", namespace, "Name", operatorMetricsBearerTokenSecretName) - if err := r.Client.Create(ctx, desiredSecret); err != nil { - return 0, err - } - return requeueAfter, nil - } - return 0, getErr - } - - existingSecret.Type = corev1.SecretTypeOpaque - existingSecret.Data = desiredSecret.Data - reqLogger.Info("Updating metrics monitor bearer token Secret", - "Namespace", namespace, "Name", operatorMetricsBearerTokenSecretName) - if err := r.Client.Update(ctx, existingSecret); err != nil { - return 0, err + if err := r.applyMetricsBearerTokenSecret(ctx, namespace, desiredSecret, existing, reqLogger); err != nil { + return 0, false, err } - return requeueAfter, nil + return requeueAfter, true, nil } func parseBearerTokenTimestamp(value []byte) (time.Time, error) { @@ -332,9 +492,7 @@ func parseBearerTokenTimestamp(value []byte) (time.Time, error) { } // bearerTokenRenewalLead is how long before expiry renewal should happen. It -// matches 20% of the requested token lifetime: renew once 80% of -// the nominal TTL remain (equivalent to renewing after one third has elapsed on -// a fully granted token). +// matches 20% of the requested token lifetime. func bearerTokenRenewalLead() time.Duration { return operatorMetricsTokenExpiry * operatorMetricsTokenRenewalPercent / 100 } diff --git a/controllers/operator_metrics_controller_test.go b/controllers/operator_metrics_controller_test.go index fb0b3c8a5ea..8a3dfcc73eb 100644 --- a/controllers/operator_metrics_controller_test.go +++ b/controllers/operator_metrics_controller_test.go @@ -85,7 +85,7 @@ func newOperatorMetricsClientBuilder(s *runtime.Scheme) *fake.ClientBuilder { if err := c.Get(ctx, client.ObjectKeyFromObject(secret), existing); err != nil { return c.Update(ctx, obj, opts...) } - if existing.Type == corev1.SecretTypeServiceAccountToken && secret.Type != existing.Type { + if existing.Type != secret.Type { return fmt.Errorf("secret type is immutable") } return c.Update(ctx, obj, opts...) @@ -148,6 +148,67 @@ func TestBearerTokenRenewalLead(t *testing.T) { assert.Assert(t, lead > 11*time.Minute && lead <= 12*time.Minute) } +func TestIsLegacyMetricsBearerTokenSecret(t *testing.T) { + assert.Assert(t, isLegacyMetricsBearerTokenSecret(&corev1.Secret{ + Type: corev1.SecretTypeServiceAccountToken, + })) + assert.Assert(t, !isLegacyMetricsBearerTokenSecret(&corev1.Secret{ + Type: corev1.SecretTypeOpaque, + })) +} + +func TestMetricsBearerTokenSecretMustReplace(t *testing.T) { + legacy := &corev1.Secret{Type: corev1.SecretTypeServiceAccountToken} + assert.Assert(t, metricsBearerTokenSecretMustReplace(legacy)) + + opaque := &corev1.Secret{Type: corev1.SecretTypeOpaque} + assert.Assert(t, !metricsBearerTokenSecretMustReplace(opaque)) + + other := &corev1.Secret{Type: corev1.SecretTypeDockercfg} + assert.Assert(t, metricsBearerTokenSecretMustReplace(other)) +} + +func TestOperatorMetricsTokenReconciler_replacesIncompatibleSecretType(t *testing.T) { + writeOperatorNamespaceFile(t, testOperatorNamespace) + + s := newOperatorMetricsTokenScheme() + incompatibleSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: operatorMetricsBearerTokenSecretName, + Namespace: testOperatorNamespace, + }, + Type: corev1.SecretTypeDockercfg, + } + c := fake.NewClientBuilder().WithScheme(s).WithObjects(incompatibleSecret).Build() + + expiry := time.Now().Add(operatorMetricsTokenExpiry) + r := &OperatorMetricsTokenReconciler{ + Client: c, + Scheme: s, + TokenRequester: &fakeTokenRequester{ + token: "replaced-token", + expiry: expiry, + }, + } + + _, err := r.Reconcile(context.Background(), reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: operatorMetricsMonitorName, + Namespace: testOperatorNamespace, + }, + }) + assert.NilError(t, err) + + secret := &corev1.Secret{} + err = c.Get(context.Background(), types.NamespacedName{ + Name: operatorMetricsBearerTokenSecretName, + Namespace: testOperatorNamespace, + }, secret) + assert.NilError(t, err) + assert.Equal(t, secret.Type, corev1.SecretTypeOpaque) + assert.Equal(t, string(secret.Data[operatorMetricsBearerTokenKey]), "replaced-token") +} + func TestEvaluateBearerTokenRenewal(t *testing.T) { now := time.Now() expiry := now.Add(operatorMetricsTokenExpiry) @@ -164,6 +225,50 @@ func TestEvaluateBearerTokenRenewal(t *testing.T) { assert.Assert(t, needsRefresh) } +func TestOperatorMetricsTokenReconciler_createsServiceMonitorAfterSecret(t *testing.T) { + writeOperatorNamespaceFile(t, testOperatorNamespace) + + s := newOperatorMetricsTokenScheme() + c := fake.NewClientBuilder().WithScheme(s).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) + + secret := &corev1.Secret{} + err = c.Get(context.Background(), types.NamespacedName{ + Name: operatorMetricsBearerTokenSecretName, + Namespace: testOperatorNamespace, + }, secret) + assert.NilError(t, err) + assert.Equal(t, secret.Type, corev1.SecretTypeOpaque) + assert.Equal(t, string(secret.Data[operatorMetricsBearerTokenKey]), "test-token") + + serviceMonitor := &monitoringv1.ServiceMonitor{} + err = c.Get(context.Background(), types.NamespacedName{ + Name: operatorMetricsMonitorName, + Namespace: testOperatorNamespace, + }, serviceMonitor) + assert.NilError(t, err) + assert.Assert(t, serviceMonitor.Spec.Endpoints[0].Authorization != nil) + assert.Equal(t, serviceMonitor.Spec.Endpoints[0].Authorization.Credentials.Name, operatorMetricsBearerTokenSecretName) + assert.Equal(t, *serviceMonitor.Spec.Endpoints[0].TLSConfig.ServerName, operatorMetricsServiceName+"."+testOperatorNamespace+".svc") +} + func TestOperatorMetricsTokenReconciler_migratesServiceMonitorAuth(t *testing.T) { writeOperatorNamespaceFile(t, testOperatorNamespace) @@ -262,16 +367,6 @@ func TestOperatorMetricsTokenReconciler_keepsLegacySecretIfTokenRequestFails(t * writeOperatorNamespaceFile(t, testOperatorNamespace) s := newOperatorMetricsTokenScheme() - serviceMonitor := newOperatorMetricsServiceMonitor(testOperatorNamespace, false) - serviceMonitor.Spec.Endpoints[0].Authorization = &monitoringv1.SafeAuthorization{ - Type: "Bearer", - Credentials: &corev1.SecretKeySelector{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: operatorMetricsBearerTokenSecretName, - }, - Key: operatorMetricsBearerTokenKey, - }, - } legacySecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: operatorMetricsBearerTokenSecretName, @@ -282,7 +377,7 @@ func TestOperatorMetricsTokenReconciler_keepsLegacySecretIfTokenRequestFails(t * operatorMetricsBearerTokenKey: []byte("legacy-token"), }, } - c := fake.NewClientBuilder().WithScheme(s).WithObjects(serviceMonitor, legacySecret).Build() + c := fake.NewClientBuilder().WithScheme(s).WithObjects(legacySecret).Build() r := &OperatorMetricsTokenReconciler{ Client: c, @@ -308,6 +403,13 @@ func TestOperatorMetricsTokenReconciler_keepsLegacySecretIfTokenRequestFails(t * assert.NilError(t, err) assert.Equal(t, secret.Type, corev1.SecretTypeServiceAccountToken) assert.Equal(t, string(secret.Data[operatorMetricsBearerTokenKey]), "legacy-token") + + serviceMonitor := &monitoringv1.ServiceMonitor{} + err = c.Get(context.Background(), types.NamespacedName{ + Name: operatorMetricsMonitorName, + Namespace: testOperatorNamespace, + }, serviceMonitor) + assert.ErrorContains(t, err, "not found") } func TestOperatorMetricsTokenReconciler_refreshesExpiredToken(t *testing.T) { diff --git a/test/openshift/e2e/ginkgo/sequential/1-133_validate_operator_metrics_bearer_token_test.go b/test/openshift/e2e/ginkgo/sequential/1-133_validate_operator_metrics_bearer_token_test.go new file mode 100644 index 00000000000..c649317ddfd --- /dev/null +++ b/test/openshift/e2e/ginkgo/sequential/1-133_validate_operator_metrics_bearer_token_test.go @@ -0,0 +1,343 @@ +/* +Copyright 2025. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// E2E coverage for OperatorMetricsTokenReconciler. These tests run sequentially +// because they mutate shared resources in openshift-gitops-operator (the metrics +// bearer token Secret and ServiceMonitor). Mutating cases snapshot and restore +// cluster state so later tests are not affected. +package sequential + +import ( + "context" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" + "github.com/redhat-developer/gitops-operator/test/openshift/e2e/ginkgo/fixture" + k8sFixture "github.com/redhat-developer/gitops-operator/test/openshift/e2e/ginkgo/fixture/k8s" + secretFixture "github.com/redhat-developer/gitops-operator/test/openshift/e2e/ginkgo/fixture/secret" + "github.com/redhat-developer/gitops-operator/test/openshift/e2e/ginkgo/fixture/utils" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +const ( + // Shared operator namespace and resources managed by OperatorMetricsTokenReconciler. + operatorMetricsNS = "openshift-gitops-operator" + operatorMetricsMonitorName = "openshift-gitops-operator-metrics-monitor" + operatorMetricsBearerTokenSecretName = "openshift-gitops-operator-metrics-monitor-bearer-token" + operatorMetricsBearerTokenKey = "token" + operatorMetricsBearerTokenExpiryKey = "expiry" + operatorMetricsControllerSAName = "openshift-gitops-operator-controller-manager" + + // Mirror controller renewal settings: 20% of the requested one-hour TTL (~12 minutes). + operatorMetricsTokenExpiry = time.Hour + operatorMetricsTokenRenewalPercent = 20 + + // Bumping this annotation triggers a reconcile via the ServiceMonitor or Secret watch. + operatorMetricsTriggerAnnotation = "test.gitops.redhat.com/trigger-metrics-token-reconcile" +) + +// operatorMetricsBearerTokenSecret returns an empty Secret object reference for matchers and updates. +func operatorMetricsBearerTokenSecret() *corev1.Secret { + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: operatorMetricsBearerTokenSecretName, + Namespace: operatorMetricsNS, + }, + } +} + +// operatorMetricsServiceMonitor returns an empty ServiceMonitor object reference for matchers and updates. +func operatorMetricsServiceMonitor() *monitoringv1.ServiceMonitor { + return &monitoringv1.ServiceMonitor{ + ObjectMeta: metav1.ObjectMeta{ + Name: operatorMetricsMonitorName, + Namespace: operatorMetricsNS, + }, + } +} + +// operatorMetricsBearerTokenRenewalLead matches bearerTokenRenewalLead in the controller. +func operatorMetricsBearerTokenRenewalLead() time.Duration { + return operatorMetricsTokenExpiry * operatorMetricsTokenRenewalPercent / 100 +} + +// snapshotOperatorMetricsBearerTokenResources waits for and deep-copies the current +// metrics bearer token Secret and ServiceMonitor so tests can restore them afterward. +func snapshotOperatorMetricsBearerTokenResources(ctx context.Context, k8sClient client.Client) (*corev1.Secret, *monitoringv1.ServiceMonitor) { + serviceMonitor := operatorMetricsServiceMonitor() + Eventually(serviceMonitor).Should(k8sFixture.ExistByName()) + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(serviceMonitor), serviceMonitor)).To(Succeed()) + + bearerTokenSecret := operatorMetricsBearerTokenSecret() + Eventually(bearerTokenSecret).Should(k8sFixture.ExistByName()) + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(bearerTokenSecret), bearerTokenSecret)).To(Succeed()) + + return bearerTokenSecret.DeepCopy(), serviceMonitor.DeepCopy() +} + +// restoreOperatorMetricsBearerTokenResources puts the metrics bearer token Secret and +// ServiceMonitor back to the state captured by snapshotOperatorMetricsBearerTokenResources. +func restoreOperatorMetricsBearerTokenResources( + ctx context.Context, + k8sClient client.Client, + originalSecret *corev1.Secret, + originalServiceMonitor *monitoringv1.ServiceMonitor, +) { + if originalServiceMonitor != nil { + desired := originalServiceMonitor.DeepCopy() + k8sFixture.Update(operatorMetricsServiceMonitor(), func(obj client.Object) { + current := obj.(*monitoringv1.ServiceMonitor) + current.Spec = desired.Spec + current.Annotations = desired.Annotations + current.Labels = desired.Labels + }) + } + + if originalSecret == nil { + return + } + + desired := originalSecret.DeepCopy() + existing := operatorMetricsBearerTokenSecret() + err := k8sClient.Get(ctx, client.ObjectKeyFromObject(existing), existing) + if apierrors.IsNotFound(err) { + Expect(k8sClient.Create(ctx, desired)).To(Succeed()) + return + } + Expect(err).NotTo(HaveOccurred()) + secretFixture.Update(existing, func(secret *corev1.Secret) { + secret.Type = desired.Type + secret.Data = desired.Data + secret.Annotations = desired.Annotations + secret.Labels = desired.Labels + }) +} + +// triggerOperatorMetricsTokenReconcileViaServiceMonitor enqueues the reconciler through +// the metrics ServiceMonitor watch by updating a test-only annotation. +func triggerOperatorMetricsTokenReconcileViaServiceMonitor() { + k8sFixture.Update(operatorMetricsServiceMonitor(), func(obj client.Object) { + current := obj.(*monitoringv1.ServiceMonitor) + if current.Annotations == nil { + current.Annotations = map[string]string{} + } + current.Annotations[operatorMetricsTriggerAnnotation] = time.Now().Format(time.RFC3339Nano) + }) +} + +// triggerOperatorMetricsTokenReconcileViaSecret enqueues the reconciler through the +// bearer token Secret watch by updating a test-only annotation. +func triggerOperatorMetricsTokenReconcileViaSecret() { + secretFixture.Update(operatorMetricsBearerTokenSecret(), func(secret *corev1.Secret) { + if secret.Annotations == nil { + secret.Annotations = map[string]string{} + } + secret.Annotations[operatorMetricsTriggerAnnotation] = time.Now().Format(time.RFC3339Nano) + }) +} + +var _ = Describe("GitOps Operator Sequential E2E Tests", func() { + + // OperatorMetricsTokenReconciler creates the metrics ServiceMonitor after minting + // the bearer token Secret. These cases exercise renewal, migration, and drift correction. + Context("1-133_validate_operator_metrics_bearer_token", func() { + + var ( + ctx context.Context + k8sClient client.Client + ) + + BeforeEach(func() { + fixture.EnsureSequentialCleanSlate() + k8sClient, _ = utils.GetE2ETestKubeClient() + ctx = context.Background() + }) + + It("verifies metrics bearer token Secret stores token and expiry with valid timestamps", func() { + if fixture.EnvLocalRun() || fixture.EnvNonOLM() { + Skip("this test requires the operator to be installed via OLM in openshift-gitops-operator namespace") + } + + By("waiting for the operator metrics bearer token Secret to exist") + bearerTokenSecret := operatorMetricsBearerTokenSecret() + Eventually(bearerTokenSecret).Should(k8sFixture.ExistByName()) + Eventually(bearerTokenSecret).Should(secretFixture.HaveNonEmptyKeyValue(operatorMetricsBearerTokenKey)) + Eventually(bearerTokenSecret).Should(secretFixture.HaveNonEmptyKeyValue(operatorMetricsBearerTokenExpiryKey)) + + By("verifying the bearer token Secret is Opaque with a future expiry") + Expect(bearerTokenSecret.Type).To(Equal(corev1.SecretTypeOpaque)) + expiry, err := time.Parse(time.RFC3339, string(bearerTokenSecret.Data[operatorMetricsBearerTokenExpiryKey])) + Expect(err).NotTo(HaveOccurred()) + Expect(expiry.After(time.Now())).To(BeTrue()) + + By("restores ServiceMonitor authorization when deprecated bearerTokenSecret is set") + + By("setting deprecated bearerTokenSecret on the operator metrics ServiceMonitor") + k8sFixture.Update(operatorMetricsServiceMonitor(), func(obj client.Object) { + current := obj.(*monitoringv1.ServiceMonitor) + endpoint := ¤t.Spec.Endpoints[0] + endpoint.Authorization = nil + endpoint.BearerTokenSecret = &corev1.SecretKeySelector{ //nolint:staticcheck // SA1019: test migration from deprecated bearerTokenSecret + LocalObjectReference: corev1.LocalObjectReference{ + Name: operatorMetricsBearerTokenSecretName, + }, + Key: operatorMetricsBearerTokenKey, + } + }) + + By("waiting for the operator to restore Bearer authorization on the ServiceMonitor") + Eventually(func(g Gomega) { + current := operatorMetricsServiceMonitor() + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(current), current)).To(Succeed()) + endpoint := current.Spec.Endpoints[0] + g.Expect(endpoint.Authorization).NotTo(BeNil()) + g.Expect(endpoint.Authorization.Type).To(Equal("Bearer")) + g.Expect(endpoint.Authorization.Credentials).NotTo(BeNil()) + g.Expect(endpoint.Authorization.Credentials.Name).To(Equal(operatorMetricsBearerTokenSecretName)) + g.Expect(endpoint.Authorization.Credentials.Key).To(Equal(operatorMetricsBearerTokenKey)) + g.Expect(endpoint.BearerTokenSecret).To(BeNil()) //nolint:staticcheck // SA1019: deprecated bearerTokenSecret must be cleared + }, "2m", "5s").Should(Succeed()) + + }) + + It("refreshes metrics bearer token when expiry is in the past", func() { + if fixture.EnvLocalRun() || fixture.EnvNonOLM() { + Skip("this test requires the operator to be installed via OLM in openshift-gitops-operator namespace") + } + + By("capturing current metrics bearer token Secret and ServiceMonitor state") + originalSecret, originalServiceMonitor := snapshotOperatorMetricsBearerTokenResources(ctx, k8sClient) + defer restoreOperatorMetricsBearerTokenResources(ctx, k8sClient, originalSecret, originalServiceMonitor) + + By("setting bearer token expiry to a timestamp in the past") + bearerTokenSecret := operatorMetricsBearerTokenSecret() + Eventually(bearerTokenSecret).Should(k8sFixture.ExistByName()) + originalToken := string(bearerTokenSecret.Data[operatorMetricsBearerTokenKey]) + + secretFixture.Update(bearerTokenSecret, func(secret *corev1.Secret) { + secret.Data[operatorMetricsBearerTokenExpiryKey] = []byte(time.Now().Add(-time.Hour).UTC().Format(time.RFC3339)) + }) + + By("triggering operator metrics token reconciliation") + triggerOperatorMetricsTokenReconcileViaServiceMonitor() + + By("waiting for the bearer token Secret to be refreshed") + Eventually(func(g Gomega) { + secret := operatorMetricsBearerTokenSecret() + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)).To(Succeed()) + expiry, err := time.Parse(time.RFC3339, string(secret.Data[operatorMetricsBearerTokenExpiryKey])) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(expiry.After(time.Now())).To(BeTrue()) + g.Expect(string(secret.Data[operatorMetricsBearerTokenKey])).NotTo(Equal(originalToken)) + }, "2m", "5s").Should(Succeed()) + + By("refreshes metrics bearer token when token key is missing") + + By("removing the token key from the bearer token Secret") + secretFixture.Update(operatorMetricsBearerTokenSecret(), func(secret *corev1.Secret) { + delete(secret.Data, operatorMetricsBearerTokenKey) + secret.Data[operatorMetricsBearerTokenExpiryKey] = []byte(time.Now().Add(operatorMetricsTokenExpiry).UTC().Format(time.RFC3339)) + }) + + By("triggering operator metrics token reconciliation") + triggerOperatorMetricsTokenReconcileViaServiceMonitor() + + By("waiting for the bearer token Secret to be repopulated") + Eventually(operatorMetricsBearerTokenSecret()).Should(secretFixture.HaveNonEmptyKeyValue(operatorMetricsBearerTokenKey)) + Eventually(operatorMetricsBearerTokenSecret()).Should(secretFixture.HaveNonEmptyKeyValue(operatorMetricsBearerTokenExpiryKey)) + + By("refreshes metrics bearer token when expiry is unparseable") + + By("setting bearer token expiry to an invalid value") + bearerTokenSecret = operatorMetricsBearerTokenSecret() + Eventually(bearerTokenSecret).Should(k8sFixture.ExistByName()) + originalToken = string(bearerTokenSecret.Data[operatorMetricsBearerTokenKey]) + + secretFixture.Update(bearerTokenSecret, func(secret *corev1.Secret) { + secret.Data[operatorMetricsBearerTokenExpiryKey] = []byte("not-a-valid-rfc3339-timestamp") + }) + + By("triggering operator metrics token reconciliation") + triggerOperatorMetricsTokenReconcileViaServiceMonitor() + + By("waiting for the bearer token Secret to be refreshed") + Eventually(func(g Gomega) { + secret := operatorMetricsBearerTokenSecret() + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)).To(Succeed()) + expiry, err := time.Parse(time.RFC3339, string(secret.Data[operatorMetricsBearerTokenExpiryKey])) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(expiry.After(time.Now())).To(BeTrue()) + g.Expect(string(secret.Data[operatorMetricsBearerTokenKey])).NotTo(Equal(originalToken)) + }, "2m", "5s").Should(Succeed()) + }) + + It("does not refresh valid metrics bearer token before renewal deadline", func() { + if fixture.EnvLocalRun() || fixture.EnvNonOLM() { + Skip("this test requires the operator to be installed via OLM in openshift-gitops-operator namespace") + } + + By("capturing current metrics bearer token Secret and ServiceMonitor state") + originalSecret, originalServiceMonitor := snapshotOperatorMetricsBearerTokenResources(ctx, k8sClient) + defer restoreOperatorMetricsBearerTokenResources(ctx, k8sClient, originalSecret, originalServiceMonitor) + + By("setting a valid bearer token with a far-future expiry") + markerToken := "e2e-valid-metrics-token-marker" + secretFixture.Update(operatorMetricsBearerTokenSecret(), func(secret *corev1.Secret) { + secret.Data[operatorMetricsBearerTokenKey] = []byte(markerToken) + secret.Data[operatorMetricsBearerTokenExpiryKey] = []byte(time.Now().Add(operatorMetricsTokenExpiry).UTC().Format(time.RFC3339)) + }) + + By("triggering operator metrics token reconciliation") + triggerOperatorMetricsTokenReconcileViaServiceMonitor() + triggerOperatorMetricsTokenReconcileViaSecret() + + By("verifying the bearer token is not refreshed early") + Consistently(func(g Gomega) { + secret := operatorMetricsBearerTokenSecret() + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)).To(Succeed()) + g.Expect(string(secret.Data[operatorMetricsBearerTokenKey])).To(Equal(markerToken)) + }, "30s", "3s").Should(Succeed()) + + By("refreshes metrics bearer token at renewal deadline") + + By("setting bearer token expiry to the renewal deadline") + markerToken = "e2e-due-metrics-token-marker" + secretFixture.Update(operatorMetricsBearerTokenSecret(), func(secret *corev1.Secret) { + secret.Data[operatorMetricsBearerTokenKey] = []byte(markerToken) + secret.Data[operatorMetricsBearerTokenExpiryKey] = []byte(time.Now().Add(operatorMetricsBearerTokenRenewalLead()).UTC().Format(time.RFC3339)) + }) + + By("triggering operator metrics token reconciliation") + triggerOperatorMetricsTokenReconcileViaServiceMonitor() + + By("waiting for the bearer token Secret to be refreshed") + Eventually(func(g Gomega) { + secret := operatorMetricsBearerTokenSecret() + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)).To(Succeed()) + g.Expect(string(secret.Data[operatorMetricsBearerTokenKey])).NotTo(Equal(markerToken)) + + expiry, err := time.Parse(time.RFC3339, string(secret.Data[operatorMetricsBearerTokenExpiryKey])) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(expiry.After(time.Now().Add(operatorMetricsBearerTokenRenewalLead()))).To(BeTrue()) + }, "2m", "5s").Should(Succeed()) + }) + }) +}) From 9ad766e7694473adf5169a6e282da7647c66add9 Mon Sep 17 00:00:00 2001 From: Alka Kumari Date: Thu, 27 Aug 2026 23:22:23 +0530 Subject: [PATCH 7/9] Handle the refresh result after minting Signed-off-by: Alka Kumari --- controllers/operator_metrics_controller.go | 32 +++++++++++++- .../operator_metrics_controller_test.go | 43 +++++++++++++++++++ 2 files changed, 74 insertions(+), 1 deletion(-) diff --git a/controllers/operator_metrics_controller.go b/controllers/operator_metrics_controller.go index 48768426819..ac4e10ddc7a 100644 --- a/controllers/operator_metrics_controller.go +++ b/controllers/operator_metrics_controller.go @@ -58,6 +58,9 @@ const ( operatorMetricsTokenRenewalPercent = 20 operatorMetricsControlPlaneLabel = "control-plane" operatorMetricsControlPlaneLabelVal = "gitops-operator" + // bearerTokenMinRequeueAfter is the minimum RequeueAfter after minting when + // the API server grants a lifetime shorter than bearerTokenRenewalLead(). + bearerTokenMinRequeueAfter = time.Minute ) type serviceAccountTokenRequester interface { @@ -478,7 +481,7 @@ func (r *OperatorMetricsTokenReconciler) reconcileBearerTokenSecret(ctx context. operatorMetricsBearerTokenExpiryKey: []byte(expiry.UTC().Format(time.RFC3339)), }, } - requeueAfter, _ := evaluateBearerTokenRenewal(expiry, time.Now()) + requeueAfter := bearerTokenRequeueAfterMint(expiry, time.Now(), reqLogger) if err := r.applyMetricsBearerTokenSecret(ctx, namespace, desiredSecret, existing, reqLogger); err != nil { return 0, false, err @@ -497,6 +500,33 @@ func bearerTokenRenewalLead() time.Duration { return operatorMetricsTokenExpiry * operatorMetricsTokenRenewalPercent / 100 } +// bearerTokenRequeueAfterMint returns when to requeue after a successful +// TokenRequest. Normal lifetimes use evaluateBearerTokenRenewal unchanged; when +// the granted lifetime is at or below the renewal lead, requeue on a positive +// minimum interval instead of zero so renewal is scheduled. +func bearerTokenRequeueAfterMint(expiry, now time.Time, reqLogger logr.Logger) time.Duration { + requeueAfter, refreshAgain := evaluateBearerTokenRenewal(expiry, now) + if !refreshAgain { + return requeueAfter + } + remaining := expiry.Sub(now) + if remaining > bearerTokenRenewalLead() { + return requeueAfter + } + reqLogger.Info("Granted token lifetime is shorter than the renewal lead", + "remaining", remaining.String(), + "renewalLead", bearerTokenRenewalLead().String(), + "expiry", expiry.UTC().Format(time.RFC3339)) + requeueAfter = bearerTokenMinRequeueAfter + if remaining > 0 && requeueAfter >= remaining { + requeueAfter = remaining - time.Second + } + if requeueAfter <= 0 { + return bearerTokenMinRequeueAfter + } + return requeueAfter +} + // evaluateBearerTokenRenewal decides whether to refresh now and, if not, when // to requeue. Only expiry is stored in the Secret; the renewal boundary is // expiry minus a fixed lead derived from the requested TTL. diff --git a/controllers/operator_metrics_controller_test.go b/controllers/operator_metrics_controller_test.go index 8a3dfcc73eb..dc3a849d3c0 100644 --- a/controllers/operator_metrics_controller_test.go +++ b/controllers/operator_metrics_controller_test.go @@ -25,6 +25,7 @@ import ( "time" monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" + "github.com/go-logr/logr" "gotest.tools/assert" is "gotest.tools/assert/cmp" corev1 "k8s.io/api/core/v1" @@ -209,6 +210,48 @@ func TestOperatorMetricsTokenReconciler_replacesIncompatibleSecretType(t *testin assert.Equal(t, string(secret.Data[operatorMetricsBearerTokenKey]), "replaced-token") } +func TestBearerTokenRequeueAfterMint(t *testing.T) { + now := time.Now() + + requeueAfter := bearerTokenRequeueAfterMint(now.Add(operatorMetricsTokenExpiry), now, logr.Discard()) + assert.Assert(t, requeueAfter > 47*time.Minute && requeueAfter <= 48*time.Minute) + + requeueAfter = bearerTokenRequeueAfterMint(now.Add(5*time.Minute), now, logr.Discard()) + assert.Assert(t, requeueAfter > 0) + assert.Assert(t, requeueAfter <= 5*time.Minute) + + requeueAfter = bearerTokenRequeueAfterMint(now.Add(30*time.Second), now, logr.Discard()) + assert.Assert(t, requeueAfter > 0) + assert.Assert(t, requeueAfter < 30*time.Second) +} + +func TestOperatorMetricsTokenReconciler_schedulesRequeueForShortGrantedLifetime(t *testing.T) { + writeOperatorNamespaceFile(t, testOperatorNamespace) + + s := newOperatorMetricsTokenScheme() + c := fake.NewClientBuilder().WithScheme(s).Build() + shortExpiry := time.Now().Add(5 * time.Minute) + + r := &OperatorMetricsTokenReconciler{ + Client: c, + Scheme: s, + TokenRequester: &fakeTokenRequester{ + token: "short-lived-token", + expiry: shortExpiry, + }, + } + + 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) + assert.Assert(t, result.RequeueAfter <= 5*time.Minute) +} + func TestEvaluateBearerTokenRenewal(t *testing.T) { now := time.Now() expiry := now.Add(operatorMetricsTokenExpiry) From f19957fd8df226927feda943e1d6a02e671ccb55 Mon Sep 17 00:00:00 2001 From: Alka Kumari Date: Thu, 27 Aug 2026 23:36:36 +0530 Subject: [PATCH 8/9] resolved lint error Signed-off-by: Alka Kumari --- controllers/operator_metrics_controller.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/controllers/operator_metrics_controller.go b/controllers/operator_metrics_controller.go index ac4e10ddc7a..ad707b93bbd 100644 --- a/controllers/operator_metrics_controller.go +++ b/controllers/operator_metrics_controller.go @@ -323,7 +323,7 @@ func (r *OperatorMetricsTokenReconciler) reconcileServiceMonitor(ctx context.Con updated = true } - desiredServerName := desiredEndpoint.TLSConfig.SafeTLSConfig.ServerName + desiredServerName := desiredEndpoint.TLSConfig.ServerName if endpoint.TLSConfig == nil || endpoint.TLSConfig.ServerName == nil || desiredServerName == nil || @@ -335,7 +335,7 @@ func (r *OperatorMetricsTokenReconciler) reconcileServiceMonitor(ctx context.Con updated = true } - desiredCA := desiredEndpoint.TLSConfig.SafeTLSConfig.CA.ConfigMap + desiredCA := desiredEndpoint.TLSConfig.CA.ConfigMap if endpoint.TLSConfig == nil || endpoint.TLSConfig.CA.ConfigMap == nil || desiredCA == nil || From 1d6fc3abdf251c2f16293afb9132aa1a8545de46 Mon Sep 17 00:00:00 2001 From: Alka Kumari Date: Fri, 28 Aug 2026 00:06:33 +0530 Subject: [PATCH 9/9] Trigger tests Signed-off-by: Alka Kumari