Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
  •  
  •  
  •  
2 changes: 2 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -266,3 +266,5 @@ require (
sigs.k8s.io/kustomize/kyaml v0.14.3-0.20230601165947-6ce0bf390ce3 // indirect
sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect
)

replace github.com/openshift/api => github.com/anirudhAgniRedhat/openshift-api v0.1.2
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ github.com/alexkohler/prealloc v1.0.0 h1:Hbq0/3fJPQhNkN0dR95AVrr6R7tou91y0uHG5pO
github.com/alexkohler/prealloc v1.0.0/go.mod h1:VetnK3dIgFBBKmg0YnD9F9x6Icjd+9cvfHR56wJVlKE=
github.com/alingse/asasalint v0.0.11 h1:SFwnQXJ49Kx/1GghOFz1XGqHYKp21Kq1nHad/0WQRnw=
github.com/alingse/asasalint v0.0.11/go.mod h1:nCaoMhw7a9kSJObvQyVzNTPBDbNpdocqrSP7t/cW5+I=
github.com/anirudhAgniRedhat/openshift-api v0.1.2 h1:hm9pEd3ySb3XoiTdeqcc2fT4boM9TIlMy3ERMCc21S4=
github.com/anirudhAgniRedhat/openshift-api v0.1.2/go.mod h1:CxgbWAlvu2iQB0UmKTtRu1YfepRg1/vJ64n2DlIEVz4=
github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5 h1:0CwZNZbxp69SHPdPJAN/hZIm0C4OItdklCFmMRWYpio=
github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5/go.mod h1:wHh0iHkYZB8zMSxRWpUBQtwG5a7fFgvEO+odwuTv2gs=
github.com/ashanbrown/forbidigo v1.5.1 h1:WXhzLjOlnuDYPYQo/eFlcFMi8X/kLfvWLYu6CSoebis=
Expand Down Expand Up @@ -479,8 +481,6 @@ 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/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=
Expand Down
6 changes: 3 additions & 3 deletions hack/crds-sync.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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%%:*}"
Expand Down
737 changes: 447 additions & 290 deletions install/0000_30_machine-api-operator_02_machine.crd.yaml

Large diffs are not rendered by default.

840 changes: 522 additions & 318 deletions install/0000_30_machine-api-operator_03_machineset.crd.yaml

Large diffs are not rendered by default.

404 changes: 236 additions & 168 deletions install/0000_30_machine-api-operator_07_machinehealthcheck.crd.yaml

Large diffs are not rendered by default.

76 changes: 76 additions & 0 deletions pkg/webhooks/machine_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"strconv"
"strings"

_ "github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This goes in the tools.go file

corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/resource"
Expand Down Expand Up @@ -192,6 +193,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:
Expand Down Expand Up @@ -925,6 +932,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
Expand Down Expand Up @@ -2055,3 +2069,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)
}
83 changes: 83 additions & 0 deletions pkg/webhooks/machine_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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())
}
})
}
}
4 changes: 3 additions & 1 deletion vendor/github.com/openshift/api/Makefile

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.