fix(ho): honor deprecated global config fields

openshift · a-dsouza · Apr 11, 2022 · Apr 27, 2022 · May 31, 2022 · Jun 14, 2022
commit 374bbb414c60aa9425707a5d15d3357da7076751
diff --git a/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go b/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
@@ -3606,37 +3606,9 @@ func (r *HostedClusterReconciler) validateConfigAndClusterCapabilities(ctx conte
 
 	// TODO: Drop when we no longer need to support versions < 4.11
 	if hc.Spec.Configuration != nil {
-		globalConfig, err := globalconfig.ParseGlobalConfig(ctx, hc.Spec.Configuration)
+		_, err := globalconfig.ParseGlobalConfig(ctx, hc.Spec.Configuration)
 		if err != nil {
 			errs = append(errs, fmt.Errorf("failed to parse cluster configuration: %w", err))
-		} else {
-			if globalConfig.APIServer != nil && hc.Spec.Configuration.APIServer != nil {
-				errs = append(errs, fmt.Errorf("apiServer raw extension config is invalid when APIServer configuration field is set"))
-			}
-			if globalConfig.Authentication != nil && hc.Spec.Configuration.Authentication != nil {
-				errs = append(errs, fmt.Errorf("authentication raw extension config is invalid when Authentication configuration field is set"))
-			}
-			if globalConfig.FeatureGate != nil && hc.Spec.Configuration.FeatureGate != nil {
-				errs = append(errs, fmt.Errorf("featureGate raw extension config is invalid when FeatureGate configuration field is set"))
-			}
-			if globalConfig.Image != nil && hc.Spec.Configuration.Image != nil {
-				errs = append(errs, fmt.Errorf("image raw extension config is invalid when Image configuration field is set"))
-			}
-			if globalConfig.Ingress != nil && hc.Spec.Configuration.Ingress != nil {
-				errs = append(errs, fmt.Errorf("ingress raw extension config is invalid when Ingress configuration field is set"))
-			}
-			if globalConfig.Network != nil && hc.Spec.Configuration.Network != nil {
-				errs = append(errs, fmt.Errorf("network raw extension config is invalid when Network configuration field is set"))
-			}
-			if globalConfig.OAuth != nil && hc.Spec.Configuration.OAuth != nil {
-				errs = append(errs, fmt.Errorf("oAuth raw extension config is invalid when OAuth configuration field is set"))
-			}
-			if globalConfig.Proxy != nil && hc.Spec.Configuration.Proxy != nil {
-				errs = append(errs, fmt.Errorf("proxy raw extension config is invalid when Proxy configuration field is set"))
-			}
-			if globalConfig.Scheduler != nil && hc.Spec.Configuration.Scheduler != nil {
-				errs = append(errs, fmt.Errorf("scheduler raw extension config is invalid when Scheduler configuration field is set"))
-			}
 		}
 	}
 
@@ -4501,40 +4473,34 @@ func (r *HostedClusterReconciler) reconcileDeprecatedGlobalConfig(ctx context.Co
 		return err
 	}
 
-	// Only copy over config from the raw extension if the field is not already populated.
-	// Once populated, the field takes precedence and a conflicting raw extension config
-	// results in an invalid configuration field.
-
-	if gconfig.APIServer != nil && hc.Spec.Configuration.APIServer == nil {
+	// Copy over config from the raw extension
+	if gconfig.APIServer != nil {
 		hc.Spec.Configuration.APIServer = &gconfig.APIServer.Spec
 	}
-	if gconfig.Authentication != nil && hc.Spec.Configuration.Authentication == nil {
+	if gconfig.Authentication != nil {
 		hc.Spec.Configuration.Authentication = &gconfig.Authentication.Spec
 	}
-	if gconfig.FeatureGate != nil && hc.Spec.Configuration.FeatureGate == nil {
+	if gconfig.FeatureGate != nil {
 		hc.Spec.Configuration.FeatureGate = &gconfig.FeatureGate.Spec
 	}
-	if gconfig.Image != nil && hc.Spec.Configuration.Image == nil {
+	if gconfig.Image != nil {
 		hc.Spec.Configuration.Image = &gconfig.Image.Spec
 	}
-	if gconfig.Ingress != nil && hc.Spec.Configuration.Ingress == nil {
+	if gconfig.Ingress != nil {
 		hc.Spec.Configuration.Ingress = &gconfig.Ingress.Spec
 	}
-	if gconfig.Network != nil && hc.Spec.Configuration.Network == nil {
+	if gconfig.Network != nil {
 		hc.Spec.Configuration.Network = &gconfig.Network.Spec
 	}
-	if gconfig.OAuth != nil && hc.Spec.Configuration.OAuth == nil {
+	if gconfig.OAuth != nil {
 		hc.Spec.Configuration.OAuth = &gconfig.OAuth.Spec
 	}
-	if gconfig.Scheduler != nil && hc.Spec.Configuration.Scheduler == nil {
+	if gconfig.Scheduler != nil {
 		hc.Spec.Configuration.Scheduler = &gconfig.Scheduler.Spec
 	}
-	if gconfig.Proxy != nil && hc.Spec.Configuration.Proxy == nil {
+	if gconfig.Proxy != nil {
 		hc.Spec.Configuration.Proxy = &gconfig.Proxy.Spec
 	}
-	hc.Spec.Configuration.Items = nil
-	hc.Spec.Configuration.ConfigMapRefs = nil
-	hc.Spec.Configuration.SecretRefs = nil
 
 	return nil
 }

diff --git a/hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go b/hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go
@@ -1,6 +1,7 @@
 package hostedcluster
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"fmt"
@@ -17,6 +18,7 @@ import (
 	"github.com/openshift/hypershift/hypershift-operator/controllers/manifests/autoscaler"
 	"github.com/openshift/hypershift/hypershift-operator/controllers/manifests/controlplaneoperator"
 	"github.com/openshift/hypershift/hypershift-operator/controllers/manifests/ignitionserver"
+	hyperapi "github.com/openshift/hypershift/support/api"
 	"github.com/openshift/hypershift/support/capabilities"
 	fakecapabilities "github.com/openshift/hypershift/support/capabilities/fake"
 	fakereleaseprovider "github.com/openshift/hypershift/support/releaseinfo/fake"
@@ -28,6 +30,8 @@ import (
 	"k8s.io/apimachinery/pkg/api/equality"
 	errors2 "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	serializerjson "k8s.io/apimachinery/pkg/runtime/serializer/json"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/clock"
 	"k8s.io/apimachinery/pkg/util/intstr"
@@ -1469,3 +1473,122 @@ type fakeImageMetadataProvider struct{}
 func (*fakeImageMetadataProvider) ImageMetadata(ctx context.Context, imageRef string, pullSecret []byte) (*dockerv1client.DockerImageConfig, error) {
 	return &dockerv1client.DockerImageConfig{}, nil
 }
+
+func TestReconcileDeprecatedGlobalConfig(t *testing.T) {
+	hc := &hyperv1.HostedCluster{}
+	hc.Name = "fake-name"
+	hc.Namespace = "fake-namespace"
+
+	apiServer := &configv1.APIServer{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: configv1.SchemeGroupVersion.String(),
+			Kind:       "APIServer",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "cluster",
+		},
+		Spec: configv1.APIServerSpec{
+			Audit: configv1.Audit{
+				// Populate kubebuilder default for comparison
+				// https://github.com/openshift/api/blob/f120778bee805ad1a7a4f05a6430332cf5811813/config/v1/types_apiserver.go#L57
+				Profile: configv1.DefaultAuditProfileType,
+			},
+			ClientCA: configv1.ConfigMapNameReference{
+				Name: "fake-ca",
+			},
+		},
+	}
+
+	jsonSerializer := serializerjson.NewSerializerWithOptions(
+		serializerjson.DefaultMetaFactory, hyperapi.Scheme, hyperapi.Scheme,
+		serializerjson.SerializerOptions{Yaml: false, Pretty: true, Strict: false},
+	)
+
+	serializedAPIServer := &bytes.Buffer{}
+	err := jsonSerializer.Encode(apiServer, serializedAPIServer)
+	if err != nil {
+		t.Fatalf("failed to serialize apiserver: %v", err)
+	}
+
+	hc.Spec.Configuration = &hyperv1.ClusterConfiguration{
+		Items: []runtime.RawExtension{
+			{
+				Raw: serializedAPIServer.Bytes(),
+			},
+		},
+		ConfigMapRefs: []corev1.LocalObjectReference{
+			{
+				Name: "fake-ca",
+			},
+		},
+		SecretRefs: []corev1.LocalObjectReference{
+			{
+				Name: "fake-creds",
+			},
+		},
+	}
+
+	fakeClient := fake.NewClientBuilder().
+		WithScheme(hyperapi.Scheme).
+		WithObjects(hc).
+		Build()
+	reconciler := &HostedClusterReconciler{
+		Client: fakeClient,
+	}
+
+	originalSpec := hc.Spec.DeepCopy()
+	if err := reconciler.reconcileDeprecatedGlobalConfig(context.Background(), hc); err != nil {
+		t.Fatalf("unexpected reconcile error: %v", err)
+	}
+
+	// Update fields if required.
+	if !equality.Semantic.DeepEqual(&hc.Spec, originalSpec) {
+		err := reconciler.Client.Update(context.Background(), hc)
+		if err != nil {
+			t.Fatalf("unexpected update error: %v", err)
+		}
+	}
+
+	updatedHc := &hyperv1.HostedCluster{}
+	if err := fakeClient.Get(context.Background(), crclient.ObjectKeyFromObject(hc), updatedHc); err != nil {
+		t.Fatalf("unexpected get error: %v", err)
+	}
+	if updatedHc.Spec.Configuration == nil {
+		t.Fatalf("unexpected nil configuration")
+	}
+	if len(updatedHc.Spec.Configuration.Items) == 0 {
+		t.Errorf("empty deprecated configuration")
+	}
+	if len(updatedHc.Spec.Configuration.ConfigMapRefs) == 0 {
+		t.Errorf("empty configmap refs")
+	}
+	if len(updatedHc.Spec.Configuration.SecretRefs) == 0 {
+		t.Errorf("emtpy secret refs")
+	}
+	if !equality.Semantic.DeepEqual(&apiServer.Spec, updatedHc.Spec.Configuration.APIServer) {
+		t.Errorf("unexpected apiserver spec: %#v", updatedHc.Spec.Configuration.APIServer)
+	}
+
+	// Update deprecated field, remove test when field is unsupported
+	apiServer.Spec.ClientCA.Name = "updated-ca"
+	serializedAPIServer.Reset()
+	err = jsonSerializer.Encode(apiServer, serializedAPIServer)
+	if err != nil {
+		t.Fatalf("failed to serialize apiserver: %v", err)
+	}
+	updatedHc.Spec.Configuration.Items = []runtime.RawExtension{{Raw: serializedAPIServer.Bytes()}}
+	if err := reconciler.reconcileDeprecatedGlobalConfig(context.Background(), updatedHc); err != nil {
+		t.Fatalf("unexpected reconcile error: %v", err)
+	}
+	err = reconciler.Client.Update(context.Background(), updatedHc)
+	if err != nil {
+		t.Fatalf("unexpected update error: %v", err)
+	}
+	updatedHcAgain := &hyperv1.HostedCluster{}
+	if err := fakeClient.Get(context.Background(), crclient.ObjectKeyFromObject(updatedHc), updatedHcAgain); err != nil {
+		t.Fatalf("unexpected get error: %v", err)
+	}
+	if !equality.Semantic.DeepEqual(&apiServer.Spec, updatedHcAgain.Spec.Configuration.APIServer) {
+		t.Errorf("unexpected apiserver spec on update: %#v", updatedHcAgain.Spec.Configuration.APIServer)
+	}
+}
diff --git a/support/globalconfig/globalconfig.go b/support/globalconfig/globalconfig.go
@@ -52,6 +52,11 @@ func ParseGlobalConfig(ctx context.Context, cfg *hyperv1.ClusterConfiguration) (
 		kinds.Insert(gvk.Kind)
 		switch obj := cfgObject.(type) {
 		case *configv1.APIServer:
+			if obj.Spec.Audit.Profile == "" {
+				// Populate kubebuilder default for comparison
+				// https://github.com/openshift/api/blob/f120778bee805ad1a7a4f05a6430332cf5811813/config/v1/types_apiserver.go#L57
+				obj.Spec.Audit.Profile = configv1.DefaultAuditProfileType
+			}
 			globalConfig.APIServer = obj
 		case *configv1.Authentication:
 			globalConfig.Authentication = obj