From 314c6228cdb6cc6a5aed8a3fa1e4790487711d0f Mon Sep 17 00:00:00 2001 From: Jakub Hadvig Date: Thu, 22 Oct 2026 13:51:25 +0200 Subject: [PATCH] OCPBUGS-132701: Gate tlsGroups observation on TLSGroupPreferences The console config observer managed servingInfo.tlsGroups on every cluster, even where the TLSGroupPreferences feature gate is disabled, and re-read FIPS state from procfs on every observation. During a 5.0->5.1 upgrade this produced a transient console-config change (tlsGroups added then removed), causing an extra console Deployment rollout whose SyncLoopRefresh Progressing=True window overlapped the machine-config Progressing window on slow metal-ipi upgrades, violating the "console stays Progressing=False while MCO is Progressing=True" invariant. Gate the tlsGroups observation on the TLSGroupPreferences feature gate so the operator does not manage tlsGroups when the gate is disabled, and resolve FIPS mode once at startup (it is an immutable kernel property) instead of per observation. Both flags are threaded through the config observer's Listers. Co-Authored-By: Claude Opus 4.8 --- .../apiserver/observe_tlssecurityprofile.go | 23 ++++++++-- .../observe_tlssecurityprofile_test.go | 43 ++++++++++++------- .../configobservation/apiserver/tls_fips.go | 5 ++- .../observe_config_controller.go | 4 ++ pkg/console/configobservation/listers.go | 16 +++++++ pkg/console/starter/starter.go | 11 ++++- 6 files changed, 82 insertions(+), 20 deletions(-) diff --git a/pkg/console/configobservation/apiserver/observe_tlssecurityprofile.go b/pkg/console/configobservation/apiserver/observe_tlssecurityprofile.go index 4b7b55be4b..bf258221ea 100644 --- a/pkg/console/configobservation/apiserver/observe_tlssecurityprofile.go +++ b/pkg/console/configobservation/apiserver/observe_tlssecurityprofile.go @@ -19,6 +19,14 @@ type APIServerLister interface { APIServerLister() configlistersv1.APIServerLister } +// TLSFeatureFlags exposes the startup-resolved feature state the TLS observer +// depends on: whether the TLSGroupPreferences gate is enabled and whether the +// operator runs in FIPS mode. +type TLSFeatureFlags interface { + TLSGroupPreferencesEnabled() bool + FIPSModeEnabled() bool +} + // ObserveTLSSecurityProfile observes APIServer.Spec.TLSSecurityProfile and writes // servingInfo.minTLSVersion, servingInfo.cipherSuites, and servingInfo.tlsGroups. func ObserveTLSSecurityProfile(listers configobserver.Listers, recorder events.Recorder, existingConfig map[string]interface{}) (map[string]interface{}, []error) { @@ -43,6 +51,9 @@ func innerTLSSecurityProfileObservations( }() apiServerListers := listers.(APIServerLister) + featureFlags := listers.(TLSFeatureFlags) + tlsGroupsEnabled := featureFlags.TLSGroupPreferencesEnabled() + fipsEnabled := featureFlags.FIPSModeEnabled() errList := []error{} currentMinTLSVersion, _, versionErr := unstructured.NestedString(existingConfig, minTLSVersionPath...) @@ -68,7 +79,13 @@ func innerTLSSecurityProfileObservations( return existingConfig, append(errList, err) } - observedMinTLSVersion, observedCipherSuites, observedGroups := getSecurityProfileSettings(apiServer.Spec.TLSSecurityProfile) + observedMinTLSVersion, observedCipherSuites, observedGroups := getSecurityProfileSettings(apiServer.Spec.TLSSecurityProfile, fipsEnabled) + if !tlsGroupsEnabled { + // The TLSGroupPreferences gate is disabled: do not manage tlsGroups. + // Leaving observedGroups empty removes any previously observed value + // from the Console CR's observedConfig. + observedGroups = nil + } observedConfig := map[string]interface{}{} if err = unstructured.SetNestedField(observedConfig, observedMinTLSVersion, minTLSVersionPath...); err != nil { @@ -96,7 +113,7 @@ func innerTLSSecurityProfileObservations( return observedConfig, errList } -func getSecurityProfileSettings(profile *configv1.TLSSecurityProfile) (string, []string, []string) { +func getSecurityProfileSettings(profile *configv1.TLSSecurityProfile, fipsEnabled bool) (string, []string, []string) { profileType := crypto.DefaultTLSProfileType if profile != nil { profileType = profile.Type @@ -116,7 +133,7 @@ func getSecurityProfileSettings(profile *configv1.TLSSecurityProfile) (string, [ } srcGroups := profileSpec.Groups - if isFIPSEnabled() && len(srcGroups) > 0 { + if fipsEnabled && len(srcGroups) > 0 { approved := fipsApprovedTLSGroups(srcGroups) if len(approved) != len(srcGroups) { klog.Warningf("FIPS mode is enabled; restricting the configured TLS groups %v to the FIPS-approved set %v. Console ignores non-approved groups and falls back to the Go runtime default FIPS curves when the set is empty. To avoid this, configure only FIPS-approved NIST curves (secp256r1, secp384r1, secp521r1) in the APIServer TLSSecurityProfile.", srcGroups, approved) diff --git a/pkg/console/configobservation/apiserver/observe_tlssecurityprofile_test.go b/pkg/console/configobservation/apiserver/observe_tlssecurityprofile_test.go index 6697673abc..f57946a671 100644 --- a/pkg/console/configobservation/apiserver/observe_tlssecurityprofile_test.go +++ b/pkg/console/configobservation/apiserver/observe_tlssecurityprofile_test.go @@ -17,7 +17,7 @@ import ( clocktesting "k8s.io/utils/clock/testing" ) -func newAPIServerListers(t *testing.T, apiServer *configv1.APIServer) configobservation.Listers { +func newAPIServerListers(t *testing.T, apiServer *configv1.APIServer, tlsGroupsEnabled bool) configobservation.Listers { t.Helper() indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) if apiServer != nil { @@ -27,6 +27,8 @@ func newAPIServerListers(t *testing.T, apiServer *configv1.APIServer) configobse } return configobservation.Listers{ APIServerLister_: configlistersv1.NewAPIServerLister(indexer), + TLSGroupsEnabled: tlsGroupsEnabled, + FIPSEnabled: false, } } @@ -92,13 +94,7 @@ func TestGetSecurityProfileSettings(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // Guard: this exercises the non-FIPS path. If the test binary were - // ever run under FIPS the expected group sets would differ. - if isFIPSEnabled() { - t.Skip("skipping: expectations assume non-FIPS runtime") - } - - gotVersion, _, gotGroups := getSecurityProfileSettings(tt.profile) + gotVersion, _, gotGroups := getSecurityProfileSettings(tt.profile, false) if gotVersion != tt.wantVersion { t.Errorf("minTLSVersion = %q, want %q", gotVersion, tt.wantVersion) } @@ -114,16 +110,12 @@ func TestObserveTLSSecurityProfile(t *testing.T) { intermediate := configv1.TLSProfiles[configv1.TLSProfileIntermediateType] t.Run("writes groups from the configured profile", func(t *testing.T) { - if isFIPSEnabled() { - t.Skip("skipping: expectations assume non-FIPS runtime") - } - listers := newAPIServerListers(t, &configv1.APIServer{ ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, Spec: configv1.APIServerSpec{ TLSSecurityProfile: &configv1.TLSSecurityProfile{Type: configv1.TLSProfileIntermediateType}, }, - }) + }, true) observed, errs := ObserveTLSSecurityProfile(listers, recorder, map[string]interface{}{}) if len(errs) != 0 { @@ -148,7 +140,7 @@ func TestObserveTLSSecurityProfile(t *testing.T) { }) t.Run("missing apiserver falls back to default profile without error", func(t *testing.T) { - listers := newAPIServerListers(t, nil) + listers := newAPIServerListers(t, nil, true) observed, errs := ObserveTLSSecurityProfile(listers, recorder, map[string]interface{}{}) if len(errs) != 0 { @@ -165,7 +157,7 @@ func TestObserveTLSSecurityProfile(t *testing.T) { Spec: configv1.APIServerSpec{ TLSSecurityProfile: &configv1.TLSSecurityProfile{Type: configv1.TLSProfileIntermediateType}, }, - }) + }, true) existing := map[string]interface{}{ "servingInfo": map[string]interface{}{ @@ -181,4 +173,25 @@ func TestObserveTLSSecurityProfile(t *testing.T) { t.Errorf("expected unmanaged field to be pruned from observed config") } }) + + t.Run("omits tlsGroups when the feature gate is disabled", func(t *testing.T) { + listers := newAPIServerListers(t, &configv1.APIServer{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: configv1.APIServerSpec{ + TLSSecurityProfile: &configv1.TLSSecurityProfile{Type: configv1.TLSProfileIntermediateType}, + }, + }, false) + + observed, errs := ObserveTLSSecurityProfile(listers, recorder, map[string]interface{}{}) + if len(errs) != 0 { + t.Fatalf("unexpected errors: %v", errs) + } + if _, found, _ := unstructured.NestedStringSlice(observed, "servingInfo", "tlsGroups"); found { + t.Errorf("expected tlsGroups to be omitted when TLSGroupPreferences is disabled") + } + // minTLSVersion and cipherSuites are not gated and must still be observed. + if _, found, _ := unstructured.NestedString(observed, "servingInfo", "minTLSVersion"); !found { + t.Errorf("expected minTLSVersion to be observed even when tlsGroups is disabled") + } + }) } diff --git a/pkg/console/configobservation/apiserver/tls_fips.go b/pkg/console/configobservation/apiserver/tls_fips.go index 02d725f9fa..a25fc5b369 100644 --- a/pkg/console/configobservation/apiserver/tls_fips.go +++ b/pkg/console/configobservation/apiserver/tls_fips.go @@ -33,7 +33,10 @@ var ( readFile = os.ReadFile ) -func isFIPSEnabled() bool { +// IsFIPSEnabled reports whether the operator runs in FIPS mode. FIPS mode is an +// immutable property of the node's kernel, so callers should resolve it once at +// startup rather than per reconciliation. +func IsFIPSEnabled() bool { return detectFIPS() } diff --git a/pkg/console/configobservation/configobservercontroller/observe_config_controller.go b/pkg/console/configobservation/configobservercontroller/observe_config_controller.go index 751f0876f0..a7a5a91157 100644 --- a/pkg/console/configobservation/configobservercontroller/observe_config_controller.go +++ b/pkg/console/configobservation/configobservercontroller/observe_config_controller.go @@ -25,6 +25,8 @@ func NewConfigObserver( configInformer configinformers.SharedInformerFactory, resourceSyncer resourcesynccontroller.ResourceSyncer, eventRecorder events.Recorder, + tlsGroupsEnabled bool, + fipsEnabled bool, ) *ConfigObserver { informers := []factory.Informer{ operatorClient.Informer(), @@ -43,6 +45,8 @@ func NewConfigObserver( operatorClient.Informer().HasSynced, configInformer.Config().V1().APIServers().Informer().HasSynced, }, + TLSGroupsEnabled: tlsGroupsEnabled, + FIPSEnabled: fipsEnabled, }, informers, // Observer functions diff --git a/pkg/console/configobservation/listers.go b/pkg/console/configobservation/listers.go index 9a856b4d09..3aa63ffedc 100644 --- a/pkg/console/configobservation/listers.go +++ b/pkg/console/configobservation/listers.go @@ -11,12 +11,28 @@ type Listers struct { APIServerLister_ configlistersv1.APIServerLister ResourceSync resourcesynccontroller.ResourceSyncer PreRunCachesSynced []cache.InformerSynced + + // TLSGroupsEnabled gates observation of servingInfo.tlsGroups. The APIServer + // groups field is itself gated by the TLSGroupPreferences feature gate, so + // the console observer must not manage tlsGroups when the gate is disabled. + TLSGroupsEnabled bool + // FIPSEnabled is resolved once at startup. FIPS mode is an immutable property + // of the node's kernel, so it is not re-read per observation. + FIPSEnabled bool } func (l Listers) APIServerLister() configlistersv1.APIServerLister { return l.APIServerLister_ } +func (l Listers) TLSGroupPreferencesEnabled() bool { + return l.TLSGroupsEnabled +} + +func (l Listers) FIPSModeEnabled() bool { + return l.FIPSEnabled +} + func (l Listers) ResourceSyncer() resourcesynccontroller.ResourceSyncer { return l.ResourceSync } diff --git a/pkg/console/starter/starter.go b/pkg/console/starter/starter.go index 9904cd70eb..161c1c5af1 100644 --- a/pkg/console/starter/starter.go +++ b/pkg/console/starter/starter.go @@ -29,6 +29,7 @@ import ( operatorv1 "github.com/openshift/api/operator/v1" "github.com/openshift/console-operator/pkg/api" + consoleapiserver "github.com/openshift/console-operator/pkg/console/configobservation/apiserver" "github.com/openshift/console-operator/pkg/console/configobservation/configobservercontroller" "github.com/openshift/console-operator/pkg/console/controllers/clidownloads" "github.com/openshift/console-operator/pkg/console/controllers/clioidcclientstatus" @@ -91,6 +92,10 @@ const ( consolePluginResource = "consoleplugins" ) +// tlsGroupPreferencesFeatureGate gates observation of servingInfo.tlsGroups. +// Defined locally until the openshift/api features package is vendored. +const tlsGroupPreferencesFeatureGate configv1.FeatureGateName = "TLSGroupPreferences" + func RunOperator(ctx context.Context, controllerContext *controllercmd.ControllerContext) error { kubeClient, err := kubernetes.NewForConfig(controllerContext.ProtoKubeConfig) @@ -665,12 +670,16 @@ func RunOperator(ctx context.Context, controllerContext *controllercmd.Controlle logLevelController := loglevel.NewClusterOperatorLoggingController(operatorClient, controllerContext.EventRecorder) managementStateController := managementstatecontroller.NewOperatorManagementStateController(api.ClusterOperatorName, operatorClient, controllerContext.EventRecorder) - // Config observer watches APIServer and writes TLS config to Console CR's observedConfig + // Config observer watches APIServer and writes TLS config to Console CR's observedConfig. + // tlsGroups is gated on the TLSGroupPreferences feature gate; FIPS mode is resolved once + // here since it is an immutable property of the node's kernel. configObserver := configobservercontroller.NewConfigObserver( operatorClient, configInformers, resourceSyncer, recorder, + featureGates.Enabled(tlsGroupPreferencesFeatureGate), + consoleapiserver.IsFIPSEnabled(), ) for _, informer := range []interface {