Skip to content

Commit

Permalink
Move webhooks to a separate package
Browse files Browse the repository at this point in the history
  • Loading branch information
JoelSpeed committed Jul 26, 2023
1 parent ce33c29 commit f09e167
Show file tree
Hide file tree
Showing 18 changed files with 678 additions and 498 deletions.
10 changes: 10 additions & 0 deletions api/v1beta1/machinehealthcheck_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 5 additions & 1 deletion controlplane/kubeadm/internal/controllers/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.
Expand Down
5 changes: 4 additions & 1 deletion internal/controllers/topology/cluster/desired_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -1015,7 +1016,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(context.TODO(), mhc); err != nil {
panic(err)
}

return mhc
}
Expand Down
45 changes: 31 additions & 14 deletions internal/controllers/topology/cluster/reconcile_state_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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 (
Expand Down Expand Up @@ -1551,7 +1554,6 @@ func TestReconcileControlPlaneMachineHealthCheck(t *testing.T) {
InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy(),
MachineHealthCheck: mhcBuilder.Build()},
want: mhcBuilder.DeepCopy().
WithDefaulter(true).
Build(),
},
{
Expand Down Expand Up @@ -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(),
},
{
Expand Down Expand Up @@ -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"}))
})
}
}
Expand Down Expand Up @@ -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",
Expand All @@ -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{
Expand All @@ -2605,7 +2609,6 @@ func TestReconcileMachineDeploymentMachineHealthCheck(t *testing.T) {
want: []*clusterv1.MachineHealthCheck{
mhcBuilder.DeepCopy().
WithMaxUnhealthy(&maxUnhealthy).
WithDefaulter(true).
Build()},
},
{
Expand All @@ -2619,7 +2622,7 @@ func TestReconcileMachineDeploymentMachineHealthCheck(t *testing.T) {
mhcBuilder.DeepCopy().Build()),
},
want: []*clusterv1.MachineHealthCheck{
mhcBuilder.DeepCopy().WithDefaulter(true).Build(),
mhcBuilder.DeepCopy().Build(),
},
},
{
Expand Down Expand Up @@ -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
Expand All @@ -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).
Expand All @@ -2739,12 +2745,18 @@ 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.MachineDeploymentWebhook(scheme)).Default(admission.NewContextWithRequest(ctx, admission.Request{}), mdState.Object); err != nil {
panic(err)
}
return mdState
}

func toMachineDeploymentTopologyStateMap(states []*scope.MachineDeploymentState) map[string]*scope.MachineDeploymentState {
Expand Down Expand Up @@ -2779,7 +2791,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",
Expand All @@ -2798,14 +2810,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",
Expand Down Expand Up @@ -2870,7 +2882,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"}))
})
}
}
Expand Down
36 changes: 2 additions & 34 deletions internal/test/builder/builders.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +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/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"
Expand Down Expand Up @@ -1150,7 +1147,6 @@ type MachineDeploymentBuilder struct {
selector *metav1.LabelSelector
version *string
replicas *int32
defaulter bool
generation *int64
labels map[string]string
status *clusterv1.MachineDeploymentStatus
Expand Down Expand Up @@ -1206,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
Expand Down Expand Up @@ -1267,20 +1257,7 @@ func (m *MachineDeploymentBuilder) Build() *clusterv1.MachineDeployment {
clusterv1.ClusterNameLabel: m.clusterName,
}
}
if m.defaulter {
scheme, err := clusterv1.SchemeBuilder.Build()
if 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
}

Expand Down Expand Up @@ -1499,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.
Expand Down Expand Up @@ -1540,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
Expand All @@ -1569,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
}
12 changes: 6 additions & 6 deletions internal/test/envtest/environment.go
Original file line number Diff line number Diff line change
Expand Up @@ -267,27 +267,27 @@ 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})
webhooks.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)
}
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 {
Expand Down
2 changes: 1 addition & 1 deletion internal/webhooks/clusterclass.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading

0 comments on commit f09e167

Please sign in to comment.