diff --git a/exp/runtime/hooks/api/v1alpha1/discovery_types.go b/exp/runtime/hooks/api/v1alpha1/discovery_types.go index 514f4c03e504..6766efe45d5a 100644 --- a/exp/runtime/hooks/api/v1alpha1/discovery_types.go +++ b/exp/runtime/hooks/api/v1alpha1/discovery_types.go @@ -54,9 +54,11 @@ type ExtensionHandler struct { RequestHook GroupVersionHook `json:"requestHook"` // TimeoutSeconds defines the timeout duration for client calls to the ExtensionHandler. + // This is defaulted to 10 if left undefined. TimeoutSeconds *int32 `json:"timeoutSeconds,omitempty"` // FailurePolicy defines how failures in calls to the ExtensionHandler should be handled by a client. + // This is defaulted to FailurePolicyFail if not defined. FailurePolicy *FailurePolicy `json:"failurePolicy,omitempty"` } diff --git a/exp/runtime/hooks/api/v1alpha1/zz_generated.openapi.go b/exp/runtime/hooks/api/v1alpha1/zz_generated.openapi.go index e495d9281b1e..46e9a40de661 100644 --- a/exp/runtime/hooks/api/v1alpha1/zz_generated.openapi.go +++ b/exp/runtime/hooks/api/v1alpha1/zz_generated.openapi.go @@ -797,14 +797,14 @@ func schema_runtime_hooks_api_v1alpha1_ExtensionHandler(ref common.ReferenceCall }, "timeoutSeconds": { SchemaProps: spec.SchemaProps{ - Description: "TimeoutSeconds defines the timeout duration for client calls to the ExtensionHandler.", + Description: "TimeoutSeconds defines the timeout duration for client calls to the ExtensionHandler. This is defaulted to 10 if left undefined.", Type: []string{"integer"}, Format: "int32", }, }, "failurePolicy": { SchemaProps: spec.SchemaProps{ - Description: "FailurePolicy defines how failures in calls to the ExtensionHandler should be handled by a client.", + Description: "FailurePolicy defines how failures in calls to the ExtensionHandler should be handled by a client. This is defaulted to FailurePolicyFail if not defined.", Type: []string{"string"}, Format: "", }, diff --git a/exp/runtime/internal/controllers/extensionconfig_controller_test.go b/exp/runtime/internal/controllers/extensionconfig_controller_test.go index f29519c7738f..c4d43082602b 100644 --- a/exp/runtime/internal/controllers/extensionconfig_controller_test.go +++ b/exp/runtime/internal/controllers/extensionconfig_controller_test.go @@ -38,6 +38,7 @@ import ( runtimecatalog "sigs.k8s.io/cluster-api/internal/runtime/catalog" runtimeclient "sigs.k8s.io/cluster-api/internal/runtime/client" runtimeregistry "sigs.k8s.io/cluster-api/internal/runtime/registry" + fakev1alpha1 "sigs.k8s.io/cluster-api/internal/runtime/test/v1alpha1" "sigs.k8s.io/cluster-api/util" ) @@ -50,6 +51,8 @@ func TestExtensionReconciler_Reconcile(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) cat := runtimecatalog.New() + g.Expect(fakev1alpha1.AddToCatalog(cat)).To(Succeed()) + registry := runtimeregistry.New() runtimeClient := runtimeclient.New(runtimeclient.Options{ Catalog: cat, @@ -187,6 +190,8 @@ func TestExtensionReconciler_discoverExtensionConfig(t *testing.T) { t.Run("test discovery of a single extension", func(t *testing.T) { cat := runtimecatalog.New() + g.Expect(fakev1alpha1.AddToCatalog(cat)).To(Succeed()) + registry := runtimeregistry.New() g.Expect(runtimehooksv1.AddToCatalog(cat)).To(Succeed()) extensionName := "ext1" @@ -217,6 +222,7 @@ func TestExtensionReconciler_discoverExtensionConfig(t *testing.T) { }) t.Run("fail discovery for non-running extension", func(t *testing.T) { cat := runtimecatalog.New() + g.Expect(fakev1alpha1.AddToCatalog(cat)).To(Succeed()) registry := runtimeregistry.New() g.Expect(runtimehooksv1.AddToCatalog(cat)).To(Succeed()) extensionName := "ext1" @@ -254,8 +260,8 @@ func discoveryHandler(handlerList ...string) func(http.ResponseWriter, *http.Req handlers = append(handlers, runtimehooksv1.ExtensionHandler{ Name: name, RequestHook: runtimehooksv1.GroupVersionHook{ - Hook: name, - APIVersion: runtimehooksv1.GroupVersion.String(), + Hook: "FakeHook", + APIVersion: fakev1alpha1.GroupVersion.String(), }, }) } diff --git a/exp/runtime/internal/controllers/warmup_test.go b/exp/runtime/internal/controllers/warmup_test.go index 4bef04337b92..bc6b1ffae8f1 100644 --- a/exp/runtime/internal/controllers/warmup_test.go +++ b/exp/runtime/internal/controllers/warmup_test.go @@ -32,6 +32,7 @@ import ( runtimecatalog "sigs.k8s.io/cluster-api/internal/runtime/catalog" runtimeclient "sigs.k8s.io/cluster-api/internal/runtime/client" runtimeregistry "sigs.k8s.io/cluster-api/internal/runtime/registry" + fakev1alpha1 "sigs.k8s.io/cluster-api/internal/runtime/test/v1alpha1" ) func Test_warmupRunnable_Start(t *testing.T) { @@ -44,6 +45,8 @@ func Test_warmupRunnable_Start(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) cat := runtimecatalog.New() + g.Expect(fakev1alpha1.AddToCatalog(cat)).To(Succeed()) + registry := runtimeregistry.New() g.Expect(runtimehooksv1.AddToCatalog(cat)).To(Succeed()) @@ -93,6 +96,7 @@ func Test_warmupRunnable_Start(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) cat := runtimecatalog.New() + g.Expect(fakev1alpha1.AddToCatalog(cat)).To(Succeed()) registry := runtimeregistry.New() g.Expect(runtimehooksv1.AddToCatalog(cat)).To(Succeed()) diff --git a/internal/runtime/catalog/catalog.go b/internal/runtime/catalog/catalog.go index e7506ead2fe1..1112d18802ad 100644 --- a/internal/runtime/catalog/catalog.go +++ b/internal/runtime/catalog/catalog.go @@ -315,6 +315,12 @@ func (c *Catalog) ValidateResponse(hook GroupVersionHook, obj runtime.Object) er return nil } +// IsHookRegistered returns true if the GroupVersionHook is registered with the catalog. +func (c *Catalog) IsHookRegistered(gvh GroupVersionHook) bool { + _, found := c.gvhToType[gvh] + return found +} + // GroupVersionHook unambiguously identifies a Hook. type GroupVersionHook struct { Group string diff --git a/internal/runtime/client/client.go b/internal/runtime/client/client.go index bd2c36d00e9c..39778c261867 100644 --- a/internal/runtime/client/client.go +++ b/internal/runtime/client/client.go @@ -31,8 +31,12 @@ import ( "github.com/pkg/errors" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + kerrors "k8s.io/apimachinery/pkg/util/errors" utilnet "k8s.io/apimachinery/pkg/util/net" + "k8s.io/apimachinery/pkg/util/validation" "k8s.io/client-go/transport" + "k8s.io/utils/pointer" runtimev1 "sigs.k8s.io/cluster-api/exp/runtime/api/v1alpha1" runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1" @@ -126,6 +130,11 @@ func (c *client) Discover(ctx context.Context, extensionConfig *runtimev1.Extens return nil, errors.Errorf("discovery failed with %v", response.GetMessage()) } + // Check to see if the response is valid. + if err = defaultAndValidateDiscoveryResponse(c.catalog, response); err != nil { + return nil, errors.Wrapf(err, "discovery response validation failed") + } + modifiedExtensionConfig := extensionConfig.DeepCopy() // Reset the handlers that were previously registered with the ExtensionConfig. modifiedExtensionConfig.Status.Handlers = []runtimev1.ExtensionHandler{} @@ -476,3 +485,67 @@ func urlForExtension(config runtimev1.ClientConfig, gvh runtimecatalog.GroupVers u.Path = path.Join(u.Path, runtimecatalog.GVHToPath(gvh, name)) return u, nil } + +// defaultAndValidateDiscoveryResponse defaults unset values and runs a set of validations on the Discovery Response. +// If any of these checks fails the response is invalid and an error is returned. +func defaultAndValidateDiscoveryResponse(cat *runtimecatalog.Catalog, discovery *runtimehooksv1.DiscoveryResponse) error { + names := make(map[string]bool) + var errs []error + + if discovery == nil { + return errors.New("error validating discovery: response is empty") + } + discovery = defaultDiscoveryResponse(discovery) + for _, handler := range discovery.Handlers { + // Names should be unique. + if _, ok := names[handler.Name]; ok { + errs = append(errs, errors.Errorf("duplicate name for handler %s found", handler.Name)) + } + names[handler.Name] = true + + // Name should match Kubernetes naming conventions - validated based on Kubernetes DNS1123 Subdomain rules. + if errStrings := validation.IsDNS1123Subdomain(handler.Name); len(errStrings) > 0 { + errs = append(errs, errors.Errorf("handler name %s is not valid: %s", handler.Name, errStrings)) + } + + // Timeout should be a positive integer not greater than 30. + if *handler.TimeoutSeconds < 0 || *handler.TimeoutSeconds > 30 { + errs = append(errs, errors.Errorf("handler %s timeoutSeconds %d must be between 0 and 30", handler.Name, *handler.TimeoutSeconds)) + } + + // FailurePolicy must be one of Ignore or Fail. + if *handler.FailurePolicy != runtimehooksv1.FailurePolicyFail && *handler.FailurePolicy != runtimehooksv1.FailurePolicyIgnore { + errs = append(errs, errors.Errorf("handler %s failurePolicy %s must equal \"Ignore\" or \"Fail\"", handler.Name, *handler.FailurePolicy)) + } + + gv, err := schema.ParseGroupVersion(handler.RequestHook.APIVersion) + if err != nil { + errs = append(errs, errors.Wrapf(err, "handler %s requestHook APIVersion %s is not valid", handler.Name, handler.RequestHook.APIVersion)) + } else if !cat.IsHookRegistered(runtimecatalog.GroupVersionHook{ + Group: gv.Group, + Version: gv.Version, + Hook: handler.RequestHook.Hook, + }) { + errs = append(errs, errors.Errorf("handler %s requestHook %s/%s is not in the Runtime SDK catalog", handler.Name, handler.RequestHook.APIVersion, handler.RequestHook.Hook)) + } + } + return kerrors.NewAggregate(errs) +} + +// defaultDiscoveryResponse defaults FailurePolicy and TimeoutSeconds for all discovered handlers. +func defaultDiscoveryResponse(discovery *runtimehooksv1.DiscoveryResponse) *runtimehooksv1.DiscoveryResponse { + for i, handler := range discovery.Handlers { + // If FailurePolicy is not defined set to "Fail" + if handler.FailurePolicy == nil { + defaultFailPolicy := runtimehooksv1.FailurePolicyFail + handler.FailurePolicy = &defaultFailPolicy + } + // If TimeoutSeconds is not defined set to ten. + if handler.TimeoutSeconds == nil { + handler.TimeoutSeconds = pointer.Int32(10) + } + + discovery.Handlers[i] = handler + } + return discovery +} diff --git a/internal/runtime/client/client_test.go b/internal/runtime/client/client_test.go index 2980ceea4c58..6dcf66139377 100644 --- a/internal/runtime/client/client_test.go +++ b/internal/runtime/client/client_test.go @@ -325,6 +325,184 @@ func TestURLForExtension(t *testing.T) { } } +func Test_defaultAndValidateDiscoveryResponse(t *testing.T) { + var invalidFailurePolicy runtimehooksv1.FailurePolicy = "DONT_FAIL" + cat := runtimecatalog.New() + _ = fakev1alpha1.AddToCatalog(cat) + + tests := []struct { + name string + discovery *runtimehooksv1.DiscoveryResponse + wantErr bool + }{ + { + name: "succeed with valid skeleton DiscoveryResponse", + discovery: &runtimehooksv1.DiscoveryResponse{ + TypeMeta: metav1.TypeMeta{ + Kind: "DiscoveryResponse", + APIVersion: runtimehooksv1.GroupVersion.String(), + }, + Handlers: []runtimehooksv1.ExtensionHandler{{ + Name: "extension", + RequestHook: runtimehooksv1.GroupVersionHook{ + Hook: "FakeHook", + APIVersion: fakev1alpha1.GroupVersion.String(), + }, + }}, + }, + wantErr: false, + }, + { + name: "error with name violating DNS1123", + discovery: &runtimehooksv1.DiscoveryResponse{ + TypeMeta: metav1.TypeMeta{ + Kind: "DiscoveryResponse", + APIVersion: runtimehooksv1.GroupVersion.String(), + }, + Handlers: []runtimehooksv1.ExtensionHandler{{ + Name: "HAS-CAPITAL-LETTERS", + RequestHook: runtimehooksv1.GroupVersionHook{ + Hook: "FakeHook", + APIVersion: fakev1alpha1.GroupVersion.String(), + }, + }}, + }, + wantErr: true, + }, + { + name: "error with Timeout of over 30 seconds", + discovery: &runtimehooksv1.DiscoveryResponse{ + TypeMeta: metav1.TypeMeta{ + Kind: "DiscoveryResponse", + APIVersion: runtimehooksv1.GroupVersion.String(), + }, + Handlers: []runtimehooksv1.ExtensionHandler{{ + Name: "ext1", + RequestHook: runtimehooksv1.GroupVersionHook{ + Hook: "FakeHook", + APIVersion: fakev1alpha1.GroupVersion.String(), + }, + TimeoutSeconds: pointer.Int32(100), + }}, + }, + wantErr: true, + }, + { + name: "error with Timeout of less than 0", + discovery: &runtimehooksv1.DiscoveryResponse{ + TypeMeta: metav1.TypeMeta{ + Kind: "DiscoveryResponse", + APIVersion: runtimehooksv1.GroupVersion.String(), + }, + Handlers: []runtimehooksv1.ExtensionHandler{{ + Name: "ext1", + RequestHook: runtimehooksv1.GroupVersionHook{ + Hook: "FakeHook", + APIVersion: fakev1alpha1.GroupVersion.String(), + }, + TimeoutSeconds: pointer.Int32(-1), + }}, + }, + wantErr: true, + }, + { + name: "error with FailurePolicy not Fail or Ignore", + discovery: &runtimehooksv1.DiscoveryResponse{ + TypeMeta: metav1.TypeMeta{ + Kind: "DiscoveryResponse", + APIVersion: runtimehooksv1.GroupVersion.String(), + }, + Handlers: []runtimehooksv1.ExtensionHandler{{ + Name: "ext1", + RequestHook: runtimehooksv1.GroupVersionHook{ + Hook: "FakeHook", + APIVersion: fakev1alpha1.GroupVersion.String(), + }, + TimeoutSeconds: pointer.Int32(20), + FailurePolicy: &invalidFailurePolicy, + }}, + }, + wantErr: true, + }, + { + name: "error when handler name is duplicated", + discovery: &runtimehooksv1.DiscoveryResponse{ + TypeMeta: metav1.TypeMeta{ + Kind: "DiscoveryResponse", + APIVersion: runtimehooksv1.GroupVersion.String(), + }, + Handlers: []runtimehooksv1.ExtensionHandler{ + { + Name: "ext1", + RequestHook: runtimehooksv1.GroupVersionHook{ + Hook: "FakeHook", + APIVersion: fakev1alpha1.GroupVersion.String(), + }, + }, + { + Name: "ext1", + RequestHook: runtimehooksv1.GroupVersionHook{ + Hook: "FakeHook", + APIVersion: fakev1alpha1.GroupVersion.String(), + }, + }, + { + Name: "ext2", + RequestHook: runtimehooksv1.GroupVersionHook{ + Hook: "FakeHook", + APIVersion: fakev1alpha1.GroupVersion.String(), + }, + }, + }, + }, + wantErr: true, + }, + { + name: "error if handler GroupVersionHook is not registered", + discovery: &runtimehooksv1.DiscoveryResponse{ + TypeMeta: metav1.TypeMeta{ + Kind: "DiscoveryResponse", + APIVersion: runtimehooksv1.GroupVersion.String(), + }, + Handlers: []runtimehooksv1.ExtensionHandler{{ + Name: "ext1", + RequestHook: runtimehooksv1.GroupVersionHook{ + Hook: "FakeHook", + // Version v1alpha2 is not registered with the catalog + APIVersion: fakev1alpha2.GroupVersion.String(), + }, + }}, + }, + wantErr: true, + }, + { + name: "error if handler GroupVersion can not be parsed", + discovery: &runtimehooksv1.DiscoveryResponse{ + TypeMeta: metav1.TypeMeta{ + Kind: "DiscoveryResponse", + APIVersion: runtimehooksv1.GroupVersion.String(), + }, + Handlers: []runtimehooksv1.ExtensionHandler{{ + Name: "ext1", + RequestHook: runtimehooksv1.GroupVersionHook{ + Hook: "FakeHook", + // Version v1alpha2 is not registered with the catalog + APIVersion: "too/many/slashes", + }, + }}, + }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := defaultAndValidateDiscoveryResponse(cat, tt.discovery); (err != nil) != tt.wantErr { + t.Errorf("defaultAndValidateDiscoveryResponse() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + func TestClient_CallExtension(t *testing.T) { testHostPort := "127.0.0.1:9090"