Skip to content

Commit

Permalink
Merge pull request #9438 from Ankitasw/move-exp-addons-webhooks
Browse files Browse the repository at this point in the history 
🌱 Move experimental addons API v1beta1 webhooks to separate package
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed Sep 19, 2023
2 parents c286992 + 4f32c4f commit 1ab45a6
Show file tree
Hide file tree
Showing 10 changed files with 205 additions and 78 deletions.
1 change: 1 addition & 0 deletions Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,7 @@ generate-manifests-core: $(CONTROLLER_GEN) $(KUSTOMIZE) ## Generate manifests e.
paths=./$(EXP_DIR)/internal/webhooks/... \
paths=./$(EXP_DIR)/addons/api/... \
paths=./$(EXP_DIR)/addons/internal/controllers/... \
paths=./$(EXP_DIR)/addons/internal/webhooks/... \
paths=./$(EXP_DIR)/ipam/api/... \
paths=./$(EXP_DIR)/ipam/internal/webhooks/... \
paths=./$(EXP_DIR)/runtime/api/... \
Expand Down
70 changes: 46 additions & 24 deletions ...api/v1beta1/clusterresourceset_webhook.go → ...al/webhooks/clusterresourceset_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
"reflect"

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

addonsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1"
"sigs.k8s.io/cluster-api/feature"
)

