diff --git a/Makefile b/Makefile index 525b1c894b..db0265f409 100644 --- a/Makefile +++ b/Makefile @@ -306,7 +306,7 @@ generate-api-docs-%: $(GEN_CRD_API_REFERENCE_DOCS) FORCE .PHONY: docker-build docker-build: ## Build the docker image for controller-manager - docker build -f Dockerfile --build-arg goproxy=$(GOPROXY) --build-arg ARCH=$(ARCH) --build-arg LDFLAGS="$(LDFLAGS)" . -t $(CONTROLLER_IMG_TAG) + docker build -f Dockerfile --build-arg goproxy=$(GOPROXY) --build-arg ARCH=$(ARCH) --build-arg ldflags="$(LDFLAGS)" . -t $(CONTROLLER_IMG_TAG) .PHONY: docker-push docker-push: ## Push the docker image diff --git a/api/v1alpha5/zz_generated.conversion.go b/api/v1alpha5/zz_generated.conversion.go index a77967fb5a..fa585fef41 100644 --- a/api/v1alpha5/zz_generated.conversion.go +++ b/api/v1alpha5/zz_generated.conversion.go @@ -489,7 +489,7 @@ func Convert_v1beta1_AddressPair_To_v1alpha5_AddressPair(in *v1beta1.AddressPair } func autoConvert_v1alpha5_Bastion_To_v1beta1_Bastion(in *Bastion, out *v1beta1.Bastion, s conversion.Scope) error { - if err := optional.Convert_bool_To_optional_Bool(&in.Enabled, &out.Enabled, s); err != nil { + if err := v1.Convert_bool_To_Pointer_bool(&in.Enabled, &out.Enabled, s); err != nil { return err } // WARNING: in.Instance requires manual conversion: does not exist in peer-type @@ -500,7 +500,7 @@ func autoConvert_v1alpha5_Bastion_To_v1beta1_Bastion(in *Bastion, out *v1beta1.B } func autoConvert_v1beta1_Bastion_To_v1alpha5_Bastion(in *v1beta1.Bastion, out *Bastion, s conversion.Scope) error { - if err := optional.Convert_optional_Bool_To_bool(&in.Enabled, &out.Enabled, s); err != nil { + if err := v1.Convert_Pointer_bool_To_bool(&in.Enabled, &out.Enabled, s); err != nil { return err } // WARNING: in.Spec requires manual conversion: does not exist in peer-type diff --git a/api/v1alpha6/openstackcluster_conversion.go b/api/v1alpha6/openstackcluster_conversion.go index 665977d7e1..3bbb3f97d8 100644 --- a/api/v1alpha6/openstackcluster_conversion.go +++ b/api/v1alpha6/openstackcluster_conversion.go @@ -478,7 +478,10 @@ func restorev1beta1Bastion(previous **infrav1.Bastion, dst **infrav1.Bastion) { optional.RestoreString(&(*previous).FloatingIP, &(*dst).FloatingIP) optional.RestoreString(&(*previous).AvailabilityZone, &(*dst).AvailabilityZone) - optional.RestoreBool(&(*previous).Enabled, &(*dst).Enabled) + + if (*dst).Enabled != nil && !*(*dst).Enabled { + (*dst).Enabled = (*previous).Enabled + } } func Convert_v1alpha6_Bastion_To_v1beta1_Bastion(in *Bastion, out *infrav1.Bastion, s apiconversion.Scope) error { diff --git a/api/v1alpha6/zz_generated.conversion.go b/api/v1alpha6/zz_generated.conversion.go index 59dba6198a..829e7d66a7 100644 --- a/api/v1alpha6/zz_generated.conversion.go +++ b/api/v1alpha6/zz_generated.conversion.go @@ -503,7 +503,7 @@ func Convert_v1beta1_AddressPair_To_v1alpha6_AddressPair(in *v1beta1.AddressPair } func autoConvert_v1alpha6_Bastion_To_v1beta1_Bastion(in *Bastion, out *v1beta1.Bastion, s conversion.Scope) error { - if err := optional.Convert_bool_To_optional_Bool(&in.Enabled, &out.Enabled, s); err != nil { + if err := v1.Convert_bool_To_Pointer_bool(&in.Enabled, &out.Enabled, s); err != nil { return err } // WARNING: in.Instance requires manual conversion: does not exist in peer-type @@ -514,7 +514,7 @@ func autoConvert_v1alpha6_Bastion_To_v1beta1_Bastion(in *Bastion, out *v1beta1.B } func autoConvert_v1beta1_Bastion_To_v1alpha6_Bastion(in *v1beta1.Bastion, out *Bastion, s conversion.Scope) error { - if err := optional.Convert_optional_Bool_To_bool(&in.Enabled, &out.Enabled, s); err != nil { + if err := v1.Convert_Pointer_bool_To_bool(&in.Enabled, &out.Enabled, s); err != nil { return err } // WARNING: in.Spec requires manual conversion: does not exist in peer-type diff --git a/api/v1alpha7/openstackcluster_conversion.go b/api/v1alpha7/openstackcluster_conversion.go index 96e03b5c87..6b4a82a9bf 100644 --- a/api/v1alpha7/openstackcluster_conversion.go +++ b/api/v1alpha7/openstackcluster_conversion.go @@ -416,7 +416,10 @@ func restorev1beta1Bastion(previous **infrav1.Bastion, dst **infrav1.Bastion) { restorev1beta1MachineSpec((*previous).Spec, (*dst).Spec) optional.RestoreString(&(*previous).FloatingIP, &(*dst).FloatingIP) optional.RestoreString(&(*previous).AvailabilityZone, &(*dst).AvailabilityZone) - optional.RestoreBool(&(*previous).Enabled, &(*dst).Enabled) + + if (*dst).Enabled != nil && !*(*dst).Enabled { + (*dst).Enabled = (*previous).Enabled + } } func Convert_v1alpha7_Bastion_To_v1beta1_Bastion(in *Bastion, out *infrav1.Bastion, s apiconversion.Scope) error { diff --git a/api/v1alpha7/zz_generated.conversion.go b/api/v1alpha7/zz_generated.conversion.go index f67c228983..a4144215b5 100644 --- a/api/v1alpha7/zz_generated.conversion.go +++ b/api/v1alpha7/zz_generated.conversion.go @@ -567,7 +567,7 @@ func Convert_v1beta1_AddressPair_To_v1alpha7_AddressPair(in *v1beta1.AddressPair } func autoConvert_v1alpha7_Bastion_To_v1beta1_Bastion(in *Bastion, out *v1beta1.Bastion, s conversion.Scope) error { - if err := optional.Convert_bool_To_optional_Bool(&in.Enabled, &out.Enabled, s); err != nil { + if err := v1.Convert_bool_To_Pointer_bool(&in.Enabled, &out.Enabled, s); err != nil { return err } // WARNING: in.Instance requires manual conversion: does not exist in peer-type @@ -578,7 +578,7 @@ func autoConvert_v1alpha7_Bastion_To_v1beta1_Bastion(in *Bastion, out *v1beta1.B } func autoConvert_v1beta1_Bastion_To_v1alpha7_Bastion(in *v1beta1.Bastion, out *Bastion, s conversion.Scope) error { - if err := optional.Convert_optional_Bool_To_bool(&in.Enabled, &out.Enabled, s); err != nil { + if err := v1.Convert_Pointer_bool_To_bool(&in.Enabled, &out.Enabled, s); err != nil { return err } // WARNING: in.Spec requires manual conversion: does not exist in peer-type diff --git a/api/v1beta1/types.go b/api/v1beta1/types.go index 7c13a3662d..5bbccb0504 100644 --- a/api/v1beta1/types.go +++ b/api/v1beta1/types.go @@ -795,7 +795,7 @@ type Bastion struct { // waiting until the bastion has been deleted. // +kubebuilder:default:=true // +optional - Enabled optional.Bool `json:"enabled,omitempty"` + Enabled *bool `json:"enabled,omitempty"` // Spec for the bastion itself Spec *OpenStackMachineSpec `json:"spec,omitempty"` diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml index 5ecf04a72d..6d54578783 100644 --- a/config/manager/manager.yaml +++ b/config/manager/manager.yaml @@ -46,6 +46,7 @@ spec: privileged: false runAsUser: 65532 runAsGroup: 65532 + terminationMessagePolicy: FallbackToLogsOnError terminationGracePeriodSeconds: 10 securityContext: runAsNonRoot: true diff --git a/main.go b/main.go index 4cb02726bc..2c1975aa8e 100644 --- a/main.go +++ b/main.go @@ -233,6 +233,7 @@ func main() { cfg, err := config.GetConfigWithContext(os.Getenv("KUBECONTEXT")) if err != nil { setupLog.Error(err, "unable to get kubeconfig") + os.Exit(1) } cfg.QPS = restConfigQPS cfg.Burst = restConfigBurst diff --git a/pkg/cloud/services/networking/port.go b/pkg/cloud/services/networking/port.go index 9acfa189f9..33fd22294a 100644 --- a/pkg/cloud/services/networking/port.go +++ b/pkg/cloud/services/networking/port.go @@ -192,7 +192,7 @@ func (s *Service) CreatePort(eventObject runtime.Object, portSpec *infrav1.Resol port, err := s.client.CreatePort(builder) if err != nil { - record.Warnf(eventObject, "FailedCreatePort", "Failed to create port %s: %v", port.Name, err) + record.Warnf(eventObject, "FailedCreatePort", "Failed to create port %s: %v", portSpec.Name, err) return nil, err } diff --git a/pkg/webhooks/fuzz_test.go b/pkg/webhooks/fuzz_test.go new file mode 100644 index 0000000000..c4e246a78e --- /dev/null +++ b/pkg/webhooks/fuzz_test.go @@ -0,0 +1,92 @@ +/* +Copyright 2024 The Kubernetes Authors. + +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 webhooks + +import ( + "context" + "runtime/debug" + "testing" + + "github.com/onsi/gomega/format" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes/scheme" + utilconversion "sigs.k8s.io/cluster-api/util/conversion" + "sigs.k8s.io/controller-runtime/pkg/webhook" + + infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1" +) + +type pointerToObject[T any] interface { + *T + runtime.Object +} + +// fuzzCustomValidator fuzzes a CustomValidator with objects of the validator's expected type. +func fuzzCustomValidator[O any, PO pointerToObject[O]](t *testing.T, name string, validator webhook.CustomValidator) { + t.Helper() + fuzz := utilconversion.GetFuzzer(scheme.Scheme) + ctx := context.TODO() + + t.Run(name, func(t *testing.T) { + for i := 0; i < 1000; i++ { + var previous PO = new(O) + var dst PO = new(O) + fuzz.Fuzz(previous) + fuzz.Fuzz(dst) + + checkPanic := func(f func(), name string, args ...runtime.Object) { + defer func() { + if r := recover(); r != nil { + t.Errorf("PANIC in %s", name) + for i, arg := range args { + t.Errorf("arg %d:\n%s", i, format.Object(arg, 1)) + } + t.Errorf("Stack trace:\n%s", debug.Stack()) + t.FailNow() + } + }() + f() + } + + checkPanic(func() { + _, _ = validator.ValidateCreate(ctx, dst) + }, "ValidateCreate()", dst) + checkPanic(func() { + _, _ = validator.ValidateUpdate(ctx, previous, dst) + }, "ValidateUpdate()", previous, dst) + checkPanic(func() { + _, _ = validator.ValidateDelete(ctx, previous) + }, "ValidateDelete()", previous) + } + }) +} + +func Test_FuzzClusterWebhook(t *testing.T) { + fuzzCustomValidator[infrav1.OpenStackCluster](t, "OpenStackCluster", &openStackClusterWebhook{}) +} + +func Test_FuzzClusterTemplateWebhook(t *testing.T) { + fuzzCustomValidator[infrav1.OpenStackClusterTemplate](t, "OpenStackClusterTemplate", &openStackClusterTemplateWebhook{}) +} + +func Test_FuzzMachineWebhook(t *testing.T) { + fuzzCustomValidator[infrav1.OpenStackMachine](t, "OpenStackMachine", &openStackMachineWebhook{}) +} + +func Test_FuzzMachineTemplateWebhook(t *testing.T) { + fuzzCustomValidator[infrav1.OpenStackMachineTemplate](t, "OpenStackMachineTemplate", &openStackMachineTemplateWebhook{}) +} diff --git a/pkg/webhooks/openstackcluster_webhook.go b/pkg/webhooks/openstackcluster_webhook.go index 194933c3d6..910f761ebb 100644 --- a/pkg/webhooks/openstackcluster_webhook.go +++ b/pkg/webhooks/openstackcluster_webhook.go @@ -121,6 +121,10 @@ func (*openStackClusterWebhook) ValidateUpdate(_ context.Context, oldObjRaw, new // Allow changes to the managed allNodesSecurityGroupRules. if newObj.Spec.ManagedSecurityGroups != nil { + if oldObj.Spec.ManagedSecurityGroups == nil { + oldObj.Spec.ManagedSecurityGroups = &infrav1.ManagedSecurityGroups{} + } + oldObj.Spec.ManagedSecurityGroups.AllNodesSecurityGroupRules = []infrav1.SecurityGroupRuleSpec{} newObj.Spec.ManagedSecurityGroups.AllNodesSecurityGroupRules = []infrav1.SecurityGroupRuleSpec{} diff --git a/test/e2e/suites/apivalidations/openstackcluster_test.go b/test/e2e/suites/apivalidations/openstackcluster_test.go index 9af5a730b1..80320e167c 100644 --- a/test/e2e/suites/apivalidations/openstackcluster_test.go +++ b/test/e2e/suites/apivalidations/openstackcluster_test.go @@ -215,6 +215,24 @@ var _ = Describe("OpenStackCluster API validations", func() { Expect(cluster.Spec.ControlPlaneEndpoint).To(Equal(*infrav1Cluster.Spec.ControlPlaneEndpoint), "Control plane endpoint should be restored") Expect(cluster.Spec.IdentityRef.Kind).To(Equal("FakeKind"), "IdentityRef.Kind should be restored") }) + + It("should not enable an explicitly disabled bastion when converting to v1beta1", func() { + cluster.Spec.Bastion = &infrav1alpha7.Bastion{Enabled: false} + Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + + // Fetch the infrav1 version of the cluster + infrav1Cluster := &infrav1.OpenStackCluster{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace}, infrav1Cluster)).To(Succeed(), "OpenStackCluster fetch should succeed") + + infrav1Bastion := infrav1Cluster.Spec.Bastion + + // NOTE(mdbooth): It may be reasonable to remove the + // bastion if it is disabled with no other properties. + // It would be reasonable to update the assertions + // accordingly if we did that. + Expect(infrav1Bastion).ToNot(BeNil(), "Bastion should not have been removed") + Expect(infrav1Bastion.Enabled).To(Equal(ptr.To(false)), "Bastion should remain disabled") + }) }) Context("v1alpha6", func() { @@ -252,5 +270,23 @@ var _ = Describe("OpenStackCluster API validations", func() { Expect(cluster.Spec.ControlPlaneEndpoint).To(Equal(*infrav1Cluster.Spec.ControlPlaneEndpoint), "Control plane endpoint should be restored") Expect(cluster.Spec.IdentityRef.Kind).To(Equal("FakeKind"), "IdentityRef.Kind should be restored") }) + + It("should not enable an explicitly disabled bastion when converting to v1beta1", func() { + cluster.Spec.Bastion = &infrav1alpha6.Bastion{Enabled: false} + Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + + // Fetch the infrav1 version of the cluster + infrav1Cluster := &infrav1.OpenStackCluster{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace}, infrav1Cluster)).To(Succeed(), "OpenStackCluster fetch should succeed") + + infrav1Bastion := infrav1Cluster.Spec.Bastion + + // NOTE(mdbooth): It may be reasonable to remove the + // bastion if it is disabled with no other properties. + // It would be reasonable to update the assertions + // accordingly if we did that. + Expect(infrav1Bastion).ToNot(BeNil(), "Bastion should not have been removed") + Expect(infrav1Bastion.Enabled).To(Equal(ptr.To(false)), "Bastion should remain disabled") + }) }) })