From ce89e60370ba49bd3599f72d80920ac95bc0f994 Mon Sep 17 00:00:00 2001 From: Yuvaraj Kakaraparthi Date: Fri, 29 Apr 2022 22:40:24 -0700 Subject: [PATCH] webhooks: defaulting and validation for ExtensionConfig Co-authored-by: Yuvaraj Kakaraparthi Co-authored-by: Christian Schlotter --- internal/webhooks/cluster_test.go | 7 +- internal/webhooks/clusterclass_test.go | 3 +- .../runtime/extensionconfig_webhook.go | 122 ++++++++++- .../runtime/extensionconfig_webhook_test.go | 207 ++++++++++++++++++ .../webhooks/{util_test.go => util/util.go} | 11 +- 5 files changed, 340 insertions(+), 10 deletions(-) rename internal/webhooks/{util_test.go => util/util.go} (83%) diff --git a/internal/webhooks/cluster_test.go b/internal/webhooks/cluster_test.go index 018c1145ef26..c8033aed6394 100644 --- a/internal/webhooks/cluster_test.go +++ b/internal/webhooks/cluster_test.go @@ -31,6 +31,7 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/feature" "sigs.k8s.io/cluster-api/internal/test/builder" + "sigs.k8s.io/cluster-api/internal/webhooks/util" ) func TestClusterDefaultNamespaces(t *testing.T) { @@ -46,7 +47,7 @@ func TestClusterDefaultNamespaces(t *testing.T) { }, } webhook := &Cluster{} - tFunc := customDefaultValidateTest(ctx, c, webhook) + tFunc := util.CustomDefaultValidateTest(ctx, c, webhook) t.Run("for Cluster", tFunc) g.Expect(webhook.Default(ctx, c)).To(Succeed()) @@ -317,7 +318,7 @@ func TestClusterDefaultVariables(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Test if defaulting works in combination with validation. - customDefaultValidateTest(ctx, cluster, webhook)(t) + util.CustomDefaultValidateTest(ctx, cluster, webhook)(t) // Test defaulting. t.Run("default", func(t *testing.T) { g := NewWithT(t) @@ -350,7 +351,7 @@ func TestClusterDefaultTopologyVersion(t *testing.T) { // Create the webhook and add the fakeClient as its client. webhook := &Cluster{Client: fakeClient} - tFunc := customDefaultValidateTest(ctx, c, webhook) + tFunc := util.CustomDefaultValidateTest(ctx, c, webhook) t.Run("for Cluster", tFunc) g.Expect(webhook.Default(ctx, c)).To(Succeed()) diff --git a/internal/webhooks/clusterclass_test.go b/internal/webhooks/clusterclass_test.go index bfc9a2b1f8b7..a7c827cb7900 100644 --- a/internal/webhooks/clusterclass_test.go +++ b/internal/webhooks/clusterclass_test.go @@ -34,6 +34,7 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/feature" "sigs.k8s.io/cluster-api/internal/test/builder" + "sigs.k8s.io/cluster-api/internal/webhooks/util" ) var ( @@ -76,7 +77,7 @@ func TestClusterClassDefaultNamespaces(t *testing.T) { // Create the webhook and add the fakeClient as its client. webhook := &ClusterClass{Client: fakeClient} - tFunc := customDefaultValidateTest(ctx, in, webhook) + tFunc := util.CustomDefaultValidateTest(ctx, in, webhook) t.Run("for ClusterClass", tFunc) diff --git a/internal/webhooks/runtime/extensionconfig_webhook.go b/internal/webhooks/runtime/extensionconfig_webhook.go index eac945e5e2fe..346ed1c9af19 100644 --- a/internal/webhooks/runtime/extensionconfig_webhook.go +++ b/internal/webhooks/runtime/extensionconfig_webhook.go @@ -19,11 +19,15 @@ package runtime import ( "context" "fmt" + "net/url" + "strings" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/utils/pointer" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/webhook" @@ -58,6 +62,11 @@ func (webhook *ExtensionConfig) Default(ctx context.Context, obj runtime.Object) if extensionConfig.Spec.NamespaceSelector == nil { extensionConfig.Spec.NamespaceSelector = &metav1.LabelSelector{} } + if extensionConfig.Spec.ClientConfig.Service != nil { + if extensionConfig.Spec.ClientConfig.Service.Port == nil { + extensionConfig.Spec.ClientConfig.Service.Port = pointer.Int32(8443) + } + } return nil } @@ -84,7 +93,7 @@ func (webhook *ExtensionConfig) ValidateUpdate(ctx context.Context, old, updated } // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type. -func (webhook *ExtensionConfig) validate(_ context.Context, _, _ *runtimev1.ExtensionConfig) error { +func (webhook *ExtensionConfig) validate(_ context.Context, _, newExtensionConfig *runtimev1.ExtensionConfig) error { // NOTE: ExtensionConfig is behind the RuntimeSDK feature gate flag; the web hook // must prevent creating and updating objects in case the feature flag is disabled. if !feature.Gates.Enabled(feature.RuntimeSDK) { @@ -93,6 +102,13 @@ func (webhook *ExtensionConfig) validate(_ context.Context, _, _ *runtimev1.Exte "can be set only if the RuntimeSDK feature flag is enabled", ) } + + var allErrs field.ErrorList + allErrs = append(allErrs, validateExtensionConfigSpec(newExtensionConfig)...) + + if len(allErrs) > 0 { + return apierrors.NewInvalid(runtimev1.GroupVersion.WithKind("ExtensionConfig").GroupKind(), newExtensionConfig.Name, allErrs) + } return nil } @@ -100,3 +116,107 @@ func (webhook *ExtensionConfig) validate(_ context.Context, _, _ *runtimev1.Exte func (webhook *ExtensionConfig) ValidateDelete(_ context.Context, _ runtime.Object) error { return nil } + +func validateExtensionConfigSpec(e *runtimev1.ExtensionConfig) field.ErrorList { + var allErrs field.ErrorList + + specPath := field.NewPath("spec") + + if e.Spec.ClientConfig.URL == nil && e.Spec.ClientConfig.Service == nil { + allErrs = append(allErrs, field.Required( + specPath.Child("clientConfig"), + "either URL or Service must be defined", + )) + } + if e.Spec.ClientConfig.URL != nil && e.Spec.ClientConfig.Service != nil { + allErrs = append(allErrs, field.Forbidden( + specPath.Child("clientConfig"), + "only one of URL or Service can be defined", + )) + } + + // Validate URL + if e.Spec.ClientConfig.URL != nil { + if uri, err := url.ParseRequestURI(*e.Spec.ClientConfig.URL); err != nil { + allErrs = append(allErrs, field.Invalid( + specPath.Child("clientConfig", "url"), + *e.Spec.ClientConfig.URL, + fmt.Sprintf("must be a valid URL, e.g. https://example.com: %v", err), + )) + } else if uri.Scheme != "http" && uri.Scheme != "https" { + allErrs = append(allErrs, field.Invalid( + specPath.Child("clientConfig", "url"), + *e.Spec.ClientConfig.URL, + "'https' and 'http' are the only allowed URL schemes, e.g. https://example.com", + )) + } + } + + // Validate Service if defined + if e.Spec.ClientConfig.Service != nil { + // Validate that the name is not empty and is a Valid RFC1123 name. + if e.Spec.ClientConfig.Service.Name == "" { + allErrs = append(allErrs, field.Required( + specPath.Child("clientConfig", "service", "name"), + "must not be empty", + )) + } + + for _, msg := range validation.IsDNS1035Label(e.Spec.ClientConfig.Service.Name) { + allErrs = append(allErrs, field.Invalid( + specPath.Child("clientConfig", "service", "name"), + e.Spec.ClientConfig.Service.Name, + msg, + )) + } + + if e.Spec.ClientConfig.Service.Namespace == "" { + allErrs = append(allErrs, field.Required( + specPath.Child("clientConfig", "service", "namespace"), + "must not be empty", + )) + } + + for _, msg := range validation.IsDNS1123Label(e.Spec.ClientConfig.Service.Namespace) { + allErrs = append(allErrs, field.Invalid( + specPath.Child("clientConfig", "service", "namespace"), + e.Spec.ClientConfig.Service.Name, + msg, + )) + } + + if e.Spec.ClientConfig.Service.Path != nil { + path := *e.Spec.ClientConfig.Service.Path + if _, err := url.ParseRequestURI(path); err != nil { + allErrs = append(allErrs, field.Invalid( + specPath.Child("clientConfig", "service", "path"), + path, + fmt.Sprintf("must be a valid URL path e.g. /path/to/hook: %v", err), + )) + } + if !strings.HasPrefix(path, "/") { + allErrs = append(allErrs, field.Invalid( + specPath.Child("clientConfig", "service", "path"), + path, + "must start with \"/\" to be a valid URL path", + )) + } + } + if e.Spec.ClientConfig.Service.Port != nil { + for _, msg := range validation.IsValidPortNum(int(*e.Spec.ClientConfig.Service.Port)) { + allErrs = append(allErrs, field.Invalid( + specPath.Child("clientConfig", "service", "port"), + *e.Spec.ClientConfig.Service.Port, + msg, + )) + } + } + } + if e.Spec.NamespaceSelector == nil { + allErrs = append(allErrs, field.Required( + specPath.Child("NamespaceSelector"), + "must be defined", + )) + } + return allErrs +} diff --git a/internal/webhooks/runtime/extensionconfig_webhook_test.go b/internal/webhooks/runtime/extensionconfig_webhook_test.go index 3e17dde9ae6c..33544c6587f3 100644 --- a/internal/webhooks/runtime/extensionconfig_webhook_test.go +++ b/internal/webhooks/runtime/extensionconfig_webhook_test.go @@ -25,12 +25,15 @@ import ( "k8s.io/apimachinery/pkg/runtime" utilfeature "k8s.io/component-base/featuregate/testing" "k8s.io/utils/pointer" + ctrl "sigs.k8s.io/controller-runtime" runtimev1 "sigs.k8s.io/cluster-api/exp/runtime/api/v1alpha1" "sigs.k8s.io/cluster-api/feature" + "sigs.k8s.io/cluster-api/internal/webhooks/util" ) var ( + ctx = ctrl.SetupSignalHandler() fakeScheme = runtime.NewScheme() ) @@ -47,6 +50,7 @@ func TestExtensionConfigValidationFeatureGated(t *testing.T) { ClientConfig: runtimev1.ClientConfig{ URL: pointer.String("https://extension-address.com"), }, + NamespaceSelector: &metav1.LabelSelector{}, }, } updatedExtension := extension.DeepCopy() @@ -100,3 +104,206 @@ func TestExtensionConfigValidationFeatureGated(t *testing.T) { }) } } + +func TestExtensionConfigDefault(t *testing.T) { + g := NewWithT(t) + defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.RuntimeSDK, true)() + + extensionConfig := &runtimev1.ExtensionConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-extension", + }, + Spec: runtimev1.ExtensionConfigSpec{ + ClientConfig: runtimev1.ClientConfig{ + Service: &runtimev1.ServiceReference{ + Name: "name", + Namespace: "namespace", + }, + }, + }, + } + + extensionConfigWebhook := &ExtensionConfig{} + t.Run("for Extension", util.CustomDefaultValidateTest(ctx, extensionConfig, extensionConfigWebhook)) + + g.Expect(extensionConfigWebhook.Default(ctx, extensionConfig)).To(Succeed()) + g.Expect(extensionConfig.Spec.NamespaceSelector).To(Equal(&metav1.LabelSelector{})) + g.Expect(extensionConfig.Spec.ClientConfig.Service.Port).To(Equal(pointer.Int32(8443))) +} + +func TestExtensionConfigValidate(t *testing.T) { + extensionWithURL := &runtimev1.ExtensionConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-extension", + }, + Spec: runtimev1.ExtensionConfigSpec{ + ClientConfig: runtimev1.ClientConfig{ + URL: pointer.String("https://extension-address.com"), + }, + }, + } + + extensionWithService := &runtimev1.ExtensionConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-extension", + }, + Spec: runtimev1.ExtensionConfigSpec{ + ClientConfig: runtimev1.ClientConfig{ + Service: &runtimev1.ServiceReference{ + Path: pointer.StringPtr("/path/to/handler"), + Port: pointer.Int32(1), + Name: "foo", + Namespace: "bar", + }}, + }, + } + + extensionWithServiceAndURL := extensionWithURL.DeepCopy() + extensionWithServiceAndURL.Spec.ClientConfig.Service = extensionWithService.Spec.ClientConfig.Service + + // Valid updated Extension + updatedExtension := extensionWithURL.DeepCopy() + updatedExtension.Spec.ClientConfig.URL = pointer.StringPtr("https://a-in-extension-address.com") + + extensionWithoutURLOrService := extensionWithURL.DeepCopy() + extensionWithoutURLOrService.Spec.ClientConfig.URL = nil + + extensionWithInvalidServicePath := extensionWithService.DeepCopy() + extensionWithInvalidServicePath.Spec.ClientConfig.Service.Path = pointer.StringPtr("https://example.com") + + extensionWithNoServiceName := extensionWithService.DeepCopy() + extensionWithNoServiceName.Spec.ClientConfig.Service.Name = "" + + extensionWithBadServiceName := extensionWithService.DeepCopy() + extensionWithBadServiceName.Spec.ClientConfig.Service.Name = "NOT_ALLOWED" + + extensionWithNoServiceNamespace := extensionWithService.DeepCopy() + extensionWithNoServiceNamespace.Spec.ClientConfig.Service.Namespace = "" + + extensionWithBadServiceNamespace := extensionWithService.DeepCopy() + extensionWithBadServiceNamespace.Spec.ClientConfig.Service.Namespace = "INVALID" + + badURLExtension := extensionWithURL.DeepCopy() + badURLExtension.Spec.ClientConfig.URL = pointer.String("https//extension-address.com") + + badSchemeExtension := extensionWithURL.DeepCopy() + badSchemeExtension.Spec.ClientConfig.URL = pointer.String("unknown://extension-address.com") + + extensionWithInvalidServicePort := extensionWithService.DeepCopy() + extensionWithInvalidServicePort.Spec.ClientConfig.Service.Port = pointer.Int32(90000) + + tests := []struct { + name string + in *runtimev1.ExtensionConfig + old *runtimev1.ExtensionConfig + featureGate bool + expectErr bool + }{ + { + name: "creation should fail if feature flag is disabled", + in: extensionWithURL, + featureGate: false, + expectErr: true, + }, + { + name: "update should fail if feature flag is disabled", + old: extensionWithURL, + in: updatedExtension, + featureGate: false, + expectErr: true, + }, + { + name: "creation should fail if no Service Name is defined", + in: extensionWithNoServiceName, + featureGate: true, + expectErr: true, + }, + { + name: "creation should fail if Service Name violates Kubernetes naming rules", + in: extensionWithBadServiceName, + featureGate: true, + expectErr: true, + }, + { + name: "creation should fail if no Service Namespace is defined", + in: extensionWithNoServiceNamespace, + featureGate: true, + expectErr: true, + }, + { + name: "creation should fail if Service Namespace violates Kubernetes naming rules", + in: extensionWithBadServiceNamespace, + featureGate: true, + expectErr: true, + }, + { + name: "update should fail if URL is invalid", + old: extensionWithURL, + in: badURLExtension, + featureGate: true, + expectErr: true, + }, + { + name: "update should fail if URL scheme is invalid", + old: extensionWithURL, + in: badSchemeExtension, + featureGate: true, + expectErr: true, + }, + { + name: "update should fail if URL and Service are both nil", + old: extensionWithURL, + in: extensionWithoutURLOrService, + featureGate: true, + expectErr: true, + }, + { + name: "update should fail if both URL and Service are defined", + old: extensionWithService, + in: extensionWithServiceAndURL, + featureGate: true, + expectErr: true, + }, + { + name: "update should fail if Service Path is invalid", + old: extensionWithService, + in: extensionWithInvalidServicePath, + featureGate: true, + expectErr: true, + }, + { + name: "update should fail if Service Port is invalid", + old: extensionWithService, + in: extensionWithInvalidServicePort, + featureGate: true, + expectErr: true, + }, + { + name: "update should pass if updated Extension is valid", + old: extensionWithService, + in: extensionWithService, + featureGate: true, + expectErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.RuntimeSDK, tt.featureGate)() + g := NewWithT(t) + webhook := &ExtensionConfig{} + // Default the objects so we're not handling defaulted cases. + g.Expect(webhook.Default(ctx, tt.in)).To(Succeed()) + if tt.old != nil { + g.Expect(webhook.Default(ctx, tt.old)).To(Succeed()) + } + + err := webhook.validate(ctx, tt.old, tt.in) + if tt.expectErr { + g.Expect(err).To(HaveOccurred()) + return + } + g.Expect(err).ToNot(HaveOccurred()) + }) + } +} diff --git a/internal/webhooks/util_test.go b/internal/webhooks/util/util.go similarity index 83% rename from internal/webhooks/util_test.go rename to internal/webhooks/util/util.go index 899bf0e9904b..cd3c6f2ee89a 100644 --- a/internal/webhooks/util_test.go +++ b/internal/webhooks/util/util.go @@ -14,7 +14,8 @@ See the License for the specific language governing permissions and limitations under the License. */ -package webhooks +// Package util includes the utility functions for testing webhooks. +package util import ( "context" @@ -25,17 +26,17 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook" ) -// customDefaulterValidator interface is for objects that define both custom defaulting +// CustomDefaulterValidator interface is for objects that define both custom defaulting // and custom validating webhooks. -type customDefaulterValidator interface { +type CustomDefaulterValidator interface { webhook.CustomDefaulter webhook.CustomValidator } -// customDefaultValidateTest returns a new testing function to be used in tests to +// CustomDefaultValidateTest returns a new testing function to be used in tests to // make sure custom defaulting webhooks also pass validation tests on create, // update and delete. -func customDefaultValidateTest(ctx context.Context, obj runtime.Object, webhook customDefaulterValidator) func(*testing.T) { +func CustomDefaultValidateTest(ctx context.Context, obj runtime.Object, webhook CustomDefaulterValidator) func(*testing.T) { return func(t *testing.T) { t.Helper()