Skip to content

Commit

Permalink
Merge pull request #327 from openshift-splat-team/debug-validation-af…
Browse files Browse the repository at this point in the history 
…ter-upgrade

OCPBUGS-42660: relax validation on delete and if failureDomains not configured
  • Loading branch information
@openshift-merge-bot
openshift-merge-bot[bot] authored Oct 31, 2024
2 parents 8d85b2c + da8ff67 commit d17bf3a
Show file tree
Hide file tree
Showing 3 changed files with 131 additions and 48 deletions.
51 changes: 51 additions & 0 deletions pkg/webhooks/controlplanemachineset/test_util.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
Copyright 2023 Red Hat, Inc.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package controlplanemachineset

import (
"context"

. "github.com/onsi/gomega"

configv1 "github.com/openshift/api/config/v1"
machinev1 "github.com/openshift/api/machine/v1"
machinev1resourcebuilder "github.com/openshift/cluster-api-actuator-pkg/testutils/resourcebuilder/machine/v1"
machinev1beta1resourcebuilder "github.com/openshift/cluster-api-actuator-pkg/testutils/resourcebuilder/machine/v1beta1"
"sigs.k8s.io/controller-runtime/pkg/client"
)

// createVSphereControlPlaneMachinesWithZones creates control plane machines which are in corresponding failure domains and returns those failure domains.
func createVSphereControlPlaneMachinesWithZones(
ctx context.Context,
k8sClient client.Client,
namespaceName string,
infrastructure *configv1.Infrastructure) *machinev1.FailureDomains {
fdBuilder := machinev1resourcebuilder.VSphereFailureDomains()
failureDomains := fdBuilder.BuildFailureDomains()

for _, fd := range failureDomains.VSphere {
machineProviderSpec := machinev1beta1resourcebuilder.VSphereProviderSpec().WithInfrastructure(*infrastructure).WithZone(fd.Name)

machineBuilder := machinev1beta1resourcebuilder.Machine().WithNamespace(namespaceName)
controlPlaneMachineBuilder := machineBuilder.WithGenerateName("control-plane-machine-").AsMaster().WithProviderSpecBuilder(machineProviderSpec)

controlPlaneMachine := controlPlaneMachineBuilder.Build()
Eventually(k8sClient.Create(ctx, controlPlaneMachine)).Should(Succeed(), "expected to be able to create the control plane machine")
}

return &failureDomains
}
42 changes: 38 additions & 4 deletions pkg/webhooks/controlplanemachineset/webhooks.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,20 @@ func (r *ControlPlaneMachineSetWebhook) ValidateCreate(ctx context.Context, obj
return warnings, nil
}

// shouldValidateSpecUpdate determines if the spec should be validated. For vSphere, in particular, cpms's created before 4.16
// may have fields that no longer pass validatation due to using the infrastructure resource as a configuration source. as such,
// it is possible to have the cpms be configured such that it can't be deleted as validation blocks removal of finalizers.
func shouldValidateSpecUpdate(oldCpms, newCpms *machinev1.ControlPlaneMachineSet) bool {
// If cpms is being deleted and finalizers are being dropped, do not validate further.
if newCpms.DeletionTimestamp != nil {
if len(oldCpms.Finalizers) > len(newCpms.Finalizers) && len(newCpms.Finalizers) == 0 {
return false
}
}

return true
}

// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
func (r *ControlPlaneMachineSetWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
var errs []error
Expand All @@ -152,11 +166,20 @@ func (r *ControlPlaneMachineSetWebhook) ValidateUpdate(ctx context.Context, oldO
return warnings, errUpdateNilCPMS
}

oldCpms, ok := oldObj.(*machinev1.ControlPlaneMachineSet)
if !ok {
return warnings, errObjNotCPMS
}

cpms, ok := newObj.(*machinev1.ControlPlaneMachineSet)
if !ok {
return warnings, errObjNotCPMS
}

if !shouldValidateSpecUpdate(oldCpms, cpms) {
return warnings, nil
}

infrastructure, err := util.GetInfrastructure(ctx, r.client)
if err != nil {
return warnings, fmt.Errorf("error getting infrastructure resource: %w", err)
Expand Down Expand Up @@ -386,7 +409,7 @@ func validateOpenShiftProviderConfig(logger logr.Logger, parentPath *field.Path,
case configv1.OpenStackPlatformType:
return nil, validateOpenShiftOpenStackProviderConfig(providerSpecPath.Child("value"), providerConfig.OpenStack())
case configv1.VSpherePlatformType:
return validateOpenShiftVSphereProviderConfig(providerSpecPath.Child("value"), providerConfig.VSphere(), infrastructure)
return validateOpenShiftVSphereProviderConfig(providerSpecPath.Child("value"), template, providerConfig.VSphere(), infrastructure)
case configv1.NutanixPlatformType:
return nil, validateOpenShiftNutanixProviderConfig(providerSpecPath.Child("value"), providerConfig.Nutanix(), template.FailureDomains)
}
Expand Down Expand Up @@ -458,14 +481,14 @@ func validateOpenShiftOpenStackProviderConfig(parentPath *field.Path, providerCo

// validateOpenShiftVSphereProviderConfig runs VSphere specific checks on the provider config on the ControlPlaneMachineSet.
// This ensures that the ControlPlaneMachineSet can safely replace VSphere control plane machines.
func validateOpenShiftVSphereProviderConfig(parentPath *field.Path, providerConfig providerconfig.VSphereProviderConfig, infrastructure *configv1.Infrastructure) (admission.Warnings, []error) {
func validateOpenShiftVSphereProviderConfig(parentPath *field.Path, template machinev1.OpenShiftMachineV1Beta1MachineTemplate, providerConfig providerconfig.VSphereProviderConfig, infrastructure *configv1.Infrastructure) (admission.Warnings, []error) {
warnings := admission.Warnings{}
errs := []error{}

infraProviderConfig := infrastructure.Spec.PlatformSpec.VSphere
if infraProviderConfig != nil && len(infraProviderConfig.FailureDomains) > 0 {
// If failure domains are configured, we do not want to have user provide any FD overrides
warn, err := checkForFailureDomainIgnoredFields(parentPath, providerConfig)
warn, err := checkForFailureDomainIgnoredFields(parentPath, template, providerConfig)
warnings = append(warnings, warn...)
errs = append(errs, err...)
} else {
Expand All @@ -477,7 +500,9 @@ func validateOpenShiftVSphereProviderConfig(parentPath *field.Path, providerConf

// checkForFailureDomainIgnoredFields checks all fields that are controlled by failure domain. Each infraction will result
// in an error being returned in the error collection.
func checkForFailureDomainIgnoredFields(parentPath *field.Path, providerConfig providerconfig.VSphereProviderConfig) (admission.Warnings, []error) {
//
//nolint:cyclop
func checkForFailureDomainIgnoredFields(parentPath *field.Path, template machinev1.OpenShiftMachineV1Beta1MachineTemplate, providerConfig providerconfig.VSphereProviderConfig) (admission.Warnings, []error) {
warnings := admission.Warnings{}
errs := []error{}

Expand All @@ -491,6 +516,15 @@ func checkForFailureDomainIgnoredFields(parentPath *field.Path, providerConfig p
errs = append(errs, validateTemplate(parentPath, providerConfig.Config().Template)...)
}

// If a cluster born before OCP 4.16 has a configured cpms that cpms may(and likely will) contain fields that
// conflict with the model of using the infrastructure resource as the configuration source.
// In those situations, it is necessary to only validate for field conflicts if failure
// domains are defined in the cpms. otherwise, the cpms could be unmodifiable/undeletable due to it
// failing validation.
if template.FailureDomains == nil || len(template.FailureDomains.VSphere) == 0 {
return warnings, errs
}

// Static IP configuration is the only thing we need to allow. Networks are required to be defined in the FD.
devices := providerConfig.Config().Network.Devices
for _, device := range devices {
Expand Down
86 changes: 42 additions & 44 deletions pkg/webhooks/controlplanemachineset/webhooks_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,7 @@ var _ = Describe("Webhooks", Ordered, func() {
Expect(vSphereTemplateWarningCounter.Value()).To(Equal(1), "Setting template should give a warning")
})

It("when providing network name in vSphere configuration", func() {
It("when providing network name in vSphere configuration without failure domains", func() {

By("Creating a selection of Machines")

Expand Down Expand Up @@ -468,6 +468,35 @@ var _ = Describe("Webhooks", Ordered, func() {
}
cpms.Spec.Template.OpenShiftMachineV1Beta1Machine.Spec.ProviderSpec.Value = raw

Expect(k8sClient.Create(ctx, cpms)).To(Succeed(), "expected to be able to create the control plane machine set")
})

It("when providing network name in vSphere configuration with failure domains", func() {

By("Creating a selection of Machines")
failureDomains := createVSphereControlPlaneMachinesWithZones(ctx, k8sClient, namespaceName, infrastructure)

cpmsBuilder := machinev1resourcebuilder.ControlPlaneMachineSet().WithNamespace(namespaceName).WithMachineTemplateBuilder(machineTemplate)
cpms := cpmsBuilder.Build()

cpms.Spec.Template.OpenShiftMachineV1Beta1Machine.FailureDomains = failureDomains

// Add fields not needed for CPMS when failure domain in use to create error
vsProviderSpec := &machinev1beta1.VSphereMachineProviderSpec{}
Expect(json.Unmarshal(cpms.Spec.Template.OpenShiftMachineV1Beta1Machine.Spec.ProviderSpec.Value.Raw, vsProviderSpec)).
To(Succeed(), "expect to unmarshal current provider spec")
vsProviderSpec.Network.Devices = []machinev1beta1.NetworkDeviceSpec{
{
NetworkName: "test-network",
},
}
specData, err := json.Marshal(vsProviderSpec)
Expect(err).To(BeNil(), "expect to be able to marshal changes")
raw := &runtime.RawExtension{
Raw: specData,
}
cpms.Spec.Template.OpenShiftMachineV1Beta1Machine.Spec.ProviderSpec.Value = raw

Expect(k8sClient.Create(ctx, cpms)).To(MatchError(SatisfyAll(
Equal("admission webhook \"controlplanemachineset.machine.openshift.io\" denied the request: spec.template.machines_v1beta1_machine_openshift_io.spec.providerSpec.value.network: Internal error: network devices should not be set when control plane nodes are in a failure domain: []v1beta1.NetworkDeviceSpec{v1beta1.NetworkDeviceSpec{NetworkName:\"test-network\", Gateway:\"\", IPAddrs:[]string(nil), Nameservers:[]string(nil), AddressesFromPools:[]v1beta1.AddressesFromPool(nil)}}"),
)), "expected to fail with only error about network")
Expand Down Expand Up @@ -520,20 +549,13 @@ var _ = Describe("Webhooks", Ordered, func() {

By("Creating a selection of Machines")

providerSpec := machinev1beta1resourcebuilder.VSphereProviderSpec().AsControlPlaneMachineSetProviderSpec()
machineTemplate = machinev1resourcebuilder.OpenShiftMachineV1Beta1Template().WithProviderSpecBuilder(providerSpec)

machineBuilder := machinev1beta1resourcebuilder.Machine().WithNamespace(namespaceName)
controlPlaneMachineBuilder := machineBuilder.WithGenerateName("control-plane-machine-").AsMaster().WithProviderSpecBuilder(providerSpec)

for i := 0; i < 3; i++ {
controlPlaneMachine := controlPlaneMachineBuilder.Build()
Expect(k8sClient.Create(ctx, controlPlaneMachine)).To(Succeed(), "expected to be able to create the control plane machine set")
}
failureDomains := createVSphereControlPlaneMachinesWithZones(ctx, k8sClient, namespaceName, infrastructure)

cpmsBuilder := machinev1resourcebuilder.ControlPlaneMachineSet().WithNamespace(namespaceName).WithMachineTemplateBuilder(machineTemplate)
cpms := cpmsBuilder.Build()

cpms.Spec.Template.OpenShiftMachineV1Beta1Machine.FailureDomains = failureDomains

// Add fields not needed for CPMS when failure domain in use to create error
vsProviderSpec := &machinev1beta1.VSphereMachineProviderSpec{}
Expect(json.Unmarshal(cpms.Spec.Template.OpenShiftMachineV1Beta1Machine.Spec.ProviderSpec.Value.Raw, vsProviderSpec)).
Expand All @@ -557,20 +579,13 @@ var _ = Describe("Webhooks", Ordered, func() {

By("Creating a selection of Machines")

providerSpec := machinev1beta1resourcebuilder.VSphereProviderSpec().AsControlPlaneMachineSetProviderSpec()
machineTemplate = machinev1resourcebuilder.OpenShiftMachineV1Beta1Template().WithProviderSpecBuilder(providerSpec)

machineBuilder := machinev1beta1resourcebuilder.Machine().WithNamespace(namespaceName)
controlPlaneMachineBuilder := machineBuilder.WithGenerateName("control-plane-machine-").AsMaster().WithProviderSpecBuilder(providerSpec)

for i := 0; i < 3; i++ {
controlPlaneMachine := controlPlaneMachineBuilder.Build()
Expect(k8sClient.Create(ctx, controlPlaneMachine)).To(Succeed(), "expected to be able to create the control plane machine set")
}
failureDomains := createVSphereControlPlaneMachinesWithZones(ctx, k8sClient, namespaceName, infrastructure)

cpmsBuilder := machinev1resourcebuilder.ControlPlaneMachineSet().WithNamespace(namespaceName).WithMachineTemplateBuilder(machineTemplate)
cpms := cpmsBuilder.Build()

cpms.Spec.Template.OpenShiftMachineV1Beta1Machine.FailureDomains = failureDomains

// Add fields not needed for CPMS when failure domain in use to create error
vsProviderSpec := &machinev1beta1.VSphereMachineProviderSpec{}
Expect(json.Unmarshal(cpms.Spec.Template.OpenShiftMachineV1Beta1Machine.Spec.ProviderSpec.Value.Raw, vsProviderSpec)).
Expand All @@ -594,20 +609,13 @@ var _ = Describe("Webhooks", Ordered, func() {

By("Creating a selection of Machines")

providerSpec := machinev1beta1resourcebuilder.VSphereProviderSpec().AsControlPlaneMachineSetProviderSpec()
machineTemplate = machinev1resourcebuilder.OpenShiftMachineV1Beta1Template().WithProviderSpecBuilder(providerSpec)

machineBuilder := machinev1beta1resourcebuilder.Machine().WithNamespace(namespaceName)
controlPlaneMachineBuilder := machineBuilder.WithGenerateName("control-plane-machine-").AsMaster().WithProviderSpecBuilder(providerSpec)

for i := 0; i < 3; i++ {
controlPlaneMachine := controlPlaneMachineBuilder.Build()
Expect(k8sClient.Create(ctx, controlPlaneMachine)).To(Succeed(), "expected to be able to create the control plane machine set")
}
failureDomains := createVSphereControlPlaneMachinesWithZones(ctx, k8sClient, namespaceName, infrastructure)

cpmsBuilder := machinev1resourcebuilder.ControlPlaneMachineSet().WithNamespace(namespaceName).WithMachineTemplateBuilder(machineTemplate)
cpms := cpmsBuilder.Build()

cpms.Spec.Template.OpenShiftMachineV1Beta1Machine.FailureDomains = failureDomains

// Add fields not needed for CPMS when failure domain in use to create error
vsProviderSpec := &machinev1beta1.VSphereMachineProviderSpec{}
Expect(json.Unmarshal(cpms.Spec.Template.OpenShiftMachineV1Beta1Machine.Spec.ProviderSpec.Value.Raw, vsProviderSpec)).
Expand All @@ -629,23 +637,13 @@ var _ = Describe("Webhooks", Ordered, func() {

// "providing" covers a cpms created with a list of failure domains
It("when providing failure domains in vSphere configuration", func() {
providerSpec := machinev1beta1resourcebuilder.VSphereProviderSpec().WithInfrastructure(*infrastructure).AsControlPlaneMachineSetProviderSpec().WithTemplate("/IBMCloud/vm/rhcos-415.92.202310310037-0-vmware.x86_64.ova-hw19")
machineTemplate = machinev1resourcebuilder.OpenShiftMachineV1Beta1Template().WithProviderSpecBuilder(providerSpec).WithFailureDomainsBuilder(machinev1resourcebuilder.VSphereFailureDomains())

zones := []string{"us-central1-a", "us-central1-b", "us-central1-c"}
for _, zone := range zones {
machineProviderSpec := machinev1beta1resourcebuilder.VSphereProviderSpec().WithInfrastructure(*infrastructure).WithZone(zone)

machineBuilder := machinev1beta1resourcebuilder.Machine().WithNamespace(namespaceName)
controlPlaneMachineBuilder := machineBuilder.WithGenerateName("control-plane-machine-").AsMaster().WithProviderSpecBuilder(machineProviderSpec)

controlPlaneMachine := controlPlaneMachineBuilder.Build()
Expect(k8sClient.Create(ctx, controlPlaneMachine)).To(Succeed(), "expected to be able to create the control plane machine set")
}
failureDomains := createVSphereControlPlaneMachinesWithZones(ctx, k8sClient, namespaceName, infrastructure)

cpmsBuilder := machinev1resourcebuilder.ControlPlaneMachineSet().WithNamespace(namespaceName).WithMachineTemplateBuilder(machineTemplate)
cpms := cpmsBuilder.Build()

cpms.Spec.Template.OpenShiftMachineV1Beta1Machine.FailureDomains = failureDomains

Expect(k8sClient.Create(ctx, cpms)).To(Succeed(), "expected to be able to create the control plane machine set")
})

Expand Down

0 comments on commit d17bf3a

Please sign in to comment.