From 7e679ba6cc5e82158f9d41ca6ccbaec1ef4bb063 Mon Sep 17 00:00:00 2001 From: Steve Goodwin Date: Thu, 16 Jul 2026 16:34:46 -0400 Subject: [PATCH 1/2] OCPBUGS-90370: Stabilize telemetry config to prevent continuous console pod rollouts --- pkg/console/operator/sync_v400.go | 11 +- pkg/console/operator/sync_v400_test.go | 258 ++++++++++++++++++++++++- 2 files changed, 262 insertions(+), 7 deletions(-) diff --git a/pkg/console/operator/sync_v400.go b/pkg/console/operator/sync_v400.go index dac445ea1..1710ead86 100644 --- a/pkg/console/operator/sync_v400.go +++ b/pkg/console/operator/sync_v400.go @@ -463,14 +463,13 @@ func (co *consoleOperator) GetTelemetryConfiguration(ctx context.Context, operat if err != nil { return telemetryConfig, err } - if !telemeterClientIsAvailable { - telemetryConfig["TELEMETER_CLIENT_DISABLED"] = "true" - return telemetryConfig, nil - } + telemetryConfig["TELEMETER_CLIENT_DISABLED"] = fmt.Sprintf("%t", !telemeterClientIsAvailable) - accessToken, err := telemetry.GetAccessToken(co.configNSSecretLister) + var accessToken string + accessToken, err = telemetry.GetAccessToken(co.configNSSecretLister) if err != nil { - return nil, err + klog.V(4).Infof("telemetry config: failed to get access token, proceeding without organization metadata: %v", err) + accessToken = "" } organizationID, accountMail, refreshCache := telemetry.GetOrganizationMeta(telemetryConfig, co.trackables.organizationID, co.trackables.accountMail, clusterID, accessToken) // cache fetched ORGANIZATION_ID and ACCOUNT_MAIL diff --git a/pkg/console/operator/sync_v400_test.go b/pkg/console/operator/sync_v400_test.go index 723ca50e0..78cbfd3f5 100644 --- a/pkg/console/operator/sync_v400_test.go +++ b/pkg/console/operator/sync_v400_test.go @@ -1,12 +1,25 @@ package operator import ( + "context" + "encoding/json" + "sort" "testing" "github.com/go-test/deep" - "github.com/openshift/console-operator/pkg/api" + + configv1 "github.com/openshift/api/config/v1" + operatorv1 "github.com/openshift/api/operator/v1" + configlistersv1 "github.com/openshift/client-go/config/listers/config/v1" + appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + appsv1listers "k8s.io/client-go/listers/apps/v1" + corev1listers "k8s.io/client-go/listers/core/v1" + "k8s.io/client-go/tools/cache" + + "github.com/openshift/console-operator/pkg/api" + "github.com/openshift/console-operator/pkg/console/telemetry" ) func TestGetNodeComputeEnvironments(t *testing.T) { @@ -210,3 +223,246 @@ func TestGetNodeComputeEnvironments(t *testing.T) { }) } } + +func newIndexer(keyFunc cache.KeyFunc) cache.Indexer { + return cache.NewIndexer(keyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) +} + +func newTestConsoleOperator(t *testing.T, telemeterAvailable bool, pullSecretHasCloudAuth bool) *consoleOperator { + t.Helper() + + // telemetry-config ConfigMap in operator namespace + telemetryConfigCM := &v1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: telemetry.TelemetryConfigMapName, + Namespace: api.OpenShiftConsoleOperatorNamespace, + }, + Data: map[string]string{ + "SEGMENT_API_HOST": "https://segment.example.com", + "SEGMENT_JS_HOST": "https://segment-js.example.com", + "SEGMENT_PUBLIC_API_KEY": "test-key", + }, + } + operatorNSCMIndexer := newIndexer(cache.MetaNamespaceKeyFunc) + if err := operatorNSCMIndexer.Add(telemetryConfigCM); err != nil { + t.Fatalf("failed to add telemetry configmap to indexer: %v", err) + } + + // ClusterVersion + cv := &configv1.ClusterVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: api.VersionResourceName, + }, + Spec: configv1.ClusterVersionSpec{ + ClusterID: "test-cluster-id", + }, + } + cvIndexer := newIndexer(func(obj interface{}) (string, error) { + meta := obj.(metav1.ObjectMetaAccessor).GetObjectMeta() + return meta.GetName(), nil + }) + if err := cvIndexer.Add(cv); err != nil { + t.Fatalf("failed to add clusterversion to indexer: %v", err) + } + + // telemeter-client deployment + var replicas int32 + if telemeterAvailable { + replicas = 1 + } + telemeterDeploy := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: telemetry.TelemeterClientDeploymentName, + Namespace: telemetry.TelemeterClientDeploymentNamespace, + }, + Status: appsv1.DeploymentStatus{ + AvailableReplicas: replicas, + }, + } + deployIndexer := newIndexer(cache.MetaNamespaceKeyFunc) + if err := deployIndexer.Add(telemeterDeploy); err != nil { + t.Fatalf("failed to add telemeter deployment to indexer: %v", err) + } + + // pull-secret + pullSecret := &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: telemetry.PullSecretName, + Namespace: api.OpenShiftConfigNamespace, + }, + Data: map[string][]byte{}, + } + if pullSecretHasCloudAuth { + dockerConfig := telemetry.DockerConfig{ + Auths: map[string]telemetry.DockerAuthEntry{ + "cloud.openshift.com": {Auth: "test-token"}, + }, + } + configBytes, err := json.Marshal(dockerConfig) + if err != nil { + t.Fatalf("failed to marshal docker config: %v", err) + } + pullSecret.Data[".dockerconfigjson"] = configBytes + } else { + dockerConfig := telemetry.DockerConfig{ + Auths: map[string]telemetry.DockerAuthEntry{ + "quay.io": {Auth: "quay-token"}, + }, + } + configBytes, err := json.Marshal(dockerConfig) + if err != nil { + t.Fatalf("failed to marshal docker config: %v", err) + } + pullSecret.Data[".dockerconfigjson"] = configBytes + } + secretIndexer := newIndexer(cache.MetaNamespaceKeyFunc) + if err := secretIndexer.Add(pullSecret); err != nil { + t.Fatalf("failed to add pull-secret to indexer: %v", err) + } + + return &consoleOperator{ + operatorNSConfigMapLister: corev1listers.NewConfigMapLister(operatorNSCMIndexer), + clusterVersionLister: configlistersv1.NewClusterVersionLister(cvIndexer), + monitoringDeploymentLister: appsv1listers.NewDeploymentLister(deployIndexer), + configNSSecretLister: corev1listers.NewSecretLister(secretIndexer), + } +} + +func TestGetTelemetryConfiguration_StableKeySet(t *testing.T) { + tests := []struct { + name string + telemeterAvailable bool + hasCloudAuth bool + expectDisabledVal string + }{ + { + name: "telemeter available with cloud auth", + telemeterAvailable: true, + hasCloudAuth: true, + expectDisabledVal: "false", + }, + { + name: "telemeter unavailable with cloud auth", + telemeterAvailable: false, + hasCloudAuth: true, + expectDisabledVal: "true", + }, + { + name: "telemeter unavailable without cloud auth (disconnected)", + telemeterAvailable: false, + hasCloudAuth: false, + expectDisabledVal: "true", + }, + { + name: "telemeter available without cloud auth (disconnected)", + telemeterAvailable: true, + hasCloudAuth: false, + expectDisabledVal: "false", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + co := newTestConsoleOperator(t, tt.telemeterAvailable, tt.hasCloudAuth) + operatorConfig := &operatorv1.Console{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + } + + config, err := co.GetTelemetryConfiguration(context.Background(), operatorConfig) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if _, ok := config["CLUSTER_ID"]; !ok { + t.Error("expected CLUSTER_ID key to be present") + } + if _, ok := config["ORGANIZATION_ID"]; !ok { + t.Error("expected ORGANIZATION_ID key to always be present") + } + if _, ok := config["ACCOUNT_MAIL"]; !ok { + t.Error("expected ACCOUNT_MAIL key to always be present") + } + + disabledVal, ok := config["TELEMETER_CLIENT_DISABLED"] + if !ok { + t.Error("expected TELEMETER_CLIENT_DISABLED key to always be present") + } + if disabledVal != tt.expectDisabledVal { + t.Errorf("expected TELEMETER_CLIENT_DISABLED=%q, got %q", tt.expectDisabledVal, disabledVal) + } + }) + } +} + +func TestGetTelemetryConfiguration_KeySetStableAcrossAvailabilityChange(t *testing.T) { + operatorConfig := &operatorv1.Console{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + } + + coUnavailable := newTestConsoleOperator(t, false, true) + configUnavailable, err := coUnavailable.GetTelemetryConfiguration(context.Background(), operatorConfig) + if err != nil { + t.Fatalf("unexpected error (unavailable): %v", err) + } + + coAvailable := newTestConsoleOperator(t, true, true) + configAvailable, err := coAvailable.GetTelemetryConfiguration(context.Background(), operatorConfig) + if err != nil { + t.Fatalf("unexpected error (available): %v", err) + } + + keysUnavailable := sortedKeys(configUnavailable) + keysAvailable := sortedKeys(configAvailable) + + // All keys must be present in both states — no key-set difference allowed. + sharedKeys := []string{"CLUSTER_ID", "ORGANIZATION_ID", "ACCOUNT_MAIL", "TELEMETER_CLIENT_DISABLED", "SEGMENT_API_HOST", "SEGMENT_JS_HOST", "SEGMENT_PUBLIC_API_KEY"} + for _, key := range sharedKeys { + if _, ok := configUnavailable[key]; !ok { + t.Errorf("key %q missing from unavailable config, keys present: %v", key, keysUnavailable) + } + if _, ok := configAvailable[key]; !ok { + t.Errorf("key %q missing from available config, keys present: %v", key, keysAvailable) + } + } + + if len(keysUnavailable) != len(keysAvailable) { + t.Fatalf("key set sizes differ: unavailable=%v, available=%v", keysUnavailable, keysAvailable) + } + for i := range keysUnavailable { + if keysUnavailable[i] != keysAvailable[i] { + t.Errorf("key set mismatch at index %d: unavailable has %q, available has %q", i, keysUnavailable[i], keysAvailable[i]) + } + } +} + +func sortedKeys(m map[string]string) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + +// Verify that GetAccessToken failure does not cause GetTelemetryConfiguration to error +func TestGetTelemetryConfiguration_DisconnectedClusterNoError(t *testing.T) { + co := newTestConsoleOperator(t, false, false) + operatorConfig := &operatorv1.Console{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + } + + config, err := co.GetTelemetryConfiguration(context.Background(), operatorConfig) + if err != nil { + t.Fatalf("expected no error on disconnected cluster, got: %v", err) + } + + if config["TELEMETER_CLIENT_DISABLED"] != "true" { + t.Error("expected TELEMETER_CLIENT_DISABLED=true on disconnected cluster") + } + if _, ok := config["ORGANIZATION_ID"]; !ok { + t.Error("expected ORGANIZATION_ID key to be present even on disconnected cluster") + } + if _, ok := config["ACCOUNT_MAIL"]; !ok { + t.Error("expected ACCOUNT_MAIL key to be present even on disconnected cluster") + } +} From 0ff70c91d6991c77750ef310b351eac807a88416 Mon Sep 17 00:00:00 2001 From: Steve Goodwin Date: Tue, 23 Jun 2026 11:56:47 -0400 Subject: [PATCH 2/2] OCPBUGS-87348: Make GetAccessToken graceful when cloud.openshift.com is absent Return ("", nil) instead of a hard error when cloud.openshift.com is missing from the pull-secret. This is expected on disconnected clusters and should not degrade the console operator. Also replaces fmt.Println with klog.Errorf for JSON parse errors. Co-authored-by: Santiago Greco Assisted-by: Claude (Opus 4.6) --- pkg/console/telemetry/telemetry.go | 6 +-- pkg/console/telemetry/telemetry_test.go | 50 +++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/pkg/console/telemetry/telemetry.go b/pkg/console/telemetry/telemetry.go index 0c2b18ac9..e51d7cea3 100644 --- a/pkg/console/telemetry/telemetry.go +++ b/pkg/console/telemetry/telemetry.go @@ -82,12 +82,12 @@ func GetAccessToken(secretsLister v1.SecretLister) (string, error) { var config DockerConfig err = json.Unmarshal(configBytes, &config) if err != nil { - fmt.Println("Error decoding JSON:", err) - return "", err + klog.Errorf("error decoding pull-secret JSON: %v", err) + return "", fmt.Errorf("error decoding pull-secret JSON: %w", err) } authsBytes, ok := config.Auths["cloud.openshift.com"] if !ok { - return "", fmt.Errorf("failed to parse 'cloud.openshift.com' field from pull-secret") + return "", nil } return authsBytes.Auth, nil } diff --git a/pkg/console/telemetry/telemetry_test.go b/pkg/console/telemetry/telemetry_test.go index 2ce56d7e0..bf1bd5870 100644 --- a/pkg/console/telemetry/telemetry_test.go +++ b/pkg/console/telemetry/telemetry_test.go @@ -3,6 +3,13 @@ package telemetry import ( "testing" "time" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + corev1listers "k8s.io/client-go/listers/core/v1" + "k8s.io/client-go/tools/cache" + + "github.com/openshift/console-operator/pkg/api" ) // Helpers to set rate-limit timestamps and restore after test @@ -18,6 +25,49 @@ func withLastSuccessTime(t *testing.T, ts time.Time) { t.Cleanup(func() { lastSuccessTime = prev }) } +func newFakeSecretLister(t *testing.T, secrets ...*corev1.Secret) corev1listers.SecretLister { + t.Helper() + indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) + for _, s := range secrets { + if err := indexer.Add(s.DeepCopy()); err != nil { + t.Fatalf("failed to add secret to indexer: %v", err) + } + } + return corev1listers.NewSecretLister(indexer) +} + +func TestGetAccessToken_MissingCloudEntry(t *testing.T) { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: PullSecretName, Namespace: api.OpenShiftConfigNamespace}, + Data: map[string][]byte{".dockerconfigjson": []byte(`{"auths":{}}`)}, + } + lister := newFakeSecretLister(t, secret) + + token, err := GetAccessToken(lister) + if err != nil { + t.Fatalf("expected no error for missing cloud.openshift.com, got: %v", err) + } + if token != "" { + t.Fatalf("expected empty token, got %q", token) + } +} + +func TestGetAccessToken_PresentCloudEntry(t *testing.T) { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: PullSecretName, Namespace: api.OpenShiftConfigNamespace}, + Data: map[string][]byte{".dockerconfigjson": []byte(`{"auths":{"cloud.openshift.com":{"auth":"my-token"}}}`)}, + } + lister := newFakeSecretLister(t, secret) + + token, err := GetAccessToken(lister) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if token != "my-token" { + t.Fatalf("expected %q, got %q", "my-token", token) + } +} + func TestGetOrganizationMeta_UsesCustomOverrides(t *testing.T) { telemetryConfig := map[string]string{ "ORGANIZATION_ID": "org-custom",