Merge pull request #1571 from openshift-cherrypick-robot/cherry-pick-…

…1522-to-release-4.12

[release-4.12] OCPBUGS-22736: pkg/operator/configobserver: check that the serving certificate refer…

pkg/operator/configobservation/apiserver/observe_apiserver.go

@@ -52,7 +52,7 @@ type resourceSyncFunc func(destination, source resourcesynccontroller.ResourceLo
 // resources to sync.
 // It returns the observed config, sync rules and possibly an error. Nil sync rules mean to ignore all resources
 // in case of error. Otherwise, resources are deleted by default and the returned sync rules are taken as overrides of that.
-type observeAPIServerConfigFunc func(apiServer *configv1.APIServer, recorder events.Recorder, previouslyObservedConfig map[string]interface{}) (map[string]interface{}, syncActionRules, []error)
+type observeAPIServerConfigFunc func(apiServer *configv1.APIServer, recorder events.Recorder, previouslyObservedConfig map[string]interface{}, listers *configobservation.Listers) (map[string]interface{}, syncActionRules, []error)
 
 // ObserveUserClientCABundle returns an ObserveConfigFunc that observes a user managed certificate bundle containing
 // signers that will be recognized for incoming client certificates in addition to the operator managed signers.
@@ -74,7 +74,7 @@ var ObserveNamedCertificates configobserver.ObserveConfigFunc = (&apiServerObser
 
 // observeUserClientCABundle observes a user managed ConfigMap containing a certificate bundle for the signers that will
 // be recognized for incoming client certificates in addition to the operator managed signers.
-func observeUserClientCABundle(apiServer *configv1.APIServer, recorder events.Recorder, previouslyObservedConfig map[string]interface{}) (map[string]interface{}, syncActionRules, []error) {
+func observeUserClientCABundle(apiServer *configv1.APIServer, recorder events.Recorder, previouslyObservedConfig map[string]interface{}, listers *configobservation.Listers) (map[string]interface{}, syncActionRules, []error) {
 	configMapName := apiServer.Spec.ClientCA.Name
 	if len(configMapName) == 0 {
 		return nil, nil, nil // previously observed resource (if any) should be deleted
@@ -88,7 +88,7 @@ func observeUserClientCABundle(apiServer *configv1.APIServer, recorder events.Re
 
 // observeNamedCertificates observes user managed Secrets containing TLS cert info for serving secure traffic to
 // specific hostnames.
-func observeNamedCertificates(apiServer *configv1.APIServer, recorder events.Recorder, previouslyObservedConfig map[string]interface{}) (map[string]interface{}, syncActionRules, []error) {
+func observeNamedCertificates(apiServer *configv1.APIServer, recorder events.Recorder, previouslyObservedConfig map[string]interface{}, listers *configobservation.Listers) (map[string]interface{}, syncActionRules, []error) {
 	var errs []error
 	observedConfig := map[string]interface{}{}
 
@@ -142,6 +142,11 @@ func observeNamedCertificates(apiServer *configv1.APIServer, recorder events.Rec
 			recorder.Warningf("ObserveNamedCertificatesFailed", err.Error())
 			return previouslyObservedConfig, nil, append(errs, err)
 		}
+
+		// check that secret exists and readable by operator
+		if _, err := listers.ConfigSecretLister().Secrets(operatorclient.GlobalUserSpecifiedConfigNamespace).Get(namedCertificate.ServingCertificate.Name); err != nil {
+			return previouslyObservedConfig, nil, append(errs, err)
+		}
 		// pick one of the available target resource names
 		targetSecretName := fmt.Sprintf(namedUserServingCertResourceNameFormat, index)
 
@@ -209,7 +214,7 @@ func (o *apiServerObserver) observe(genericListers configobserver.Listers, recor
 		return previouslyObservedConfig, append(errs, err)
 	}
 
-	observedConfig, observedResources, errs := o.observerFunc(apiServer, recorder, previouslyObservedConfig)
+	observedConfig, observedResources, errs := o.observerFunc(apiServer, recorder, previouslyObservedConfig, &listers)
 
 	// if we get error during observation, skip the merging and return previous config and errors.
 	if len(errs) > 0 {

pkg/operator/configobservation/apiserver/observe_apiserver_test.go

@@ -9,6 +9,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/diff"
+	corelistersv1 "k8s.io/client-go/listers/core/v1"
 	"k8s.io/client-go/tools/cache"
 
 	configv1 "github.com/openshift/api/config/v1"
@@ -101,6 +102,7 @@ func TestObserveNamedCertificates(t *testing.T) {
 	testCases := []struct {
 		name           string
 		config         *configv1.APIServer
+		missingSecret  string
 		existing       map[string]interface{}
 		expected       map[string]interface{}
 		expectErrs     bool
@@ -446,6 +448,19 @@ func TestObserveNamedCertificates(t *testing.T) {
 			expected:   existingConfig,
 			expectErrs: true,
 		},
+		{
+			name: "NoSuchSecret",
+			config: newAPIServerConfig(
+				withCertificate(
+					withNames("*.foo.org"),
+					withSecret("foo"),
+				),
+			),
+			missingSecret: "foo",
+			existing:      existingConfig,
+			expected:      existingConfig,
+			expectErrs:    true,
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
@@ -459,6 +474,9 @@ func TestObserveNamedCertificates(t *testing.T) {
 			var objs []runtime.Object
 			if tc.config != nil {
 				for _, nc := range tc.config.Spec.ServingCerts.NamedCertificates {
+					if nc.ServingCertificate.Name == tc.missingSecret {
+						continue
+					}
 					objs = append(objs, &corev1.Secret{
 						ObjectMeta: metav1.ObjectMeta{
 							Name:      nc.ServingCertificate.Name,
@@ -471,11 +489,17 @@ func TestObserveNamedCertificates(t *testing.T) {
 					})
 				}
 			}
+			for _, obj := range objs {
+				if err := indexer.Add(obj); err != nil {
+					t.Fatal(err)
+				}
+			}
 
 			synced := map[string]string{}
 			listers := configobservation.Listers{
-				APIServerLister_: configlistersv1.NewAPIServerLister(indexer),
-				ResourceSync:     &mockResourceSyncer{t: t, synced: synced},
+				APIServerLister_:    configlistersv1.NewAPIServerLister(indexer),
+				ResourceSync:        &mockResourceSyncer{t: t, synced: synced},
+				ConfigSecretLister_: corelistersv1.NewSecretLister(indexer),
 			}
 			result, errs := ObserveNamedCertificates(listers, events.NewInMemoryRecorder(t.Name()), tc.existing)
 			if tc.expectErrs && len(errs) == 0 {

test/e2e/user_certs_test.go

@@ -56,14 +56,17 @@ func TestNamedCertificates(t *testing.T) {
 
 	// create secrets for named serving certificates
 	for _, info := range testCertInfoById {
-		defer func(info *testCertInfo) {
-			err := deleteSecret(kubeClient, "openshift-config", info.secretName)
-			require.NoError(t, err)
-		}(info)
 		_, err := createTLSSecret(kubeClient, "openshift-config", info.secretName, info.crypto.PrivateKey, info.crypto.Certificate)
 		require.NoError(t, err)
 	}
 
+	defer func() {
+		for _, info := range testCertInfoById {
+			err := deleteSecret(kubeClient, "openshift-config", info.secretName)
+			require.NoError(t, err)
+		}
+	}()
+
 	// configure named certificates
 	defer func() {
 		_, err := updateAPIServerClusterConfigSpec(configClient, func(apiserver *configv1.APIServer) {