Merge pull request #1234 from anirudhAgniRedhat/anirudhAgniRedhat/cap…

…acityReservationGroupIDWebhookValidation

CFE-1051: Add the webhook validation for "CapacityReservationGroupID" to "AzureMachineProviderSpec" in openshift/machine-api-operator

Makefile

@@ -55,7 +55,7 @@ endif
 PROJECT_DIR := $(shell dirname $(abspath $(lastword $(MAKEFILE_LIST))))
 ENVTEST = go run ${PROJECT_DIR}/vendor/sigs.k8s.io/controller-runtime/tools/setup-envtest
 # ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary.
-ENVTEST_K8S_VERSION = 1.29
+ENVTEST_K8S_VERSION = 1.29.1
 
 .PHONY: vendor
 vendor:
@@ -103,7 +103,7 @@ test-sec:
 test: unit
 
 unit:
-	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path --bin-dir $(PROJECT_DIR)/bin)" ./hack/ci-test.sh
+	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path --bin-dir $(PROJECT_DIR)/bin --remote-bucket openshift-kubebuilder-tools)" ./hack/ci-test.sh
 
 .PHONY: image
 image: ## Build docker image

cmd/vsphere/main.go

@@ -8,6 +8,7 @@ import (
 	"time"
 
 	configv1 "github.com/openshift/api/config/v1"
+	apifeatures "github.com/openshift/api/features"
 	machinev1 "github.com/openshift/api/machine/v1beta1"
 	configv1client "github.com/openshift/client-go/config/clientset/versioned"
 	configinformers "github.com/openshift/client-go/config/informers/externalversions"
@@ -173,7 +174,7 @@ func main() {
 		klog.Fatalf("unable to retrieve current feature gates: %v", err)
 	}
 	// read featuregate read and usage to set a variable to pass to a controller
-	staticIPFeatureGateEnabled := featureGates.Enabled(configv1.FeatureGateVSphereStaticIPs)
+	staticIPFeatureGateEnabled := featureGates.Enabled(apifeatures.FeatureGateVSphereStaticIPs)
 
 	// Initialize machine actuator.
 	machineActuator := machine.NewActuator(machine.ActuatorParams{

go.mod

@@ -10,7 +10,7 @@ require (
 	github.com/google/uuid v1.4.0
 	github.com/onsi/ginkgo/v2 v2.14.0
 	github.com/onsi/gomega v1.30.0
-	github.com/openshift/api v0.0.0-20240124164020-e2ce40831f2e
+	github.com/openshift/api v0.0.0-20240521141249-8af21b7ed3e3
 	github.com/openshift/client-go v0.0.0-20240115204758-e6bf7d631d5e
 	github.com/openshift/library-go v0.0.0-20240116081341-964bcb3f545c
 	github.com/prometheus/client_golang v1.18.0

go.sum

@@ -479,8 +479,8 @@ github.com/onsi/ginkgo/v2 v2.14.0 h1:vSmGj2Z5YPb9JwCWT6z6ihcUvDhuXLc3sJiqd3jMKAY
 github.com/onsi/ginkgo/v2 v2.14.0/go.mod h1:JkUdW7JkN0V6rFvsHcJ478egV3XH9NxpD27Hal/PhZw=
 github.com/onsi/gomega v1.30.0 h1:hvMK7xYz4D3HapigLTeGdId/NcfQx1VHMJc60ew99+8=
 github.com/onsi/gomega v1.30.0/go.mod h1:9sxs+SwGrKI0+PWe4Fxa9tFQQBG5xSsSbMXOI8PPpoQ=
-github.com/openshift/api v0.0.0-20240124164020-e2ce40831f2e h1:cxgCNo/R769CO23AK5TCh45H9SMUGZ8RukiF2/Qif3o=
-github.com/openshift/api v0.0.0-20240124164020-e2ce40831f2e/go.mod h1:CxgbWAlvu2iQB0UmKTtRu1YfepRg1/vJ64n2DlIEVz4=
+github.com/openshift/api v0.0.0-20240521141249-8af21b7ed3e3 h1:fxqjG8C/fU1UfUalZhNB01jqIQDlBsCVsFnWZ1V17Rg=
+github.com/openshift/api v0.0.0-20240521141249-8af21b7ed3e3/go.mod h1:CxgbWAlvu2iQB0UmKTtRu1YfepRg1/vJ64n2DlIEVz4=
 github.com/openshift/client-go v0.0.0-20240115204758-e6bf7d631d5e h1:qGjfKX8i0h4efMNEnhgTdxcdx6gwwOwhTfBJ20WFqA8=
 github.com/openshift/client-go v0.0.0-20240115204758-e6bf7d631d5e/go.mod h1:2am3qrggh9LlDCf/MDGzcFWMhdaushxFQi0+ZZDhdVk=
 github.com/openshift/library-go v0.0.0-20240116081341-964bcb3f545c h1:gLylEQQryG+A6nqWYIwE1wUzn1eFUmthjADvflMWKnM=

hack/crds-sync.sh

@@ -3,9 +3,9 @@
 set -euo pipefail
 
 # map names of CRD files between the vendored openshift/api repository and the ./install directory
-CRDS_MAPPING=( "vendor/github.com/openshift/api/machine/v1beta1/0000_10_machine.crd.yaml:0000_30_machine-api-operator_02_machine.crd.yaml"
-               "vendor/github.com/openshift/api/machine/v1beta1/0000_10_machineset.crd.yaml:0000_30_machine-api-operator_03_machineset.crd.yaml"
-               "vendor/github.com/openshift/api/machine/v1beta1/0000_10_machinehealthcheck.yaml:0000_30_machine-api-operator_07_machinehealthcheck.crd.yaml")
+CRDS_MAPPING=( "vendor/github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines.crd.yaml:0000_30_machine-api-operator_02_machine.crd.yaml"
+               "vendor/github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets.crd.yaml:0000_30_machine-api-operator_03_machineset.crd.yaml"
+               "vendor/github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinehealthchecks.crd.yaml:0000_30_machine-api-operator_07_machinehealthcheck.crd.yaml")
 
 for crd in "${CRDS_MAPPING[@]}" ; do
     SRC="${crd%%:*}"

pkg/controller/vsphere/actuator_test.go

@@ -62,7 +62,7 @@ func TestMachineEvents(t *testing.T) {
 	testEnv := &envtest.Environment{
 		CRDDirectoryPaths: []string{
 			filepath.Join("..", "..", "..", "install"),
-			filepath.Join("..", "..", "..", "vendor", "github.com", "openshift", "api", "config", "v1"),
+			filepath.Join("..", "..", "..", "vendor", "github.com", "openshift", "api", "config", "v1", "zz_generated.crd-manifests"),
 			filepath.Join("..", "..", "..", "third_party", "cluster-api", "crd")},
 	}
 

pkg/controller/vsphere/machine_scope_test.go

@@ -287,7 +287,7 @@ func TestPatchMachine(t *testing.T) {
 	testEnv := &envtest.Environment{
 		CRDDirectoryPaths: []string{
 			filepath.Join("..", "..", "..", "install"),
-			filepath.Join("..", "..", "..", "vendor", "github.com", "openshift", "api", "config", "v1")},
+			filepath.Join("..", "..", "..", "vendor", "github.com", "openshift", "api", "config", "v1", "zz_generated.crd-manifests")},
 	}
 
 	cfg, err := testEnv.Start()
@@ -546,7 +546,7 @@ func TestNodeGetter(t *testing.T) {
 	testEnv := &envtest.Environment{
 		CRDDirectoryPaths: []string{
 			filepath.Join("..", "..", "..", "install"),
-			filepath.Join("..", "..", "..", "vendor", "github.com", "openshift", "api", "config", "v1")},
+			filepath.Join("..", "..", "..", "vendor", "github.com", "openshift", "api", "config", "v1", "zz_generated.crd-manifests")},
 	}
 
 	cfg, err := testEnv.Start()

pkg/controller/vsphere/util.go

@@ -22,7 +22,7 @@ import (
 
 const (
 	globalInfrastuctureName  = "cluster"
-	openshiftConfigNamespace = "openshift-config"
+	openshiftConfigNamespace = "openshift-config-1"
 )
 
 // vSphereConfig is a copy of the Kubernetes vSphere cloud provider config type

pkg/operator/operator.go

@@ -8,6 +8,7 @@ import (
 	"time"
 
 	osconfigv1 "github.com/openshift/api/config/v1"
+	apifeatures "github.com/openshift/api/features"
 	osoperatorv1 "github.com/openshift/api/operator/v1"
 	osclientset "github.com/openshift/client-go/config/clientset/versioned"
 	configinformersv1 "github.com/openshift/client-go/config/informers/externalversions/config/v1"
@@ -171,9 +172,9 @@ func New(
 
 		klog.V(4).InfoS("FeatureGates changed", "enabled", featureChange.New.Enabled, "disabled", featureChange.New.Disabled)
 		prevDisableMHC := featuregates.NewFeatureGate(featureChange.Previous.Enabled, featureChange.Previous.Disabled).
-			Enabled(osconfigv1.FeatureGateMachineAPIOperatorDisableMachineHealthCheckController)
+			Enabled(apifeatures.FeatureGateMachineAPIOperatorDisableMachineHealthCheckController)
 		newDisableMHC := featuregates.NewFeatureGate(featureChange.New.Enabled, featureChange.New.Disabled).
-			Enabled(osconfigv1.FeatureGateMachineAPIOperatorDisableMachineHealthCheckController)
+			Enabled(apifeatures.FeatureGateMachineAPIOperatorDisableMachineHealthCheckController)
 
 		if prevDisableMHC != newDisableMHC {
 			klog.V(2).InfoS("Resync for modified feature gate",
@@ -461,7 +462,7 @@ func (optr *Operator) maoConfigFromInfrastructure() (*OperatorConfig, error) {
 	if err != nil {
 		return nil, err
 	}
-	if featureGates.Enabled(osconfigv1.FeatureGateMachineAPIOperatorDisableMachineHealthCheckController) {
+	if featureGates.Enabled(apifeatures.FeatureGateMachineAPIOperatorDisableMachineHealthCheckController) {
 		klog.V(2).Info("Disabling MHC controller")
 		mhcImage = ""
 	}

pkg/operator/operator_test.go

@@ -12,6 +12,7 @@ import (
 
 	. "github.com/onsi/gomega"
 	openshiftv1 "github.com/openshift/api/config/v1"
+	apifeatures "github.com/openshift/api/features"
 	fakeos "github.com/openshift/client-go/config/clientset/versioned/fake"
 	configinformersv1 "github.com/openshift/client-go/config/informers/externalversions"
 	fakemachine "github.com/openshift/client-go/machine/clientset/versioned/fake"
@@ -61,7 +62,7 @@ func newFakeOperator(kubeObjects, osObjects, machineObjects []runtime.Object, im
 					{
 						Version:  "",
 						Enabled:  []openshiftv1.FeatureGateAttributes{},
-						Disabled: []openshiftv1.FeatureGateAttributes{{Name: openshiftv1.FeatureGateMachineAPIOperatorDisableMachineHealthCheckController}},
+						Disabled: []openshiftv1.FeatureGateAttributes{{Name: apifeatures.FeatureGateMachineAPIOperatorDisableMachineHealthCheckController}},
 					},
 				},
 			},
@@ -571,7 +572,7 @@ func TestMAOConfigFromInfrastructure(t *testing.T) {
 						{
 							Version:  "",
 							Enabled:  []openshiftv1.FeatureGateAttributes{},
-							Disabled: []openshiftv1.FeatureGateAttributes{{Name: openshiftv1.FeatureGateMachineAPIOperatorDisableMachineHealthCheckController}},
+							Disabled: []openshiftv1.FeatureGateAttributes{{Name: apifeatures.FeatureGateMachineAPIOperatorDisableMachineHealthCheckController}},
 						},
 					},
 				},
@@ -603,7 +604,7 @@ func TestMAOConfigFromInfrastructure(t *testing.T) {
 					FeatureGates: []openshiftv1.FeatureGateDetails{
 						{
 							Version:  "",
-							Enabled:  []openshiftv1.FeatureGateAttributes{{Name: openshiftv1.FeatureGateMachineAPIOperatorDisableMachineHealthCheckController}},
+							Enabled:  []openshiftv1.FeatureGateAttributes{{Name: apifeatures.FeatureGateMachineAPIOperatorDisableMachineHealthCheckController}},
 							Disabled: []openshiftv1.FeatureGateAttributes{},
 						},
 					},

pkg/webhooks/machine_webhook.go

@@ -192,6 +192,12 @@ const (
 	powerVSImage                    = "image"
 	powerVSSystemTypeE880           = "e880"
 	powerVSSystemTypeE980           = "e980"
+	azureProviderIDPrefix           = "azure://"
+	azureProvidersKey               = "providers"
+	azureSubscriptionsKey           = "subscriptions"
+	azureResourceGroupsLowerKey     = "resourcegroups"
+	azureLocationsKey               = "locations"
+	azureBuiltInResourceNamespace   = "Microsoft.Resources"
 )
 
 // GCP Confidential VM supports Compute Engine machine types in the following series:
@@ -925,6 +931,13 @@ func validateAzure(m *machinev1beta1.Machine, config *admissionConfig) (bool, []
 			fmt.Sprintf("osDisk.diskSettings.ephemeralStorageLocation can either be omitted or set to %s", azureEphemeralStorageLocationLocal)))
 	}
 
+	if providerSpec.CapacityReservationGroupID != "" {
+		err := validateAzureCapacityReservationGroupID(providerSpec.CapacityReservationGroupID)
+		if err != nil {
+			errs = append(errs, field.Invalid(field.NewPath("providerSpec", "capacityReservationGroupID"), providerSpec.CapacityReservationGroupID, err.Error()))
+		}
+	}
+
 	switch providerSpec.OSDisk.CachingType {
 	case azureCachingTypeNone, azureCachingTypeReadOnly, azureCachingTypeReadWrite, "":
 		// Valid scenarios, do nothing
@@ -2055,3 +2068,65 @@ func validateGVK(gvk schema.GroupVersionKind, platform osconfigv1.PlatformType)
 		return true
 	}
 }
+
+func validateAzureCapacityReservationGroupID(capacityReservationGroupID string) error {
+	id := strings.TrimPrefix(capacityReservationGroupID, azureProviderIDPrefix)
+	err := parseAzureResourceID(id)
+	if err != nil {
+		return err
+	}
+	return nil
+}
+
+// parseAzureResourceID parses a string to an instance of ResourceID
+func parseAzureResourceID(id string) error {
+	if len(id) == 0 {
+		return fmt.Errorf("invalid resource ID: id cannot be empty")
+	}
+	if !strings.HasPrefix(id, "/") {
+		return fmt.Errorf("invalid resource ID: resource id '%s' must start with '/'", id)
+	}
+	parts := splitStringAndOmitEmpty(id, "/")
+
+	if len(parts) < 2 {
+		return fmt.Errorf("invalid resource ID: %s", id)
+	}
+	if !strings.EqualFold(parts[0], azureSubscriptionsKey) && !strings.EqualFold(parts[0], azureProvidersKey) {
+		return fmt.Errorf("invalid resource ID: %s", id)
+	}
+	return appendNextAzureResourceIDValidation(parts, id)
+}
+
+func splitStringAndOmitEmpty(v, sep string) []string {
+	r := make([]string, 0)
+	for _, s := range strings.Split(v, sep) {
+		if len(s) == 0 {
+			continue
+		}
+		r = append(r, s)
+	}
+	return r
+}
+
+func appendNextAzureResourceIDValidation(parts []string, id string) error {
+	if len(parts) == 0 {
+		return nil
+	}
+	if len(parts) == 1 {
+		// subscriptions and resourceGroups are not valid ids without their names
+		if strings.EqualFold(parts[0], azureSubscriptionsKey) || strings.EqualFold(parts[0], azureResourceGroupsLowerKey) {
+			return fmt.Errorf("invalid resource ID: %s", id)
+		}
+		return nil
+	}
+	if strings.EqualFold(parts[0], azureProvidersKey) && (len(parts) == 2 || strings.EqualFold(parts[2], azureProvidersKey)) {
+		return appendNextAzureResourceIDValidation(parts[2:], id)
+	}
+	if len(parts) > 3 && strings.EqualFold(parts[0], azureProvidersKey) {
+		return appendNextAzureResourceIDValidation(parts[4:], id)
+	}
+	if len(parts) > 1 && !strings.EqualFold(parts[0], azureProvidersKey) {
+		return appendNextAzureResourceIDValidation(parts[2:], id)
+	}
+	return fmt.Errorf("invalid resource ID: %s", id)
+}

pkg/webhooks/machine_webhook_test.go

@@ -332,6 +332,48 @@ func TestMachineCreation(t *testing.T) {
 			},
 			expectedError: "",
 		},
+		{
+			name:         "with Azure and CapacityReservationID is empty",
+			platformType: osconfigv1.AzurePlatformType,
+			clusterID:    "azure-cluster",
+			providerSpecValue: &kruntime.RawExtension{
+				Object: &machinev1beta1.AzureMachineProviderSpec{
+					OSDisk: machinev1beta1.OSDisk{
+						DiskSizeGB: 128,
+					},
+					CapacityReservationGroupID: "",
+				},
+			},
+			expectedError: "",
+		},
+		{
+			name:         "with Azure and CapacityReservationID is valid",
+			platformType: osconfigv1.AzurePlatformType,
+			clusterID:    "azure-cluster",
+			providerSpecValue: &kruntime.RawExtension{
+				Object: &machinev1beta1.AzureMachineProviderSpec{
+					OSDisk: machinev1beta1.OSDisk{
+						DiskSizeGB: 128,
+					},
+					CapacityReservationGroupID: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myResourceGroupName/providers/Microsoft.Compute/capacityReservationGroups/myCapacityReservationGroup",
+				},
+			},
+			expectedError: "",
+		},
+		{
+			name:         "with Azure and CapacityReservationID is not valid",
+			platformType: osconfigv1.AzurePlatformType,
+			clusterID:    "azure-cluster",
+			providerSpecValue: &kruntime.RawExtension{
+				Object: &machinev1beta1.AzureMachineProviderSpec{
+					OSDisk: machinev1beta1.OSDisk{
+						DiskSizeGB: 128,
+					},
+					CapacityReservationGroupID: "/subscri/00000000-0000-0000-0000-000000000000/resour/myResourceGroupName/providers/Microsoft.Compute/capacityReservationGroups/myCapacityReservationGroup",
+				},
+			},
+			expectedError: "admission webhook \"validation.machine.machine.openshift.io\" denied the request: providerSpec.capacityReservationGroupID: Invalid value: \"/subscri/00000000-0000-0000-0000-000000000000/resour/myResourceGroupName/providers/Microsoft.Compute/capacityReservationGroups/myCapacityReservationGroup\": invalid resource ID: /subscri/00000000-0000-0000-0000-000000000000/resour/myResourceGroupName/providers/Microsoft.Compute/capacityReservationGroups/myCapacityReservationGroup",
+		},
 		{
 			name:         "with Azure and a Premium Disk Data Disk set",
 			platformType: osconfigv1.AzurePlatformType,
@@ -5040,3 +5082,44 @@ func TestValidateNutanixProviderSpec(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateAzureCapacityReservationGroupID(t *testing.T) {
+	testCases := []struct {
+		name        string
+		inputID     string
+		expectError bool
+	}{
+		{
+			name:        "validation for capacityReservationGroupID should return nil error if field input is valid",
+			inputID:     "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myResourceGroupName/providers/Microsoft.Compute/capacityReservationGroups/myCapacityReservationGroup",
+			expectError: false,
+		},
+		{
+			name:        "validation for capacityReservationGroupID should return error if field input does not start with '/'",
+			inputID:     "subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myResourceGroupName/providers/Microsoft.Compute/capacityReservationGroups/myCapacityReservationGroup",
+			expectError: true,
+		},
+		{
+			name:        "validation for capacityReservationGroupID should return error if field input does not have field name subscriptions",
+			inputID:     "/subscripti/00000000-0000-0000-0000-000000000000/resourceGroups/myResourceGroupName/providers/Microsoft.Compute/capacityReservationGroups/myCapacityReservationGroup",
+			expectError: true,
+		},
+		{
+			name:        "validation for capacityReservationGroupID should return error if field input is empty",
+			inputID:     "",
+			expectError: true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+			err := validateAzureCapacityReservationGroupID(tc.inputID)
+			if tc.expectError {
+				g.Expect(err).To(HaveOccurred())
+			} else {
+				g.Expect(err).ToNot(HaveOccurred())
+			}
+		})
+	}
+}

pkg/webhooks/v1beta1_suite_test.go

@@ -55,7 +55,7 @@ func TestMain(m *testing.M) {
 	testEnv = &envtest.Environment{
 		CRDDirectoryPaths: []string{
 			filepath.Join("..", "..", "install"),
-			filepath.Join("..", "..", "..", "..", "vendor", "github.com", "openshift", "api", "config", "v1"),
+			filepath.Join("..", "..", "..", "..", "vendor", "github.com", "openshift", "api", "config", "v1", "zz_generated.crd-manifests"),
 		},
 		WebhookInstallOptions: envtest.WebhookInstallOptions{
 			MutatingWebhooks:   []*admissionregistrationv1.MutatingWebhookConfiguration{NewMachineMutatingWebhookConfiguration()},

tools.go

@@ -6,5 +6,7 @@ package tools
 import (
 	_ "github.com/golangci/golangci-lint/cmd/golangci-lint"
 	_ "github.com/onsi/ginkgo/v2/ginkgo"
+	_ "github.com/openshift/api/config/v1/zz_generated.crd-manifests"
+	_ "github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests"
 	_ "sigs.k8s.io/controller-runtime/tools/setup-envtest"
 )