diff --git a/api/v1beta1/machinehealthcheck_types.go b/api/v1beta1/machinehealthcheck_types.go index 5e0dffc71599..4f17b5f9cc92 100644 --- a/api/v1beta1/machinehealthcheck_types.go +++ b/api/v1beta1/machinehealthcheck_types.go @@ -17,11 +17,21 @@ limitations under the License. package v1beta1 import ( + "time" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" ) +var ( + // DefaultNodeStartupTimeout is the time allowed for a node to start up. + // Can be made longer as part of spec if required for particular provider. + // 10 minutes should allow the instance to start and the node to join the + // cluster on most providers. + DefaultNodeStartupTimeout = metav1.Duration{Duration: 10 * time.Minute} +) + // ANCHOR: MachineHealthCheckSpec // MachineHealthCheckSpec defines the desired state of MachineHealthCheck. diff --git a/api/v1beta1/zz_generated.openapi.go b/api/v1beta1/zz_generated.openapi.go index c46127a35fd7..8526a8cf22cf 100644 --- a/api/v1beta1/zz_generated.openapi.go +++ b/api/v1beta1/zz_generated.openapi.go @@ -97,7 +97,6 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA "sigs.k8s.io/cluster-api/api/v1beta1.VariableSchema": schema_sigsk8sio_cluster_api_api_v1beta1_VariableSchema(ref), "sigs.k8s.io/cluster-api/api/v1beta1.WorkersClass": schema_sigsk8sio_cluster_api_api_v1beta1_WorkersClass(ref), "sigs.k8s.io/cluster-api/api/v1beta1.WorkersTopology": schema_sigsk8sio_cluster_api_api_v1beta1_WorkersTopology(ref), - "sigs.k8s.io/cluster-api/api/v1beta1.machineDeploymentDefaulter": schema_sigsk8sio_cluster_api_api_v1beta1_machineDeploymentDefaulter(ref), } } @@ -3599,24 +3598,3 @@ func schema_sigsk8sio_cluster_api_api_v1beta1_WorkersTopology(ref common.Referen "sigs.k8s.io/cluster-api/api/v1beta1.MachineDeploymentTopology", "sigs.k8s.io/cluster-api/api/v1beta1.MachinePoolTopology"}, } } - -func schema_sigsk8sio_cluster_api_api_v1beta1_machineDeploymentDefaulter(ref common.ReferenceCallback) common.OpenAPIDefinition { - return common.OpenAPIDefinition{ - Schema: spec.Schema{ - SchemaProps: spec.SchemaProps{ - Description: "machineDeploymentDefaulter implements a defaulting webhook for MachineDeployment.", - Type: []string{"object"}, - Properties: map[string]spec.Schema{ - "decoder": { - SchemaProps: spec.SchemaProps{ - Ref: ref("sigs.k8s.io/controller-runtime/pkg/webhook/admission.Decoder"), - }, - }, - }, - Required: []string{"decoder"}, - }, - }, - Dependencies: []string{ - "sigs.k8s.io/controller-runtime/pkg/webhook/admission.Decoder"}, - } -} diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index 1054578ea4f7..e1e75129d95d 100644 --- a/config/webhook/manifests.yaml +++ b/config/webhook/manifests.yaml @@ -11,10 +11,10 @@ webhooks: service: name: webhook-service namespace: system - path: /mutate-cluster-x-k8s-io-v1beta1-machine + path: /mutate-cluster-x-k8s-io-v1beta1-cluster failurePolicy: Fail matchPolicy: Equivalent - name: default.machine.cluster.x-k8s.io + name: default.cluster.cluster.x-k8s.io rules: - apiGroups: - cluster.x-k8s.io @@ -24,7 +24,7 @@ webhooks: - CREATE - UPDATE resources: - - machines + - clusters sideEffects: None - admissionReviewVersions: - v1 @@ -33,10 +33,10 @@ webhooks: service: name: webhook-service namespace: system - path: /mutate-cluster-x-k8s-io-v1beta1-machinedeployment + path: /mutate-cluster-x-k8s-io-v1beta1-clusterclass failurePolicy: Fail matchPolicy: Equivalent - name: default.machinedeployment.cluster.x-k8s.io + name: default.clusterclass.cluster.x-k8s.io rules: - apiGroups: - cluster.x-k8s.io @@ -46,7 +46,7 @@ webhooks: - CREATE - UPDATE resources: - - machinedeployments + - clusterclasses sideEffects: None - admissionReviewVersions: - v1 @@ -55,10 +55,10 @@ webhooks: service: name: webhook-service namespace: system - path: /mutate-cluster-x-k8s-io-v1beta1-machinehealthcheck + path: /mutate-cluster-x-k8s-io-v1beta1-machine failurePolicy: Fail matchPolicy: Equivalent - name: default.machinehealthcheck.cluster.x-k8s.io + name: default.machine.cluster.x-k8s.io rules: - apiGroups: - cluster.x-k8s.io @@ -68,7 +68,7 @@ webhooks: - CREATE - UPDATE resources: - - machinehealthchecks + - machines sideEffects: None - admissionReviewVersions: - v1 @@ -77,10 +77,10 @@ webhooks: service: name: webhook-service namespace: system - path: /mutate-cluster-x-k8s-io-v1beta1-machineset + path: /mutate-cluster-x-k8s-io-v1beta1-machinedeployment failurePolicy: Fail matchPolicy: Equivalent - name: default.machineset.cluster.x-k8s.io + name: default.machinedeployment.cluster.x-k8s.io rules: - apiGroups: - cluster.x-k8s.io @@ -90,7 +90,7 @@ webhooks: - CREATE - UPDATE resources: - - machinesets + - machinedeployments sideEffects: None - admissionReviewVersions: - v1 @@ -99,10 +99,10 @@ webhooks: service: name: webhook-service namespace: system - path: /mutate-cluster-x-k8s-io-v1beta1-cluster + path: /mutate-cluster-x-k8s-io-v1beta1-machinehealthcheck failurePolicy: Fail matchPolicy: Equivalent - name: default.cluster.cluster.x-k8s.io + name: default.machinehealthcheck.cluster.x-k8s.io rules: - apiGroups: - cluster.x-k8s.io @@ -112,7 +112,7 @@ webhooks: - CREATE - UPDATE resources: - - clusters + - machinehealthchecks sideEffects: None - admissionReviewVersions: - v1 @@ -121,10 +121,10 @@ webhooks: service: name: webhook-service namespace: system - path: /mutate-cluster-x-k8s-io-v1beta1-clusterclass + path: /mutate-cluster-x-k8s-io-v1beta1-machineset failurePolicy: Fail matchPolicy: Equivalent - name: default.clusterclass.cluster.x-k8s.io + name: default.machineset.cluster.x-k8s.io rules: - apiGroups: - cluster.x-k8s.io @@ -134,7 +134,7 @@ webhooks: - CREATE - UPDATE resources: - - clusterclasses + - machinesets sideEffects: None - admissionReviewVersions: - v1 @@ -215,10 +215,10 @@ webhooks: service: name: webhook-service namespace: system - path: /validate-cluster-x-k8s-io-v1beta1-machine + path: /validate-cluster-x-k8s-io-v1beta1-cluster failurePolicy: Fail matchPolicy: Equivalent - name: validation.machine.cluster.x-k8s.io + name: validation.cluster.cluster.x-k8s.io rules: - apiGroups: - cluster.x-k8s.io @@ -227,8 +227,9 @@ webhooks: operations: - CREATE - UPDATE + - DELETE resources: - - machines + - clusters sideEffects: None - admissionReviewVersions: - v1 @@ -237,10 +238,10 @@ webhooks: service: name: webhook-service namespace: system - path: /validate-cluster-x-k8s-io-v1beta1-machinedeployment + path: /validate-cluster-x-k8s-io-v1beta1-clusterclass failurePolicy: Fail matchPolicy: Equivalent - name: validation.machinedeployment.cluster.x-k8s.io + name: validation.clusterclass.cluster.x-k8s.io rules: - apiGroups: - cluster.x-k8s.io @@ -249,8 +250,9 @@ webhooks: operations: - CREATE - UPDATE + - DELETE resources: - - machinedeployments + - clusterclasses sideEffects: None - admissionReviewVersions: - v1 @@ -259,10 +261,10 @@ webhooks: service: name: webhook-service namespace: system - path: /validate-cluster-x-k8s-io-v1beta1-machinehealthcheck + path: /validate-cluster-x-k8s-io-v1beta1-machine failurePolicy: Fail matchPolicy: Equivalent - name: validation.machinehealthcheck.cluster.x-k8s.io + name: validation.machine.cluster.x-k8s.io rules: - apiGroups: - cluster.x-k8s.io @@ -272,7 +274,7 @@ webhooks: - CREATE - UPDATE resources: - - machinehealthchecks + - machines sideEffects: None - admissionReviewVersions: - v1 @@ -281,10 +283,10 @@ webhooks: service: name: webhook-service namespace: system - path: /validate-cluster-x-k8s-io-v1beta1-machineset + path: /validate-cluster-x-k8s-io-v1beta1-machinedeployment failurePolicy: Fail matchPolicy: Equivalent - name: validation.machineset.cluster.x-k8s.io + name: validation.machinedeployment.cluster.x-k8s.io rules: - apiGroups: - cluster.x-k8s.io @@ -294,7 +296,7 @@ webhooks: - CREATE - UPDATE resources: - - machinesets + - machinedeployments sideEffects: None - admissionReviewVersions: - v1 @@ -303,10 +305,10 @@ webhooks: service: name: webhook-service namespace: system - path: /validate-cluster-x-k8s-io-v1beta1-cluster + path: /validate-cluster-x-k8s-io-v1beta1-machinehealthcheck failurePolicy: Fail matchPolicy: Equivalent - name: validation.cluster.cluster.x-k8s.io + name: validation.machinehealthcheck.cluster.x-k8s.io rules: - apiGroups: - cluster.x-k8s.io @@ -315,9 +317,8 @@ webhooks: operations: - CREATE - UPDATE - - DELETE resources: - - clusters + - machinehealthchecks sideEffects: None - admissionReviewVersions: - v1 @@ -326,10 +327,10 @@ webhooks: service: name: webhook-service namespace: system - path: /validate-cluster-x-k8s-io-v1beta1-clusterclass + path: /validate-cluster-x-k8s-io-v1beta1-machineset failurePolicy: Fail matchPolicy: Equivalent - name: validation.clusterclass.cluster.x-k8s.io + name: validation.machineset.cluster.x-k8s.io rules: - apiGroups: - cluster.x-k8s.io @@ -338,9 +339,8 @@ webhooks: operations: - CREATE - UPDATE - - DELETE resources: - - clusterclasses + - machinesets sideEffects: None - admissionReviewVersions: - v1 diff --git a/controlplane/kubeadm/internal/controllers/controller_test.go b/controlplane/kubeadm/internal/controllers/controller_test.go index 8f8f73472382..565de4625849 100644 --- a/controlplane/kubeadm/internal/controllers/controller_test.go +++ b/controlplane/kubeadm/internal/controllers/controller_test.go @@ -55,6 +55,7 @@ import ( "sigs.k8s.io/cluster-api/internal/contract" "sigs.k8s.io/cluster-api/internal/test/builder" "sigs.k8s.io/cluster-api/internal/util/ssa" + "sigs.k8s.io/cluster-api/internal/webhooks" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/certs" "sigs.k8s.io/cluster-api/util/collections" @@ -2418,7 +2419,10 @@ func createMachineNodePair(name string, cluster *clusterv1.Cluster, kcp *control }, }, } - machine.Default() + webhook := webhooks.Machine{} + if err := webhook.Default(ctx, machine); err != nil { + panic(err) + } node := &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ diff --git a/internal/controllers/machinehealthcheck/machinehealthcheck_controller_test.go b/internal/controllers/machinehealthcheck/machinehealthcheck_controller_test.go index ca35e2c5d984..9ad91664c9bc 100644 --- a/internal/controllers/machinehealthcheck/machinehealthcheck_controller_test.go +++ b/internal/controllers/machinehealthcheck/machinehealthcheck_controller_test.go @@ -46,6 +46,7 @@ import ( "sigs.k8s.io/cluster-api/controllers/remote" capierrors "sigs.k8s.io/cluster-api/errors" "sigs.k8s.io/cluster-api/internal/test/builder" + "sigs.k8s.io/cluster-api/internal/webhooks" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/conditions" "sigs.k8s.io/cluster-api/util/patch" @@ -1248,7 +1249,7 @@ func TestMachineHealthCheck_Reconcile(t *testing.T) { }, }, } - machineSet.Default() + g.Expect((&webhooks.MachineSet{}).Default(ctx, machineSet)).Should(Succeed()) g.Expect(env.Create(ctx, machineSet)).To(Succeed()) // Ensure machines have been created. diff --git a/internal/controllers/topology/cluster/desired_state.go b/internal/controllers/topology/cluster/desired_state.go index ce222170c28a..57c681e17b67 100644 --- a/internal/controllers/topology/cluster/desired_state.go +++ b/internal/controllers/topology/cluster/desired_state.go @@ -38,6 +38,7 @@ import ( "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/scope" "sigs.k8s.io/cluster-api/internal/hooks" tlog "sigs.k8s.io/cluster-api/internal/log" + "sigs.k8s.io/cluster-api/internal/webhooks" "sigs.k8s.io/cluster-api/util" ) @@ -84,6 +85,7 @@ func (r *Reconciler) computeDesiredState(ctx context.Context, s *scope.Scope) (* // The MachineHealthCheck will have the same name as the ControlPlane Object and a selector for the ControlPlane InfrastructureMachines. if s.Blueprint.IsControlPlaneMachineHealthCheckEnabled() { desiredState.ControlPlane.MachineHealthCheck = computeMachineHealthCheck( + ctx, desiredState.ControlPlane.Object, selectorForControlPlaneMHC(), s.Current.Cluster.Name, @@ -550,7 +552,7 @@ func (r *Reconciler) computeMachineDeployments(ctx context.Context, s *scope.Sco // computeMachineDeployment computes the desired state for a MachineDeploymentTopology. // The generated machineDeployment object is calculated using the values from the machineDeploymentTopology and // the machineDeployment class. -func computeMachineDeployment(_ context.Context, s *scope.Scope, machineDeploymentTopology clusterv1.MachineDeploymentTopology) (*scope.MachineDeploymentState, error) { +func computeMachineDeployment(ctx context.Context, s *scope.Scope, machineDeploymentTopology clusterv1.MachineDeploymentTopology) (*scope.MachineDeploymentState, error) { desiredMachineDeployment := &scope.MachineDeploymentState{} // Gets the blueprint for the MachineDeployment class. @@ -741,6 +743,7 @@ func computeMachineDeployment(_ context.Context, s *scope.Scope, machineDeployme if s.Blueprint.IsMachineDeploymentMachineHealthCheckEnabled(&machineDeploymentTopology) { // Note: The MHC is going to use a selector that provides a minimal set of labels which are common to all MachineSets belonging to the MachineDeployment. desiredMachineDeployment.MachineHealthCheck = computeMachineHealthCheck( + ctx, desiredMachineDeploymentObj, selectorForMachineDeploymentMHC(desiredMachineDeploymentObj), s.Current.Cluster.Name, @@ -991,7 +994,7 @@ func ownerReferenceTo(obj client.Object) *metav1.OwnerReference { } } -func computeMachineHealthCheck(healthCheckTarget client.Object, selector *metav1.LabelSelector, clusterName string, check *clusterv1.MachineHealthCheckClass) *clusterv1.MachineHealthCheck { +func computeMachineHealthCheck(ctx context.Context, healthCheckTarget client.Object, selector *metav1.LabelSelector, clusterName string, check *clusterv1.MachineHealthCheckClass) *clusterv1.MachineHealthCheck { // Create a MachineHealthCheck with the spec given in the ClusterClass. mhc := &clusterv1.MachineHealthCheck{ TypeMeta: metav1.TypeMeta{ @@ -1015,7 +1018,9 @@ func computeMachineHealthCheck(healthCheckTarget client.Object, selector *metav1 // Default all fields in the MachineHealthCheck using the same function called in the webhook. This ensures the desired // state of the object won't be different from the current state due to webhook Defaulting. - mhc.Default() + if err := (&webhooks.MachineHealthCheck{}).Default(ctx, mhc); err != nil { + panic(err) + } return mhc } diff --git a/internal/controllers/topology/cluster/desired_state_test.go b/internal/controllers/topology/cluster/desired_state_test.go index 6bd8a1992d40..f5860d356fe7 100644 --- a/internal/controllers/topology/cluster/desired_state_test.go +++ b/internal/controllers/topology/cluster/desired_state_test.go @@ -2275,7 +2275,7 @@ func Test_computeMachineHealthCheck(t *testing.T) { t.Run("set all fields correctly", func(t *testing.T) { g := NewWithT(t) - got := computeMachineHealthCheck(healthCheckTarget, selector, clusterName, mhcSpec) + got := computeMachineHealthCheck(ctx, healthCheckTarget, selector, clusterName, mhcSpec) g.Expect(got).To(Equal(want), cmp.Diff(got, want)) }) diff --git a/internal/controllers/topology/cluster/reconcile_state_test.go b/internal/controllers/topology/cluster/reconcile_state_test.go index 5e395b71c540..2633bc527635 100644 --- a/internal/controllers/topology/cluster/reconcile_state_test.go +++ b/internal/controllers/topology/cluster/reconcile_state_test.go @@ -30,12 +30,14 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" . "sigs.k8s.io/controller-runtime/pkg/envtest/komega" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" runtimev1 "sigs.k8s.io/cluster-api/exp/runtime/api/v1alpha1" @@ -48,6 +50,7 @@ import ( fakeruntimeclient "sigs.k8s.io/cluster-api/internal/runtime/client/fake" "sigs.k8s.io/cluster-api/internal/test/builder" "sigs.k8s.io/cluster-api/internal/util/ssa" + "sigs.k8s.io/cluster-api/internal/webhooks" ) var ( @@ -1551,7 +1554,6 @@ func TestReconcileControlPlaneMachineHealthCheck(t *testing.T) { InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy(), MachineHealthCheck: mhcBuilder.Build()}, want: mhcBuilder.DeepCopy(). - WithDefaulter(true). Build(), }, { @@ -1582,7 +1584,6 @@ func TestReconcileControlPlaneMachineHealthCheck(t *testing.T) { }, // Want to get the updated version of the MachineHealthCheck after reconciliation. want: mhcBuilder.DeepCopy().WithMaxUnhealthy(&maxUnhealthy). - WithDefaulter(true). Build(), }, { @@ -1675,8 +1676,11 @@ func TestReconcileControlPlaneMachineHealthCheck(t *testing.T) { return } + want := tt.want.DeepCopy() + g.Expect((&webhooks.MachineHealthCheck{}).Default(ctx, want)).To(Succeed()) + g.Expect(err).ToNot(HaveOccurred()) - g.Expect(gotMHC).To(EqualObject(tt.want, IgnoreAutogeneratedMetadata, IgnorePaths{".kind", ".apiVersion"})) + g.Expect(gotMHC).To(EqualObject(want, IgnoreAutogeneratedMetadata, IgnorePaths{".kind", ".apiVersion"})) }) } } @@ -2579,7 +2583,7 @@ func TestReconcileMachineDeploymentMachineHealthCheck(t *testing.T) { mhcBuilder.DeepCopy().Build()), }, want: []*clusterv1.MachineHealthCheck{ - mhcBuilder.DeepCopy().WithDefaulter(true).Build()}, + mhcBuilder.DeepCopy().Build()}, }, { name: "Create a new MachineHealthCheck if the MachineDeployment is modified to include one", @@ -2592,7 +2596,7 @@ func TestReconcileMachineDeploymentMachineHealthCheck(t *testing.T) { mhcBuilder.DeepCopy().Build()), }, want: []*clusterv1.MachineHealthCheck{ - mhcBuilder.DeepCopy().WithDefaulter(true).Build()}}, + mhcBuilder.DeepCopy().Build()}}, { name: "Update MachineHealthCheck spec adding a field if the spec adds a field", current: []*scope.MachineDeploymentState{ @@ -2605,7 +2609,6 @@ func TestReconcileMachineDeploymentMachineHealthCheck(t *testing.T) { want: []*clusterv1.MachineHealthCheck{ mhcBuilder.DeepCopy(). WithMaxUnhealthy(&maxUnhealthy). - WithDefaulter(true). Build()}, }, { @@ -2619,7 +2622,7 @@ func TestReconcileMachineDeploymentMachineHealthCheck(t *testing.T) { mhcBuilder.DeepCopy().Build()), }, want: []*clusterv1.MachineHealthCheck{ - mhcBuilder.DeepCopy().WithDefaulter(true).Build(), + mhcBuilder.DeepCopy().Build(), }, }, { @@ -2711,7 +2714,10 @@ func TestReconcileMachineDeploymentMachineHealthCheck(t *testing.T) { g.Expect(tt.want).To(HaveLen(len(gotMachineHealthCheckList.Items))) - for _, wantMHC := range tt.want { + for _, wantMHCOrig := range tt.want { + wantMHC := wantMHCOrig.DeepCopy() + g.Expect((&webhooks.MachineHealthCheck{}).Default(ctx, wantMHC)).To(Succeed()) + for _, gotMHC := range gotMachineHealthCheckList.Items { if wantMHC.Name == gotMHC.Name { actual := gotMHC @@ -2729,7 +2735,7 @@ func TestReconcileMachineDeploymentMachineHealthCheck(t *testing.T) { } func newFakeMachineDeploymentTopologyState(name string, infrastructureMachineTemplate, bootstrapTemplate *unstructured.Unstructured, machineHealthCheck *clusterv1.MachineHealthCheck) *scope.MachineDeploymentState { - return &scope.MachineDeploymentState{ + mdState := &scope.MachineDeploymentState{ Object: builder.MachineDeployment(metav1.NamespaceDefault, name). WithInfrastructureTemplate(infrastructureMachineTemplate). WithBootstrapTemplate(bootstrapTemplate). @@ -2739,12 +2745,20 @@ func newFakeMachineDeploymentTopologyState(name string, infrastructureMachineTem }). WithClusterName("cluster-1"). WithReplicas(1). - WithDefaulter(true). Build(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy(), BootstrapTemplate: bootstrapTemplate.DeepCopy(), MachineHealthCheck: machineHealthCheck.DeepCopy(), } + + scheme := runtime.NewScheme() + _ = clusterv1.AddToScheme(scheme) + if err := (&webhooks.MachineDeployment{ + Decoder: admission.NewDecoder(scheme), + }).Default(admission.NewContextWithRequest(ctx, admission.Request{}), mdState.Object); err != nil { + panic(err) + } + return mdState } func toMachineDeploymentTopologyStateMap(states []*scope.MachineDeploymentState) map[string]*scope.MachineDeploymentState { @@ -2779,7 +2793,7 @@ func TestReconciler_reconcileMachineHealthCheck(t *testing.T) { name: "Create a MachineHealthCheck", current: nil, desired: mhcBuilder.DeepCopy().Build(), - want: mhcBuilder.DeepCopy().WithDefaulter(true).Build(), + want: mhcBuilder.DeepCopy().Build(), }, { name: "Update a MachineHealthCheck with changes", @@ -2798,14 +2812,14 @@ func TestReconciler_reconcileMachineHealthCheck(t *testing.T) { Status: corev1.ConditionUnknown, Timeout: metav1.Duration{Duration: 1000 * time.Minute}, }, - }).WithDefaulter(true).Build(), + }).Build(), }, { name: "Don't change a MachineHealthCheck with no difference between desired and current", current: mhcBuilder.DeepCopy().Build(), // update the unhealthy conditions in the MachineHealthCheck desired: mhcBuilder.DeepCopy().Build(), - want: mhcBuilder.DeepCopy().WithDefaulter(true).Build(), + want: mhcBuilder.DeepCopy().Build(), }, { name: "Delete a MachineHealthCheck", @@ -2870,7 +2884,12 @@ func TestReconciler_reconcileMachineHealthCheck(t *testing.T) { } } - g.Expect(got).To(EqualObject(tt.want, IgnoreAutogeneratedMetadata, IgnorePaths{".kind", ".apiVersion"})) + want := tt.want.DeepCopy() + if want != nil { + g.Expect((&webhooks.MachineHealthCheck{}).Default(ctx, want)).To(Succeed()) + } + + g.Expect(got).To(EqualObject(want, IgnoreAutogeneratedMetadata, IgnorePaths{".kind", ".apiVersion"})) }) } } diff --git a/internal/test/builder/builders.go b/internal/test/builder/builders.go index 25629561cf69..af37d75d4f56 100644 --- a/internal/test/builder/builders.go +++ b/internal/test/builder/builders.go @@ -17,18 +17,14 @@ limitations under the License. package builder import ( - "context" "fmt" "strings" - admissionv1 "k8s.io/api/admission/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1" @@ -1151,7 +1147,6 @@ type MachineDeploymentBuilder struct { selector *metav1.LabelSelector version *string replicas *int32 - defaulter bool generation *int64 labels map[string]string status *clusterv1.MachineDeploymentStatus @@ -1207,12 +1202,6 @@ func (m *MachineDeploymentBuilder) WithReplicas(replicas int32) *MachineDeployme return m } -// WithDefaulter runs the Default function on the MachineDeploymentClassBuilder object. -func (m *MachineDeploymentBuilder) WithDefaulter(defaulter bool) *MachineDeploymentBuilder { - m.defaulter = defaulter - return m -} - // WithGeneration sets the passed value on the machine deployments object metadata. func (m *MachineDeploymentBuilder) WithGeneration(generation int64) *MachineDeploymentBuilder { m.generation = &generation @@ -1268,20 +1257,7 @@ func (m *MachineDeploymentBuilder) Build() *clusterv1.MachineDeployment { clusterv1.ClusterNameLabel: m.clusterName, } } - if m.defaulter { - scheme := runtime.NewScheme() - if err := clusterv1.AddToScheme(scheme); err != nil { - panic(err) - } - ctx := admission.NewContextWithRequest(context.Background(), admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Create, - }, - }) - if err := clusterv1.MachineDeploymentDefaulter(scheme).Default(ctx, obj); err != nil { - panic(err) - } - } + return obj } @@ -1500,7 +1476,6 @@ type MachineHealthCheckBuilder struct { clusterName string conditions []clusterv1.UnhealthyCondition maxUnhealthy *intstr.IntOrString - defaulter bool } // MachineHealthCheck returns a MachineHealthCheckBuilder with the given name and namespace. @@ -1541,12 +1516,6 @@ func (m *MachineHealthCheckBuilder) WithMaxUnhealthy(maxUnhealthy *intstr.IntOrS return m } -// WithDefaulter runs the Default function on the MachineHealthCheck object. -func (m *MachineHealthCheckBuilder) WithDefaulter(defaulter bool) *MachineHealthCheckBuilder { - m.defaulter = defaulter - return m -} - // Build returns a MachineHealthCheck with the supplied details. func (m *MachineHealthCheckBuilder) Build() *clusterv1.MachineHealthCheck { // create a MachineHealthCheck with the spec given in the ClusterClass @@ -1570,8 +1539,6 @@ func (m *MachineHealthCheckBuilder) Build() *clusterv1.MachineHealthCheck { if m.clusterName != "" { mhc.Labels = map[string]string{clusterv1.ClusterNameLabel: m.clusterName} } - if m.defaulter { - mhc.Default() - } + return mhc } diff --git a/internal/test/envtest/environment.go b/internal/test/envtest/environment.go index bd274e0c49cd..663b9337bdb0 100644 --- a/internal/test/envtest/environment.go +++ b/internal/test/envtest/environment.go @@ -59,6 +59,7 @@ import ( expipamwebhooks "sigs.k8s.io/cluster-api/exp/ipam/webhooks" runtimev1 "sigs.k8s.io/cluster-api/exp/runtime/api/v1alpha1" "sigs.k8s.io/cluster-api/internal/test/builder" + internalwebhooks "sigs.k8s.io/cluster-api/internal/webhooks" runtimewebhooks "sigs.k8s.io/cluster-api/internal/webhooks/runtime" "sigs.k8s.io/cluster-api/util/kubeconfig" "sigs.k8s.io/cluster-api/version" @@ -267,7 +268,7 @@ func newEnvironment(uncachedObjs ...client.Object) *Environment { } // Set minNodeStartupTimeout for Test, so it does not need to be at least 30s - clusterv1.SetMinNodeStartupTimeout(metav1.Duration{Duration: 1 * time.Millisecond}) + internalwebhooks.SetMinNodeStartupTimeout(metav1.Duration{Duration: 1 * time.Millisecond}) if err := (&webhooks.Cluster{Client: mgr.GetClient()}).SetupWebhookWithManager(mgr); err != nil { klog.Fatalf("unable to create webhook: %+v", err) @@ -275,19 +276,19 @@ func newEnvironment(uncachedObjs ...client.Object) *Environment { if err := (&webhooks.ClusterClass{Client: mgr.GetClient()}).SetupWebhookWithManager(mgr); err != nil { klog.Fatalf("unable to create webhook: %+v", err) } - if err := (&clusterv1.Machine{}).SetupWebhookWithManager(mgr); err != nil { + if err := (&webhooks.Machine{}).SetupWebhookWithManager(mgr); err != nil { klog.Fatalf("unable to create webhook: %+v", err) } - if err := (&clusterv1.MachineHealthCheck{}).SetupWebhookWithManager(mgr); err != nil { + if err := (&webhooks.MachineHealthCheck{}).SetupWebhookWithManager(mgr); err != nil { klog.Fatalf("unable to create webhook: %+v", err) } - if err := (&clusterv1.Machine{}).SetupWebhookWithManager(mgr); err != nil { + if err := (&webhooks.Machine{}).SetupWebhookWithManager(mgr); err != nil { klog.Fatalf("unable to create webhook: %+v", err) } - if err := (&clusterv1.MachineSet{}).SetupWebhookWithManager(mgr); err != nil { + if err := (&webhooks.MachineSet{}).SetupWebhookWithManager(mgr); err != nil { klog.Fatalf("unable to create webhook: %+v", err) } - if err := (&clusterv1.MachineDeployment{}).SetupWebhookWithManager(mgr); err != nil { + if err := (&webhooks.MachineDeployment{}).SetupWebhookWithManager(mgr); err != nil { klog.Fatalf("unable to create webhook: %+v", err) } if err := (&bootstrapv1.KubeadmConfig{}).SetupWebhookWithManager(mgr); err != nil { diff --git a/internal/webhooks/clusterclass.go b/internal/webhooks/clusterclass.go index d802691d99b3..ecf3bb9b7f0b 100644 --- a/internal/webhooks/clusterclass.go +++ b/internal/webhooks/clusterclass.go @@ -366,7 +366,7 @@ func validateMachineHealthCheckClass(fldPath *field.Path, namepace string, m *cl RemediationTemplate: m.RemediationTemplate, }} - return mhc.ValidateCommonFields(fldPath) + return (&MachineHealthCheck{}).validateCommonFields(&mhc, fldPath) } func validateClusterClassMetadata(clusterClass *clusterv1.ClusterClass) field.ErrorList { diff --git a/api/v1beta1/machine_webhook.go b/internal/webhooks/machine.go similarity index 59% rename from api/v1beta1/machine_webhook.go rename to internal/webhooks/machine.go index c5d9ea4bb0c1..6c76eb0cf849 100644 --- a/api/v1beta1/machine_webhook.go +++ b/internal/webhooks/machine.go @@ -14,9 +14,10 @@ See the License for the specific language governing permissions and limitations under the License. */ -package v1beta1 +package webhooks import ( + "context" "fmt" "strings" "time" @@ -29,29 +30,40 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/util/version" ) const defaultNodeDeletionTimeout = 10 * time.Second -func (m *Machine) SetupWebhookWithManager(mgr ctrl.Manager) error { +func (webhook *Machine) SetupWebhookWithManager(mgr ctrl.Manager) error { return ctrl.NewWebhookManagedBy(mgr). - For(m). + For(&clusterv1.Machine{}). + WithDefaulter(webhook). + WithValidator(webhook). Complete() } // +kubebuilder:webhook:verbs=create;update,path=/validate-cluster-x-k8s-io-v1beta1-machine,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=cluster.x-k8s.io,resources=machines,versions=v1beta1,name=validation.machine.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1 // +kubebuilder:webhook:verbs=create;update,path=/mutate-cluster-x-k8s-io-v1beta1-machine,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=cluster.x-k8s.io,resources=machines,versions=v1beta1,name=default.machine.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1 -var _ webhook.Validator = &Machine{} -var _ webhook.Defaulter = &Machine{} +// Machine implements a validation and defaulting webhook for Machine. +type Machine struct{} + +var _ webhook.CustomValidator = &Machine{} +var _ webhook.CustomDefaulter = &Machine{} // Default implements webhook.Defaulter so a webhook will be registered for the type. -func (m *Machine) Default() { +func (webhook *Machine) Default(_ context.Context, obj runtime.Object) error { + m, ok := obj.(*clusterv1.Machine) + if !ok { + return apierrors.NewBadRequest(fmt.Sprintf("expected a Machine but got a %T", obj)) + } + if m.Labels == nil { m.Labels = make(map[string]string) } - m.Labels[ClusterNameLabel] = m.Spec.ClusterName + m.Labels[clusterv1.ClusterNameLabel] = m.Spec.ClusterName if m.Spec.Bootstrap.ConfigRef != nil && m.Spec.Bootstrap.ConfigRef.Namespace == "" { m.Spec.Bootstrap.ConfigRef.Namespace = m.Namespace @@ -69,33 +81,46 @@ func (m *Machine) Default() { if m.Spec.NodeDeletionTimeout == nil { m.Spec.NodeDeletionTimeout = &metav1.Duration{Duration: defaultNodeDeletionTimeout} } + + return nil } -// ValidateCreate implements webhook.Validator so a webhook will be registered for the type. -func (m *Machine) ValidateCreate() (admission.Warnings, error) { - return nil, m.validate(nil) +// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type. +func (webhook *Machine) ValidateCreate(_ context.Context, obj runtime.Object) (admission.Warnings, error) { + m, ok := obj.(*clusterv1.Machine) + if !ok { + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a Machine but got a %T", obj)) + } + + return nil, webhook.validate(nil, m) } // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type. -func (m *Machine) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { - oldM, ok := old.(*Machine) +func (webhook *Machine) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { + oldM, ok := oldObj.(*clusterv1.Machine) + if !ok { + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a Machine but got a %T", oldObj)) + } + + newM, ok := newObj.(*clusterv1.Machine) if !ok { - return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a Machine but got a %T", old)) + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a Machine but got a %T", newObj)) } - return nil, m.validate(oldM) + + return nil, webhook.validate(oldM, newM) } -// ValidateDelete implements webhook.Validator so a webhook will be registered for the type. -func (m *Machine) ValidateDelete() (admission.Warnings, error) { +// ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type. +func (webhook *Machine) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) { return nil, nil } -func (m *Machine) validate(old *Machine) error { +func (webhook *Machine) validate(oldM, newM *clusterv1.Machine) error { var allErrs field.ErrorList specPath := field.NewPath("spec") - if m.Spec.Bootstrap.ConfigRef == nil && m.Spec.Bootstrap.DataSecretName == nil { + if newM.Spec.Bootstrap.ConfigRef == nil && newM.Spec.Bootstrap.DataSecretName == nil { // MachinePool Machines don't have a bootstrap configRef, so don't require it. The bootstrap config is instead owned by the MachinePool. - if !isMachinePoolMachine(m) { + if !isMachinePoolMachine(newM) { allErrs = append( allErrs, field.Required( @@ -106,48 +131,48 @@ func (m *Machine) validate(old *Machine) error { } } - if m.Spec.Bootstrap.ConfigRef != nil && m.Spec.Bootstrap.ConfigRef.Namespace != m.Namespace { + if newM.Spec.Bootstrap.ConfigRef != nil && newM.Spec.Bootstrap.ConfigRef.Namespace != newM.Namespace { allErrs = append( allErrs, field.Invalid( specPath.Child("bootstrap", "configRef", "namespace"), - m.Spec.Bootstrap.ConfigRef.Namespace, + newM.Spec.Bootstrap.ConfigRef.Namespace, "must match metadata.namespace", ), ) } - if m.Spec.InfrastructureRef.Namespace != m.Namespace { + if newM.Spec.InfrastructureRef.Namespace != newM.Namespace { allErrs = append( allErrs, field.Invalid( specPath.Child("infrastructureRef", "namespace"), - m.Spec.InfrastructureRef.Namespace, + newM.Spec.InfrastructureRef.Namespace, "must match metadata.namespace", ), ) } - if old != nil && old.Spec.ClusterName != m.Spec.ClusterName { + if oldM != nil && oldM.Spec.ClusterName != newM.Spec.ClusterName { allErrs = append( allErrs, field.Forbidden(specPath.Child("clusterName"), "field is immutable"), ) } - if m.Spec.Version != nil { - if !version.KubeSemver.MatchString(*m.Spec.Version) { - allErrs = append(allErrs, field.Invalid(specPath.Child("version"), *m.Spec.Version, "must be a valid semantic version")) + if newM.Spec.Version != nil { + if !version.KubeSemver.MatchString(*newM.Spec.Version) { + allErrs = append(allErrs, field.Invalid(specPath.Child("version"), *newM.Spec.Version, "must be a valid semantic version")) } } if len(allErrs) == 0 { return nil } - return apierrors.NewInvalid(GroupVersion.WithKind("Machine").GroupKind(), m.Name, allErrs) + return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("Machine").GroupKind(), newM.Name, allErrs) } -func isMachinePoolMachine(m *Machine) bool { +func isMachinePoolMachine(m *clusterv1.Machine) bool { for _, owner := range m.OwnerReferences { if owner.Kind == "MachinePool" { return true diff --git a/api/v1beta1/machine_webhook_test.go b/internal/webhooks/machine_test.go similarity index 70% rename from api/v1beta1/machine_webhook_test.go rename to internal/webhooks/machine_test.go index 78425822cc09..50d3f0dbf868 100644 --- a/api/v1beta1/machine_webhook_test.go +++ b/internal/webhooks/machine_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package v1beta1 +package webhooks import ( "testing" @@ -24,25 +24,29 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/pointer" - utildefaulting "sigs.k8s.io/cluster-api/util/defaulting" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + "sigs.k8s.io/cluster-api/internal/webhooks/util" ) func TestMachineDefault(t *testing.T) { g := NewWithT(t) - m := &Machine{ + m := &clusterv1.Machine{ ObjectMeta: metav1.ObjectMeta{ Namespace: "foobar", }, - Spec: MachineSpec{ - Bootstrap: Bootstrap{ConfigRef: &corev1.ObjectReference{}}, + Spec: clusterv1.MachineSpec{ + Bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{}}, Version: pointer.String("1.17.5"), }, } - t.Run("for Machine", utildefaulting.DefaultValidateTest(m)) - m.Default() - g.Expect(m.Labels[ClusterNameLabel]).To(Equal(m.Spec.ClusterName)) + webhook := &Machine{} + + t.Run("for Machine", util.CustomDefaultValidateTest(ctx, m, webhook)) + g.Expect(webhook.Default(ctx, m)).To(Succeed()) + + g.Expect(m.Labels[clusterv1.ClusterNameLabel]).To(Equal(m.Spec.ClusterName)) g.Expect(m.Spec.Bootstrap.ConfigRef.Namespace).To(Equal(m.Namespace)) g.Expect(m.Spec.InfrastructureRef.Namespace).To(Equal(m.Namespace)) g.Expect(*m.Spec.Version).To(Equal("v1.17.5")) @@ -52,22 +56,22 @@ func TestMachineDefault(t *testing.T) { func TestMachineBootstrapValidation(t *testing.T) { tests := []struct { name string - bootstrap Bootstrap + bootstrap clusterv1.Bootstrap expectErr bool }{ { name: "should return error if configref and data are nil", - bootstrap: Bootstrap{ConfigRef: nil, DataSecretName: nil}, + bootstrap: clusterv1.Bootstrap{ConfigRef: nil, DataSecretName: nil}, expectErr: true, }, { name: "should not return error if dataSecretName is set", - bootstrap: Bootstrap{ConfigRef: nil, DataSecretName: pointer.String("test")}, + bootstrap: clusterv1.Bootstrap{ConfigRef: nil, DataSecretName: pointer.String("test")}, expectErr: false, }, { name: "should not return error if config ref is set", - bootstrap: Bootstrap{ConfigRef: &corev1.ObjectReference{}, DataSecretName: nil}, + bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{}, DataSecretName: nil}, expectErr: false, }, } @@ -75,21 +79,23 @@ func TestMachineBootstrapValidation(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - m := &Machine{ - Spec: MachineSpec{Bootstrap: tt.bootstrap}, + m := &clusterv1.Machine{ + Spec: clusterv1.MachineSpec{Bootstrap: tt.bootstrap}, } + webhook := &Machine{} + if tt.expectErr { - warnings, err := m.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, m) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = m.ValidateUpdate(m) + warnings, err = webhook.ValidateUpdate(ctx, m, m) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } else { - warnings, err := m.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, m) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = m.ValidateUpdate(m) + warnings, err = webhook.ValidateUpdate(ctx, m, m) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } @@ -101,7 +107,7 @@ func TestMachineNamespaceValidation(t *testing.T) { tests := []struct { name string expectErr bool - bootstrap Bootstrap + bootstrap clusterv1.Bootstrap infraRef corev1.ObjectReference namespace string }{ @@ -109,28 +115,28 @@ func TestMachineNamespaceValidation(t *testing.T) { name: "should succeed if all namespaces match", expectErr: false, namespace: "foobar", - bootstrap: Bootstrap{ConfigRef: &corev1.ObjectReference{Namespace: "foobar"}}, + bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{Namespace: "foobar"}}, infraRef: corev1.ObjectReference{Namespace: "foobar"}, }, { name: "should return error if namespace and bootstrap namespace don't match", expectErr: true, namespace: "foobar", - bootstrap: Bootstrap{ConfigRef: &corev1.ObjectReference{Namespace: "foobar123"}}, + bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{Namespace: "foobar123"}}, infraRef: corev1.ObjectReference{Namespace: "foobar"}, }, { name: "should return error if namespace and infrastructure ref namespace don't match", expectErr: true, namespace: "foobar", - bootstrap: Bootstrap{ConfigRef: &corev1.ObjectReference{Namespace: "foobar"}}, + bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{Namespace: "foobar"}}, infraRef: corev1.ObjectReference{Namespace: "foobar123"}, }, { name: "should return error if no namespaces match", expectErr: true, namespace: "foobar1", - bootstrap: Bootstrap{ConfigRef: &corev1.ObjectReference{Namespace: "foobar2"}}, + bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{Namespace: "foobar2"}}, infraRef: corev1.ObjectReference{Namespace: "foobar3"}, }, } @@ -139,23 +145,24 @@ func TestMachineNamespaceValidation(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - m := &Machine{ + m := &clusterv1.Machine{ ObjectMeta: metav1.ObjectMeta{Namespace: tt.namespace}, - Spec: MachineSpec{Bootstrap: tt.bootstrap, InfrastructureRef: tt.infraRef}, + Spec: clusterv1.MachineSpec{Bootstrap: tt.bootstrap, InfrastructureRef: tt.infraRef}, } + webhook := &Machine{} if tt.expectErr { - warnings, err := m.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, m) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = m.ValidateUpdate(m) + warnings, err = webhook.ValidateUpdate(ctx, m, m) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } else { - warnings, err := m.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, m) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = m.ValidateUpdate(m) + warnings, err = webhook.ValidateUpdate(ctx, m, m) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } @@ -188,20 +195,20 @@ func TestMachineClusterNameImmutable(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - newMachine := &Machine{ - Spec: MachineSpec{ + newMachine := &clusterv1.Machine{ + Spec: clusterv1.MachineSpec{ ClusterName: tt.newClusterName, - Bootstrap: Bootstrap{ConfigRef: &corev1.ObjectReference{}}, + Bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{}}, }, } - oldMachine := &Machine{ - Spec: MachineSpec{ + oldMachine := &clusterv1.Machine{ + Spec: clusterv1.MachineSpec{ ClusterName: tt.oldClusterName, - Bootstrap: Bootstrap{ConfigRef: &corev1.ObjectReference{}}, + Bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{}}, }, } - warnings, err := newMachine.ValidateUpdate(oldMachine) + warnings, err := (&Machine{}).ValidateUpdate(ctx, oldMachine, newMachine) if tt.expectErr { g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) @@ -216,12 +223,12 @@ func TestMachineClusterNameImmutable(t *testing.T) { func TestIsMachinePoolMachine(t *testing.T) { tests := []struct { name string - machine Machine + machine clusterv1.Machine isMPM bool }{ { name: "machine is a MachinePoolMachine", - machine: Machine{ + machine: clusterv1.Machine{ ObjectMeta: metav1.ObjectMeta{ OwnerReferences: []metav1.OwnerReference{ { @@ -234,7 +241,7 @@ func TestIsMachinePoolMachine(t *testing.T) { }, { name: "machine is not a MachinePoolMachine", - machine: Machine{ + machine: clusterv1.Machine{ ObjectMeta: metav1.ObjectMeta{ OwnerReferences: []metav1.OwnerReference{ { @@ -247,7 +254,7 @@ func TestIsMachinePoolMachine(t *testing.T) { }, { name: "machine is not a MachinePoolMachine, no owner references", - machine: Machine{ + machine: clusterv1.Machine{ ObjectMeta: metav1.ObjectMeta{ OwnerReferences: nil, }, @@ -303,25 +310,26 @@ func TestMachineVersionValidation(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - m := &Machine{ - Spec: MachineSpec{ + m := &clusterv1.Machine{ + Spec: clusterv1.MachineSpec{ Version: &tt.version, - Bootstrap: Bootstrap{ConfigRef: nil, DataSecretName: pointer.String("test")}, + Bootstrap: clusterv1.Bootstrap{ConfigRef: nil, DataSecretName: pointer.String("test")}, }, } + webhook := &Machine{} if tt.expectErr { - warnings, err := m.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, m) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = m.ValidateUpdate(m) + warnings, err = webhook.ValidateUpdate(ctx, m, m) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } else { - warnings, err := m.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, m) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = m.ValidateUpdate(m) + warnings, err = webhook.ValidateUpdate(ctx, m, m) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } diff --git a/api/v1beta1/machinedeployment_webhook.go b/internal/webhooks/machinedeployment.go similarity index 69% rename from api/v1beta1/machinedeployment_webhook.go rename to internal/webhooks/machinedeployment.go index c749f8c50913..0e8e86377f4f 100644 --- a/api/v1beta1/machinedeployment_webhook.go +++ b/internal/webhooks/machinedeployment.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package v1beta1 +package webhooks import ( "context" @@ -36,40 +36,37 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/feature" "sigs.k8s.io/cluster-api/util/version" ) -func (m *MachineDeployment) SetupWebhookWithManager(mgr ctrl.Manager) error { - // This registers MachineDeployment as a validating webhook and - // machineDeploymentDefaulter as a defaulting webhook. +func (webhook *MachineDeployment) SetupWebhookWithManager(mgr ctrl.Manager) error { + if webhook.Decoder == nil { + webhook.Decoder = admission.NewDecoder(mgr.GetScheme()) + } + return ctrl.NewWebhookManagedBy(mgr). - For(m). - WithDefaulter(MachineDeploymentDefaulter(mgr.GetScheme())). + For(&clusterv1.MachineDeployment{}). + WithDefaulter(webhook). + WithValidator(webhook). Complete() } // +kubebuilder:webhook:verbs=create;update,path=/validate-cluster-x-k8s-io-v1beta1-machinedeployment,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=cluster.x-k8s.io,resources=machinedeployments,versions=v1beta1,name=validation.machinedeployment.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1 // +kubebuilder:webhook:verbs=create;update,path=/mutate-cluster-x-k8s-io-v1beta1-machinedeployment,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=cluster.x-k8s.io,resources=machinedeployments,versions=v1beta1,name=default.machinedeployment.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1 -var _ webhook.CustomDefaulter = &machineDeploymentDefaulter{} -var _ webhook.Validator = &MachineDeployment{} - -// MachineDeploymentDefaulter creates a new CustomDefaulter for MachineDeployments. -func MachineDeploymentDefaulter(scheme *runtime.Scheme) webhook.CustomDefaulter { - return &machineDeploymentDefaulter{ - decoder: admission.NewDecoder(scheme), - } +// MachineDeployment implements a validation and defaulting webhook for MachineDeployment. +type MachineDeployment struct { + Decoder *admission.Decoder } -// machineDeploymentDefaulter implements a defaulting webhook for MachineDeployment. -type machineDeploymentDefaulter struct { - decoder *admission.Decoder -} +var _ webhook.CustomDefaulter = &MachineDeployment{} +var _ webhook.CustomValidator = &MachineDeployment{} // Default implements webhook.CustomDefaulter. -func (webhook *machineDeploymentDefaulter) Default(ctx context.Context, obj runtime.Object) error { - m, ok := obj.(*MachineDeployment) +func (webhook *MachineDeployment) Default(ctx context.Context, obj runtime.Object) error { + m, ok := obj.(*clusterv1.MachineDeployment) if !ok { return apierrors.NewBadRequest(fmt.Sprintf("expected a MachineDeployment but got a %T", obj)) } @@ -83,10 +80,10 @@ func (webhook *machineDeploymentDefaulter) Default(ctx context.Context, obj runt dryRun = *req.DryRun } - var oldMD *MachineDeployment + var oldMD *clusterv1.MachineDeployment if req.Operation == v1.Update { - oldMD = &MachineDeployment{} - if err := webhook.decoder.DecodeRaw(req.OldObject, oldMD); err != nil { + oldMD = &clusterv1.MachineDeployment{} + if err := webhook.Decoder.DecodeRaw(req.OldObject, oldMD); err != nil { return errors.Wrapf(err, "failed to decode oldObject to MachineDeployment") } } @@ -94,7 +91,7 @@ func (webhook *machineDeploymentDefaulter) Default(ctx context.Context, obj runt if m.Labels == nil { m.Labels = make(map[string]string) } - m.Labels[ClusterNameLabel] = m.Spec.ClusterName + m.Labels[clusterv1.ClusterNameLabel] = m.Spec.ClusterName replicas, err := calculateMachineDeploymentReplicas(ctx, oldMD, m, dryRun) if err != nil { @@ -119,11 +116,11 @@ func (webhook *machineDeploymentDefaulter) Default(ctx context.Context, obj runt } if m.Spec.Strategy == nil { - m.Spec.Strategy = &MachineDeploymentStrategy{} + m.Spec.Strategy = &clusterv1.MachineDeploymentStrategy{} } if m.Spec.Strategy.Type == "" { - m.Spec.Strategy.Type = RollingUpdateMachineDeploymentStrategyType + m.Spec.Strategy.Type = clusterv1.RollingUpdateMachineDeploymentStrategyType } if m.Spec.Template.Labels == nil { @@ -131,9 +128,9 @@ func (webhook *machineDeploymentDefaulter) Default(ctx context.Context, obj runt } // Default RollingUpdate strategy only if strategy type is RollingUpdate. - if m.Spec.Strategy.Type == RollingUpdateMachineDeploymentStrategyType { + if m.Spec.Strategy.Type == clusterv1.RollingUpdateMachineDeploymentStrategyType { if m.Spec.Strategy.RollingUpdate == nil { - m.Spec.Strategy.RollingUpdate = &MachineRollingUpdateDeployment{} + m.Spec.Strategy.RollingUpdate = &clusterv1.MachineRollingUpdateDeployment{} } if m.Spec.Strategy.RollingUpdate.MaxSurge == nil { ios1 := intstr.FromInt(1) @@ -148,12 +145,12 @@ func (webhook *machineDeploymentDefaulter) Default(ctx context.Context, obj runt // If no selector has been provided, add label and selector for the // MachineDeployment's name as a default way of providing uniqueness. if len(m.Spec.Selector.MatchLabels) == 0 && len(m.Spec.Selector.MatchExpressions) == 0 { - m.Spec.Selector.MatchLabels[MachineDeploymentNameLabel] = m.Name - m.Spec.Template.Labels[MachineDeploymentNameLabel] = m.Name + m.Spec.Selector.MatchLabels[clusterv1.MachineDeploymentNameLabel] = m.Name + m.Spec.Template.Labels[clusterv1.MachineDeploymentNameLabel] = m.Name } // Make sure selector and template to be in the same cluster. - m.Spec.Selector.MatchLabels[ClusterNameLabel] = m.Spec.ClusterName - m.Spec.Template.Labels[ClusterNameLabel] = m.Spec.ClusterName + m.Spec.Selector.MatchLabels[clusterv1.ClusterNameLabel] = m.Spec.ClusterName + m.Spec.Template.Labels[clusterv1.ClusterNameLabel] = m.Spec.ClusterName // tolerate version strings without a "v" prefix: prepend it if it's not there if m.Spec.Template.Spec.Version != nil && !strings.HasPrefix(*m.Spec.Template.Spec.Version, "v") { @@ -164,48 +161,59 @@ func (webhook *machineDeploymentDefaulter) Default(ctx context.Context, obj runt return nil } -// ValidateCreate implements webhook.Validator so a webhook will be registered for the type. -func (m *MachineDeployment) ValidateCreate() (admission.Warnings, error) { - return nil, m.validate(nil) +// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type. +func (webhook *MachineDeployment) ValidateCreate(_ context.Context, obj runtime.Object) (admission.Warnings, error) { + m, ok := obj.(*clusterv1.MachineDeployment) + if !ok { + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a MachineDeployment but got a %T", obj)) + } + + return nil, webhook.validate(nil, m) } // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type. -func (m *MachineDeployment) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { - oldMD, ok := old.(*MachineDeployment) +func (webhook *MachineDeployment) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { + oldMD, ok := oldObj.(*clusterv1.MachineDeployment) + if !ok { + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a MachineDeployment but got a %T", oldObj)) + } + + newMD, ok := newObj.(*clusterv1.MachineDeployment) if !ok { - return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a MachineDeployment but got a %T", old)) + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a MachineDeployment but got a %T", newObj)) } - return nil, m.validate(oldMD) + + return nil, webhook.validate(oldMD, newMD) } // ValidateDelete implements webhook.Validator so a webhook will be registered for the type. -func (m *MachineDeployment) ValidateDelete() (admission.Warnings, error) { +func (webhook *MachineDeployment) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) { return nil, nil } -func (m *MachineDeployment) validate(old *MachineDeployment) error { +func (webhook *MachineDeployment) validate(oldMD, newMD *clusterv1.MachineDeployment) error { var allErrs field.ErrorList // The MachineDeployment name is used as a label value. This check ensures names which are not be valid label values are rejected. - if errs := validation.IsValidLabelValue(m.Name); len(errs) != 0 { + if errs := validation.IsValidLabelValue(newMD.Name); len(errs) != 0 { for _, err := range errs { allErrs = append( allErrs, field.Invalid( field.NewPath("metadata", "name"), - m.Name, + newMD.Name, fmt.Sprintf("must be a valid label value: %s", err), ), ) } } specPath := field.NewPath("spec") - selector, err := metav1.LabelSelectorAsSelector(&m.Spec.Selector) + selector, err := metav1.LabelSelectorAsSelector(&newMD.Spec.Selector) if err != nil { allErrs = append( allErrs, - field.Invalid(specPath.Child("selector"), m.Spec.Selector, err.Error()), + field.Invalid(specPath.Child("selector"), newMD.Spec.Selector, err.Error()), ) - } else if !selector.Matches(labels.Set(m.Spec.Template.Labels)) { + } else if !selector.Matches(labels.Set(newMD.Spec.Template.Labels)) { allErrs = append( allErrs, field.Forbidden( @@ -218,12 +226,12 @@ func (m *MachineDeployment) validate(old *MachineDeployment) error { // MachineSet preflight checks that should be skipped could also be set as annotation on the MachineDeployment // since MachineDeployment annotations are synced to the MachineSet. if feature.Gates.Enabled(feature.MachineSetPreflightChecks) { - if err := validateSkippedMachineSetPreflightChecks(m); err != nil { + if err := validateSkippedMachineSetPreflightChecks(newMD); err != nil { allErrs = append(allErrs, err) } } - if old != nil && old.Spec.ClusterName != m.Spec.ClusterName { + if oldMD != nil && oldMD.Spec.ClusterName != newMD.Spec.ClusterName { allErrs = append( allErrs, field.Forbidden( @@ -233,47 +241,47 @@ func (m *MachineDeployment) validate(old *MachineDeployment) error { ) } - if m.Spec.Strategy != nil && m.Spec.Strategy.RollingUpdate != nil { + if newMD.Spec.Strategy != nil && newMD.Spec.Strategy.RollingUpdate != nil { total := 1 - if m.Spec.Replicas != nil { - total = int(*m.Spec.Replicas) + if newMD.Spec.Replicas != nil { + total = int(*newMD.Spec.Replicas) } - if m.Spec.Strategy.RollingUpdate.MaxSurge != nil { - if _, err := intstr.GetScaledValueFromIntOrPercent(m.Spec.Strategy.RollingUpdate.MaxSurge, total, true); err != nil { + if newMD.Spec.Strategy.RollingUpdate.MaxSurge != nil { + if _, err := intstr.GetScaledValueFromIntOrPercent(newMD.Spec.Strategy.RollingUpdate.MaxSurge, total, true); err != nil { allErrs = append( allErrs, field.Invalid(specPath.Child("strategy", "rollingUpdate", "maxSurge"), - m.Spec.Strategy.RollingUpdate.MaxSurge, fmt.Sprintf("must be either an int or a percentage: %v", err.Error())), + newMD.Spec.Strategy.RollingUpdate.MaxSurge, fmt.Sprintf("must be either an int or a percentage: %v", err.Error())), ) } } - if m.Spec.Strategy.RollingUpdate.MaxUnavailable != nil { - if _, err := intstr.GetScaledValueFromIntOrPercent(m.Spec.Strategy.RollingUpdate.MaxUnavailable, total, true); err != nil { + if newMD.Spec.Strategy.RollingUpdate.MaxUnavailable != nil { + if _, err := intstr.GetScaledValueFromIntOrPercent(newMD.Spec.Strategy.RollingUpdate.MaxUnavailable, total, true); err != nil { allErrs = append( allErrs, field.Invalid(specPath.Child("strategy", "rollingUpdate", "maxUnavailable"), - m.Spec.Strategy.RollingUpdate.MaxUnavailable, fmt.Sprintf("must be either an int or a percentage: %v", err.Error())), + newMD.Spec.Strategy.RollingUpdate.MaxUnavailable, fmt.Sprintf("must be either an int or a percentage: %v", err.Error())), ) } } } - if m.Spec.Template.Spec.Version != nil { - if !version.KubeSemver.MatchString(*m.Spec.Template.Spec.Version) { - allErrs = append(allErrs, field.Invalid(specPath.Child("template", "spec", "version"), *m.Spec.Template.Spec.Version, "must be a valid semantic version")) + if newMD.Spec.Template.Spec.Version != nil { + if !version.KubeSemver.MatchString(*newMD.Spec.Template.Spec.Version) { + allErrs = append(allErrs, field.Invalid(specPath.Child("template", "spec", "version"), *newMD.Spec.Template.Spec.Version, "must be a valid semantic version")) } } // Validate the metadata of the template. - allErrs = append(allErrs, m.Spec.Template.ObjectMeta.Validate(specPath.Child("template", "metadata"))...) + allErrs = append(allErrs, newMD.Spec.Template.ObjectMeta.Validate(specPath.Child("template", "metadata"))...) if len(allErrs) == 0 { return nil } - return apierrors.NewInvalid(GroupVersion.WithKind("MachineDeployment").GroupKind(), m.Name, allErrs) + return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("MachineDeployment").GroupKind(), newMD.Name, allErrs) } // calculateMachineDeploymentReplicas calculates the default value of the replicas field. @@ -305,7 +313,7 @@ func (m *MachineDeployment) validate(old *MachineDeployment) error { // Notes: // - While the min size and max size annotations of the autoscaler provide the best UX, other autoscalers can use the // DefaultReplicasAnnotation if they have similar use cases. -func calculateMachineDeploymentReplicas(ctx context.Context, oldMD *MachineDeployment, newMD *MachineDeployment, dryRun bool) (int32, error) { +func calculateMachineDeploymentReplicas(ctx context.Context, oldMD *clusterv1.MachineDeployment, newMD *clusterv1.MachineDeployment, dryRun bool) (int32, error) { // If replicas is already set => Keep the current value. if newMD.Spec.Replicas != nil { return *newMD.Spec.Replicas, nil @@ -314,23 +322,23 @@ func calculateMachineDeploymentReplicas(ctx context.Context, oldMD *MachineDeplo log := ctrl.LoggerFrom(ctx) // If both autoscaler annotations are set, use them to calculate the default value. - minSizeString, hasMinSizeAnnotation := newMD.Annotations[AutoscalerMinSizeAnnotation] - maxSizeString, hasMaxSizeAnnotation := newMD.Annotations[AutoscalerMaxSizeAnnotation] + minSizeString, hasMinSizeAnnotation := newMD.Annotations[clusterv1.AutoscalerMinSizeAnnotation] + maxSizeString, hasMaxSizeAnnotation := newMD.Annotations[clusterv1.AutoscalerMaxSizeAnnotation] if hasMinSizeAnnotation && hasMaxSizeAnnotation { minSize, err := strconv.ParseInt(minSizeString, 10, 32) if err != nil { - return 0, errors.Wrapf(err, "failed to caculate MachineDeployment replicas value: could not parse the value of the %q annotation", AutoscalerMinSizeAnnotation) + return 0, errors.Wrapf(err, "failed to caculate MachineDeployment replicas value: could not parse the value of the %q annotation", clusterv1.AutoscalerMinSizeAnnotation) } maxSize, err := strconv.ParseInt(maxSizeString, 10, 32) if err != nil { - return 0, errors.Wrapf(err, "failed to caculate MachineDeployment replicas value: could not parse the value of the %q annotation", AutoscalerMaxSizeAnnotation) + return 0, errors.Wrapf(err, "failed to caculate MachineDeployment replicas value: could not parse the value of the %q annotation", clusterv1.AutoscalerMaxSizeAnnotation) } // If it's a new MachineDeployment => Use the min size. // Note: This will result in a scale up to get into the range where autoscaler takes over. if oldMD == nil { if !dryRun { - log.V(2).Info(fmt.Sprintf("Replica field has been defaulted to %d based on the %s annotation (MD is a new MD)", minSize, AutoscalerMinSizeAnnotation)) + log.V(2).Info(fmt.Sprintf("Replica field has been defaulted to %d based on the %s annotation (MD is a new MD)", minSize, clusterv1.AutoscalerMinSizeAnnotation)) } return int32(minSize), nil } @@ -344,21 +352,21 @@ func calculateMachineDeploymentReplicas(ctx context.Context, oldMD *MachineDeplo // We only have this handling to be 100% safe against panics. case oldMD.Spec.Replicas == nil: if !dryRun { - log.V(2).Info(fmt.Sprintf("Replica field has been defaulted to %d based on the %s annotation (old MD didn't have replicas set)", minSize, AutoscalerMinSizeAnnotation)) + log.V(2).Info(fmt.Sprintf("Replica field has been defaulted to %d based on the %s annotation (old MD didn't have replicas set)", minSize, clusterv1.AutoscalerMinSizeAnnotation)) } return int32(minSize), nil // If the old MachineDeployment replicas are lower than min size => Use the min size. // Note: This will result in a scale up to get into the range where autoscaler takes over. case *oldMD.Spec.Replicas < int32(minSize): if !dryRun { - log.V(2).Info(fmt.Sprintf("Replica field has been defaulted to %d based on the %s annotation (old MD had replicas below min size)", minSize, AutoscalerMinSizeAnnotation)) + log.V(2).Info(fmt.Sprintf("Replica field has been defaulted to %d based on the %s annotation (old MD had replicas below min size)", minSize, clusterv1.AutoscalerMinSizeAnnotation)) } return int32(minSize), nil // If the old MachineDeployment replicas are higher than max size => Use the max size. // Note: This will result in a scale down to get into the range where autoscaler takes over. case *oldMD.Spec.Replicas > int32(maxSize): if !dryRun { - log.V(2).Info(fmt.Sprintf("Replica field has been defaulted to %d based on the %s annotation (old MD had replicas above max size)", maxSize, AutoscalerMaxSizeAnnotation)) + log.V(2).Info(fmt.Sprintf("Replica field has been defaulted to %d based on the %s annotation (old MD had replicas above max size)", maxSize, clusterv1.AutoscalerMaxSizeAnnotation)) } return int32(maxSize), nil // If the old MachineDeployment replicas are between min and max size => Keep the current value. diff --git a/api/v1beta1/machinedeployment_webhook_test.go b/internal/webhooks/machinedeployment_test.go similarity index 67% rename from api/v1beta1/machinedeployment_webhook_test.go rename to internal/webhooks/machinedeployment_test.go index d258010f6b77..ba7f9991eb6b 100644 --- a/api/v1beta1/machinedeployment_webhook_test.go +++ b/internal/webhooks/machinedeployment_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package v1beta1 +package webhooks import ( "context" @@ -28,18 +28,21 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + "sigs.k8s.io/cluster-api/internal/webhooks/util" ) func TestMachineDeploymentDefault(t *testing.T) { g := NewWithT(t) - md := &MachineDeployment{ + md := &clusterv1.MachineDeployment{ ObjectMeta: metav1.ObjectMeta{ Name: "test-md", }, - Spec: MachineDeploymentSpec{ + Spec: clusterv1.MachineDeploymentSpec{ ClusterName: "test-cluster", - Template: MachineTemplateSpec{ - Spec: MachineSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ Version: pointer.String("1.19.10"), }, }, @@ -47,19 +50,21 @@ func TestMachineDeploymentDefault(t *testing.T) { } scheme := runtime.NewScheme() - g.Expect(AddToScheme(scheme)).To(Succeed()) - defaulter := MachineDeploymentDefaulter(scheme) - - t.Run("for MachineDeployment", defaultValidateTestCustomDefaulter(md, defaulter)) + g.Expect(clusterv1.AddToScheme(scheme)).To(Succeed()) + webhook := &MachineDeployment{ + Decoder: admission.NewDecoder(scheme), + } - ctx := admission.NewContextWithRequest(context.Background(), admission.Request{ + reqCtx := admission.NewContextWithRequest(ctx, admission.Request{ AdmissionRequest: admissionv1.AdmissionRequest{ Operation: admissionv1.Create, }, }) - g.Expect(defaulter.Default(ctx, md)).To(Succeed()) + t.Run("for MachineDeployment", util.CustomDefaultValidateTest(reqCtx, md, webhook)) - g.Expect(md.Labels[ClusterNameLabel]).To(Equal(md.Spec.ClusterName)) + g.Expect(webhook.Default(reqCtx, md)).To(Succeed()) + + g.Expect(md.Labels[clusterv1.ClusterNameLabel]).To(Equal(md.Spec.ClusterName)) g.Expect(md.Spec.MinReadySeconds).To(Equal(pointer.Int32(0))) g.Expect(md.Spec.Replicas).To(Equal(pointer.Int32(1))) @@ -67,12 +72,12 @@ func TestMachineDeploymentDefault(t *testing.T) { g.Expect(md.Spec.ProgressDeadlineSeconds).To(Equal(pointer.Int32(600))) g.Expect(md.Spec.Strategy).ToNot(BeNil()) - g.Expect(md.Spec.Selector.MatchLabels).To(HaveKeyWithValue(MachineDeploymentNameLabel, "test-md")) - g.Expect(md.Spec.Template.Labels).To(HaveKeyWithValue(MachineDeploymentNameLabel, "test-md")) - g.Expect(md.Spec.Selector.MatchLabels).To(HaveKeyWithValue(ClusterNameLabel, "test-cluster")) - g.Expect(md.Spec.Template.Labels).To(HaveKeyWithValue(ClusterNameLabel, "test-cluster")) + g.Expect(md.Spec.Selector.MatchLabels).To(HaveKeyWithValue(clusterv1.MachineDeploymentNameLabel, "test-md")) + g.Expect(md.Spec.Template.Labels).To(HaveKeyWithValue(clusterv1.MachineDeploymentNameLabel, "test-md")) + g.Expect(md.Spec.Selector.MatchLabels).To(HaveKeyWithValue(clusterv1.ClusterNameLabel, "test-cluster")) + g.Expect(md.Spec.Template.Labels).To(HaveKeyWithValue(clusterv1.ClusterNameLabel, "test-cluster")) - g.Expect(md.Spec.Strategy.Type).To(Equal(RollingUpdateMachineDeploymentStrategyType)) + g.Expect(md.Spec.Strategy.Type).To(Equal(clusterv1.RollingUpdateMachineDeploymentStrategyType)) g.Expect(md.Spec.Strategy.RollingUpdate).ToNot(BeNil()) g.Expect(md.Spec.Strategy.RollingUpdate.MaxSurge.IntValue()).To(Equal(1)) g.Expect(md.Spec.Strategy.RollingUpdate.MaxUnavailable.IntValue()).To(Equal(0)) @@ -83,15 +88,15 @@ func TestMachineDeploymentDefault(t *testing.T) { func TestCalculateMachineDeploymentReplicas(t *testing.T) { tests := []struct { name string - newMD *MachineDeployment - oldMD *MachineDeployment + newMD *clusterv1.MachineDeployment + oldMD *clusterv1.MachineDeployment expectedReplicas int32 expectErr bool }{ { name: "if new MD has replicas set, keep that value", - newMD: &MachineDeployment{ - Spec: MachineDeploymentSpec{ + newMD: &clusterv1.MachineDeployment{ + Spec: clusterv1.MachineDeploymentSpec{ Replicas: pointer.Int32(5), }, }, @@ -99,15 +104,15 @@ func TestCalculateMachineDeploymentReplicas(t *testing.T) { }, { name: "if new MD does not have replicas set and no annotations, use 1", - newMD: &MachineDeployment{}, + newMD: &clusterv1.MachineDeployment{}, expectedReplicas: 1, }, { name: "if new MD only has min size annotation, fallback to 1", - newMD: &MachineDeployment{ + newMD: &clusterv1.MachineDeployment{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - AutoscalerMinSizeAnnotation: "3", + clusterv1.AutoscalerMinSizeAnnotation: "3", }, }, }, @@ -115,10 +120,10 @@ func TestCalculateMachineDeploymentReplicas(t *testing.T) { }, { name: "if new MD only has max size annotation, fallback to 1", - newMD: &MachineDeployment{ + newMD: &clusterv1.MachineDeployment{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - AutoscalerMaxSizeAnnotation: "7", + clusterv1.AutoscalerMaxSizeAnnotation: "7", }, }, }, @@ -126,11 +131,11 @@ func TestCalculateMachineDeploymentReplicas(t *testing.T) { }, { name: "if new MD has min and max size annotation and min size is invalid, fail", - newMD: &MachineDeployment{ + newMD: &clusterv1.MachineDeployment{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - AutoscalerMinSizeAnnotation: "abc", - AutoscalerMaxSizeAnnotation: "7", + clusterv1.AutoscalerMinSizeAnnotation: "abc", + clusterv1.AutoscalerMaxSizeAnnotation: "7", }, }, }, @@ -138,11 +143,11 @@ func TestCalculateMachineDeploymentReplicas(t *testing.T) { }, { name: "if new MD has min and max size annotation and max size is invalid, fail", - newMD: &MachineDeployment{ + newMD: &clusterv1.MachineDeployment{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - AutoscalerMinSizeAnnotation: "3", - AutoscalerMaxSizeAnnotation: "abc", + clusterv1.AutoscalerMinSizeAnnotation: "3", + clusterv1.AutoscalerMaxSizeAnnotation: "abc", }, }, }, @@ -150,11 +155,11 @@ func TestCalculateMachineDeploymentReplicas(t *testing.T) { }, { name: "if new MD has min and max size annotation and new MD is a new MD, use min size", - newMD: &MachineDeployment{ + newMD: &clusterv1.MachineDeployment{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - AutoscalerMinSizeAnnotation: "3", - AutoscalerMaxSizeAnnotation: "7", + clusterv1.AutoscalerMinSizeAnnotation: "3", + clusterv1.AutoscalerMaxSizeAnnotation: "7", }, }, }, @@ -162,29 +167,29 @@ func TestCalculateMachineDeploymentReplicas(t *testing.T) { }, { name: "if new MD has min and max size annotation and old MD doesn't have replicas set, use min size", - newMD: &MachineDeployment{ + newMD: &clusterv1.MachineDeployment{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - AutoscalerMinSizeAnnotation: "3", - AutoscalerMaxSizeAnnotation: "7", + clusterv1.AutoscalerMinSizeAnnotation: "3", + clusterv1.AutoscalerMaxSizeAnnotation: "7", }, }, }, - oldMD: &MachineDeployment{}, + oldMD: &clusterv1.MachineDeployment{}, expectedReplicas: 3, }, { name: "if new MD has min and max size annotation and old MD replicas is below min size, use min size", - newMD: &MachineDeployment{ + newMD: &clusterv1.MachineDeployment{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - AutoscalerMinSizeAnnotation: "3", - AutoscalerMaxSizeAnnotation: "7", + clusterv1.AutoscalerMinSizeAnnotation: "3", + clusterv1.AutoscalerMaxSizeAnnotation: "7", }, }, }, - oldMD: &MachineDeployment{ - Spec: MachineDeploymentSpec{ + oldMD: &clusterv1.MachineDeployment{ + Spec: clusterv1.MachineDeploymentSpec{ Replicas: pointer.Int32(1), }, }, @@ -192,16 +197,16 @@ func TestCalculateMachineDeploymentReplicas(t *testing.T) { }, { name: "if new MD has min and max size annotation and old MD replicas is above max size, use max size", - newMD: &MachineDeployment{ + newMD: &clusterv1.MachineDeployment{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - AutoscalerMinSizeAnnotation: "3", - AutoscalerMaxSizeAnnotation: "7", + clusterv1.AutoscalerMinSizeAnnotation: "3", + clusterv1.AutoscalerMaxSizeAnnotation: "7", }, }, }, - oldMD: &MachineDeployment{ - Spec: MachineDeploymentSpec{ + oldMD: &clusterv1.MachineDeployment{ + Spec: clusterv1.MachineDeploymentSpec{ Replicas: pointer.Int32(15), }, }, @@ -209,16 +214,16 @@ func TestCalculateMachineDeploymentReplicas(t *testing.T) { }, { name: "if new MD has min and max size annotation and old MD replicas is between min and max size, use old MD replicas", - newMD: &MachineDeployment{ + newMD: &clusterv1.MachineDeployment{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - AutoscalerMinSizeAnnotation: "3", - AutoscalerMaxSizeAnnotation: "7", + clusterv1.AutoscalerMinSizeAnnotation: "3", + clusterv1.AutoscalerMaxSizeAnnotation: "7", }, }, }, - oldMD: &MachineDeployment{ - Spec: MachineDeploymentSpec{ + oldMD: &clusterv1.MachineDeployment{ + Spec: clusterv1.MachineDeploymentSpec{ Replicas: pointer.Int32(4), }, }, @@ -254,11 +259,11 @@ func TestMachineDeploymentValidation(t *testing.T) { goodMaxUnavailableInt := intstr.FromInt(0) tests := []struct { name string - md MachineDeployment + md *clusterv1.MachineDeployment mdName string selectors map[string]string labels map[string]string - strategy MachineDeploymentStrategy + strategy clusterv1.MachineDeploymentStrategy expectErr bool }{ { @@ -325,9 +330,9 @@ func TestMachineDeploymentValidation(t *testing.T) { name: "should return error for invalid maxSurge", selectors: map[string]string{"foo": "bar"}, labels: map[string]string{"foo": "bar"}, - strategy: MachineDeploymentStrategy{ - Type: RollingUpdateMachineDeploymentStrategyType, - RollingUpdate: &MachineRollingUpdateDeployment{ + strategy: clusterv1.MachineDeploymentStrategy{ + Type: clusterv1.RollingUpdateMachineDeploymentStrategyType, + RollingUpdate: &clusterv1.MachineRollingUpdateDeployment{ MaxUnavailable: &goodMaxUnavailableInt, MaxSurge: &badMaxSurge, }, @@ -338,9 +343,9 @@ func TestMachineDeploymentValidation(t *testing.T) { name: "should return error for invalid maxUnavailable", selectors: map[string]string{"foo": "bar"}, labels: map[string]string{"foo": "bar"}, - strategy: MachineDeploymentStrategy{ - Type: RollingUpdateMachineDeploymentStrategyType, - RollingUpdate: &MachineRollingUpdateDeployment{ + strategy: clusterv1.MachineDeploymentStrategy{ + Type: clusterv1.RollingUpdateMachineDeploymentStrategyType, + RollingUpdate: &clusterv1.MachineRollingUpdateDeployment{ MaxUnavailable: &badMaxUnavailable, MaxSurge: &goodMaxSurgeInt, }, @@ -351,9 +356,9 @@ func TestMachineDeploymentValidation(t *testing.T) { name: "should not return error for valid int maxSurge and maxUnavailable", selectors: map[string]string{"foo": "bar"}, labels: map[string]string{"foo": "bar"}, - strategy: MachineDeploymentStrategy{ - Type: RollingUpdateMachineDeploymentStrategyType, - RollingUpdate: &MachineRollingUpdateDeployment{ + strategy: clusterv1.MachineDeploymentStrategy{ + Type: clusterv1.RollingUpdateMachineDeploymentStrategyType, + RollingUpdate: &clusterv1.MachineRollingUpdateDeployment{ MaxUnavailable: &goodMaxUnavailableInt, MaxSurge: &goodMaxSurgeInt, }, @@ -364,9 +369,9 @@ func TestMachineDeploymentValidation(t *testing.T) { name: "should not return error for valid percentage string maxSurge and maxUnavailable", selectors: map[string]string{"foo": "bar"}, labels: map[string]string{"foo": "bar"}, - strategy: MachineDeploymentStrategy{ - Type: RollingUpdateMachineDeploymentStrategyType, - RollingUpdate: &MachineRollingUpdateDeployment{ + strategy: clusterv1.MachineDeploymentStrategy{ + Type: clusterv1.RollingUpdateMachineDeploymentStrategyType, + RollingUpdate: &clusterv1.MachineRollingUpdateDeployment{ MaxUnavailable: &goodMaxUnavailablePercentage, MaxSurge: &goodMaxSurgePercentage, }, @@ -378,34 +383,41 @@ func TestMachineDeploymentValidation(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - md := &MachineDeployment{ + md := &clusterv1.MachineDeployment{ ObjectMeta: metav1.ObjectMeta{ Name: tt.mdName, }, - Spec: MachineDeploymentSpec{ + Spec: clusterv1.MachineDeploymentSpec{ Strategy: &tt.strategy, Selector: metav1.LabelSelector{ MatchLabels: tt.selectors, }, - Template: MachineTemplateSpec{ - ObjectMeta: ObjectMeta{ + Template: clusterv1.MachineTemplateSpec{ + ObjectMeta: clusterv1.ObjectMeta{ Labels: tt.labels, }, }, }, } + + scheme := runtime.NewScheme() + g.Expect(clusterv1.AddToScheme(scheme)).To(Succeed()) + webhook := MachineDeployment{ + Decoder: admission.NewDecoder(scheme), + } + if tt.expectErr { - warnings, err := md.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, md) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = md.ValidateUpdate(md) + warnings, err = webhook.ValidateUpdate(ctx, md, md) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } else { - warnings, err := md.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, md) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = md.ValidateUpdate(md) + warnings, err = webhook.ValidateUpdate(ctx, md, md) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } @@ -450,29 +462,34 @@ func TestMachineDeploymentVersionValidation(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - md := &MachineDeployment{ - Spec: MachineDeploymentSpec{ - - Template: MachineTemplateSpec{ - Spec: MachineSpec{ + md := &clusterv1.MachineDeployment{ + Spec: clusterv1.MachineDeploymentSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ Version: pointer.String(tt.version), }, }, }, } + scheme := runtime.NewScheme() + g.Expect(clusterv1.AddToScheme(scheme)).To(Succeed()) + webhook := MachineDeployment{ + Decoder: admission.NewDecoder(scheme), + } + if tt.expectErr { - warnings, err := md.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, md) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = md.ValidateUpdate(md) + warnings, err = webhook.ValidateUpdate(ctx, md, md) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } else { - warnings, err := md.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, md) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = md.ValidateUpdate(md) + warnings, err = webhook.ValidateUpdate(ctx, md, md) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } @@ -505,19 +522,25 @@ func TestMachineDeploymentClusterNameImmutable(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - newMD := &MachineDeployment{ - Spec: MachineDeploymentSpec{ + newMD := &clusterv1.MachineDeployment{ + Spec: clusterv1.MachineDeploymentSpec{ ClusterName: tt.newClusterName, }, } - oldMD := &MachineDeployment{ - Spec: MachineDeploymentSpec{ + oldMD := &clusterv1.MachineDeployment{ + Spec: clusterv1.MachineDeploymentSpec{ ClusterName: tt.oldClusterName, }, } - warnings, err := newMD.ValidateUpdate(oldMD) + scheme := runtime.NewScheme() + g.Expect(clusterv1.AddToScheme(scheme)).To(Succeed()) + webhook := MachineDeployment{ + Decoder: admission.NewDecoder(scheme), + } + + warnings, err := webhook.ValidateUpdate(ctx, oldMD, newMD) if tt.expectErr { g.Expect(err).To(HaveOccurred()) } else { @@ -528,50 +551,6 @@ func TestMachineDeploymentClusterNameImmutable(t *testing.T) { } } -// defaultValidateTestCustomDefaulter returns a new testing function to be used in tests to -// make sure defaulting webhooks also pass validation tests on create, update and delete. -// Note: The difference to util/defaulting.DefaultValidateTest is that this function takes an additional -// CustomDefaulter as the defaulting is not implemented on the object directly. -func defaultValidateTestCustomDefaulter(object admission.Validator, customDefaulter admission.CustomDefaulter) func(*testing.T) { - return func(t *testing.T) { - t.Helper() - - createCopy := object.DeepCopyObject().(admission.Validator) - updateCopy := object.DeepCopyObject().(admission.Validator) - deleteCopy := object.DeepCopyObject().(admission.Validator) - defaultingUpdateCopy := updateCopy.DeepCopyObject().(admission.Validator) - - ctx := admission.NewContextWithRequest(context.Background(), admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Create, - }, - }) - - t.Run("validate-on-create", func(t *testing.T) { - g := NewWithT(t) - g.Expect(customDefaulter.Default(ctx, createCopy)).To(Succeed()) - warnings, err := createCopy.ValidateCreate() - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(warnings).To(BeEmpty()) - }) - t.Run("validate-on-update", func(t *testing.T) { - g := NewWithT(t) - g.Expect(customDefaulter.Default(ctx, defaultingUpdateCopy)).To(Succeed()) - g.Expect(customDefaulter.Default(ctx, updateCopy)).To(Succeed()) - warnings, err := defaultingUpdateCopy.ValidateUpdate(updateCopy) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(warnings).To(BeEmpty()) - }) - t.Run("validate-on-delete", func(t *testing.T) { - g := NewWithT(t) - g.Expect(customDefaulter.Default(ctx, deleteCopy)).To(Succeed()) - warnings, err := deleteCopy.ValidateDelete() - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(warnings).To(BeEmpty()) - }) - } -} - func TestMachineDeploymentTemplateMetadataValidation(t *testing.T) { tests := []struct { name string @@ -596,28 +575,35 @@ func TestMachineDeploymentTemplateMetadataValidation(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - md := &MachineDeployment{ - Spec: MachineDeploymentSpec{ - Template: MachineTemplateSpec{ - ObjectMeta: ObjectMeta{ + md := &clusterv1.MachineDeployment{ + Spec: clusterv1.MachineDeploymentSpec{ + Template: clusterv1.MachineTemplateSpec{ + ObjectMeta: clusterv1.ObjectMeta{ Labels: tt.labels, Annotations: tt.annotations, }, }, }, } + + scheme := runtime.NewScheme() + g.Expect(clusterv1.AddToScheme(scheme)).To(Succeed()) + webhook := MachineDeployment{ + Decoder: admission.NewDecoder(scheme), + } + if tt.expectErr { - warnings, err := md.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, md) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = md.ValidateUpdate(md) + warnings, err = webhook.ValidateUpdate(ctx, md, md) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } else { - warnings, err := md.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, md) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = md.ValidateUpdate(md) + warnings, err = webhook.ValidateUpdate(ctx, md, md) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } diff --git a/api/v1beta1/machinehealthcheck_webhook.go b/internal/webhooks/machinehealthcheck.go similarity index 60% rename from api/v1beta1/machinehealthcheck_webhook.go rename to internal/webhooks/machinehealthcheck.go index 44c596feb5ee..a6cbfb95ab93 100644 --- a/api/v1beta1/machinehealthcheck_webhook.go +++ b/internal/webhooks/machinehealthcheck.go @@ -14,9 +14,10 @@ See the License for the specific language governing permissions and limitations under the License. */ -package v1beta1 +package webhooks import ( + "context" "fmt" "time" @@ -28,18 +29,15 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) var ( - // DefaultNodeStartupTimeout is the time allowed for a node to start up. - // Can be made longer as part of spec if required for particular provider. - // 10 minutes should allow the instance to start and the node to join the - // cluster on most providers. - DefaultNodeStartupTimeout = metav1.Duration{Duration: 10 * time.Minute} // Minimum time allowed for a node to start up. minNodeStartupTimeout = metav1.Duration{Duration: 30 * time.Second} // We allow users to disable the nodeStartupTimeout by setting the duration to 0. - disabledNodeStartupTimeout = ZeroDuration + disabledNodeStartupTimeout = clusterv1.ZeroDuration ) // SetMinNodeStartupTimeout allows users to optionally set a custom timeout @@ -51,24 +49,34 @@ func SetMinNodeStartupTimeout(d metav1.Duration) { minNodeStartupTimeout = d } -func (m *MachineHealthCheck) SetupWebhookWithManager(mgr ctrl.Manager) error { +func (webhook *MachineHealthCheck) SetupWebhookWithManager(mgr ctrl.Manager) error { return ctrl.NewWebhookManagedBy(mgr). - For(m). + For(&clusterv1.MachineHealthCheck{}). + WithDefaulter(webhook). + WithValidator(webhook). Complete() } // +kubebuilder:webhook:verbs=create;update,path=/validate-cluster-x-k8s-io-v1beta1-machinehealthcheck,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=cluster.x-k8s.io,resources=machinehealthchecks,versions=v1beta1,name=validation.machinehealthcheck.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1 // +kubebuilder:webhook:verbs=create;update,path=/mutate-cluster-x-k8s-io-v1beta1-machinehealthcheck,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=cluster.x-k8s.io,resources=machinehealthchecks,versions=v1beta1,name=default.machinehealthcheck.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1 -var _ webhook.Defaulter = &MachineHealthCheck{} -var _ webhook.Validator = &MachineHealthCheck{} +// MachineHealthCheck implements a validation and defaulting webhook for MachineHealthCheck. +type MachineHealthCheck struct{} + +var _ webhook.CustomDefaulter = &MachineHealthCheck{} +var _ webhook.CustomValidator = &MachineHealthCheck{} + +// Default implements webhook.CustomDefaulter so a webhook will be registered for the type. +func (webhook *MachineHealthCheck) Default(_ context.Context, obj runtime.Object) error { + m, ok := obj.(*clusterv1.MachineHealthCheck) + if !ok { + return apierrors.NewBadRequest(fmt.Sprintf("expected a MachineHealthCheck but got a %T", obj)) + } -// Default implements webhook.Defaulter so a webhook will be registered for the type. -func (m *MachineHealthCheck) Default() { if m.Labels == nil { m.Labels = make(map[string]string) } - m.Labels[ClusterNameLabel] = m.Spec.ClusterName + m.Labels[clusterv1.ClusterNameLabel] = m.Spec.ClusterName if m.Spec.MaxUnhealthy == nil { defaultMaxUnhealthy := intstr.FromString("100%") @@ -76,43 +84,55 @@ func (m *MachineHealthCheck) Default() { } if m.Spec.NodeStartupTimeout == nil { - m.Spec.NodeStartupTimeout = &DefaultNodeStartupTimeout + m.Spec.NodeStartupTimeout = &clusterv1.DefaultNodeStartupTimeout } if m.Spec.RemediationTemplate != nil && m.Spec.RemediationTemplate.Namespace == "" { m.Spec.RemediationTemplate.Namespace = m.Namespace } + + return nil } -// ValidateCreate implements webhook.Validator so a webhook will be registered for the type. -func (m *MachineHealthCheck) ValidateCreate() (admission.Warnings, error) { - return nil, m.validate(nil) +// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type. +func (webhook *MachineHealthCheck) ValidateCreate(_ context.Context, obj runtime.Object) (admission.Warnings, error) { + m, ok := obj.(*clusterv1.MachineHealthCheck) + if !ok { + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a MachineHealthCheck but got a %T", obj)) + } + + return nil, webhook.validate(nil, m) } -// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type. -func (m *MachineHealthCheck) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { - mhc, ok := old.(*MachineHealthCheck) +// ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type. +func (webhook *MachineHealthCheck) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { + oldM, ok := oldObj.(*clusterv1.MachineHealthCheck) if !ok { - return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a MachineHealthCheck but got a %T", old)) + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a MachineHealthCheck but got a %T", oldObj)) } - return nil, m.validate(mhc) + newM, ok := newObj.(*clusterv1.MachineHealthCheck) + if !ok { + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a MachineHealthCheck but got a %T", newObj)) + } + + return nil, webhook.validate(oldM, newM) } -// ValidateDelete implements webhook.Validator so a webhook will be registered for the type. -func (m *MachineHealthCheck) ValidateDelete() (admission.Warnings, error) { +// ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type. +func (webhook *MachineHealthCheck) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) { return nil, nil } -func (m *MachineHealthCheck) validate(old *MachineHealthCheck) error { +func (webhook *MachineHealthCheck) validate(oldMHC, newMHC *clusterv1.MachineHealthCheck) error { var allErrs field.ErrorList specPath := field.NewPath("spec") // Validate selector parses as Selector - selector, err := metav1.LabelSelectorAsSelector(&m.Spec.Selector) + selector, err := metav1.LabelSelectorAsSelector(&newMHC.Spec.Selector) if err != nil { allErrs = append( allErrs, - field.Invalid(specPath.Child("selector"), m.Spec.Selector, err.Error()), + field.Invalid(specPath.Child("selector"), newMHC.Spec.Selector, err.Error()), ) } @@ -124,30 +144,30 @@ func (m *MachineHealthCheck) validate(old *MachineHealthCheck) error { ) } - if clusterName, ok := m.Spec.Selector.MatchLabels[ClusterNameLabel]; ok && clusterName != m.Spec.ClusterName { + if clusterName, ok := newMHC.Spec.Selector.MatchLabels[clusterv1.ClusterNameLabel]; ok && clusterName != newMHC.Spec.ClusterName { allErrs = append( allErrs, - field.Invalid(specPath.Child("selector"), m.Spec.Selector, "cannot specify a cluster selector other than the one specified by ClusterName")) + field.Invalid(specPath.Child("selector"), newMHC.Spec.Selector, "cannot specify a cluster selector other than the one specified by ClusterName")) } - if old != nil && old.Spec.ClusterName != m.Spec.ClusterName { + if oldMHC != nil && oldMHC.Spec.ClusterName != newMHC.Spec.ClusterName { allErrs = append( allErrs, field.Forbidden(specPath.Child("clusterName"), "field is immutable"), ) } - allErrs = append(allErrs, m.ValidateCommonFields(specPath)...) + allErrs = append(allErrs, webhook.validateCommonFields(newMHC, specPath)...) if len(allErrs) == 0 { return nil } - return apierrors.NewInvalid(GroupVersion.WithKind("MachineHealthCheck").GroupKind(), m.Name, allErrs) + return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("MachineHealthCheck").GroupKind(), newMHC.Name, allErrs) } // ValidateCommonFields validates UnhealthyConditions NodeStartupTimeout, MaxUnhealthy, and RemediationTemplate of the MHC. // These are the fields in common with other types which define MachineHealthChecks such as MachineHealthCheckClass and MachineHealthCheckTopology. -func (m *MachineHealthCheck) ValidateCommonFields(fldPath *field.Path) field.ErrorList { +func (webhook *MachineHealthCheck) validateCommonFields(m *clusterv1.MachineHealthCheck, fldPath *field.Path) field.ErrorList { var allErrs field.ErrorList if m.Spec.NodeStartupTimeout != nil && diff --git a/api/v1beta1/machinehealthcheck_webhook_test.go b/internal/webhooks/machinehealthcheck_test.go similarity index 72% rename from api/v1beta1/machinehealthcheck_webhook_test.go rename to internal/webhooks/machinehealthcheck_test.go index c56e5bce3a4a..d05fe57938b4 100644 --- a/api/v1beta1/machinehealthcheck_webhook_test.go +++ b/internal/webhooks/machinehealthcheck_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package v1beta1 +package webhooks import ( "testing" @@ -25,21 +25,22 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" - utildefaulting "sigs.k8s.io/cluster-api/util/defaulting" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + "sigs.k8s.io/cluster-api/internal/webhooks/util" ) func TestMachineHealthCheckDefault(t *testing.T) { g := NewWithT(t) - mhc := &MachineHealthCheck{ + mhc := &clusterv1.MachineHealthCheck{ ObjectMeta: metav1.ObjectMeta{ Namespace: "foo", }, - Spec: MachineHealthCheckSpec{ + Spec: clusterv1.MachineHealthCheckSpec{ Selector: metav1.LabelSelector{ MatchLabels: map[string]string{"foo": "bar"}, }, RemediationTemplate: &corev1.ObjectReference{}, - UnhealthyConditions: []UnhealthyCondition{ + UnhealthyConditions: []clusterv1.UnhealthyCondition{ { Type: corev1.NodeReady, Status: corev1.ConditionFalse, @@ -47,10 +48,12 @@ func TestMachineHealthCheckDefault(t *testing.T) { }, }, } - t.Run("for MachineHealthCheck", utildefaulting.DefaultValidateTest(mhc)) - mhc.Default() + webhook := &MachineHealthCheck{} - g.Expect(mhc.Labels[ClusterNameLabel]).To(Equal(mhc.Spec.ClusterName)) + t.Run("for MachineHealthCheck", util.CustomDefaultValidateTest(ctx, mhc, webhook)) + g.Expect(webhook.Default(ctx, mhc)).To(Succeed()) + + g.Expect(mhc.Labels[clusterv1.ClusterNameLabel]).To(Equal(mhc.Spec.ClusterName)) g.Expect(mhc.Spec.MaxUnhealthy.String()).To(Equal("100%")) g.Expect(mhc.Spec.NodeStartupTimeout).ToNot(BeNil()) g.Expect(*mhc.Spec.NodeStartupTimeout).To(Equal(metav1.Duration{Duration: 10 * time.Minute})) @@ -78,12 +81,12 @@ func TestMachineHealthCheckLabelSelectorAsSelectorValidation(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - mhc := &MachineHealthCheck{ - Spec: MachineHealthCheckSpec{ + mhc := &clusterv1.MachineHealthCheck{ + Spec: clusterv1.MachineHealthCheckSpec{ Selector: metav1.LabelSelector{ MatchLabels: tt.selectors, }, - UnhealthyConditions: []UnhealthyCondition{ + UnhealthyConditions: []clusterv1.UnhealthyCondition{ { Type: corev1.NodeReady, Status: corev1.ConditionFalse, @@ -91,18 +94,20 @@ func TestMachineHealthCheckLabelSelectorAsSelectorValidation(t *testing.T) { }, }, } + webhook := &MachineHealthCheck{} + if tt.expectErr { - warnings, err := mhc.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, mhc) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = mhc.ValidateUpdate(mhc) + warnings, err = webhook.ValidateUpdate(ctx, mhc, mhc) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } else { - warnings, err := mhc.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, mhc) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = mhc.ValidateUpdate(mhc) + warnings, err = webhook.ValidateUpdate(ctx, mhc, mhc) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } @@ -135,15 +140,15 @@ func TestMachineHealthCheckClusterNameImmutable(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - newMHC := &MachineHealthCheck{ - Spec: MachineHealthCheckSpec{ + newMHC := &clusterv1.MachineHealthCheck{ + Spec: clusterv1.MachineHealthCheckSpec{ ClusterName: tt.newClusterName, Selector: metav1.LabelSelector{ MatchLabels: map[string]string{ "test": "test", }, }, - UnhealthyConditions: []UnhealthyCondition{ + UnhealthyConditions: []clusterv1.UnhealthyCondition{ { Type: corev1.NodeReady, Status: corev1.ConditionFalse, @@ -151,15 +156,15 @@ func TestMachineHealthCheckClusterNameImmutable(t *testing.T) { }, }, } - oldMHC := &MachineHealthCheck{ - Spec: MachineHealthCheckSpec{ + oldMHC := &clusterv1.MachineHealthCheck{ + Spec: clusterv1.MachineHealthCheckSpec{ ClusterName: tt.oldClusterName, Selector: metav1.LabelSelector{ MatchLabels: map[string]string{ "test": "test", }, }, - UnhealthyConditions: []UnhealthyCondition{ + UnhealthyConditions: []clusterv1.UnhealthyCondition{ { Type: corev1.NodeReady, Status: corev1.ConditionFalse, @@ -168,7 +173,7 @@ func TestMachineHealthCheckClusterNameImmutable(t *testing.T) { }, } - warnings, err := newMHC.ValidateUpdate(oldMHC) + warnings, err := (&MachineHealthCheck{}).ValidateUpdate(ctx, oldMHC, newMHC) if tt.expectErr { g.Expect(err).To(HaveOccurred()) } else { @@ -182,12 +187,12 @@ func TestMachineHealthCheckClusterNameImmutable(t *testing.T) { func TestMachineHealthCheckUnhealthyConditions(t *testing.T) { tests := []struct { name string - unhealthConditions []UnhealthyCondition + unhealthConditions []clusterv1.UnhealthyCondition expectErr bool }{ { name: "pass with correctly defined unhealthyConditions", - unhealthConditions: []UnhealthyCondition{ + unhealthConditions: []clusterv1.UnhealthyCondition{ { Type: corev1.NodeReady, Status: corev1.ConditionFalse, @@ -202,7 +207,7 @@ func TestMachineHealthCheckUnhealthyConditions(t *testing.T) { }, { name: "fail if the UnhealthCondition array is empty", - unhealthConditions: []UnhealthyCondition{}, + unhealthConditions: []clusterv1.UnhealthyCondition{}, expectErr: true, }, } @@ -210,8 +215,8 @@ func TestMachineHealthCheckUnhealthyConditions(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - mhc := &MachineHealthCheck{ - Spec: MachineHealthCheckSpec{ + mhc := &clusterv1.MachineHealthCheck{ + Spec: clusterv1.MachineHealthCheckSpec{ Selector: metav1.LabelSelector{ MatchLabels: map[string]string{ "test": "test", @@ -220,18 +225,20 @@ func TestMachineHealthCheckUnhealthyConditions(t *testing.T) { UnhealthyConditions: tt.unhealthConditions, }, } + webhook := &MachineHealthCheck{} + if tt.expectErr { - warnings, err := mhc.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, mhc) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = mhc.ValidateUpdate(mhc) + warnings, err = webhook.ValidateUpdate(ctx, mhc, mhc) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } else { - warnings, err := mhc.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, mhc) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = mhc.ValidateUpdate(mhc) + warnings, err = webhook.ValidateUpdate(ctx, mhc, mhc) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } @@ -286,15 +293,15 @@ func TestMachineHealthCheckNodeStartupTimeout(t *testing.T) { for _, tt := range tests { g := NewWithT(t) - mhc := &MachineHealthCheck{ - Spec: MachineHealthCheckSpec{ + mhc := &clusterv1.MachineHealthCheck{ + Spec: clusterv1.MachineHealthCheckSpec{ NodeStartupTimeout: tt.timeout, Selector: metav1.LabelSelector{ MatchLabels: map[string]string{ "test": "test", }, }, - UnhealthyConditions: []UnhealthyCondition{ + UnhealthyConditions: []clusterv1.UnhealthyCondition{ { Type: corev1.NodeReady, Status: corev1.ConditionFalse, @@ -302,19 +309,20 @@ func TestMachineHealthCheckNodeStartupTimeout(t *testing.T) { }, }, } + webhook := &MachineHealthCheck{} if tt.expectErr { - warnings, err := mhc.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, mhc) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = mhc.ValidateUpdate(mhc) + warnings, err = webhook.ValidateUpdate(ctx, mhc, mhc) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } else { - warnings, err := mhc.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, mhc) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = mhc.ValidateUpdate(mhc) + warnings, err = webhook.ValidateUpdate(ctx, mhc, mhc) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } @@ -353,15 +361,15 @@ func TestMachineHealthCheckMaxUnhealthy(t *testing.T) { g := NewWithT(t) maxUnhealthy := tt.value - mhc := &MachineHealthCheck{ - Spec: MachineHealthCheckSpec{ + mhc := &clusterv1.MachineHealthCheck{ + Spec: clusterv1.MachineHealthCheckSpec{ MaxUnhealthy: &maxUnhealthy, Selector: metav1.LabelSelector{ MatchLabels: map[string]string{ "test": "test", }, }, - UnhealthyConditions: []UnhealthyCondition{ + UnhealthyConditions: []clusterv1.UnhealthyCondition{ { Type: corev1.NodeReady, Status: corev1.ConditionFalse, @@ -369,19 +377,20 @@ func TestMachineHealthCheckMaxUnhealthy(t *testing.T) { }, }, } + webhook := &MachineHealthCheck{} if tt.expectErr { - warnings, err := mhc.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, mhc) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = mhc.ValidateUpdate(mhc) + warnings, err = webhook.ValidateUpdate(ctx, mhc, mhc) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } else { - warnings, err := mhc.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, mhc) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = mhc.ValidateUpdate(mhc) + warnings, err = webhook.ValidateUpdate(ctx, mhc, mhc) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } @@ -390,9 +399,9 @@ func TestMachineHealthCheckMaxUnhealthy(t *testing.T) { func TestMachineHealthCheckSelectorValidation(t *testing.T) { g := NewWithT(t) - mhc := &MachineHealthCheck{ - Spec: MachineHealthCheckSpec{ - UnhealthyConditions: []UnhealthyCondition{ + mhc := &clusterv1.MachineHealthCheck{ + Spec: clusterv1.MachineHealthCheckSpec{ + UnhealthyConditions: []clusterv1.UnhealthyCondition{ { Type: corev1.NodeReady, Status: corev1.ConditionFalse, @@ -400,23 +409,25 @@ func TestMachineHealthCheckSelectorValidation(t *testing.T) { }, }, } - err := mhc.validate(nil) + webhook := &MachineHealthCheck{} + + err := webhook.validate(nil, mhc) g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring("selector must not be empty")) } func TestMachineHealthCheckClusterNameSelectorValidation(t *testing.T) { g := NewWithT(t) - mhc := &MachineHealthCheck{ - Spec: MachineHealthCheckSpec{ + mhc := &clusterv1.MachineHealthCheck{ + Spec: clusterv1.MachineHealthCheckSpec{ ClusterName: "foo", Selector: metav1.LabelSelector{ MatchLabels: map[string]string{ - ClusterNameLabel: "bar", - "baz": "qux", + clusterv1.ClusterNameLabel: "bar", + "baz": "qux", }, }, - UnhealthyConditions: []UnhealthyCondition{ + UnhealthyConditions: []clusterv1.UnhealthyCondition{ { Type: corev1.NodeReady, Status: corev1.ConditionFalse, @@ -424,25 +435,27 @@ func TestMachineHealthCheckClusterNameSelectorValidation(t *testing.T) { }, }, } - err := mhc.validate(nil) + webhook := &MachineHealthCheck{} + + err := webhook.validate(nil, mhc) g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring("cannot specify a cluster selector other than the one specified by ClusterName")) - mhc.Spec.Selector.MatchLabels[ClusterNameLabel] = "foo" - g.Expect(mhc.validate(nil)).To(Succeed()) - delete(mhc.Spec.Selector.MatchLabels, ClusterNameLabel) - g.Expect(mhc.validate(nil)).To(Succeed()) + mhc.Spec.Selector.MatchLabels[clusterv1.ClusterNameLabel] = "foo" + g.Expect(webhook.validate(nil, mhc)).To(Succeed()) + delete(mhc.Spec.Selector.MatchLabels, clusterv1.ClusterNameLabel) + g.Expect(webhook.validate(nil, mhc)).To(Succeed()) } func TestMachineHealthCheckRemediationTemplateNamespaceValidation(t *testing.T) { - valid := &MachineHealthCheck{ + valid := &clusterv1.MachineHealthCheck{ ObjectMeta: metav1.ObjectMeta{ Namespace: "foo", }, - Spec: MachineHealthCheckSpec{ + Spec: clusterv1.MachineHealthCheckSpec{ Selector: metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, RemediationTemplate: &corev1.ObjectReference{Namespace: "foo"}, - UnhealthyConditions: []UnhealthyCondition{ + UnhealthyConditions: []clusterv1.UnhealthyCondition{ { Type: corev1.NodeReady, Status: corev1.ConditionFalse, @@ -456,7 +469,7 @@ func TestMachineHealthCheckRemediationTemplateNamespaceValidation(t *testing.T) tests := []struct { name string expectErr bool - c *MachineHealthCheck + c *clusterv1.MachineHealthCheck }{ { name: "should return error when MachineHealthCheck namespace and RemediationTemplate ref namespace mismatch", @@ -473,11 +486,12 @@ func TestMachineHealthCheckRemediationTemplateNamespaceValidation(t *testing.T) for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) + webhook := &MachineHealthCheck{} if tt.expectErr { - g.Expect(tt.c.validate(nil)).NotTo(Succeed()) + g.Expect(webhook.validate(nil, tt.c)).NotTo(Succeed()) } else { - g.Expect(tt.c.validate(nil)).To(Succeed()) + g.Expect(webhook.validate(nil, tt.c)).To(Succeed()) } }) } diff --git a/api/v1beta1/machineset_webhook.go b/internal/webhooks/machineset.go similarity index 57% rename from api/v1beta1/machineset_webhook.go rename to internal/webhooks/machineset.go index d9ea53dc3b63..b9156544aa3f 100644 --- a/api/v1beta1/machineset_webhook.go +++ b/internal/webhooks/machineset.go @@ -14,9 +14,10 @@ See the License for the specific language governing permissions and limitations under the License. */ -package v1beta1 +package webhooks import ( + "context" "fmt" "strings" @@ -31,32 +32,43 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/feature" "sigs.k8s.io/cluster-api/util/labels/format" "sigs.k8s.io/cluster-api/util/version" ) -func (m *MachineSet) SetupWebhookWithManager(mgr ctrl.Manager) error { +func (webhook *MachineSet) SetupWebhookWithManager(mgr ctrl.Manager) error { return ctrl.NewWebhookManagedBy(mgr). - For(m). + For(&clusterv1.MachineSet{}). + WithDefaulter(webhook). + WithValidator(webhook). Complete() } // +kubebuilder:webhook:verbs=create;update,path=/validate-cluster-x-k8s-io-v1beta1-machineset,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=cluster.x-k8s.io,resources=machinesets,versions=v1beta1,name=validation.machineset.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1 // +kubebuilder:webhook:verbs=create;update,path=/mutate-cluster-x-k8s-io-v1beta1-machineset,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=cluster.x-k8s.io,resources=machinesets,versions=v1beta1,name=default.machineset.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1 -var _ webhook.Defaulter = &MachineSet{} -var _ webhook.Validator = &MachineSet{} +// MachineSet implements a validation and defaulting webhook for MachineSet. +type MachineSet struct{} + +var _ webhook.CustomDefaulter = &MachineSet{} +var _ webhook.CustomValidator = &MachineSet{} // Default sets default MachineSet field values. -func (m *MachineSet) Default() { +func (webhook *MachineSet) Default(_ context.Context, obj runtime.Object) error { + m, ok := obj.(*clusterv1.MachineSet) + if !ok { + return apierrors.NewBadRequest(fmt.Sprintf("expected a MachineSet but got a %T", obj)) + } + if m.Labels == nil { m.Labels = make(map[string]string) } - m.Labels[ClusterNameLabel] = m.Spec.ClusterName + m.Labels[clusterv1.ClusterNameLabel] = m.Spec.ClusterName if m.Spec.DeletePolicy == "" { - randomPolicy := string(RandomMachineSetDeletePolicy) + randomPolicy := string(clusterv1.RandomMachineSetDeletePolicy) m.Spec.DeletePolicy = randomPolicy } @@ -70,66 +82,78 @@ func (m *MachineSet) Default() { if len(m.Spec.Selector.MatchLabels) == 0 && len(m.Spec.Selector.MatchExpressions) == 0 { // Note: MustFormatValue is used here as the value of this label will be a hash if the MachineSet name is longer than 63 characters. - m.Spec.Selector.MatchLabels[MachineSetNameLabel] = format.MustFormatValue(m.Name) - m.Spec.Template.Labels[MachineSetNameLabel] = format.MustFormatValue(m.Name) + m.Spec.Selector.MatchLabels[clusterv1.MachineSetNameLabel] = format.MustFormatValue(m.Name) + m.Spec.Template.Labels[clusterv1.MachineSetNameLabel] = format.MustFormatValue(m.Name) } if m.Spec.Template.Spec.Version != nil && !strings.HasPrefix(*m.Spec.Template.Spec.Version, "v") { normalizedVersion := "v" + *m.Spec.Template.Spec.Version m.Spec.Template.Spec.Version = &normalizedVersion } + + return nil } // ValidateCreate implements webhook.Validator so a webhook will be registered for the type. -func (m *MachineSet) ValidateCreate() (admission.Warnings, error) { - return nil, m.validate(nil) +func (webhook *MachineSet) ValidateCreate(_ context.Context, obj runtime.Object) (admission.Warnings, error) { + m, ok := obj.(*clusterv1.MachineSet) + if !ok { + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a MachineSet but got a %T", obj)) + } + + return nil, webhook.validate(nil, m) } // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type. -func (m *MachineSet) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { - oldMS, ok := old.(*MachineSet) +func (webhook *MachineSet) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { + oldMS, ok := oldObj.(*clusterv1.MachineSet) if !ok { - return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a MachineSet but got a %T", old)) + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a MachineSet but got a %T", oldObj)) } - return nil, m.validate(oldMS) + newMS, ok := newObj.(*clusterv1.MachineSet) + if !ok { + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a MachineSet but got a %T", newObj)) + } + + return nil, webhook.validate(oldMS, newMS) } // ValidateDelete implements webhook.Validator so a webhook will be registered for the type. -func (m *MachineSet) ValidateDelete() (admission.Warnings, error) { +func (webhook *MachineSet) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) { return nil, nil } -func (m *MachineSet) validate(old *MachineSet) error { +func (webhook *MachineSet) validate(oldMS, newMS *clusterv1.MachineSet) error { var allErrs field.ErrorList specPath := field.NewPath("spec") - selector, err := metav1.LabelSelectorAsSelector(&m.Spec.Selector) + selector, err := metav1.LabelSelectorAsSelector(&newMS.Spec.Selector) if err != nil { allErrs = append( allErrs, field.Invalid( specPath.Child("selector"), - m.Spec.Selector, + newMS.Spec.Selector, err.Error(), ), ) - } else if !selector.Matches(labels.Set(m.Spec.Template.Labels)) { + } else if !selector.Matches(labels.Set(newMS.Spec.Template.Labels)) { allErrs = append( allErrs, field.Invalid( specPath.Child("template", "metadata", "labels"), - m.Spec.Template.ObjectMeta.Labels, + newMS.Spec.Template.ObjectMeta.Labels, fmt.Sprintf("must match spec.selector %q", selector.String()), ), ) } if feature.Gates.Enabled(feature.MachineSetPreflightChecks) { - if err := validateSkippedMachineSetPreflightChecks(m); err != nil { + if err := validateSkippedMachineSetPreflightChecks(newMS); err != nil { allErrs = append(allErrs, err) } } - if old != nil && old.Spec.ClusterName != m.Spec.ClusterName { + if oldMS != nil && oldMS.Spec.ClusterName != newMS.Spec.ClusterName { allErrs = append( allErrs, field.Forbidden( @@ -139,13 +163,13 @@ func (m *MachineSet) validate(old *MachineSet) error { ) } - if m.Spec.Template.Spec.Version != nil { - if !version.KubeSemver.MatchString(*m.Spec.Template.Spec.Version) { + if newMS.Spec.Template.Spec.Version != nil { + if !version.KubeSemver.MatchString(*newMS.Spec.Template.Spec.Version) { allErrs = append( allErrs, field.Invalid( specPath.Child("template", "spec", "version"), - *m.Spec.Template.Spec.Version, + *newMS.Spec.Template.Spec.Version, "must be a valid semantic version", ), ) @@ -153,42 +177,42 @@ func (m *MachineSet) validate(old *MachineSet) error { } // Validate the metadata of the template. - allErrs = append(allErrs, m.Spec.Template.ObjectMeta.Validate(specPath.Child("template", "metadata"))...) + allErrs = append(allErrs, newMS.Spec.Template.ObjectMeta.Validate(specPath.Child("template", "metadata"))...) if len(allErrs) == 0 { return nil } - return apierrors.NewInvalid(GroupVersion.WithKind("MachineSet").GroupKind(), m.Name, allErrs) + return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("MachineSet").GroupKind(), newMS.Name, allErrs) } func validateSkippedMachineSetPreflightChecks(o client.Object) *field.Error { if o == nil { return nil } - skip := o.GetAnnotations()[MachineSetSkipPreflightChecksAnnotation] + skip := o.GetAnnotations()[clusterv1.MachineSetSkipPreflightChecksAnnotation] if skip == "" { return nil } - supported := sets.New[MachineSetPreflightCheck]( - MachineSetPreflightCheckAll, - MachineSetPreflightCheckKubeadmVersionSkew, - MachineSetPreflightCheckKubernetesVersionSkew, - MachineSetPreflightCheckControlPlaneIsStable, + supported := sets.New[clusterv1.MachineSetPreflightCheck]( + clusterv1.MachineSetPreflightCheckAll, + clusterv1.MachineSetPreflightCheckKubeadmVersionSkew, + clusterv1.MachineSetPreflightCheckKubernetesVersionSkew, + clusterv1.MachineSetPreflightCheckControlPlaneIsStable, ) skippedList := strings.Split(skip, ",") - invalid := []MachineSetPreflightCheck{} + invalid := []clusterv1.MachineSetPreflightCheck{} for i := range skippedList { - skipped := MachineSetPreflightCheck(strings.TrimSpace(skippedList[i])) + skipped := clusterv1.MachineSetPreflightCheck(strings.TrimSpace(skippedList[i])) if !supported.Has(skipped) { invalid = append(invalid, skipped) } } if len(invalid) > 0 { return field.Invalid( - field.NewPath("metadata", "annotations", MachineSetSkipPreflightChecksAnnotation), + field.NewPath("metadata", "annotations", clusterv1.MachineSetSkipPreflightChecksAnnotation), invalid, fmt.Sprintf("skipped preflight check(s) must be among: %v", sets.List(supported)), ) diff --git a/api/v1beta1/machineset_webhook_test.go b/internal/webhooks/machineset_test.go similarity index 71% rename from api/v1beta1/machineset_webhook_test.go rename to internal/webhooks/machineset_test.go index e5d6ef89a36a..8ee8ab916601 100644 --- a/api/v1beta1/machineset_webhook_test.go +++ b/internal/webhooks/machineset_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package v1beta1 +package webhooks import ( "strings" @@ -24,30 +24,33 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/pointer" - utildefaulting "sigs.k8s.io/cluster-api/util/defaulting" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + "sigs.k8s.io/cluster-api/internal/webhooks/util" ) func TestMachineSetDefault(t *testing.T) { g := NewWithT(t) - ms := &MachineSet{ + ms := &clusterv1.MachineSet{ ObjectMeta: metav1.ObjectMeta{ Name: "test-ms", }, - Spec: MachineSetSpec{ - Template: MachineTemplateSpec{ - Spec: MachineSpec{ + Spec: clusterv1.MachineSetSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ Version: pointer.String("1.19.10"), }, }, }, } - t.Run("for MachineSet", utildefaulting.DefaultValidateTest(ms)) - ms.Default() + webhook := &MachineSet{} - g.Expect(ms.Labels[ClusterNameLabel]).To(Equal(ms.Spec.ClusterName)) - g.Expect(ms.Spec.DeletePolicy).To(Equal(string(RandomMachineSetDeletePolicy))) - g.Expect(ms.Spec.Selector.MatchLabels).To(HaveKeyWithValue(MachineSetNameLabel, "test-ms")) - g.Expect(ms.Spec.Template.Labels).To(HaveKeyWithValue(MachineSetNameLabel, "test-ms")) + t.Run("for MachineSet", util.CustomDefaultValidateTest(ctx, ms, webhook)) + g.Expect(webhook.Default(ctx, ms)).To(Succeed()) + + g.Expect(ms.Labels[clusterv1.ClusterNameLabel]).To(Equal(ms.Spec.ClusterName)) + g.Expect(ms.Spec.DeletePolicy).To(Equal(string(clusterv1.RandomMachineSetDeletePolicy))) + g.Expect(ms.Spec.Selector.MatchLabels).To(HaveKeyWithValue(clusterv1.MachineSetNameLabel, "test-ms")) + g.Expect(ms.Spec.Template.Labels).To(HaveKeyWithValue(clusterv1.MachineSetNameLabel, "test-ms")) g.Expect(*ms.Spec.Template.Spec.Version).To(Equal("v1.19.10")) } @@ -93,31 +96,32 @@ func TestMachineSetLabelSelectorMatchValidation(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - ms := &MachineSet{ - Spec: MachineSetSpec{ + ms := &clusterv1.MachineSet{ + Spec: clusterv1.MachineSetSpec{ Selector: metav1.LabelSelector{ MatchLabels: tt.selectors, }, - Template: MachineTemplateSpec{ - ObjectMeta: ObjectMeta{ + Template: clusterv1.MachineTemplateSpec{ + ObjectMeta: clusterv1.ObjectMeta{ Labels: tt.labels, }, }, }, } + webhook := &MachineSet{} if tt.expectErr { - warnings, err := ms.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, ms) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = ms.ValidateUpdate(ms) + warnings, err = webhook.ValidateUpdate(ctx, ms, ms) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } else { - warnings, err := ms.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, ms) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = ms.ValidateUpdate(ms) + warnings, err = webhook.ValidateUpdate(ctx, ms, ms) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } @@ -150,19 +154,19 @@ func TestMachineSetClusterNameImmutable(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - newMS := &MachineSet{ - Spec: MachineSetSpec{ + newMS := &clusterv1.MachineSet{ + Spec: clusterv1.MachineSetSpec{ ClusterName: tt.newClusterName, }, } - oldMS := &MachineSet{ - Spec: MachineSetSpec{ + oldMS := &clusterv1.MachineSet{ + Spec: clusterv1.MachineSetSpec{ ClusterName: tt.oldClusterName, }, } - warnings, err := newMS.ValidateUpdate(oldMS) + warnings, err := (&MachineSet{}).ValidateUpdate(ctx, oldMS, newMS) if tt.expectErr { g.Expect(err).To(HaveOccurred()) } else { @@ -210,28 +214,29 @@ func TestMachineSetVersionValidation(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - md := &MachineSet{ - Spec: MachineSetSpec{ - Template: MachineTemplateSpec{ - Spec: MachineSpec{ + ms := &clusterv1.MachineSet{ + Spec: clusterv1.MachineSetSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ Version: pointer.String(tt.version), }, }, }, } + webhook := &MachineSet{} if tt.expectErr { - warnings, err := md.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, ms) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = md.ValidateUpdate(md) + warnings, err = webhook.ValidateUpdate(ctx, ms, ms) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } else { - warnings, err := md.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, ms) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = md.ValidateUpdate(md) + warnings, err = webhook.ValidateUpdate(ctx, ms, ms) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } @@ -242,20 +247,20 @@ func TestMachineSetVersionValidation(t *testing.T) { func TestValidateSkippedMachineSetPreflightChecks(t *testing.T) { tests := []struct { name string - ms *MachineSet + ms *clusterv1.MachineSet expectErr bool }{ { name: "should pass if the machine set skip preflight checks annotation is not set", - ms: &MachineSet{}, + ms: &clusterv1.MachineSet{}, expectErr: false, }, { name: "should pass if not preflight checks are skipped", - ms: &MachineSet{ + ms: &clusterv1.MachineSet{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - MachineSetSkipPreflightChecksAnnotation: "", + clusterv1.MachineSetSkipPreflightChecksAnnotation: "", }, }, }, @@ -263,10 +268,10 @@ func TestValidateSkippedMachineSetPreflightChecks(t *testing.T) { }, { name: "should pass if only valid preflight checks are skipped (single)", - ms: &MachineSet{ + ms: &clusterv1.MachineSet{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - MachineSetSkipPreflightChecksAnnotation: string(MachineSetPreflightCheckKubeadmVersionSkew), + clusterv1.MachineSetSkipPreflightChecksAnnotation: string(clusterv1.MachineSetPreflightCheckKubeadmVersionSkew), }, }, }, @@ -274,10 +279,10 @@ func TestValidateSkippedMachineSetPreflightChecks(t *testing.T) { }, { name: "should pass if only valid preflight checks are skipped (multiple)", - ms: &MachineSet{ + ms: &clusterv1.MachineSet{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - MachineSetSkipPreflightChecksAnnotation: string(MachineSetPreflightCheckKubeadmVersionSkew) + "," + string(MachineSetPreflightCheckControlPlaneIsStable), + clusterv1.MachineSetSkipPreflightChecksAnnotation: string(clusterv1.MachineSetPreflightCheckKubeadmVersionSkew) + "," + string(clusterv1.MachineSetPreflightCheckControlPlaneIsStable), }, }, }, @@ -285,10 +290,10 @@ func TestValidateSkippedMachineSetPreflightChecks(t *testing.T) { }, { name: "should fail if invalid preflight checks are skipped", - ms: &MachineSet{ + ms: &clusterv1.MachineSet{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - MachineSetSkipPreflightChecksAnnotation: string(MachineSetPreflightCheckKubeadmVersionSkew) + ",invalid-preflight-check-name", + clusterv1.MachineSetSkipPreflightChecksAnnotation: string(clusterv1.MachineSetPreflightCheckKubeadmVersionSkew) + ",invalid-preflight-check-name", }, }, }, @@ -333,28 +338,31 @@ func TestMachineSetTemplateMetadataValidation(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - ms := &MachineSet{ - Spec: MachineSetSpec{ - Template: MachineTemplateSpec{ - ObjectMeta: ObjectMeta{ + ms := &clusterv1.MachineSet{ + Spec: clusterv1.MachineSetSpec{ + Template: clusterv1.MachineTemplateSpec{ + ObjectMeta: clusterv1.ObjectMeta{ Labels: tt.labels, Annotations: tt.annotations, }, }, }, } + + webhook := &MachineSet{} + if tt.expectErr { - warnings, err := ms.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, ms) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = ms.ValidateUpdate(ms) + warnings, err = webhook.ValidateUpdate(ctx, ms, ms) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } else { - warnings, err := ms.ValidateCreate() + warnings, err := webhook.ValidateCreate(ctx, ms) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = ms.ValidateUpdate(ms) + warnings, err = webhook.ValidateUpdate(ctx, ms, ms) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } diff --git a/main.go b/main.go index 2890605f7654..24ca78248271 100644 --- a/main.go +++ b/main.go @@ -574,17 +574,17 @@ func setupWebhooks(mgr ctrl.Manager) { os.Exit(1) } - if err := (&clusterv1.Machine{}).SetupWebhookWithManager(mgr); err != nil { + if err := (&webhooks.Machine{}).SetupWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "Machine") os.Exit(1) } - if err := (&clusterv1.MachineSet{}).SetupWebhookWithManager(mgr); err != nil { + if err := (&webhooks.MachineSet{}).SetupWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "MachineSet") os.Exit(1) } - if err := (&clusterv1.MachineDeployment{}).SetupWebhookWithManager(mgr); err != nil { + if err := (&webhooks.MachineDeployment{}).SetupWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "MachineDeployment") os.Exit(1) } @@ -609,7 +609,7 @@ func setupWebhooks(mgr ctrl.Manager) { os.Exit(1) } - if err := (&clusterv1.MachineHealthCheck{}).SetupWebhookWithManager(mgr); err != nil { + if err := (&webhooks.MachineHealthCheck{}).SetupWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "MachineHealthCheck") os.Exit(1) } diff --git a/webhooks/alias.go b/webhooks/alias.go index 9c1160c95e45..ee0646bdc19e 100644 --- a/webhooks/alias.go +++ b/webhooks/alias.go @@ -19,6 +19,7 @@ package webhooks import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "sigs.k8s.io/cluster-api/internal/webhooks" ) @@ -46,3 +47,39 @@ func (webhook *ClusterClass) SetupWebhookWithManager(mgr ctrl.Manager) error { Client: webhook.Client, }).SetupWebhookWithManager(mgr) } + +// Machine implements a validating and defaulting webhook for Machine. +type Machine struct{} + +// SetupWebhookWithManager sets up Machine webhooks. +func (webhook *Machine) SetupWebhookWithManager(mgr ctrl.Manager) error { + return (&webhooks.Machine{}).SetupWebhookWithManager(mgr) +} + +// MachineDeployment implements a validating and defaulting webhook for MachineDeployment. +type MachineDeployment struct { + Decoder *admission.Decoder +} + +// SetupWebhookWithManager sets up MachineDeployment webhooks. +func (webhook *MachineDeployment) SetupWebhookWithManager(mgr ctrl.Manager) error { + return (&webhooks.MachineDeployment{ + Decoder: webhook.Decoder, + }).SetupWebhookWithManager(mgr) +} + +// MachineSet implements a validating and defaulting webhook for MachineSet. +type MachineSet struct{} + +// SetupWebhookWithManager sets up MachineSet webhooks. +func (webhook *MachineSet) SetupWebhookWithManager(mgr ctrl.Manager) error { + return (&webhooks.MachineSet{}).SetupWebhookWithManager(mgr) +} + +// MachineHealthCheck implements a validating and defaulting webhook for MachineHealthCheck. +type MachineHealthCheck struct{} + +// SetupWebhookWithManager sets up MachineHealthCheck webhooks. +func (webhook *MachineHealthCheck) SetupWebhookWithManager(mgr ctrl.Manager) error { + return (&webhooks.MachineHealthCheck{}).SetupWebhookWithManager(mgr) +}