From 3bab8855a9d5ef6e1f153e504ccaa0fe4d39f493 Mon Sep 17 00:00:00 2001 From: "Karan.Magdani" Date: Fri, 24 Sep 2021 20:01:46 -0700 Subject: [PATCH] imageconfig controller --- README.md | 2 + cmd/aro/operator.go | 6 + pkg/api/defaults.go | 2 + pkg/operator/controllers/const.go | 1 + .../imageconfig/image_controller.go | 149 +++++++++++ .../imageconfig/image_controller_test.go | 239 ++++++++++++++++++ 6 files changed, 399 insertions(+) create mode 100644 pkg/operator/controllers/imageconfig/image_controller.go create mode 100644 pkg/operator/controllers/imageconfig/image_controller_test.go diff --git a/README.md b/README.md index 8e8edeb6199..6b87e5a907e 100644 --- a/README.md +++ b/README.md @@ -108,6 +108,8 @@ questions or comments. `openshift-azure-logging` namespace matches the pre-defined specification found in `pkg/operator/controllers/genevalogging/genevalogging.go`. + * imageconfig: Ensures that required registries are not blocked in `image.config` + * machine: validate machine objects have the correct provider spec, vm type, vm image, disk size, three master nodes exist, and the number of worker nodes match the desired worker replicas diff --git a/cmd/aro/operator.go b/cmd/aro/operator.go index b71e77403cd..ba9a3be12fd 100644 --- a/cmd/aro/operator.go +++ b/cmd/aro/operator.go @@ -28,6 +28,7 @@ import ( "github.com/Azure/ARO-RP/pkg/operator/controllers/clusteroperatoraro" "github.com/Azure/ARO-RP/pkg/operator/controllers/dnsmasq" "github.com/Azure/ARO-RP/pkg/operator/controllers/genevalogging" + "github.com/Azure/ARO-RP/pkg/operator/controllers/imageconfig" "github.com/Azure/ARO-RP/pkg/operator/controllers/machine" "github.com/Azure/ARO-RP/pkg/operator/controllers/machineset" "github.com/Azure/ARO-RP/pkg/operator/controllers/monitoring" @@ -186,6 +187,11 @@ func operator(ctx context.Context, log *logrus.Entry) error { arocli, maocli)).SetupWithManager(mgr); err != nil { return fmt.Errorf("unable to create controller MachineSet: %v", err) } + if err = (imageconfig.NewReconciler( + log.WithField("controller", controllers.ImageConfigControllerName), + arocli, configcli)).SetupWithManager(mgr); err != nil { + return fmt.Errorf("unable to create controller ImageConfig: %v", err) + } } if err = (checker.NewReconciler( diff --git a/pkg/api/defaults.go b/pkg/api/defaults.go index ac0d8c0661a..fd0e714da97 100644 --- a/pkg/api/defaults.go +++ b/pkg/api/defaults.go @@ -61,6 +61,8 @@ var DefaultOperatorFlags OperatorFlags = OperatorFlags{ "aro.genevalogging.enabled": FLAG_TRUE, + "aro.imageconfig.enabled": FLAG_TRUE, + "aro.machine.enabled": FLAG_TRUE, "aro.machineset.enabled": FLAG_TRUE, diff --git a/pkg/operator/controllers/const.go b/pkg/operator/controllers/const.go index 71fe40aa0ed..5f31009f3a7 100644 --- a/pkg/operator/controllers/const.go +++ b/pkg/operator/controllers/const.go @@ -21,4 +21,5 @@ const ( NodeControllerName = "Node" MachineControllerName = "Machine" MachineSetControllerName = "MachineSet" + ImageConfigControllerName = "ImageConfig" ) diff --git a/pkg/operator/controllers/imageconfig/image_controller.go b/pkg/operator/controllers/imageconfig/image_controller.go new file mode 100644 index 00000000000..eefc7861148 --- /dev/null +++ b/pkg/operator/controllers/imageconfig/image_controller.go @@ -0,0 +1,149 @@ +package imageconfig + +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + +import ( + "context" + "errors" + "strings" + + "github.com/Azure/go-autorest/autorest/azure" + configv1 "github.com/openshift/api/config/v1" + configclient "github.com/openshift/client-go/config/clientset/versioned" + "github.com/sirupsen/logrus" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1" + aroclient "github.com/Azure/ARO-RP/pkg/operator/clientset/versioned" + "github.com/Azure/ARO-RP/pkg/operator/controllers" + "github.com/Azure/ARO-RP/pkg/util/azureclient" +) + +const imageConfigResource = "cluster" + +const ( + CONFIG_NAMESPACE string = "aro.imageconfig" + ENABLED string = CONFIG_NAMESPACE + ".enabled" +) + +type Reconciler struct { + log *logrus.Entry + arocli aroclient.Interface + configcli configclient.Interface +} + +func NewReconciler(log *logrus.Entry, arocli aroclient.Interface, configcli configclient.Interface) *Reconciler { + return &Reconciler{ + log: log, + arocli: arocli, + configcli: configcli, + } +} + +// watches the ARO object for changes and reconciles image.config.openshift.io/cluster object. +// - If blockedRegistries is not nil, makes sure required registries are not added +// - If AllowedRegistries is not nil, makes sure required registries are added +// - Fails fast if both are not nil, unsupported +func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) { + // Get cluster + instance, err := r.arocli.AroV1alpha1().Clusters().Get(ctx, request.Name, metav1.GetOptions{}) + if err != nil { + return reconcile.Result{}, err + } + + if !instance.Spec.OperatorFlags.GetSimpleBoolean(ENABLED) { + // controller is disabled + return reconcile.Result{}, nil + } + + // Check for cloud type + requiredRegistries, err := getCloudAwareRegistries(instance) + if err != nil { + // Not returning error as it will requeue again + return reconcile.Result{}, nil + } + + // Get image.config yaml + imageconfig, err := r.configcli.ConfigV1().Images().Get(ctx, request.Name, metav1.GetOptions{}) + if err != nil { + return reconcile.Result{}, err + } + + // Fail fast if both are not nil + if imageconfig.Spec.RegistrySources.AllowedRegistries != nil && imageconfig.Spec.RegistrySources.BlockedRegistries != nil { + err := errors.New("both AllowedRegistries and BlockedRegistries are present") + return reconcile.Result{}, err + } + + removeDuplicateRegistries := func(item string) bool { + for _, v := range requiredRegistries { + if strings.EqualFold(item, v) { + return false + } + } + return true + } + + // Append to allowed registries + if imageconfig.Spec.RegistrySources.AllowedRegistries != nil { + + imageconfig.Spec.RegistrySources.AllowedRegistries = filterSliceInPlace(imageconfig.Spec.RegistrySources.AllowedRegistries, removeDuplicateRegistries) + imageconfig.Spec.RegistrySources.AllowedRegistries = append(imageconfig.Spec.RegistrySources.AllowedRegistries, requiredRegistries...) + } + + // Remove from blocked registries + if imageconfig.Spec.RegistrySources.BlockedRegistries != nil { + imageconfig.Spec.RegistrySources.BlockedRegistries = filterSliceInPlace(imageconfig.Spec.RegistrySources.BlockedRegistries, removeDuplicateRegistries) + } + + // Update image config registry + _, err = r.configcli.ConfigV1().Images().Update(ctx, imageconfig, metav1.UpdateOptions{}) + return reconcile.Result{}, err +} + +// SetupWithManager setup the manager +func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { + imagePredicate := predicate.NewPredicateFuncs(func(o client.Object) bool { + return o.GetName() == imageConfigResource + }) + + return ctrl.NewControllerManagedBy(mgr). + For(&configv1.Image{}, builder.WithPredicates(imagePredicate)). + Named(controllers.ImageConfigControllerName). + Complete(r) +} + +// Switch case to ensure the correct registries are added depending on the cloud environment (Gov or Public cloud) +func getCloudAwareRegistries(instance *arov1alpha1.Cluster) ([]string, error) { + var requiredRegistries []string + switch instance.Spec.AZEnvironment { + case azureclient.PublicCloud.Environment.Name: + requiredRegistries = []string{instance.Spec.ACRDomain, "arosvc." + instance.Spec.Location + ".data." + azure.PublicCloud.ContainerRegistryDNSSuffix} + + case azureclient.USGovernmentCloud.Environment.Name: + requiredRegistries = []string{instance.Spec.ACRDomain, "arosvc." + instance.Spec.Location + ".data." + azure.USGovernmentCloud.ContainerRegistryDNSSuffix} + + default: + err := errors.New("cloud environment %s is not supported" + instance.Spec.AZEnvironment) + return nil, err + } + return requiredRegistries, nil +} + +// Helper function that filters registries to make sure they are added in consistent order +func filterSliceInPlace(input []string, keep func(string) bool) []string { + n := 0 + for _, x := range input { + if keep(x) { + input[n] = x + n++ + } + } + return input[:n] +} diff --git a/pkg/operator/controllers/imageconfig/image_controller_test.go b/pkg/operator/controllers/imageconfig/image_controller_test.go new file mode 100644 index 00000000000..beb8dae0d38 --- /dev/null +++ b/pkg/operator/controllers/imageconfig/image_controller_test.go @@ -0,0 +1,239 @@ +package imageconfig + +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + +import ( + "context" + "encoding/json" + "sort" + "strconv" + "strings" + "testing" + + configv1 "github.com/openshift/api/config/v1" + configclient "github.com/openshift/client-go/config/clientset/versioned" + configfake "github.com/openshift/client-go/config/clientset/versioned/fake" + "github.com/sirupsen/logrus" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + ctrl "sigs.k8s.io/controller-runtime" + + arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1" + aroclient "github.com/Azure/ARO-RP/pkg/operator/clientset/versioned" + arofake "github.com/Azure/ARO-RP/pkg/operator/clientset/versioned/fake" +) + +// fake arocli +var ( + imageConfigMetadata = metav1.ObjectMeta{Name: "cluster"} + arocli = arofake.NewSimpleClientset(&arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: arov1alpha1.SingletonClusterName, + }, + Spec: arov1alpha1.ClusterSpec{ + ACRDomain: "arointsvc.azurecr.io", + AZEnvironment: "AzurePublicCloud", + OperatorFlags: arov1alpha1.OperatorFlags{ + ENABLED: strconv.FormatBool(true), + }, + Location: "eastus", + }, + }) +) + +// Test reconcile function +func TestImageConfigReconciler(t *testing.T) { + log := logrus.NewEntry(logrus.StandardLogger()) + type test struct { + name string + arocli aroclient.Interface + configcli configclient.Interface + wantConfig string + wantErr string + } + + for _, tt := range []*test{ + { + name: "Feature Flag disabled, no action", + arocli: arofake.NewSimpleClientset(&arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: arov1alpha1.SingletonClusterName, + }, + Spec: arov1alpha1.ClusterSpec{ + ACRDomain: "arointsvc.azurecr.io", + AZEnvironment: "AzurePublicCloud", + OperatorFlags: arov1alpha1.OperatorFlags{ + ENABLED: strconv.FormatBool(false), + }, + Location: "eastus", + }, + }), + configcli: configfake.NewSimpleClientset(&configv1.Image{ + ObjectMeta: imageConfigMetadata, + Spec: configv1.ImageSpec{ + RegistrySources: configv1.RegistrySources{ + AllowedRegistries: []string{ + "quay.io", + }, + }, + }, + }), + wantConfig: `["quay.io"]`, + }, + { + name: "Image config registry source is empty, no action", + arocli: arocli, + configcli: configfake.NewSimpleClientset(&configv1.Image{ + ObjectMeta: imageConfigMetadata, + }), + wantConfig: `null`, + }, + { + name: "allowedRegistries exists with duplicates, function should appropriately add registries", + arocli: arocli, + configcli: configfake.NewSimpleClientset(&configv1.Image{ + ObjectMeta: imageConfigMetadata, + Spec: configv1.ImageSpec{ + RegistrySources: configv1.RegistrySources{ + AllowedRegistries: []string{ + "quay.io", + "arointsvc.azurecr.io", + "arointsvc.azurecr.io", + }, + }, + }, + }), + wantConfig: `["arointsvc.azurecr.io","arosvc.eastus.data.azurecr.io","quay.io"]`, + }, + { + name: "blockedRegistries exists, function should delete registries", + arocli: arocli, + configcli: configfake.NewSimpleClientset(&configv1.Image{ + ObjectMeta: imageConfigMetadata, + Spec: configv1.ImageSpec{ + RegistrySources: configv1.RegistrySources{ + BlockedRegistries: []string{ + "quay.io", "arointsvc.azurecr.io", "arosvc.eastus.data.azurecr.io", + }, + }, + }, + }), + wantConfig: `["quay.io"]`, + }, + { + name: "Gov Cloud, function should add appropriate registries", + arocli: arofake.NewSimpleClientset(&arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: arov1alpha1.SingletonClusterName, + }, + Spec: arov1alpha1.ClusterSpec{ + ACRDomain: "arointsvc.azurecr.us", + AZEnvironment: "AzureUSGovernmentCloud", + OperatorFlags: arov1alpha1.OperatorFlags{ + ENABLED: strconv.FormatBool(true), + }, + Location: "eastus", + }, + }), + configcli: configfake.NewSimpleClientset(&configv1.Image{ + ObjectMeta: imageConfigMetadata, + Spec: configv1.ImageSpec{ + RegistrySources: configv1.RegistrySources{ + AllowedRegistries: []string{ + "quay.io", + }, + }, + }, + }), + wantConfig: `["arointsvc.azurecr.us","arosvc.eastus.data.azurecr.us","quay.io"]`, + }, + { + name: "AZEnvironment is Unset", + arocli: arofake.NewSimpleClientset(&arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: arov1alpha1.SingletonClusterName, + }, + Spec: arov1alpha1.ClusterSpec{ + OperatorFlags: arov1alpha1.OperatorFlags{ + ENABLED: strconv.FormatBool(true), + }, + Location: "eastus", + }, + }), + configcli: configfake.NewSimpleClientset(&configv1.Image{ + ObjectMeta: imageConfigMetadata, + Spec: configv1.ImageSpec{ + RegistrySources: configv1.RegistrySources{ + AllowedRegistries: []string{ + "quay.io", + }, + }, + }, + }), + wantConfig: `["quay.io"]`, + }, + { + name: "Both AllowedRegistries and BlockedRegistries are present, function should fail silently and not requeue", + arocli: arocli, + configcli: configfake.NewSimpleClientset(&configv1.Image{ + ObjectMeta: imageConfigMetadata, + Spec: configv1.ImageSpec{ + RegistrySources: configv1.RegistrySources{ + BlockedRegistries: []string{ + "arointsvc.azurecr.io", "arosvc.eastus.data.azurecr.io", + }, + AllowedRegistries: []string{ + "quay.io", + }, + }, + }, + }), + wantErr: `both AllowedRegistries and BlockedRegistries are present`, + }, + } { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + r := &Reconciler{ + log: log, + arocli: tt.arocli, + configcli: tt.configcli, + } + request := ctrl.Request{} + request.Name = "cluster" + + _, err := r.Reconcile(ctx, request) + if err != nil && err.Error() != tt.wantErr || + err == nil && tt.wantErr != "" { + t.Error(err) + t.Error("----------------------") + t.Error(tt.wantErr) + } + imgcfg, err := r.configcli.ConfigV1().Images().Get(ctx, request.Name, metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + imgcfgjson := getRegistrySources(imgcfg) + + if string(imgcfgjson) != strings.TrimSpace(tt.wantConfig) && tt.wantConfig != "" { + t.Error(string(imgcfgjson)) + t.Error("----------------------") + t.Error(tt.wantConfig) + } + + }) + } +} + +func getRegistrySources(imgcfg *configv1.Image) []byte { + var registrySourceJSON []byte + if imgcfg.Spec.RegistrySources.AllowedRegistries != nil { + imgRegistries := imgcfg.Spec.RegistrySources.AllowedRegistries + sort.Strings(imgRegistries) + registrySourceJSON, _ = json.Marshal(imgRegistries) + } else { + imgRegistries := imgcfg.Spec.RegistrySources.BlockedRegistries + sort.Strings(imgRegistries) + registrySourceJSON, _ = json.Marshal(imgRegistries) + } + return registrySourceJSON +}