Skip to content

Commit 0133c47

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #3210 from willie-yao/custom-defaulter-validator
Replace util/webhook code with CustomDefaulter/CustomValidator
2 parents 17332db + bdbff7e commit 0133c47

14 files changed

+277
-297
lines changed

‎api/v1beta1/azuremachine_default.go‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,10 @@ import (
2525
"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-11-01/compute"
2626
"github.com/pkg/errors"
2727
"golang.org/x/crypto/ssh"
28+
kerrors "k8s.io/apimachinery/pkg/util/errors"
2829
"k8s.io/apimachinery/pkg/util/uuid"
2930
utilSSH "sigs.k8s.io/cluster-api-provider-azure/util/ssh"
3031
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
31-
ctrl "sigs.k8s.io/controller-runtime"
3232
"sigs.k8s.io/controller-runtime/pkg/client"
3333
)
3434

@@ -200,20 +200,21 @@ func GetSubscriptionID(cli client.Client, clusterName string, namespace string,
200200
}
201201

202202
// SetDefaults sets to the defaults for the AzureMachineSpec.
203-
func (m *AzureMachine) SetDefaults(client client.Client) {
203+
func (m *AzureMachine) SetDefaults(client client.Client) error {
204+
var errs []error
204205
if err := m.Spec.SetDefaultSSHPublicKey(); err != nil {
205-
ctrl.Log.WithName("SetDefault").Error(err, "failed to set default SSH public key")
206+
errs = append(errs, errors.Wrap(err, "failed to set default SSH public key"))
206207
}
207208

208209
// Fetch the Cluster.
209210
clusterName, ok := m.Labels[clusterv1.ClusterLabelName]
210211
if !ok {
211-
ctrl.Log.WithName("SetDefault").Error(errors.Errorf("failed to fetch owner ClusterName for AzureMachine %s/%s", m.Namespace, m.Name), "failed to fetch ClusterName")
212+
errs = append(errs, errors.Errorf("failed to fetch ClusterName for AzureMachine %s/%s", m.Namespace, m.Name))
212213
}
213214

214215
subscriptionID, err := GetSubscriptionID(client, clusterName, m.Namespace, 5)
215216
if err != nil {
216-
ctrl.Log.WithName("SetDefault").Error(err, "failed to fetch subscription ID for AzureMachine %s/%s", m.Namespace, m.Name)
217+
errs = append(errs, errors.Wrapf(err, "failed to fetch subscription ID for AzureMachine %s/%s", m.Namespace, m.Name))
217218
}
218219

219220
m.Spec.SetDefaultCachingType()
@@ -222,4 +223,6 @@ func (m *AzureMachine) SetDefaults(client client.Client) {
222223
m.Spec.SetSpotEvictionPolicyDefaults()
223224
m.Spec.SetDiagnosticsDefaults()
224225
m.Spec.SetNetworkInterfacesDefaults()
226+
227+
return kerrors.NewAggregate(errs)
225228
}

‎api/v1beta1/azuremachine_default_test.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,9 @@ import (
2626
"github.com/google/uuid"
2727
. "github.com/onsi/gomega"
2828
"github.com/pkg/errors"
29+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2930
"k8s.io/utils/pointer"
31+
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
3032
"sigs.k8s.io/controller-runtime/pkg/client"
3133
)
3234

@@ -472,6 +474,11 @@ func createMachineWithUserAssignedIdentities(identitiesList []UserAssignedIdenti
472474

473475
func hardcodedAzureMachineWithSSHKey(sshPublicKey string) *AzureMachine {
474476
return &AzureMachine{
477+
ObjectMeta: metav1.ObjectMeta{
478+
Labels: map[string]string{
479+
clusterv1.ClusterLabelName: "test-cluster",
480+
},
481+
},
475482
Spec: AzureMachineSpec{
476483
SSHPublicKey: sshPublicKey,
477484
OSDisk: generateValidOSDisk(),

‎api/v1beta1/azuremachine_webhook.go‎

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,20 +17,41 @@ limitations under the License.
1717
package v1beta1
1818

1919
import (
20+
"context"
2021
"reflect"
2122

2223
apierrors "k8s.io/apimachinery/pkg/api/errors"
2324
"k8s.io/apimachinery/pkg/runtime"
2425
"k8s.io/apimachinery/pkg/util/validation/field"
2526
webhookutils "sigs.k8s.io/cluster-api-provider-azure/util/webhook"
27+
ctrl "sigs.k8s.io/controller-runtime"
2628
"sigs.k8s.io/controller-runtime/pkg/client"
2729
)
2830

31+
// SetupAzureMachineWebhookWithManager sets up and registers the webhook with the manager.
32+
func SetupAzureMachineWebhookWithManager(mgr ctrl.Manager) error {
33+
mw := &azureMachineWebhook{Client: mgr.GetClient()}
34+
return ctrl.NewWebhookManagedBy(mgr).
35+
For(&AzureMachine{}).
36+
WithDefaulter(mw).
37+
WithValidator(mw).
38+
Complete()
39+
}
40+
2941
// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1beta1-azuremachine,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=azuremachines,versions=v1beta1,name=validation.azuremachine.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1
3042
// +kubebuilder:webhook:verbs=create;update,path=/mutate-infrastructure-cluster-x-k8s-io-v1beta1-azuremachine,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=azuremachines,versions=v1beta1,name=default.azuremachine.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1
3143

44+
// azureMachineWebhook implements a validating and defaulting webhook for AzureMachines.
45+
type azureMachineWebhook struct {
46+
Client client.Client
47+
}
48+
3249
// ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
33-
func (m *AzureMachine) ValidateCreate(client client.Client) error {
50+
func (mw *azureMachineWebhook) ValidateCreate(ctx context.Context, obj runtime.Object) error {
51+
m, ok := obj.(*AzureMachine)
52+
if !ok {
53+
return apierrors.NewBadRequest("expected an AzureMachine resource")
54+
}
3455
spec := m.Spec
3556

3657
allErrs := ValidateAzureMachineSpec(spec)
@@ -52,9 +73,16 @@ func (m *AzureMachine) ValidateCreate(client client.Client) error {
5273
}
5374

5475
// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
55-
func (m *AzureMachine) ValidateUpdate(oldRaw runtime.Object, client client.Client) error {
76+
func (mw *azureMachineWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) error {
5677
var allErrs field.ErrorList
57-
old := oldRaw.(*AzureMachine)
78+
old, ok := oldObj.(*AzureMachine)
79+
if !ok {
80+
return apierrors.NewBadRequest("expected an AzureMachine resource")
81+
}
82+
m, ok := newObj.(*AzureMachine)
83+
if !ok {
84+
return apierrors.NewBadRequest("expected an AzureMachine resource")
85+
}
5886

5987
if err := webhookutils.ValidateImmutable(
6088
field.NewPath("Spec", "Image"),
@@ -181,11 +209,15 @@ func (m *AzureMachine) ValidateUpdate(oldRaw runtime.Object, client client.Clien
181209
}
182210

183211
// ValidateDelete implements webhook.Validator so a webhook will be registered for the type.
184-
func (m *AzureMachine) ValidateDelete(client client.Client) error {
212+
func (mw *azureMachineWebhook) ValidateDelete(ctx context.Context, obj runtime.Object) error {
185213
return nil
186214
}
187215

188216
// Default implements webhook.Defaulter so a webhook will be registered for the type.
189-
func (m *AzureMachine) Default(client client.Client) {
190-
m.SetDefaults(client)
217+
func (mw *azureMachineWebhook) Default(ctx context.Context, obj runtime.Object) error {
218+
m, ok := obj.(*AzureMachine)
219+
if !ok {
220+
return apierrors.NewBadRequest("expected an AzureMachine resource")
221+
}
222+
return m.SetDefaults(mw.Client)
191223
}

‎api/v1beta1/azuremachine_webhook_test.go‎

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,9 @@ import (
2323
"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2019-12-01/compute"
2424
. "github.com/onsi/gomega"
2525
"k8s.io/apimachinery/pkg/api/resource"
26+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2627
"k8s.io/utils/pointer"
28+
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
2729
"sigs.k8s.io/controller-runtime/pkg/client"
2830
)
2931

@@ -148,7 +150,8 @@ func TestAzureMachine_ValidateCreate(t *testing.T) {
148150
}
149151
for _, tc := range tests {
150152
t.Run(tc.name, func(t *testing.T) {
151-
err := tc.machine.ValidateCreate(nil)
153+
mw := &azureMachineWebhook{}
154+
err := mw.ValidateCreate(context.Background(), tc.machine)
152155
if tc.wantErr {
153156
g.Expect(err).To(HaveOccurred())
154157
} else {
@@ -696,7 +699,8 @@ func TestAzureMachine_ValidateUpdate(t *testing.T) {
696699
}
697700
for _, tc := range tests {
698701
t.Run(tc.name, func(t *testing.T) {
699-
err := tc.newMachine.ValidateUpdate(tc.oldMachine, nil)
702+
mw := &azureMachineWebhook{}
703+
err := mw.ValidateUpdate(context.Background(), tc.oldMachine, tc.newMachine)
700704
if tc.wantErr {
701705
g.Expect(err).To(HaveOccurred())
702706
} else {
@@ -728,20 +732,33 @@ func TestAzureMachine_Default(t *testing.T) {
728732
existingPublicKey := validSSHPublicKey
729733
publicKeyExistTest := test{machine: createMachineWithSSHPublicKey(existingPublicKey)}
730734
publicKeyNotExistTest := test{machine: createMachineWithSSHPublicKey("")}
735+
testObjectMeta := metav1.ObjectMeta{
736+
Labels: map[string]string{
737+
clusterv1.ClusterLabelName: "test-cluster",
738+
},
739+
}
740+
741+
mw := &azureMachineWebhook{
742+
Client: mockClient,
743+
}
731744

732-
publicKeyExistTest.machine.Default(mockClient)
745+
err := mw.Default(context.Background(), publicKeyExistTest.machine)
746+
g.Expect(err).NotTo(HaveOccurred())
733747
g.Expect(publicKeyExistTest.machine.Spec.SSHPublicKey).To(Equal(existingPublicKey))
734748

735-
publicKeyNotExistTest.machine.Default(mockClient)
749+
err = mw.Default(context.Background(), publicKeyNotExistTest.machine)
750+
g.Expect(err).NotTo(HaveOccurred())
736751
g.Expect(publicKeyNotExistTest.machine.Spec.SSHPublicKey).To(Not(BeEmpty()))
737752

738-
cacheTypeNotSpecifiedTest := test{machine: &AzureMachine{Spec: AzureMachineSpec{OSDisk: OSDisk{CachingType: ""}}}}
739-
cacheTypeNotSpecifiedTest.machine.Default(mockClient)
753+
cacheTypeNotSpecifiedTest := test{machine: &AzureMachine{ObjectMeta: testObjectMeta, Spec: AzureMachineSpec{OSDisk: OSDisk{CachingType: ""}}}}
754+
err = mw.Default(context.Background(), cacheTypeNotSpecifiedTest.machine)
755+
g.Expect(err).NotTo(HaveOccurred())
740756
g.Expect(cacheTypeNotSpecifiedTest.machine.Spec.OSDisk.CachingType).To(Equal("None"))
741757

742758
for _, possibleCachingType := range compute.PossibleCachingTypesValues() {
743-
cacheTypeSpecifiedTest := test{machine: &AzureMachine{Spec: AzureMachineSpec{OSDisk: OSDisk{CachingType: string(possibleCachingType)}}}}
744-
cacheTypeSpecifiedTest.machine.Default(mockClient)
759+
cacheTypeSpecifiedTest := test{machine: &AzureMachine{ObjectMeta: testObjectMeta, Spec: AzureMachineSpec{OSDisk: OSDisk{CachingType: string(possibleCachingType)}}}}
760+
err = mw.Default(context.Background(), cacheTypeSpecifiedTest.machine)
761+
g.Expect(err).NotTo(HaveOccurred())
745762
g.Expect(cacheTypeSpecifiedTest.machine.Spec.OSDisk.CachingType).To(Equal(string(possibleCachingType)))
746763
}
747764
}

‎api/v1beta1/azuremanagedcontrolplane_webhook.go‎

Lines changed: 35 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -49,17 +49,29 @@ var (
4949
rScanInterval = regexp.MustCompile(`^(\d+)s$`)
5050
)
5151

52-
// SetupWebhookWithManager sets up and registers the webhook with the manager.
53-
func (m *AzureManagedControlPlane) SetupWebhookWithManager(mgr ctrl.Manager) error {
52+
// SetupAzureManagedControlPlaneWebhookWithManager sets up and registers the webhook with the manager.
53+
func SetupAzureManagedControlPlaneWebhookWithManager(mgr ctrl.Manager) error {
54+
mw := &azureManagedControlPlaneWebhook{Client: mgr.GetClient()}
5455
return ctrl.NewWebhookManagedBy(mgr).
55-
For(m).
56+
For(&AzureManagedControlPlane{}).
57+
WithDefaulter(mw).
58+
WithValidator(mw).
5659
Complete()
5760
}
5861

5962
// +kubebuilder:webhook:path=/mutate-infrastructure-cluster-x-k8s-io-v1beta1-azuremanagedcontrolplane,mutating=true,failurePolicy=fail,groups=infrastructure.cluster.x-k8s.io,resources=azuremanagedcontrolplanes,verbs=create;update,versions=v1beta1,name=default.azuremanagedcontrolplanes.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1
6063

64+
// azureManagedControlPlaneWebhook implements a validating and defaulting webhook for AzureManagedControlPlane.
65+
type azureManagedControlPlaneWebhook struct {
66+
Client client.Client
67+
}
68+
6169
// Default implements webhook.Defaulter so a webhook will be registered for the type.
62-
func (m *AzureManagedControlPlane) Default(_ client.Client) {
70+
func (mw *azureManagedControlPlaneWebhook) Default(ctx context.Context, obj runtime.Object) error {
71+
m, ok := obj.(*AzureManagedControlPlane)
72+
if !ok {
73+
return apierrors.NewBadRequest("expected an AzureManagedControlPlane")
74+
}
6375
if m.Spec.NetworkPlugin == nil {
6476
networkPlugin := "azure"
6577
m.Spec.NetworkPlugin = &networkPlugin
@@ -83,12 +95,18 @@ func (m *AzureManagedControlPlane) Default(_ client.Client) {
8395
m.setDefaultSubnet()
8496
m.setDefaultSku()
8597
m.setDefaultAutoScalerProfile()
98+
99+
return nil
86100
}
87101

88102
// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1beta1-azuremanagedcontrolplane,mutating=false,failurePolicy=fail,groups=infrastructure.cluster.x-k8s.io,resources=azuremanagedcontrolplanes,versions=v1beta1,name=validation.azuremanagedcontrolplanes.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1
89103

90104
// ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
91-
func (m *AzureManagedControlPlane) ValidateCreate(client client.Client) error {
105+
func (mw *azureManagedControlPlaneWebhook) ValidateCreate(ctx context.Context, obj runtime.Object) error {
106+
m, ok := obj.(*AzureManagedControlPlane)
107+
if !ok {
108+
return apierrors.NewBadRequest("expected an AzureManagedControlPlane")
109+
}
92110
// NOTE: AzureManagedControlPlane relies upon MachinePools, which is behind a feature gate flag.
93111
// The webhook must prevent creating new objects in case the feature flag is disabled.
94112
if !feature.Gates.Enabled(capifeature.MachinePool) {
@@ -111,13 +129,20 @@ func (m *AzureManagedControlPlane) ValidateCreate(client client.Client) error {
111129
)
112130
}
113131

114-
return m.Validate(client)
132+
return m.Validate(mw.Client)
115133
}
116134

117135
// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
118-
func (m *AzureManagedControlPlane) ValidateUpdate(oldRaw runtime.Object, client client.Client) error {
136+
func (mw *azureManagedControlPlaneWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) error {
119137
var allErrs field.ErrorList
120-
old := oldRaw.(*AzureManagedControlPlane)
138+
old, ok := oldObj.(*AzureManagedControlPlane)
139+
if !ok {
140+
return apierrors.NewBadRequest("expected an AzureManagedControlPlane")
141+
}
142+
m, ok := newObj.(*AzureManagedControlPlane)
143+
if !ok {
144+
return apierrors.NewBadRequest("expected an AzureManagedControlPlane")
145+
}
121146

122147
if err := webhookutils.ValidateImmutable(
123148
field.NewPath("Spec", "SubscriptionID"),
@@ -244,14 +269,14 @@ func (m *AzureManagedControlPlane) ValidateUpdate(oldRaw runtime.Object, client
244269
}
245270

246271
if len(allErrs) == 0 {
247-
return m.Validate(client)
272+
return m.Validate(mw.Client)
248273
}
249274

250275
return apierrors.NewInvalid(GroupVersion.WithKind("AzureManagedControlPlane").GroupKind(), m.Name, allErrs)
251276
}
252277

253278
// ValidateDelete implements webhook.Validator so a webhook will be registered for the type.
254-
func (m *AzureManagedControlPlane) ValidateDelete(_ client.Client) error {
279+
func (mw *azureManagedControlPlaneWebhook) ValidateDelete(ctx context.Context, obj runtime.Object) error {
255280
return nil
256281
}
257282

‎api/v1beta1/azuremanagedcontrolplane_webhook_test.go‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ limitations under the License.
1717
package v1beta1
1818

1919
import (
20+
"context"
2021
"testing"
2122

2223
. "github.com/onsi/gomega"
@@ -42,7 +43,9 @@ func TestDefaultingWebhook(t *testing.T) {
4243
Version: "1.17.5",
4344
},
4445
}
45-
amcp.Default(nil)
46+
mcpw := &azureManagedControlPlaneWebhook{}
47+
err := mcpw.Default(context.Background(), amcp)
48+
g.Expect(err).NotTo(HaveOccurred())
4649
g.Expect(*amcp.Spec.NetworkPlugin).To(Equal("azure"))
4750
g.Expect(*amcp.Spec.LoadBalancerSKU).To(Equal("Standard"))
4851
g.Expect(amcp.Spec.Version).To(Equal("v1.17.5"))
@@ -66,7 +69,8 @@ func TestDefaultingWebhook(t *testing.T) {
6669
amcp.Spec.VirtualNetwork.Subnet.Name = "fooSubnetName"
6770
amcp.Spec.SKU.Tier = PaidManagedControlPlaneTier
6871

69-
amcp.Default(nil)
72+
err = mcpw.Default(context.Background(), amcp)
73+
g.Expect(err).NotTo(HaveOccurred())
7074
g.Expect(*amcp.Spec.NetworkPlugin).To(Equal(netPlug))
7175
g.Expect(*amcp.Spec.LoadBalancerSKU).To(Equal(lbSKU))
7276
g.Expect(*amcp.Spec.NetworkPolicy).To(Equal(netPol))
@@ -549,10 +553,11 @@ func TestValidatingWebhook(t *testing.T) {
549553
for _, tt := range tests {
550554
tt := tt
551555
t.Run(tt.name, func(t *testing.T) {
556+
mcpw := &azureManagedControlPlaneWebhook{}
552557
if tt.expectErr {
553-
g.Expect(tt.amcp.ValidateCreate(nil)).NotTo(Succeed())
558+
g.Expect(mcpw.ValidateCreate(context.Background(), &tt.amcp)).NotTo(Succeed())
554559
} else {
555-
g.Expect(tt.amcp.ValidateCreate(nil)).To(Succeed())
560+
g.Expect(mcpw.ValidateCreate(context.Background(), &tt.amcp)).To(Succeed())
556561
}
557562
})
558563
}
@@ -672,7 +677,8 @@ func TestAzureManagedControlPlane_ValidateCreate(t *testing.T) {
672677
}
673678
for _, tc := range tests {
674679
t.Run(tc.name, func(t *testing.T) {
675-
err := tc.amcp.ValidateCreate(nil)
680+
mcpw := &azureManagedControlPlaneWebhook{}
681+
err := mcpw.ValidateCreate(context.Background(), tc.amcp)
676682
if tc.wantErr {
677683
g.Expect(err).To(HaveOccurred())
678684
if tc.errorLen > 0 {
@@ -707,7 +713,8 @@ func TestAzureManagedControlPlane_ValidateCreateFailure(t *testing.T) {
707713
for _, tc := range tests {
708714
t.Run(tc.name, func(t *testing.T) {
709715
defer tc.deferFunc()
710-
err := tc.amcp.ValidateCreate(nil)
716+
mcpw := &azureManagedControlPlaneWebhook{}
717+
err := mcpw.ValidateCreate(context.Background(), tc.amcp)
711718
g.Expect(err).To(HaveOccurred())
712719
})
713720
}
@@ -1322,7 +1329,8 @@ func TestAzureManagedControlPlane_ValidateUpdate(t *testing.T) {
13221329
}
13231330
for _, tc := range tests {
13241331
t.Run(tc.name, func(t *testing.T) {
1325-
err := tc.amcp.ValidateUpdate(tc.oldAMCP, nil)
1332+
mcpw := &azureManagedControlPlaneWebhook{}
1333+
err := mcpw.ValidateUpdate(context.Background(), tc.oldAMCP, tc.amcp)
13261334
if tc.wantErr {
13271335
g.Expect(err).To(HaveOccurred())
13281336
} else {

0 commit comments

Comments
 (0)