Skip to content

Commit 41c1c37

Browse files
anirudhAgniRedhatanirudhAgniRedhat
committed
Added immutability validations
1 parent a779f91 commit 41c1c37

File tree

4 files changed

+220
-33
lines changed

4 files changed

+220
-33
lines changed

‎pkg/webhooks/machine_webhook.go

Lines changed: 33 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -315,7 +315,7 @@ func getDNS() (*osconfigv1.DNS, error) {
315315
return dns, nil
316316
}
317317

318-
type machineAdmissionFn func(m *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList)
318+
type machineAdmissionFn func(m, mOld *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList)
319319

320320
type admissionConfig struct {
321321
clusterID string
@@ -396,7 +396,7 @@ func getMachineValidatorOperation(platform osconfigv1.PlatformType) machineAdmis
396396
return validateNutanix
397397
default:
398398
// just no-op
399-
return func(m *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
399+
return func(m, mOld *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
400400
return true, []string{}, nil
401401
}
402402
}
@@ -441,7 +441,7 @@ func getMachineDefaulterOperation(platformStatus *osconfigv1.PlatformStatus) mac
441441
return defaultNutanix
442442
default:
443443
// just no-op
444-
return func(m *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
444+
return func(m, mOld *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
445445
return true, []string{}, nil
446446
}
447447
}
@@ -462,7 +462,7 @@ func (h *machineValidatorHandler) validateMachine(m, oldM *machinev1beta1.Machin
462462

463463
errs := validateMachineLifecycleHooks(m, oldM)
464464

465-
ok, warnings, opErrs := h.webhookOperations(m, h.admissionConfig)
465+
ok, warnings, opErrs := h.webhookOperations(m, oldM, h.admissionConfig)
466466
if !ok {
467467
errs = append(errs, opErrs...)
468468
}
@@ -549,7 +549,7 @@ func (h *machineDefaulterHandler) Default(ctx context.Context, obj runtime.Objec
549549
m.Labels[machinev1beta1.MachineClusterIDLabel] = h.clusterID
550550
}
551551

552-
ok, _, errs := h.webhookOperations(m, h.admissionConfig)
552+
ok, _, errs := h.webhookOperations(m, nil, h.admissionConfig)
553553
if !ok {
554554
return errs.ToAggregate()
555555
}
@@ -561,7 +561,7 @@ type awsDefaulter struct {
561561
region string
562562
}
563563

564-
func (a awsDefaulter) defaultAWS(m *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
564+
func (a awsDefaulter) defaultAWS(m, mOld *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
565565
klog.V(3).Infof("Defaulting AWS providerSpec")
566566

567567
var errs field.ErrorList
@@ -633,7 +633,7 @@ func validateUnknownFields(m *machinev1beta1.Machine, providerSpec interface{})
633633
return nil
634634
}
635635

636-
func validateAWS(m *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
636+
func validateAWS(m, mOld *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
637637
klog.V(3).Infof("Validating AWS providerSpec")
638638

639639
var errs field.ErrorList
@@ -803,7 +803,7 @@ func getDuplicatedTags(tagSpecs []machinev1beta1.TagSpecification) []string {
803803
return duplicatedTags
804804
}
805805

806-
func defaultAzure(m *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
806+
func defaultAzure(m, mOld *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
807807
klog.V(3).Infof("Defaulting Azure providerSpec")
808808

809809
var errs field.ErrorList
@@ -864,7 +864,7 @@ func defaultAzure(m *machinev1beta1.Machine, config *admissionConfig) (bool, []s
864864
return true, warnings, nil
865865
}
866866

867-
func validateAzure(m *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
867+
func validateAzure(m, mOld *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
868868
klog.V(3).Infof("Validating Azure providerSpec")
869869

870870
var errs field.ErrorList
@@ -931,6 +931,18 @@ func validateAzure(m *machinev1beta1.Machine, config *admissionConfig) (bool, []
931931
fmt.Sprintf("osDisk.diskSettings.ephemeralStorageLocation can either be omitted or set to %s", azureEphemeralStorageLocationLocal)))
932932
}
933933

934+
if mOld != nil {
935+
oldProviderSpec := new(machinev1beta1.AzureMachineProviderSpec)
936+
if err := unmarshalInto(mOld, oldProviderSpec); err != nil {
937+
errs = append(errs, err)
938+
return false, warnings, errs
939+
}
940+
if providerSpec.CapacityReservationGroupID != "" && !validateAzureImmutabilityForCapacityReservationGroupID(oldProviderSpec.CapacityReservationGroupID, providerSpec.CapacityReservationGroupID) {
941+
errs = append(errs, field.Invalid(field.NewPath("providerSpec", "capacityReservationGroupID"),
942+
providerSpec.CapacityReservationGroupID, "capacityReservationGroupID is immutable"))
943+
}
944+
}
945+
934946
if providerSpec.CapacityReservationGroupID != "" {
935947
err := validateAzureCapacityReservationGroupID(providerSpec.CapacityReservationGroupID)
936948
if err != nil {
@@ -1031,7 +1043,7 @@ func validateAzureDiagnostics(diagnosticsSpec machinev1beta1.AzureDiagnostics, p
10311043
return errs
10321044
}
10331045

1034-
func defaultGCP(m *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
1046+
func defaultGCP(m, mOld *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
10351047
klog.V(3).Infof("Defaulting GCP providerSpec")
10361048

10371049
var errs field.ErrorList
@@ -1119,7 +1131,7 @@ func defaultGCPDisks(disks []*machinev1beta1.GCPDisk, clusterID string) []*machi
11191131
return disks
11201132
}
11211133

1122-
func validateGCP(m *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
1134+
func validateGCP(m, mOld *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
11231135
klog.V(3).Infof("Validating GCP providerSpec")
11241136

11251137
var errs field.ErrorList
@@ -1354,7 +1366,7 @@ func validateGCPServiceAccounts(serviceAccounts []machinev1beta1.GCPServiceAccou
13541366
return errs
13551367
}
13561368

1357-
func defaultVSphere(m *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
1369+
func defaultVSphere(m, mOld *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
13581370
klog.V(3).Infof("Defaulting vSphere providerSpec")
13591371

13601372
var errs field.ErrorList
@@ -1386,7 +1398,7 @@ func defaultVSphere(m *machinev1beta1.Machine, config *admissionConfig) (bool, [
13861398
return true, warnings, nil
13871399
}
13881400

1389-
func validateVSphere(m *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
1401+
func validateVSphere(m, mOld *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
13901402
klog.V(3).Infof("Validating vSphere providerSpec")
13911403

13921404
var errs field.ErrorList
@@ -1502,7 +1514,7 @@ func validateVSphereNetwork(network machinev1beta1.NetworkSpec, parentPath *fiel
15021514
return errs
15031515
}
15041516

1505-
func defaultNutanix(m *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
1517+
func defaultNutanix(m, mOld *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
15061518
klog.V(3).Infof("Defaulting nutanix providerSpec")
15071519

15081520
var errs field.ErrorList
@@ -1534,7 +1546,7 @@ func defaultNutanix(m *machinev1beta1.Machine, config *admissionConfig) (bool, [
15341546
return true, warnings, nil
15351547
}
15361548

1537-
func validateNutanix(m *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
1549+
func validateNutanix(m, mOld *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
15381550
klog.V(3).Infof("Validating nutanix providerSpec")
15391551

15401552
var errs field.ErrorList
@@ -1843,7 +1855,7 @@ func validateAzureDataDisks(machineName string, spec *machinev1beta1.AzureMachin
18431855
return errs
18441856
}
18451857

1846-
func defaultPowerVS(m *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
1858+
func defaultPowerVS(m, mOld *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
18471859
klog.V(3).Infof("Defaulting PowerVS providerSpec")
18481860

18491861
var errs field.ErrorList
@@ -1890,7 +1902,7 @@ func defaultPowerVS(m *machinev1beta1.Machine, config *admissionConfig) (bool, [
18901902
return true, warnings, nil
18911903
}
18921904

1893-
func validatePowerVS(m *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
1905+
func validatePowerVS(m, mOld *machinev1beta1.Machine, config *admissionConfig) (bool, []string, field.ErrorList) {
18941906
klog.V(3).Infof("Validating PowerVS providerSpec")
18951907

18961908
var errs field.ErrorList
@@ -2069,6 +2081,10 @@ func validateGVK(gvk schema.GroupVersionKind, platform osconfigv1.PlatformType)
20692081
}
20702082
}
20712083

2084+
func validateAzureImmutabilityForCapacityReservationGroupID(oldID string, newID string) bool {
2085+
return oldID == newID
2086+
}
2087+
20722088
func validateAzureCapacityReservationGroupID(capacityReservationGroupID string) error {
20732089
id := strings.TrimPrefix(capacityReservationGroupID, azureProviderIDPrefix)
20742090
err := parseAzureResourceID(id)

‎pkg/webhooks/machine_webhook_test.go

Lines changed: 85 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1572,6 +1572,46 @@ func TestMachineUpdate(t *testing.T) {
15721572
},
15731573
expectedError: "providerSpec.osDisk.cachingType: Invalid value: \"\": Instances using an ephemeral OS disk support only Readonly caching",
15741574
},
1575+
{
1576+
name: "with an Azure ProviderSpec, changing capacityReservationGroupID to another ID",
1577+
platformType: osconfigv1.AzurePlatformType,
1578+
clusterID: azureClusterID,
1579+
baseProviderSpecValue: func() *kruntime.RawExtension {
1580+
defaultSpec := defaultAzureProviderSpec.DeepCopy()
1581+
defaultSpec.CapacityReservationGroupID = "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myResourceGroupName/providers/Microsoft.Compute/capacityReservationGroups/myCapacityReservationGroup"
1582+
return &kruntime.RawExtension{
1583+
Object: defaultSpec,
1584+
}
1585+
}(),
1586+
updatedProviderSpecValue: func() *kruntime.RawExtension {
1587+
object := defaultAzureProviderSpec.DeepCopy()
1588+
object.CapacityReservationGroupID = "/subscriptions/00000000-0000-0000-0000-000000000001/resourceGroups/myResourceGroupName/providers/Microsoft.Compute/capacityReservationGroups/myCapacityReservationGroup"
1589+
return &kruntime.RawExtension{
1590+
Object: object,
1591+
}
1592+
},
1593+
expectedError: "admission webhook \"validation.machine.machine.openshift.io\" denied the request: providerSpec.capacityReservationGroupID: Invalid value: \"/subscriptions/00000000-0000-0000-0000-000000000001/resourceGroups/myResourceGroupName/providers/Microsoft.Compute/capacityReservationGroups/myCapacityReservationGroup\": capacityReservationGroupID is immutable",
1594+
},
1595+
{
1596+
name: "with an Azure ProviderSpec, changing capacityReservationGroupID to '' ",
1597+
platformType: osconfigv1.AzurePlatformType,
1598+
clusterID: azureClusterID,
1599+
baseProviderSpecValue: func() *kruntime.RawExtension {
1600+
defaultSpec := defaultAzureProviderSpec.DeepCopy()
1601+
defaultSpec.CapacityReservationGroupID = "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myResourceGroupName/providers/Microsoft.Compute/capacityReservationGroups/myCapacityReservationGroup"
1602+
return &kruntime.RawExtension{
1603+
Object: defaultSpec,
1604+
}
1605+
}(),
1606+
updatedProviderSpecValue: func() *kruntime.RawExtension {
1607+
object := defaultAzureProviderSpec.DeepCopy()
1608+
object.CapacityReservationGroupID = ""
1609+
return &kruntime.RawExtension{
1610+
Object: object,
1611+
}
1612+
},
1613+
expectedError: "",
1614+
},
15751615
{
15761616
name: "with a valid GCP ProviderSpec",
15771617
platformType: osconfigv1.GCPPlatformType,
@@ -2344,7 +2384,7 @@ func TestValidateAWSProviderSpec(t *testing.T) {
23442384
m.Spec.ProviderSpec.Value = &kruntime.RawExtension{Raw: tc.overrideRawBytes}
23452385
}
23462386

2347-
ok, warnings, webhookErr := h.webhookOperations(m, h.admissionConfig)
2387+
ok, warnings, webhookErr := h.webhookOperations(m, nil, h.admissionConfig)
23482388
if ok != tc.expectedOk {
23492389
t.Errorf("expected: %v, got: %v", tc.expectedOk, ok)
23502390
}
@@ -2420,7 +2460,7 @@ func TestDefaultAWSProviderSpec(t *testing.T) {
24202460
}
24212461
m.Spec.ProviderSpec.Value = &kruntime.RawExtension{Raw: rawBytes}
24222462

2423-
ok, warnings, webhookErr := h.webhookOperations(m, h.admissionConfig)
2463+
ok, warnings, webhookErr := h.webhookOperations(m, nil, h.admissionConfig)
24242464
if ok != tc.expectedOk {
24252465
t.Errorf("expected: %v, got: %v", tc.expectedOk, ok)
24262466
}
@@ -3003,7 +3043,7 @@ func TestValidateAzureProviderSpec(t *testing.T) {
30033043
m.Spec.ProviderSpec.Value = &kruntime.RawExtension{Raw: tc.overrideRawBytes}
30043044
}
30053045

3006-
ok, warnings, webhookErr := h.webhookOperations(m, h.admissionConfig)
3046+
ok, warnings, webhookErr := h.webhookOperations(m, nil, h.admissionConfig)
30073047
if ok != tc.expectedOk {
30083048
t.Errorf("expected: %v, got: %v", tc.expectedOk, ok)
30093049
}
@@ -3159,7 +3199,7 @@ func TestDefaultAzureProviderSpec(t *testing.T) {
31593199
}
31603200
m.Spec.ProviderSpec.Value = &kruntime.RawExtension{Raw: rawBytes}
31613201

3162-
ok, warnings, webhookErr := h.webhookOperations(m, h.admissionConfig)
3202+
ok, warnings, webhookErr := h.webhookOperations(m, nil, h.admissionConfig)
31633203
if ok != tc.expectedOk {
31643204
t.Errorf("expected: %v, got: %v", tc.expectedOk, ok)
31653205
}
@@ -3699,7 +3739,7 @@ func TestValidateGCPProviderSpec(t *testing.T) {
36993739
m.Spec.ProviderSpec.Value = &kruntime.RawExtension{Raw: tc.overrideRawBytes}
37003740
}
37013741

3702-
ok, warnings, webhookErr := h.webhookOperations(m, h.admissionConfig)
3742+
ok, warnings, webhookErr := h.webhookOperations(m, nil, h.admissionConfig)
37033743
if ok != tc.expectedOk {
37043744
t.Errorf("expected: %v, got: %v", tc.expectedOk, ok)
37053745
}
@@ -3838,7 +3878,7 @@ func TestDefaultGCPProviderSpec(t *testing.T) {
38383878
}
38393879
m.Spec.ProviderSpec.Value = &kruntime.RawExtension{Raw: rawBytes}
38403880

3841-
ok, warnings, webhookErr := h.webhookOperations(m, h.admissionConfig)
3881+
ok, warnings, webhookErr := h.webhookOperations(m, nil, h.admissionConfig)
38423882
if ok != tc.expectedOk {
38433883
t.Errorf("expected: %v, got: %v", tc.expectedOk, ok)
38443884
}
@@ -4136,7 +4176,7 @@ func TestValidateVSphereProviderSpec(t *testing.T) {
41364176
}
41374177
m.Spec.ProviderSpec.Value = &kruntime.RawExtension{Raw: rawBytes}
41384178

4139-
ok, warnings, webhookErr := h.webhookOperations(m, h.admissionConfig)
4179+
ok, warnings, webhookErr := h.webhookOperations(m, nil, h.admissionConfig)
41404180
if ok != tc.expectedOk {
41414181
t.Errorf("expected: %v, got: %v", tc.expectedOk, ok)
41424182
}
@@ -4201,7 +4241,7 @@ func TestDefaultVSphereProviderSpec(t *testing.T) {
42014241
}
42024242
m.Spec.ProviderSpec.Value = &kruntime.RawExtension{Raw: rawBytes}
42034243

4204-
ok, warnings, webhookErr := h.webhookOperations(m, h.admissionConfig)
4244+
ok, warnings, webhookErr := h.webhookOperations(m, nil, h.admissionConfig)
42054245
if ok != tc.expectedOk {
42064246
t.Errorf("expected: %v, got: %v", tc.expectedOk, ok)
42074247
}
@@ -4274,7 +4314,7 @@ func TestDefaultNutanixProviderSpec(t *testing.T) {
42744314
}
42754315
m.Spec.ProviderSpec.Value = &kruntime.RawExtension{Raw: rawBytes}
42764316

4277-
ok, warnings, webhookErr := h.webhookOperations(m, h.admissionConfig)
4317+
ok, warnings, webhookErr := h.webhookOperations(m, nil, h.admissionConfig)
42784318
if ok != tc.expectedOk {
42794319
t.Errorf("expected: %v, got: %v", tc.expectedOk, ok)
42804320
}
@@ -4795,7 +4835,7 @@ func TestValidatePowerVSProviderSpec(t *testing.T) {
47954835
}
47964836
m.Spec.ProviderSpec.Value = &kruntime.RawExtension{Raw: rawBytes}
47974837

4798-
ok, warnings, webhookErr := h.webhookOperations(m, h.admissionConfig)
4838+
ok, warnings, webhookErr := h.webhookOperations(m, nil, h.admissionConfig)
47994839
if ok != tc.expectedOk {
48004840
t.Errorf("expected: %v, got: %v", tc.expectedOk, ok)
48014841
}
@@ -4877,7 +4917,7 @@ func TestDefaultPowerVSProviderSpec(t *testing.T) {
48774917
}
48784918
m.Spec.ProviderSpec.Value = &kruntime.RawExtension{Raw: rawBytes}
48794919

4880-
ok, warnings, webhookErr := h.webhookOperations(m, h.admissionConfig)
4920+
ok, warnings, webhookErr := h.webhookOperations(m, nil, h.admissionConfig)
48814921
if ok != tc.expectedOk {
48824922
t.Errorf("expected: %v, got: %v", tc.expectedOk, ok)
48834923
}
@@ -5061,7 +5101,7 @@ func TestValidateNutanixProviderSpec(t *testing.T) {
50615101
}
50625102
m.Spec.ProviderSpec.Value = &kruntime.RawExtension{Raw: rawBytes}
50635103

5064-
ok, warnings, webhookErr := h.webhookOperations(m, h.admissionConfig)
5104+
ok, warnings, webhookErr := h.webhookOperations(m, nil, h.admissionConfig)
50655105
if ok != tc.expectedOk {
50665106
t.Errorf("expected: %v, got: %v", tc.expectedOk, ok)
50675107
}
@@ -5123,3 +5163,36 @@ func TestValidateAzureCapacityReservationGroupID(t *testing.T) {
51235163
})
51245164
}
51255165
}
5166+
5167+
func TestValidateImmutabilityForCapacityReservationGroupID(t *testing.T) {
5168+
testCases := []struct {
5169+
name string
5170+
oldID string
5171+
newID string
5172+
expect bool
5173+
}{
5174+
{
5175+
name: "validation for immutability of capacityReservationGroupID should return true if oldID and new ID are same",
5176+
oldID: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myResourceGroupName/providers/Microsoft.Compute/capacityReservationGroups/myCapacityReservationGroup",
5177+
newID: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myResourceGroupName/providers/Microsoft.Compute/capacityReservationGroups/myCapacityReservationGroup",
5178+
expect: true,
5179+
},
5180+
{
5181+
name: "validation for immutability of capacityReservationGroupID should return false if oldID and new ID are not same",
5182+
oldID: "subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myResourceGroupName/providers/Microsoft.Compute/capacityReservationGroups/myCapacityReservationGroup",
5183+
newID: "subscriptions/00000000-0000-0000-0000-000000000001/resourceGroups/myResourceGroupName/providers/Microsoft.Compute/capacityReservationGroups/myCapacityReservationGroup",
5184+
expect: false,
5185+
},
5186+
}
5187+
5188+
for _, tc := range testCases {
5189+
t.Run(tc.name, func(t *testing.T) {
5190+
_ = NewWithT(t)
5191+
isValid := validateAzureImmutabilityForCapacityReservationGroupID(tc.oldID, tc.newID)
5192+
if isValid != tc.expect {
5193+
t.Errorf("expected: %v, got: %v", tc.expect, isValid)
5194+
}
5195+
5196+
})
5197+
}
5198+
}

0 commit comments

Comments
 (0)