Skip to content

Commit

Permalink
Merge pull request #6578 from killianmuldoon/runtimeSDK/discovery-val…
Browse files Browse the repository at this point in the history 
…idation

🌱 Add client defaulting and validation for DiscoveryResponse
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Jun 9, 2022
2 parents cf1e9b7 + 10bd788 commit 53f0eca
Show file tree
Hide file tree
Showing 7 changed files with 273 additions and 4 deletions.
2 changes: 2 additions & 0 deletions exp/runtime/hooks/api/v1alpha1/discovery_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
}

Expand Down
4 changes: 2 additions & 2 deletions exp/runtime/hooks/api/v1alpha1/zz_generated.openapi.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 8 additions & 2 deletions exp/runtime/internal/controllers/extensionconfig_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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,
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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(),
},
})
}
Expand Down
4 changes: 4 additions & 0 deletions exp/runtime/internal/controllers/warmup_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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())

Expand Down Expand Up @@ -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())

Expand Down
6 changes: 6 additions & 0 deletions internal/runtime/catalog/catalog.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
73 changes: 73 additions & 0 deletions internal/runtime/client/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{}
Expand Down Expand Up @@ -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
}
178 changes: 178 additions & 0 deletions internal/runtime/client/client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down

0 comments on commit 53f0eca

Please sign in to comment.