Skip to content

Commit 1ab45a6

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes-sigs#9438 from Ankitasw/move-exp-addons-webhooks
🌱 Move experimental addons API v1beta1 webhooks to separate package
2 parents c286992 + 4f32c4f commit 1ab45a6

File tree

10 files changed

+205
-78
lines changed

10 files changed

+205
-78
lines changed

‎Makefile

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,7 @@ generate-manifests-core: $(CONTROLLER_GEN) $(KUSTOMIZE) ## Generate manifests e.
287287
paths=./$(EXP_DIR)/internal/webhooks/... \
288288
paths=./$(EXP_DIR)/addons/api/... \
289289
paths=./$(EXP_DIR)/addons/internal/controllers/... \
290+
paths=./$(EXP_DIR)/addons/internal/webhooks/... \
290291
paths=./$(EXP_DIR)/ipam/api/... \
291292
paths=./$(EXP_DIR)/ipam/internal/webhooks/... \
292293
paths=./$(EXP_DIR)/runtime/api/... \

‎exp/addons/api/v1beta1/clusterresourceset_webhook.go renamed to ‎exp/addons/internal/webhooks/clusterresourceset_webhook.go

Lines changed: 46 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,10 @@ See the License for the specific language governing permissions and
1414
limitations under the License.
1515
*/
1616

17-
package v1beta1
17+
package webhooks
1818

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

@@ -28,49 +29,68 @@ import (
2829
"sigs.k8s.io/controller-runtime/pkg/webhook"
2930
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
3031

32+
addonsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1"
3133
"sigs.k8s.io/cluster-api/feature"
3234
)
3335

