Allow users to configure a subset of CRDs to install (#3007)

Fixes #1433.
Fixes #2920.

* By default, no CRDs are installed.
* If no CRDs are installed, the operator pod will exit with an error
  stating that there are no CRDs.
* Operator pod --crd-pattern command-line argument will now accept more
  than '*'.
* --crd-pattern means NEW CRDs to install. Existing CRDs in the cluster
  will always be upgraded. This means that upgrading an existing ASO
  installation without specifying any new CRDs will upgrade all of the
  existing CRDs and install no new CRDs.

Taskfile.yml

@@ -335,7 +335,8 @@ tasks:
       --set azureSubscriptionID=$AZURE_SUBSCRIPTION_ID \
       --set azureTenantID=$AZURE_TENANT_ID \
       --set azureClientID=$AZURE_CLIENT_ID \
-      --set azureClientSecret=$AZURE_CLIENT_SECRET"
+      --set azureClientSecret=$AZURE_CLIENT_SECRET \
+      --set crdPattern=*"
     - "kubectl create namespace pre-release"
     # TODO: Can remove the -c parameter once we GA. -c means skip waiting for CRDs to reach established state.
     # TODO: this needs to be skipped because the CRD label for filtering doesn't exist in the old beta charts.
@@ -629,6 +630,7 @@ tasks:
       --set azureClientID=$AZURE_CLIENT_ID \
       --set azureClientSecret=$AZURE_CLIENT_SECRET \
       --set image.repository={{.LOCAL_REGISTRY_CONTROLLER_DOCKER_IMAGE}} \
+      --set crdPattern=* \
       aso2 -n {{.ASO_NAMESPACE}} --create-namespace ./charts/azure-service-operator/"
     - task: controller:wait-for-operator-ready
 
@@ -641,6 +643,7 @@ tasks:
       --set azureClientID={{.AZURE_MI_CLIENT_ID}} \
       --set useWorkloadIdentityAuth=true \
       --set image.repository={{.LOCAL_REGISTRY_CONTROLLER_DOCKER_IMAGE}} \
+      --set crdPattern=* \
       aso2 -n {{.ASO_NAMESPACE}} --create-namespace ./charts/azure-service-operator/"
       - task: controller:wait-for-operator-ready
     vars:
@@ -714,6 +717,7 @@ tasks:
     dir: "{{.CONTROLLER_ROOT}}"
     cmds:
       - "{{.SCRIPTS_ROOT}}/kustomize-build.sh -k operator -v {{.VERSION}} > out/operator.yaml"
+      - "sed -i 's/--crd-pattern=.*/--crd-pattern=*/g' out/operator.yaml"
       - "kubectl apply --server-side=true -f out/operator.yaml" # TODO: may need | sed "s_${CONFIG_REGISTRY}_${REGISTRY}/${IMG}_" at some point
 
   controller:bundle-crds:
@@ -820,6 +824,7 @@ tasks:
       # Install cluster scope chart (mode == webhooks)
       - "helm upgrade --install --set multitenant.enable=true --set azureOperatorMode=webhooks \
            --set image.repository={{.LOCAL_REGISTRY_CONTROLLER_DOCKER_IMAGE}} \
+           --set crdPattern=* \
            aso2 -n {{.ASO_NAMESPACE}} --create-namespace ./charts/azure-service-operator/"
       - task: controller:wait-for-operator-ready
       # Install tenant chart

scripts/v2/generate-helm-manifest.sh

@@ -61,6 +61,7 @@ find "$GEN_FILES_DIR" -type f -exec sed -i 's/azureserviceoperator-system/{{ .Re
 # Apply CRD guards
 sed -i "1 s/^/$IF_CRDS\n/;$ a {{- end }}" "$GEN_FILES_DIR"/*crd-manager-role*
 flow_control "--crd-pattern" "--crd-pattern" "$IF_CRDS" "$GEN_FILES_DIR"/*_deployment_*
+sed -i 's/--crd-pattern=.*/--crd-pattern={{ .Values.crdPattern }}/g' "$GEN_FILES_DIR"/*_deployment_*
 
 # Perform file level changes for cluster and tenant
 for file in $(find "$GEN_FILES_DIR" -type f)

v2/charts/azure-service-operator/values.yaml

@@ -92,6 +92,25 @@ metrics:
 # the operator will only reconcile that subset if those are the only CRDs it finds when the pod starts.
 installCRDs: true
 
+# crdPattern is a semicolon delimited string containing the CRD patterns for the operator to install.
+# Setting this has no effect if installCRDs is false.
+# This defines what new CRDs will be installed by ASO. Will always upgrade any existing CRDs even if no
+# crdPattern is defined. Leaving this field unspecified for a fresh install of ASO will result in the operator pod
+# exiting with an error saying no CRDs are configured. Leaving this field unspecified during an upgrade of ASO preserves
+# the existing set of CRDs already in the cluster. The existing CRDs will be upgraded to the latest version but no
+# new CRDs will be installed.
+# Values can be globs utilizing * or ?. The pattern is compared against the "{group}/{kind}" string of each CRD.
+# Patterns are case-insensitive.
+# Example: "resources.azure.com/*" would match the "resources.azure.com/ResourceGroup" resource.
+# Example: "compute.azure.com/*" would match all compute CRDs
+# Example: "resources.azure.com/*;compute.azure.com/*" would match all resources.azure.com resources as well as all
+# compute resources.
+# We strongly recommend including entire groups such as "dbformysql.azure.com/*". Individual CRDs such as
+# "dbformysql.azure.com/FlexibleServer" can be listed, but there are often other resources in the group which pair
+# together to enable other scenarios, such as dbformysql.azure.com/FlexibleServersFirewallRules, and it's generally
+# easier to just include the whole group.
+crdPattern: ''
+
 # podAnnotations contain the pod annotations for Azure Service Operator
 podAnnotations: {}
 

v2/cmd/controller/app/flags.go

@@ -7,22 +7,31 @@ package app
 
 import (
 	"flag"
+	"fmt"
 
-	"github.com/pkg/errors"
 	"k8s.io/klog/v2"
 
-	internalflags "github.com/Azure/azure-service-operator/v2/internal/util/flags"
 	"github.com/Azure/azure-service-operator/v2/internal/version"
 )
 
 type Flags struct {
 	MetricsAddr          string
 	HealthAddr           string
 	EnableLeaderElection bool
-	CrdPatterns          []string
+	CRDPatterns          string // This is a ; delimited string containing a collection of patterns
 	PreUpgradeCheck      bool
 }
 
+func (f Flags) String() string {
+	return fmt.Sprintf(
+		"MetricsAddr: %s, HealthAddr: %s, EnableLeaderElection: %t, CRDPatterns: %s, PreUpgradeCheck: %t",
+		f.MetricsAddr,
+		f.HealthAddr,
+		f.EnableLeaderElection,
+		f.CRDPatterns,
+		f.PreUpgradeCheck)
+}
+
 func ParseFlags(args []string) (Flags, error) {
 	exeName := args[0] + " " + version.BuildVersion
 	flagSet := flag.NewFlagSet(exeName, flag.ExitOnError)
@@ -31,32 +40,25 @@ func ParseFlags(args []string) (Flags, error) {
 	var metricsAddr string
 	var healthAddr string
 	var enableLeaderElection bool
-	var crdPatterns internalflags.SliceFlags
+	var crdPatterns string
 	var preUpgradeCheck bool
 
 	// default here for 'MetricsAddr' is set to "0", which sets metrics to be disabled if 'metrics-addr' flag is omitted.
 	flagSet.StringVar(&metricsAddr, "metrics-addr", "0", "The address the metric endpoint binds to.")
 	flagSet.StringVar(&healthAddr, "health-addr", "", "The address the healthz endpoint binds to.")
 	flagSet.BoolVar(&enableLeaderElection, "enable-leader-election", false,
 		"Enable leader election for controllers manager. Enabling this will ensure there is only one active controllers manager.")
-	flagSet.Var(&crdPatterns, "crd-pattern", "Install these CRDs. Currently the only value supported is '*'")
+	flagSet.StringVar(&crdPatterns, "crd-pattern", "", "Install these CRDs. CRDs already in the cluster will also always be upgraded.")
 	flagSet.BoolVar(&preUpgradeCheck, "pre-upgrade-check", false,
 		"Enable pre upgrade check to check if existing crds contain helm 'keep' policy.")
 
 	flagSet.Parse(args[1:]) //nolint:errcheck
 
-	if len(crdPatterns) >= 2 {
-		return Flags{}, errors.Errorf("crd-pattern flag can have at most 1 argument, had %d", len(crdPatterns))
-	}
-	if len(crdPatterns) == 1 && crdPatterns[0] != "*" {
-		return Flags{}, errors.Errorf("crd-pattern flag must be '*', was %q", crdPatterns[0])
-	}
-
 	return Flags{
 		MetricsAddr:          metricsAddr,
 		HealthAddr:           healthAddr,
 		EnableLeaderElection: enableLeaderElection,
-		CrdPatterns:          crdPatterns,
+		CRDPatterns:          crdPatterns,
 		PreUpgradeCheck:      preUpgradeCheck,
 	}, nil
 }

v2/cmd/controller/app/setup.go

@@ -120,6 +120,7 @@ func SetupControllerManager(ctx context.Context, setupLog logr.Logger, flgs Flag
 		os.Exit(1)
 	}
 
+	// TODO: Put all of the CRD stuff into a method?
 	crdManager, err := newCRDManager(clients.log, mgr.GetConfig())
 	if err != nil {
 		setupLog.Error(err, "failed to initialize CRD client")
@@ -138,16 +139,33 @@ func SetupControllerManager(ctx context.Context, setupLog logr.Logger, flgs Flag
 		os.Exit(1)
 	}
 
-	nonReadyResources := crdmanagement.GetNonReadyCRDs(cfg, crdManager, goalCRDs, existingCRDs)
+	// We only apply CRDs if we're in webhooks mode. No other mode will have CRD CRUD permissions
+	if cfg.OperatorMode.IncludesWebhooks() {
+		var installationInstructions []*crdmanagement.CRDInstallationInstruction
+		installationInstructions, err = crdManager.DetermineCRDsToInstallOrUpgrade(goalCRDs, existingCRDs, flgs.CRDPatterns)
+		if err != nil {
+			setupLog.Error(err, "failed to determine CRDs to apply")
+			os.Exit(1)
+		}
 
-	if len(flgs.CrdPatterns) > 0 {
-		err = crdManager.ApplyCRDs(ctx, goalCRDs, existingCRDs, flgs.CrdPatterns)
+		included := crdmanagement.IncludedCRDs(installationInstructions)
+		if len(included) == 0 {
+			err = errors.New("No existing CRDs in cluster and no --crd-pattern specified")
+			setupLog.Error(err, "failed to apply CRDs")
+			os.Exit(1)
+		}
+
+		err = crdManager.ApplyCRDs(ctx, installationInstructions)
 		if err != nil {
 			setupLog.Error(err, "failed to apply CRDs")
 			os.Exit(1)
 		}
 	}
 
+	// Of all the resources we know of, find any that aren't ready. We will use this collection
+	// to skip watching of these not-ready resources.
+	nonReadyResources := crdmanagement.GetNonReadyCRDs(cfg, crdManager, goalCRDs, existingCRDs)
+
 	if cfg.OperatorMode.IncludesWatchers() {
 		//nolint:contextcheck
 		err = initializeWatchers(nonReadyResources, cfg, mgr, clients)

v2/cmd/controller/main.go

@@ -26,6 +26,8 @@ func main() {
 		os.Exit(1)
 	}
 
+	setupLog.Info("Launching with flags", "flags", flgs.String())
+
 	if flgs.PreUpgradeCheck {
 		err = app.SetupPreUpgradeCheck(ctx)
 		if err != nil {

v2/config/manager/manager.yaml

@@ -37,7 +37,7 @@ spec:
         - --health-addr=:8081
         - --enable-leader-election
         - --v=2
-        - --crd-pattern=*
+        - --crd-pattern=
         ports:
           - containerPort: 8081
             name: health-port

v2/internal/crdmanagement/crd_installation_instruction.go

@@ -0,0 +1,82 @@
+/*
+ * Copyright (c) Microsoft Corporation.
+ * Licensed under the MIT license.
+ */
+
+package crdmanagement
+
+import (
+	"fmt"
+
+	apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+)
+
+type DiffResult string
+
+const (
+	NoDifference     = DiffResult("NoDifference")
+	SpecDifferent    = DiffResult("SpecDifferent")
+	VersionDifferent = DiffResult("VersionDifferent")
+)
+
+func (i DiffResult) DiffReason(crd apiextensions.CustomResourceDefinition) string {
+	switch i {
+	case NoDifference:
+		return fmt.Sprintf("No difference between existing and goal CRD %q", makeMatchString(crd))
+	case SpecDifferent:
+		return fmt.Sprintf("The spec was different between existing and goal CRD %q", makeMatchString(crd))
+	case VersionDifferent:
+		return fmt.Sprintf("The version was different between existing and goal CRD %q", makeMatchString(crd))
+	default:
+		return fmt.Sprintf("Unknown DiffResult %q", i)
+	}
+}
+
+type FilterResult string
+
+const (
+	MatchedExistingCRD = FilterResult("MatchedExistingCRD")
+	MatchedPattern     = FilterResult("MatchedPattern")
+	Excluded           = FilterResult("Excluded")
+)
+
+type CRDInstallationInstruction struct {
+	CRD apiextensions.CustomResourceDefinition
+
+	// FilterResult contains the result of if the CRD was considered for installation or not
+	FilterResult FilterResult
+	// FilterReason contains a user-facing reason for why the CRD was or was not considered for installation
+	FilterReason string
+
+	// DiffResult contains the result of the diff between the existing CRD (if any) and the goal CRD. This may
+	// be NoDifference if the CRD was filtered from consideration before the diff phase.
+	DiffResult DiffResult
+}
+
+func (i *CRDInstallationInstruction) ShouldApply() (bool, string) {
+	excluded := i.FilterResult == Excluded
+	if excluded {
+		return false, i.FilterReason
+	}
+
+	isSame := i.DiffResult == NoDifference
+	if isSame {
+		return false, i.DiffResult.DiffReason(i.CRD)
+	}
+
+	return true, i.DiffResult.DiffReason(i.CRD)
+}
+
+func IncludedCRDs(instructions []*CRDInstallationInstruction) []*CRDInstallationInstruction {
+	// Prealloc false positive: https://github.com/alexkohler/prealloc/issues/16
+	//nolint:prealloc
+	var result []*CRDInstallationInstruction
+	for _, instruction := range instructions {
+		if instruction.FilterResult == Excluded {
+			continue
+		}
+		result = append(result, instruction)
+	}
+
+	return result
+}

v2/internal/crdmanagement/helpers.go

@@ -4,6 +4,8 @@
 package crdmanagement
 
 import (
+	"fmt"
+
 	"github.com/go-logr/logr"
 	"github.com/pkg/errors"
 
@@ -31,9 +33,9 @@ func GetNonReadyCRDs(
 		equalityCheck = SpecEqualIgnoreConversionWebhook
 	}
 
-	readyResources := crdManager.FindNonMatchingCRDs(existingCRDs, goalCRDs, equalityCheck)
+	nonReadyResources := crdManager.FindNonMatchingCRDs(existingCRDs, goalCRDs, equalityCheck)
 
-	return readyResources
+	return nonReadyResources
 }
 
 func FilterStorageTypesByReadyCRDs(
@@ -58,7 +60,7 @@ func FilterStorageTypesByReadyCRDs(
 		}
 
 		if skipKinds.Contains(gvk.GroupKind()) {
-			logger.V(0).Info("Skipping reconciliation of resource because CRD needs update", "groupKind", gvk.GroupKind().String())
+			logger.V(0).Info("Skipping reconciliation of resource because CRD was not installed", "groupKind", gvk.GroupKind().String())
 			continue
 		}
 
@@ -88,7 +90,7 @@ func FilterKnownTypesByReadyCRDs(
 			return nil, errors.Wrapf(err, "creating GVK for obj %T", knownType)
 		}
 		if skipKinds.Contains(gvk.GroupKind()) {
-			logger.V(0).Info("Skipping webhooks of resource because CRD needs update", "groupKind", gvk.GroupKind().String())
+			logger.V(0).Info("Skipping webhooks of resource because CRD was not installed", "groupKind", gvk.GroupKind().String())
 			continue
 		}
 
@@ -97,3 +99,11 @@ func FilterKnownTypesByReadyCRDs(
 
 	return result, nil
 }
+
+func makeMatchString(crd apiextensions.CustomResourceDefinition) string {
+	group := crd.Spec.Group
+	kind := crd.Spec.Names.Kind
+
+	// matchString should be "group/kind"
+	return fmt.Sprintf("%s/%s", group, kind)
+}