From 19dc34ac3e531921531d7ec0b5392f9ffcfa0482 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Mon, 13 Jul 2026 13:51:01 -0400 Subject: [PATCH] feat: watch APIServer TLS profile and restart on change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When --tls-min-version is not supplied via flags, packageserver now sets up a watch on APIServer.config.openshift.io/cluster after applying the initial TLS profile at startup. If the profile changes, the process exits so it restarts with the updated settings — the same restart pattern used for OLMConfig interval changes. The watch is intentionally skipped when --tls-min-version is present (i.e. flags are injected externally). In that case the external manager is responsible for triggering restarts on profile changes, avoiding unnecessary restarts. --- .../_packageserver.clusterserviceversion.yaml | 7 ++ deploy/upstream/quickstart/olm.yaml | 7 ++ pkg/package-server/server/server.go | 118 ++++++++++++++++-- pkg/package-server/server/server_test.go | 80 ++++++++++++ 4 files changed, 201 insertions(+), 11 deletions(-) diff --git a/deploy/chart/templates/_packageserver.clusterserviceversion.yaml b/deploy/chart/templates/_packageserver.clusterserviceversion.yaml index 38658adbb2..929ee0543e 100644 --- a/deploy/chart/templates/_packageserver.clusterserviceversion.yaml +++ b/deploy/chart/templates/_packageserver.clusterserviceversion.yaml @@ -75,6 +75,13 @@ spec: - cluster verbs: - get + - apiGroups: + - "config.openshift.io" + resources: + - apiservers + verbs: + - list + - watch deployments: - name: packageserver {{- include "packageserver.deployment-spec" . | nindent 8 }} diff --git a/deploy/upstream/quickstart/olm.yaml b/deploy/upstream/quickstart/olm.yaml index 1b8c3d0970..e2aabb65c4 100644 --- a/deploy/upstream/quickstart/olm.yaml +++ b/deploy/upstream/quickstart/olm.yaml @@ -272,6 +272,13 @@ spec: - cluster verbs: - get + - apiGroups: + - "config.openshift.io" + resources: + - apiservers + verbs: + - list + - watch deployments: - name: packageserver spec: diff --git a/pkg/package-server/server/server.go b/pkg/package-server/server/server.go index e2ef2a86f8..b41fd9e57c 100644 --- a/pkg/package-server/server/server.go +++ b/pkg/package-server/server/server.go @@ -6,6 +6,7 @@ import ( "io" "net" "os" + "slices" "time" log "github.com/sirupsen/logrus" @@ -23,7 +24,9 @@ import ( "k8s.io/client-go/tools/clientcmd" "k8s.io/client-go/util/workqueue" + apiconfigv1 "github.com/openshift/api/config/v1" configv1client "github.com/openshift/client-go/config/clientset/versioned" + configinformers "github.com/openshift/client-go/config/informers/externalversions" libcrypto "github.com/openshift/library-go/pkg/crypto" operatorsv1 "github.com/operator-framework/api/pkg/operators/v1" "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client" @@ -41,8 +44,16 @@ const DefaultWakeupInterval = 12 * time.Hour type Operator struct { queueinformer.Operator - olmConfigQueue workqueue.TypedRateLimitingInterface[types.NamespacedName] - options *PackageServerOptions + olmConfigQueue workqueue.TypedRateLimitingInterface[types.NamespacedName] + apiServerTLSQueue workqueue.TypedRateLimitingInterface[types.NamespacedName] + options *PackageServerOptions + // appliedTLSMinVersion and appliedTLSCipherSuites hold the raw values from + // GetSecurityProfileConfig that were applied at startup. Storing uint16 values + // avoids ToNameOrDie / panic paths in the watch handler. + appliedTLSMinVersion uint16 + appliedTLSCipherSuites []uint16 + // exitFunc is called to terminate the process; injectable for testing. + exitFunc func(int) } // NewCommandStartPackageServer provides a CLI handler for 'start master' command @@ -222,19 +233,28 @@ func (o *PackageServerOptions) Run(ctx context.Context) error { return fmt.Errorf("unable to construct lister client config: %v", err) } - // If --tls-min-version was not supplied (e.g. no PSM-injected flags yet), fall - // back to a direct GET of the cluster APIServer CR so the packageserver still - // honours the cluster TLS security profile on first boot or during upgrades. - if o.SecureServing.MinTLSVersion == "" { - // Warn and continue on failure: the API server may not be reachable - // during initial startup (e.g. cluster bootstrap). Packageserver starts - // with secure TLS defaults; operators can supply --tls-min-version and - // --tls-cipher-suites flags to override the profile on a later restart. + // Apply the cluster TLS profile for any field not already supplied via flags, + // so a partial flag configuration (e.g. --tls-cipher-suites only) still gets + // the cluster default for the unset field. + // Warn and continue on failure: the API server may not be reachable during + // initial startup (e.g. cluster bootstrap). Packageserver starts with secure + // TLS defaults; operators can supply --tls-min-version and --tls-cipher-suites + // flags to override the profile on a later restart. + minVersionWasEmpty := o.SecureServing.MinTLSVersion == "" + cipherSuitesWasEmpty := len(o.SecureServing.CipherSuites) == 0 + if minVersionWasEmpty || cipherSuitesWasEmpty { if err := applyClusterTLSProfile(ctx, clientConfig, o.SecureServing); err != nil { log.WithError(err).Warn("Failed to apply cluster TLS profile to serving options, using defaults") } } + // The APIServer watch is only registered when *neither* TLS flag was originally + // supplied and the CR lookup succeeded. When flags are externally injected (e.g. + // by PSM), the watch is skipped; the external manager is responsible for + // triggering restarts on profile changes. + selfApplied := minVersionWasEmpty && cipherSuitesWasEmpty && + o.SecureServing.MinTLSVersion != "" && len(o.SecureServing.CipherSuites) > 0 + // Grab the config for the API server var config *apiserver.Config config, err = o.Config(ctx) @@ -265,7 +285,13 @@ func (o *PackageServerOptions) Run(ctx context.Context) error { workqueue.TypedRateLimitingQueueConfig[types.NamespacedName]{ Name: "olmConfig", }), - options: o, + apiServerTLSQueue: workqueue.NewTypedRateLimitingQueueWithConfig[types.NamespacedName]( + workqueue.DefaultTypedControllerRateLimiter[types.NamespacedName](), + workqueue.TypedRateLimitingQueueConfig[types.NamespacedName]{ + Name: "apiServerTLS", + }), + options: o, + exitFunc: os.Exit, } olmConfigInformer := olminformers.NewSharedInformerFactoryWithOptions(crClient, 0).Operators().V1().OLMConfigs() @@ -283,6 +309,42 @@ func (o *PackageServerOptions) Run(ctx context.Context) error { return err } + // If the TLS profile was self-applied at startup (neither TLS flag was set), + // capture the applied uint16 values and watch APIServer for changes so the + // process can restart when the cluster profile is updated. + // When flags are externally injected (e.g. by PSM), this watch is skipped; + // the external manager is responsible for triggering restarts on profile changes. + if selfApplied { + // Derive the comparison baseline from o.SecureServing, which was already + // populated by applyClusterTLSProfile. Using a second GET would introduce + // a race window: if the profile changes between the two calls, the snapshot + // would reflect the new profile while the server is still running with the + // old one, causing the watch to incorrectly skip the restart. + if v, verErr := libcrypto.TLSVersion(o.SecureServing.MinTLSVersion); verErr == nil { + op.appliedTLSMinVersion = v + } + op.appliedTLSCipherSuites = olmapiserver.CipherNamesToIDs(o.SecureServing.CipherSuites) + + cfgClient, cfgErr := configv1client.NewForConfig(clientConfig) + if cfgErr != nil { + log.WithError(cfgErr).Warn("Failed to create config client for APIServer TLS watch") + } else { + apiServerInformer := configinformers.NewSharedInformerFactory(cfgClient, 0).Config().V1().APIServers() + apiServerQueueInformer, qiErr := queueinformer.NewQueueInformer( + ctx, + queueinformer.WithInformer(apiServerInformer.Informer()), + queueinformer.WithQueue(op.apiServerTLSQueue), + queueinformer.WithIndexer(apiServerInformer.Informer().GetIndexer()), + queueinformer.WithSyncer(queueinformer.LegacySyncHandler(op.syncAPIServerTLSProfile).ToSyncer()), + ) + if qiErr != nil { + log.WithError(qiErr).Warn("Failed to create APIServer TLS queue informer") + } else if err := op.RegisterQueueInformer(apiServerQueueInformer); err != nil { + log.WithError(err).Warn("Failed to register APIServer TLS queue informer") + } + } + } + // Use the interval from the CLI as default if o.CurrentSyncInterval != o.DefaultSyncInterval { log.Infof("CLI argument changed default from '%v' to '%v'", o.CurrentSyncInterval, o.DefaultSyncInterval) @@ -347,6 +409,40 @@ func (op *Operator) syncOLMConfig(obj interface{}) error { return nil } +// syncAPIServerTLSProfile is called by the APIServer informer whenever the +// cluster singleton APIServer object changes. If the TLS profile differs from +// the one applied at startup the process exits so it can be restarted with the +// updated profile. This watch is only active when neither --tls-min-version nor +// --tls-cipher-suites was supplied via flags; when flags are externally managed +// (e.g. by PSM) this handler is never registered. +func (op *Operator) syncAPIServerTLSProfile(obj interface{}) error { + apiServer, ok := obj.(*apiconfigv1.APIServer) + if !ok { + return fmt.Errorf("expected *apiconfigv1.APIServer, got %T", obj) + } + + newMinVersion, newCipherSuites := olmapiserver.GetSecurityProfileConfig(apiServer.Spec.TLSSecurityProfile) + + // Compare uint16 values directly. This avoids TLSVersionToNameOrDie and + // CipherSuitesToNamesOrDie which panic on unrecognised inputs. Sort cipher + // slices before comparing so ordering differences do not cause restart loops. + newSortedCiphers := slices.Clone(newCipherSuites) + slices.Sort(newSortedCiphers) + currentSortedCiphers := slices.Clone(op.appliedTLSCipherSuites) + slices.Sort(currentSortedCiphers) + + if newMinVersion == op.appliedTLSMinVersion && slices.Equal(newSortedCiphers, currentSortedCiphers) { + return nil + } + + // Log raw numeric values to avoid TLSVersionToNameOrDie / CipherSuitesToNamesOrDie + // which can panic on unrecognised inputs. The handler must always exit cleanly. + log.Warnf("APIServer TLS profile changed (minVersion: %#x → %#x, ciphers: %v → %v), restarting to apply new profile", + op.appliedTLSMinVersion, newMinVersion, currentSortedCiphers, newSortedCiphers) + op.exitFunc(0) + return nil +} + // applyClusterTLSProfile fetches the cluster-wide APIServer TLS security profile // and applies it to the SecureServingOptions. It is a no-op on non-OpenShift clusters. // This is the fallback path used when --tls-min-version is not provided via flags diff --git a/pkg/package-server/server/server_test.go b/pkg/package-server/server/server_test.go index 529c1257e5..93c81bfe54 100644 --- a/pkg/package-server/server/server_test.go +++ b/pkg/package-server/server/server_test.go @@ -6,10 +6,13 @@ import ( apiconfigv1 "github.com/openshift/api/config/v1" configfake "github.com/openshift/client-go/config/clientset/versioned/fake" + olmapiserver "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/apiserver" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" genericoptions "k8s.io/apiserver/pkg/server/options" fakediscovery "k8s.io/client-go/discovery/fake" k8sfake "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/util/workqueue" ) // clusterAPIServer returns a minimal APIServer singleton with the given TLS profile. @@ -155,3 +158,80 @@ func TestApplyClusterTLSProfileWithClients_ErrorLeavesServingUnchanged(t *testin t.Errorf("CipherSuites should be unset after error, got %v", serving.CipherSuites) } } + +// newTestOperator builds a minimal Operator with an injected exitFunc and the +// applied uint16 TLS profile for use in syncAPIServerTLSProfile tests. +func newTestOperator(minVersion uint16, cipherSuites []uint16, exited *bool) *Operator { + return &Operator{ + apiServerTLSQueue: workqueue.NewTypedRateLimitingQueueWithConfig[types.NamespacedName]( + workqueue.DefaultTypedControllerRateLimiter[types.NamespacedName](), + workqueue.TypedRateLimitingQueueConfig[types.NamespacedName]{Name: "test"}, + ), + options: &PackageServerOptions{SecureServing: newServing()}, + appliedTLSMinVersion: minVersion, + appliedTLSCipherSuites: cipherSuites, + exitFunc: func(int) { *exited = true }, + } +} + +// TestSyncAPIServerTLSProfile_NoRestartWhenUnchanged verifies that the handler +// does not trigger a restart when the APIServer CR's TLS profile matches the +// profile that was applied at startup. +func TestSyncAPIServerTLSProfile_NoRestartWhenUnchanged(t *testing.T) { + profile := &apiconfigv1.TLSSecurityProfile{Type: apiconfigv1.TLSProfileIntermediateType} + apiServer := clusterAPIServer(profile) + minVersion, cipherSuites := olmapiserver.GetSecurityProfileConfig(profile) + + exited := false + op := newTestOperator(minVersion, cipherSuites, &exited) + + if err := op.syncAPIServerTLSProfile(apiServer); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if exited { + t.Error("expected no restart when TLS profile is unchanged") + } +} + +// TestSyncAPIServerTLSProfile_RestartWhenMinVersionChanges verifies that the +// handler triggers a restart when minTLSVersion changes. +func TestSyncAPIServerTLSProfile_RestartWhenMinVersionChanges(t *testing.T) { + intermediateProfile := &apiconfigv1.TLSSecurityProfile{Type: apiconfigv1.TLSProfileIntermediateType} + minVersion, cipherSuites := olmapiserver.GetSecurityProfileConfig(intermediateProfile) + + exited := false + op := newTestOperator(minVersion, cipherSuites, &exited) + + // Present a Modern profile (TLS 1.3) — minVersion differs from Intermediate. + modernServer := clusterAPIServer(&apiconfigv1.TLSSecurityProfile{Type: apiconfigv1.TLSProfileModernType}) + if err := op.syncAPIServerTLSProfile(modernServer); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !exited { + t.Error("expected restart when minTLSVersion changes") + } +} + +// TestSyncAPIServerTLSProfile_NoRestartOnCipherReorder verifies that cipher +// suite reordering does not cause a spurious restart / restart loop. +func TestSyncAPIServerTLSProfile_NoRestartOnCipherReorder(t *testing.T) { + profile := &apiconfigv1.TLSSecurityProfile{Type: apiconfigv1.TLSProfileIntermediateType} + apiServer := clusterAPIServer(profile) + minVersion, cipherSuites := olmapiserver.GetSecurityProfileConfig(profile) + + // Reverse the cipher slice to simulate an ordering difference. + reversed := make([]uint16, len(cipherSuites)) + for i, c := range cipherSuites { + reversed[len(cipherSuites)-1-i] = c + } + + exited := false + op := newTestOperator(minVersion, reversed, &exited) + + if err := op.syncAPIServerTLSProfile(apiServer); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if exited { + t.Error("expected no restart when cipher suites are reordered but otherwise equal") + } +}