func (m *ClusterResourceSet) SetupWebhookWithManager(mgr ctrl.Manager) error {
// ClusterResourceSet implements a validation and defaulting webhook for ClusterResourceSet.
type ClusterResourceSet struct{}

func (webhook *ClusterResourceSet) SetupWebhookWithManager(mgr ctrl.Manager) error {
return ctrl.NewWebhookManagedBy(mgr).
For(m).
For(&addonsv1.ClusterResourceSet{}).
WithDefaulter(webhook).
WithValidator(webhook).
Complete()
}

// +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
// +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

var _ webhook.Defaulter = &ClusterResourceSet{}
var _ webhook.Validator = &ClusterResourceSet{}
var _ webhook.CustomDefaulter = &ClusterResourceSet{}
var _ webhook.CustomValidator = &ClusterResourceSet{}

// Default implements webhook.Defaulter so a webhook will be registered for the type.
func (m *ClusterResourceSet) Default() {
func (webhook *ClusterResourceSet) Default(_ context.Context, obj runtime.Object) error {
crs, ok := obj.(*addonsv1.ClusterResourceSet)
if !ok {
return apierrors.NewBadRequest(fmt.Sprintf("expected a ClusterResourceSet but got a %T", obj))
}
// ClusterResourceSet Strategy defaults to ApplyOnce.
if m.Spec.Strategy == "" {
m.Spec.Strategy = string(ClusterResourceSetStrategyApplyOnce)
if crs.Spec.Strategy == "" {
crs.Spec.Strategy = string(addonsv1.ClusterResourceSetStrategyApplyOnce)
}
return nil
}

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

// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
func (m *ClusterResourceSet) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
oldCRS, ok := old.(*ClusterResourceSet)
func (webhook *ClusterResourceSet) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
oldCRS, ok := oldObj.(*addonsv1.ClusterResourceSet)
if !ok {
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a ClusterResourceSet but got a %T", old))
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a ClusterResourceSet but got a %T", oldObj))
}
return nil, m.validate(oldCRS)
newCRS, ok := newObj.(*addonsv1.ClusterResourceSet)
if !ok {
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a ClusterResourceSet but got a %T", newObj))
}
return nil, webhook.validate(oldCRS, newCRS)
}

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

func (m *ClusterResourceSet) validate(old *ClusterResourceSet) error {
func (webhook *ClusterResourceSet) validate(oldCRS, newCRS *addonsv1.ClusterResourceSet) error {
// NOTE: ClusterResourceSet is behind ClusterResourceSet feature gate flag; the web hook
// must prevent creating new objects when the feature flag is disabled.
if !feature.Gates.Enabled(feature.ClusterResourceSet) {
Expand All @@ -80,39 +100,41 @@ func (m *ClusterResourceSet) validate(old *ClusterResourceSet) error {
)
}
var allErrs field.ErrorList

// Validate selector parses as Selector
selector, err := metav1.LabelSelectorAsSelector(&m.Spec.ClusterSelector)
selector, err := metav1.LabelSelectorAsSelector(&newCRS.Spec.ClusterSelector)
if err != nil {
allErrs = append(
allErrs,
field.Invalid(field.NewPath("spec", "clusterSelector"), m.Spec.ClusterSelector, err.Error()),
field.Invalid(field.NewPath("spec", "clusterSelector"), newCRS.Spec.ClusterSelector, err.Error()),
)
}

// Validate that the selector isn't empty as null selectors do not select any objects.
if selector != nil && selector.Empty() {
allErrs = append(
allErrs,
field.Invalid(field.NewPath("spec", "clusterSelector"), m.Spec.ClusterSelector, "selector must not be empty"),
field.Invalid(field.NewPath("spec", "clusterSelector"), newCRS.Spec.ClusterSelector, "selector must not be empty"),
)
}

if old != nil && old.Spec.Strategy != "" && old.Spec.Strategy != m.Spec.Strategy {
if oldCRS != nil && oldCRS.Spec.Strategy != "" && oldCRS.Spec.Strategy != newCRS.Spec.Strategy {
allErrs = append(
allErrs,
field.Invalid(field.NewPath("spec", "strategy"), m.Spec.Strategy, "field is immutable"),
field.Invalid(field.NewPath("spec", "strategy"), newCRS.Spec.Strategy, "field is immutable"),
)
}

if old != nil && !reflect.DeepEqual(old.Spec.ClusterSelector, m.Spec.ClusterSelector) {
if oldCRS != nil && !reflect.DeepEqual(oldCRS.Spec.ClusterSelector, newCRS.Spec.ClusterSelector) {
allErrs = append(
allErrs,
field.Invalid(field.NewPath("spec", "clusterSelector"), m.Spec.ClusterSelector, "field is immutable"),
field.Invalid(field.NewPath("spec", "clusterSelector"), newCRS.Spec.ClusterSelector, "field is immutable"),
)
}

if len(allErrs) == 0 {
return nil
}
return apierrors.NewInvalid(GroupVersion.WithKind("ClusterResourceSet").GroupKind(), m.Name, allErrs)

return apierrors.NewInvalid(addonsv1.GroupVersion.WithKind("ClusterResourceSet").GroupKind(), newCRS.Name, allErrs)
}
63 changes: 36 additions & 27 deletions ...1beta1/clusterresourceset_webhook_test.go → ...bhooks/clusterresourceset_webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,28 +14,33 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package v1beta1
package webhooks

import (
"testing"

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

utildefaulting "sigs.k8s.io/cluster-api/util/defaulting"
addonsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1"
"sigs.k8s.io/cluster-api/internal/webhooks/util"
)

var ctx = ctrl.SetupSignalHandler()

func TestClusterResourcesetDefault(t *testing.T) {
g := NewWithT(t)
clusterResourceSet := &ClusterResourceSet{}
clusterResourceSet := &addonsv1.ClusterResourceSet{}
defaultingValidationCRS := clusterResourceSet.DeepCopy()
defaultingValidationCRS.Spec.ClusterSelector = metav1.LabelSelector{
MatchLabels: map[string]string{"foo": "bar"},
}
t.Run("for ClusterResourceSet", utildefaulting.DefaultValidateTest(defaultingValidationCRS))
clusterResourceSet.Default()
webhook := ClusterResourceSet{}
t.Run("for ClusterResourceSet", util.CustomDefaultValidateTest(ctx, defaultingValidationCRS, &webhook))
g.Expect(webhook.Default(ctx, clusterResourceSet)).To(Succeed())

g.Expect(clusterResourceSet.Spec.Strategy).To(Equal(string(ClusterResourceSetStrategyApplyOnce)))
g.Expect(clusterResourceSet.Spec.Strategy).To(Equal(string(addonsv1.ClusterResourceSetStrategyApplyOnce)))
}

func TestClusterResourceSetLabelSelectorAsSelectorValidation(t *testing.T) {
Expand All @@ -59,25 +64,26 @@ func TestClusterResourceSetLabelSelectorAsSelectorValidation(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)
clusterResourceSet := &ClusterResourceSet{
Spec: ClusterResourceSetSpec{
clusterResourceSet := &addonsv1.ClusterResourceSet{
Spec: addonsv1.ClusterResourceSetSpec{
ClusterSelector: metav1.LabelSelector{
MatchLabels: tt.selectors,
},
},
}
webhook := ClusterResourceSet{}
if tt.expectErr {
warnings, err := clusterResourceSet.ValidateCreate()
warnings, err := webhook.ValidateCreate(ctx, clusterResourceSet)
g.Expect(err).To(HaveOccurred())
g.Expect(warnings).To(BeEmpty())
warnings, err = clusterResourceSet.ValidateUpdate(clusterResourceSet)
warnings, err = webhook.ValidateUpdate(ctx, clusterResourceSet, clusterResourceSet)
g.Expect(err).To(HaveOccurred())
g.Expect(warnings).To(BeEmpty())
} else {
warnings, err := clusterResourceSet.ValidateCreate()
warnings, err := webhook.ValidateCreate(ctx, clusterResourceSet)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(warnings).To(BeEmpty())
warnings, err = clusterResourceSet.ValidateUpdate(clusterResourceSet)
warnings, err = webhook.ValidateUpdate(ctx, clusterResourceSet, clusterResourceSet)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(warnings).To(BeEmpty())
}
Expand All @@ -94,13 +100,13 @@ func TestClusterResourceSetStrategyImmutable(t *testing.T) {
}{
{
name: "when the Strategy has not changed",
oldStrategy: string(ClusterResourceSetStrategyApplyOnce),
newStrategy: string(ClusterResourceSetStrategyApplyOnce),
oldStrategy: string(addonsv1.ClusterResourceSetStrategyApplyOnce),
newStrategy: string(addonsv1.ClusterResourceSetStrategyApplyOnce),
expectErr: false,
},
{
name: "when the Strategy has changed",
oldStrategy: string(ClusterResourceSetStrategyApplyOnce),
oldStrategy: string(addonsv1.ClusterResourceSetStrategyApplyOnce),
newStrategy: "",
expectErr: true,
},
Expand All @@ -110,8 +116,8 @@ func TestClusterResourceSetStrategyImmutable(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)

newClusterResourceSet := &ClusterResourceSet{
Spec: ClusterResourceSetSpec{
newClusterResourceSet := &addonsv1.ClusterResourceSet{
Spec: addonsv1.ClusterResourceSetSpec{
ClusterSelector: metav1.LabelSelector{
MatchLabels: map[string]string{
"test": "test",
Expand All @@ -121,8 +127,8 @@ func TestClusterResourceSetStrategyImmutable(t *testing.T) {
},
}

oldClusterResourceSet := &ClusterResourceSet{
Spec: ClusterResourceSetSpec{
oldClusterResourceSet := &addonsv1.ClusterResourceSet{
Spec: addonsv1.ClusterResourceSetSpec{
ClusterSelector: metav1.LabelSelector{
MatchLabels: map[string]string{
"test": "test",
Expand All @@ -131,8 +137,9 @@ func TestClusterResourceSetStrategyImmutable(t *testing.T) {
Strategy: tt.oldStrategy,
},
}
webhook := ClusterResourceSet{}

warnings, err := newClusterResourceSet.ValidateUpdate(oldClusterResourceSet)
warnings, err := webhook.ValidateUpdate(ctx, oldClusterResourceSet, newClusterResourceSet)
if tt.expectErr {
g.Expect(err).To(HaveOccurred())
g.Expect(warnings).To(BeEmpty())
Expand Down Expand Up @@ -169,23 +176,24 @@ func TestClusterResourceSetClusterSelectorImmutable(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)

newClusterResourceSet := &ClusterResourceSet{
Spec: ClusterResourceSetSpec{
newClusterResourceSet := &addonsv1.ClusterResourceSet{
Spec: addonsv1.ClusterResourceSetSpec{
ClusterSelector: metav1.LabelSelector{
MatchLabels: tt.newClusterSelector,
},
},
}

oldClusterResourceSet := &ClusterResourceSet{
Spec: ClusterResourceSetSpec{
oldClusterResourceSet := &addonsv1.ClusterResourceSet{
Spec: addonsv1.ClusterResourceSetSpec{
ClusterSelector: metav1.LabelSelector{
MatchLabels: tt.oldClusterSelector,
},
},
}
webhook := ClusterResourceSet{}

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

func TestClusterResourceSetSelectorNotEmptyValidation(t *testing.T) {
g := NewWithT(t)
clusterResourceSet := &ClusterResourceSet{}
err := clusterResourceSet.validate(nil)
clusterResourceSet := &addonsv1.ClusterResourceSet{}
webhook := ClusterResourceSet{}
err := webhook.validate(nil, clusterResourceSet)
g.Expect(err).To(HaveOccurred())
g.Expect(err.Error()).To(ContainSubstring("selector must not be empty"))
}

0 comments on commit 1ab45a6

Please sign in to comment.