34-
func (m *ClusterResourceSet) SetupWebhookWithManager(mgr ctrl.Manager) error {
36+
// ClusterResourceSet implements a validation and defaulting webhook for ClusterResourceSet.
37+
type ClusterResourceSet struct{}
38+
39+
func (webhook *ClusterResourceSet) SetupWebhookWithManager(mgr ctrl.Manager) error {
3540
return ctrl.NewWebhookManagedBy(mgr).
36-
For(m).
41+
For(&addonsv1.ClusterResourceSet{}).
42+
WithDefaulter(webhook).
43+
WithValidator(webhook).
3744
Complete()
3845
}
3946

4047
// +kubebuilder:webhook:verbs=create;update,path=/validate-addons-cluster-x-k8s-io-v1beta1-clusterresourceset,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=addons.cluster.x-k8s.io,resources=clusterresourcesets,versions=v1beta1,name=validation.clusterresourceset.addons.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1
4148
// +kubebuilder:webhook:verbs=create;update,path=/mutate-addons-cluster-x-k8s-io-v1beta1-clusterresourceset,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=addons.cluster.x-k8s.io,resources=clusterresourcesets,versions=v1beta1,name=default.clusterresourceset.addons.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1
4249

43-
var _ webhook.Defaulter = &ClusterResourceSet{}
44-
var _ webhook.Validator = &ClusterResourceSet{}
50+
var _ webhook.CustomDefaulter = &ClusterResourceSet{}
51+
var _ webhook.CustomValidator = &ClusterResourceSet{}
4552

4653
// Default implements webhook.Defaulter so a webhook will be registered for the type.
47-
func (m *ClusterResourceSet) Default() {
54+
func (webhook *ClusterResourceSet) Default(_ context.Context, obj runtime.Object) error {
55+
crs, ok := obj.(*addonsv1.ClusterResourceSet)
56+
if !ok {
57+
return apierrors.NewBadRequest(fmt.Sprintf("expected a ClusterResourceSet but got a %T", obj))
58+
}
4859
// ClusterResourceSet Strategy defaults to ApplyOnce.
49-
if m.Spec.Strategy == "" {
50-
m.Spec.Strategy = string(ClusterResourceSetStrategyApplyOnce)
60+
if crs.Spec.Strategy == "" {
61+
crs.Spec.Strategy = string(addonsv1.ClusterResourceSetStrategyApplyOnce)
5162
}
63+
return nil
5264
}
5365

5466
// ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
55-
func (m *ClusterResourceSet) ValidateCreate() (admission.Warnings, error) {
56-
return nil, m.validate(nil)
67+
func (webhook *ClusterResourceSet) ValidateCreate(_ context.Context, newObj runtime.Object) (admission.Warnings, error) {
68+
newCRS, ok := newObj.(*addonsv1.ClusterResourceSet)
69+
if !ok {
70+
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a ClusterResourceSet but got a %T", newObj))
71+
}
72+
return nil, webhook.validate(nil, newCRS)
5773
}
5874

5975
// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
60-
func (m *ClusterResourceSet) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
61-
oldCRS, ok := old.(*ClusterResourceSet)
76+
func (webhook *ClusterResourceSet) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
77+
oldCRS, ok := oldObj.(*addonsv1.ClusterResourceSet)
6278
if !ok {
63-
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a ClusterResourceSet but got a %T", old))
79+
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a ClusterResourceSet but got a %T", oldObj))
6480
}
65-
return nil, m.validate(oldCRS)
81+
newCRS, ok := newObj.(*addonsv1.ClusterResourceSet)
82+
if !ok {
83+
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a ClusterResourceSet but got a %T", newObj))
84+
}
85+
return nil, webhook.validate(oldCRS, newCRS)
6686
}
6787

6888
// ValidateDelete implements webhook.Validator so a webhook will be registered for the type.
69-
func (m *ClusterResourceSet) ValidateDelete() (admission.Warnings, error) {
89+
func (webhook *ClusterResourceSet) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) {
7090
return nil, nil
7191
}
7292

73-
func (m *ClusterResourceSet) validate(old *ClusterResourceSet) error {
93+
func (webhook *ClusterResourceSet) validate(oldCRS, newCRS *addonsv1.ClusterResourceSet) error {
7494
// NOTE: ClusterResourceSet is behind ClusterResourceSet feature gate flag; the web hook
7595
// must prevent creating new objects when the feature flag is disabled.
7696
if !feature.Gates.Enabled(feature.ClusterResourceSet) {
@@ -80,39 +100,41 @@ func (m *ClusterResourceSet) validate(old *ClusterResourceSet) error {
80100
)
81101
}
82102
var allErrs field.ErrorList
103+
83104
// Validate selector parses as Selector
84-
selector, err := metav1.LabelSelectorAsSelector(&m.Spec.ClusterSelector)
105+
selector, err := metav1.LabelSelectorAsSelector(&newCRS.Spec.ClusterSelector)
85106
if err != nil {
86107
allErrs = append(
87108
allErrs,
88-
field.Invalid(field.NewPath("spec", "clusterSelector"), m.Spec.ClusterSelector, err.Error()),
109+
field.Invalid(field.NewPath("spec", "clusterSelector"), newCRS.Spec.ClusterSelector, err.Error()),
89110
)
90111
}
91112

92113
// Validate that the selector isn't empty as null selectors do not select any objects.
93114
if selector != nil && selector.Empty() {
94115
allErrs = append(
95116
allErrs,
96-
field.Invalid(field.NewPath("spec", "clusterSelector"), m.Spec.ClusterSelector, "selector must not be empty"),
117+
field.Invalid(field.NewPath("spec", "clusterSelector"), newCRS.Spec.ClusterSelector, "selector must not be empty"),
97118
)
98119
}
99120

100-
if old != nil && old.Spec.Strategy != "" && old.Spec.Strategy != m.Spec.Strategy {
121+
if oldCRS != nil && oldCRS.Spec.Strategy != "" && oldCRS.Spec.Strategy != newCRS.Spec.Strategy {
101122
allErrs = append(
102123
allErrs,
103-
field.Invalid(field.NewPath("spec", "strategy"), m.Spec.Strategy, "field is immutable"),
124+
field.Invalid(field.NewPath("spec", "strategy"), newCRS.Spec.Strategy, "field is immutable"),
104125
)
105126
}
106127

107-
if old != nil && !reflect.DeepEqual(old.Spec.ClusterSelector, m.Spec.ClusterSelector) {
128+
if oldCRS != nil && !reflect.DeepEqual(oldCRS.Spec.ClusterSelector, newCRS.Spec.ClusterSelector) {
108129
allErrs = append(
109130
allErrs,
110-
field.Invalid(field.NewPath("spec", "clusterSelector"), m.Spec.ClusterSelector, "field is immutable"),
131+
field.Invalid(field.NewPath("spec", "clusterSelector"), newCRS.Spec.ClusterSelector, "field is immutable"),
111132
)
112133
}
113134

114135
if len(allErrs) == 0 {
115136
return nil
116137
}
117-
return apierrors.NewInvalid(GroupVersion.WithKind("ClusterResourceSet").GroupKind(), m.Name, allErrs)
138+
139+
return apierrors.NewInvalid(addonsv1.GroupVersion.WithKind("ClusterResourceSet").GroupKind(), newCRS.Name, allErrs)
118140
}

‎exp/addons/api/v1beta1/clusterresourceset_webhook_test.go renamed to ‎exp/addons/internal/webhooks/clusterresourceset_webhook_test.go

Lines changed: 36 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -14,28 +14,33 @@ See the License for the specific language governing permissions and
1414
limitations under the License.
1515
*/
1616

17-
package v1beta1
17+
package webhooks
1818

1919
import (
2020
"testing"
2121

2222
. "github.com/onsi/gomega"
2323
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
24+
ctrl "sigs.k8s.io/controller-runtime"
2425

25-
utildefaulting "sigs.k8s.io/cluster-api/util/defaulting"
26+
addonsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1"
27+
"sigs.k8s.io/cluster-api/internal/webhooks/util"
2628
)
2729

30+
var ctx = ctrl.SetupSignalHandler()
31+
2832
func TestClusterResourcesetDefault(t *testing.T) {
2933
g := NewWithT(t)
30-
clusterResourceSet := &ClusterResourceSet{}
34+
clusterResourceSet := &addonsv1.ClusterResourceSet{}
3135
defaultingValidationCRS := clusterResourceSet.DeepCopy()
3236
defaultingValidationCRS.Spec.ClusterSelector = metav1.LabelSelector{
3337
MatchLabels: map[string]string{"foo": "bar"},
3438
}
35-
t.Run("for ClusterResourceSet", utildefaulting.DefaultValidateTest(defaultingValidationCRS))
36-
clusterResourceSet.Default()
39+
webhook := ClusterResourceSet{}
40+
t.Run("for ClusterResourceSet", util.CustomDefaultValidateTest(ctx, defaultingValidationCRS, &webhook))
41+
g.Expect(webhook.Default(ctx, clusterResourceSet)).To(Succeed())
3742

38-
g.Expect(clusterResourceSet.Spec.Strategy).To(Equal(string(ClusterResourceSetStrategyApplyOnce)))
43+
g.Expect(clusterResourceSet.Spec.Strategy).To(Equal(string(addonsv1.ClusterResourceSetStrategyApplyOnce)))
3944
}
4045

4146
func TestClusterResourceSetLabelSelectorAsSelectorValidation(t *testing.T) {
@@ -59,25 +64,26 @@ func TestClusterResourceSetLabelSelectorAsSelectorValidation(t *testing.T) {
5964
for _, tt := range tests {
6065
t.Run(tt.name, func(t *testing.T) {
6166
g := NewWithT(t)
62-
clusterResourceSet := &ClusterResourceSet{
63-
Spec: ClusterResourceSetSpec{
67+
clusterResourceSet := &addonsv1.ClusterResourceSet{
68+
Spec: addonsv1.ClusterResourceSetSpec{
6469
ClusterSelector: metav1.LabelSelector{
6570
MatchLabels: tt.selectors,
6671
},
6772
},
6873
}
74+
webhook := ClusterResourceSet{}
6975
if tt.expectErr {
70-
warnings, err := clusterResourceSet.ValidateCreate()
76+
warnings, err := webhook.ValidateCreate(ctx, clusterResourceSet)
7177
g.Expect(err).To(HaveOccurred())
7278
g.Expect(warnings).To(BeEmpty())
73-
warnings, err = clusterResourceSet.ValidateUpdate(clusterResourceSet)
79+
warnings, err = webhook.ValidateUpdate(ctx, clusterResourceSet, clusterResourceSet)
7480
g.Expect(err).To(HaveOccurred())
7581
g.Expect(warnings).To(BeEmpty())
7682
} else {
77-
warnings, err := clusterResourceSet.ValidateCreate()
83+
warnings, err := webhook.ValidateCreate(ctx, clusterResourceSet)
7884
g.Expect(err).ToNot(HaveOccurred())
7985
g.Expect(warnings).To(BeEmpty())
80-
warnings, err = clusterResourceSet.ValidateUpdate(clusterResourceSet)
86+
warnings, err = webhook.ValidateUpdate(ctx, clusterResourceSet, clusterResourceSet)
8187
g.Expect(err).ToNot(HaveOccurred())
8288
g.Expect(warnings).To(BeEmpty())
8389
}
@@ -94,13 +100,13 @@ func TestClusterResourceSetStrategyImmutable(t *testing.T) {
94100
}{
95101
{
96102
name: "when the Strategy has not changed",
97-
oldStrategy: string(ClusterResourceSetStrategyApplyOnce),
98-
newStrategy: string(ClusterResourceSetStrategyApplyOnce),
103+
oldStrategy: string(addonsv1.ClusterResourceSetStrategyApplyOnce),
104+
newStrategy: string(addonsv1.ClusterResourceSetStrategyApplyOnce),
99105
expectErr: false,
100106
},
101107
{
102108
name: "when the Strategy has changed",
103-
oldStrategy: string(ClusterResourceSetStrategyApplyOnce),
109+
oldStrategy: string(addonsv1.ClusterResourceSetStrategyApplyOnce),
104110
newStrategy: "",
105111
expectErr: true,
106112
},
@@ -110,8 +116,8 @@ func TestClusterResourceSetStrategyImmutable(t *testing.T) {
110116
t.Run(tt.name, func(t *testing.T) {
111117
g := NewWithT(t)
112118

113-
newClusterResourceSet := &ClusterResourceSet{
114-
Spec: ClusterResourceSetSpec{
119+
newClusterResourceSet := &addonsv1.ClusterResourceSet{
120+
Spec: addonsv1.ClusterResourceSetSpec{
115121
ClusterSelector: metav1.LabelSelector{
116122
MatchLabels: map[string]string{
117123
"test": "test",
@@ -121,8 +127,8 @@ func TestClusterResourceSetStrategyImmutable(t *testing.T) {
121127
},
122128
}
123129

124-
oldClusterResourceSet := &ClusterResourceSet{
125-
Spec: ClusterResourceSetSpec{
130+
oldClusterResourceSet := &addonsv1.ClusterResourceSet{
131+
Spec: addonsv1.ClusterResourceSetSpec{
126132
ClusterSelector: metav1.LabelSelector{
127133
MatchLabels: map[string]string{
128134
"test": "test",
@@ -131,8 +137,9 @@ func TestClusterResourceSetStrategyImmutable(t *testing.T) {
131137
Strategy: tt.oldStrategy,
132138
},
133139
}
140+
webhook := ClusterResourceSet{}
134141

135-
warnings, err := newClusterResourceSet.ValidateUpdate(oldClusterResourceSet)
142+
warnings, err := webhook.ValidateUpdate(ctx, oldClusterResourceSet, newClusterResourceSet)
136143
if tt.expectErr {
137144
g.Expect(err).To(HaveOccurred())
138145
g.Expect(warnings).To(BeEmpty())
@@ -169,23 +176,24 @@ func TestClusterResourceSetClusterSelectorImmutable(t *testing.T) {
169176
t.Run(tt.name, func(t *testing.T) {
170177
g := NewWithT(t)
171178

172-
newClusterResourceSet := &ClusterResourceSet{
173-
Spec: ClusterResourceSetSpec{
179+
newClusterResourceSet := &addonsv1.ClusterResourceSet{
180+
Spec: addonsv1.ClusterResourceSetSpec{
174181
ClusterSelector: metav1.LabelSelector{
175182
MatchLabels: tt.newClusterSelector,
176183
},
177184
},
178185
}
179186

180-
oldClusterResourceSet := &ClusterResourceSet{
181-
Spec: ClusterResourceSetSpec{
187+
oldClusterResourceSet := &addonsv1.ClusterResourceSet{
188+
Spec: addonsv1.ClusterResourceSetSpec{
182189
ClusterSelector: metav1.LabelSelector{
183190
MatchLabels: tt.oldClusterSelector,
184191
},
185192
},
186193
}
194+
webhook := ClusterResourceSet{}
187195

188-
warnings, err := newClusterResourceSet.ValidateUpdate(oldClusterResourceSet)
196+
warnings, err := webhook.ValidateUpdate(ctx, oldClusterResourceSet, newClusterResourceSet)
189197
if tt.expectErr {
190198
g.Expect(err).To(HaveOccurred())
191199
g.Expect(warnings).To(BeEmpty())
@@ -199,8 +207,9 @@ func TestClusterResourceSetClusterSelectorImmutable(t *testing.T) {
199207

200208
func TestClusterResourceSetSelectorNotEmptyValidation(t *testing.T) {
201209
g := NewWithT(t)
202-
clusterResourceSet := &ClusterResourceSet{}
203-
err := clusterResourceSet.validate(nil)
210+
clusterResourceSet := &addonsv1.ClusterResourceSet{}
211+
webhook := ClusterResourceSet{}
212+
err := webhook.validate(nil, clusterResourceSet)
204213
g.Expect(err).To(HaveOccurred())
205214
g.Expect(err.Error()).To(ContainSubstring("selector must not be empty"))
206215
}

0 commit comments

Comments
 (0)