From 6f152bf2a3aef9e2a780d68b8a32b3ef76ab6e7b Mon Sep 17 00:00:00 2001 From: Konstantinos Angelopoulos Date: Tue, 22 Oct 2024 11:08:42 +0200 Subject: [PATCH] forbid disabling share networks --- pkg/apis/openstack/validation/infrastructure.go | 14 +++++++++----- .../openstack/validation/infrastructure_test.go | 14 ++++++++++++-- pkg/controller/infrastructure/infraflow/delete.go | 4 ++++ .../infrastructure/infraflow/reconcile.go | 4 ++-- 4 files changed, 27 insertions(+), 9 deletions(-) diff --git a/pkg/apis/openstack/validation/infrastructure.go b/pkg/apis/openstack/validation/infrastructure.go index e1baff9ea..e38a89828 100644 --- a/pkg/apis/openstack/validation/infrastructure.go +++ b/pkg/apis/openstack/validation/infrastructure.go @@ -74,11 +74,15 @@ func ValidateInfrastructureConfig(infra *api.InfrastructureConfig, nodesCIDR *st func ValidateInfrastructureConfigUpdate(oldConfig, newConfig *api.InfrastructureConfig, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - newNetworks := newConfig.Networks - oldNetworks := oldConfig.Networks - // share network changes are allowed, therefore ignore them on comparing - newNetworks.ShareNetwork = nil - oldNetworks.ShareNetwork = nil + newNetworks := newConfig.DeepCopy().Networks + oldNetworks := oldConfig.DeepCopy().Networks + + // only enablement of share network enablement is allowed as update operation. Therefore we ignore it, when checking for other updates. + // TODO: allow both enabling and disabling of share networks. + if oldNetworks.ShareNetwork == nil || !oldNetworks.ShareNetwork.Enabled { + newNetworks.ShareNetwork = nil + oldNetworks.ShareNetwork = nil + } allErrs = append(allErrs, apivalidation.ValidateImmutableField(newNetworks, oldNetworks, fldPath.Child("networks"))...) allErrs = append(allErrs, apivalidation.ValidateImmutableField(newConfig.FloatingPoolName, oldConfig.FloatingPoolName, fldPath.Child("floatingPoolName"))...) allErrs = append(allErrs, apivalidation.ValidateImmutableField(newConfig.FloatingPoolSubnetName, oldConfig.FloatingPoolSubnetName, fldPath.Child("floatingPoolSubnetName"))...) diff --git a/pkg/apis/openstack/validation/infrastructure_test.go b/pkg/apis/openstack/validation/infrastructure_test.go index ec40e75e5..1fb6167ed 100644 --- a/pkg/apis/openstack/validation/infrastructure_test.go +++ b/pkg/apis/openstack/validation/infrastructure_test.go @@ -171,14 +171,24 @@ var _ = Describe("InfrastructureConfig validation", func() { })))) }) - It("should allow changing the share network section", func() { + It("should allow enabling the share network section", func() { newInfrastructureConfig := infrastructureConfig.DeepCopy() newInfrastructureConfig.Networks.ShareNetwork = &api.ShareNetwork{Enabled: true} errorList := ValidateInfrastructureConfigUpdate(infrastructureConfig, newInfrastructureConfig, nilPath) - Expect(errorList).To(BeEmpty()) }) + It("should forbid disabling the share network section", func() { + infrastructureConfig.Networks.ShareNetwork = &api.ShareNetwork{Enabled: true} + newInfrastructureConfig := infrastructureConfig.DeepCopy() + newInfrastructureConfig.Networks.ShareNetwork = nil + + errorList := ValidateInfrastructureConfigUpdate(infrastructureConfig, newInfrastructureConfig, nilPath) + Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeInvalid), + "Field": Equal("networks"), + })))) + }) It("should forbid changing the floating pool", func() { newInfrastructureConfig := infrastructureConfig.DeepCopy() diff --git a/pkg/controller/infrastructure/infraflow/delete.go b/pkg/controller/infrastructure/infraflow/delete.go index c16f94333..008db08bd 100644 --- a/pkg/controller/infrastructure/infraflow/delete.go +++ b/pkg/controller/infrastructure/infraflow/delete.go @@ -235,6 +235,10 @@ func (fctx *FlowContext) deleteSSHKeyPair(ctx context.Context) error { } func (fctx *FlowContext) deleteShareNetwork(ctx context.Context) error { + if sn := fctx.config.Networks.ShareNetwork; sn == nil || !sn.Enabled { + return nil + } + log := shared.LogFromContext(ctx) networkID := ptr.Deref(fctx.state.Get(IdentifierNetwork), "") subnetID := ptr.Deref(fctx.state.Get(IdentifierSubnet), "") diff --git a/pkg/controller/infrastructure/infraflow/reconcile.go b/pkg/controller/infrastructure/infraflow/reconcile.go index bfeb65295..90daa43d1 100644 --- a/pkg/controller/infrastructure/infraflow/reconcile.go +++ b/pkg/controller/infrastructure/infraflow/reconcile.go @@ -467,8 +467,8 @@ func (fctx *FlowContext) ensureSSHKeyPair(ctx context.Context) error { } func (fctx *FlowContext) ensureShareNetwork(ctx context.Context) error { - if fctx.config.Networks.ShareNetwork == nil || !fctx.config.Networks.ShareNetwork.Enabled { - return fctx.deleteShareNetwork(ctx) + if sn := fctx.config.Networks.ShareNetwork; sn == nil || !sn.Enabled { + return nil } log := shared.LogFromContext(ctx)