Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🌱 Move experimental addons API v1beta1 webhooks to separate package #9438

Merged
merged 1 commit into from
Sep 19, 2023
Merged

🌱 Move experimental addons API v1beta1 webhooks to separate package #9438

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
View file
Open in desktop
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
Open in desktop
Ankitasw marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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"))
}
Loading