Skip to content

Commit

Permalink
This threads a context.Context through the webhook interfaces we expo…
Browse files Browse the repository at this point in the history
…se. (#332)

Related to: #306
  • Loading branch information
mattmoor authored and knative-prow-robot committed Mar 21, 2019
1 parent 3b8bc39 commit 60fdcbc
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 36 deletions.
10 changes: 6 additions & 4 deletions apis/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,28 +17,30 @@ limitations under the License.
package apis

import (
"context"

authenticationv1 "k8s.io/api/authentication/v1"
"k8s.io/apimachinery/pkg/runtime"
)

// Defaultable defines an interface for setting the defaults for the
// uninitialized fields of this instance.
type Defaultable interface {
SetDefaults()
SetDefaults(context.Context)
}

// Validatable indicates that a particular type may have its fields validated.
type Validatable interface {
// Validate checks the validity of this types fields.
Validate() *FieldError
Validate(context.Context) *FieldError
}

// Immutable indicates that a particular type has fields that should
// not change after creation.
type Immutable interface {
// CheckImmutableFields checks that the current instance's immutable
// fields haven't changed from the provided original.
CheckImmutableFields(original Immutable) *FieldError
CheckImmutableFields(ctx context.Context, original Immutable) *FieldError
}

// Listable indicates that a particular type can be returned via the returned
Expand All @@ -51,5 +53,5 @@ type Listable interface {

// Annotatable indicates that a particular type applies various annotations.
type Annotatable interface {
AnnotateUserInfo(previous Annotatable, ui *authenticationv1.UserInfo)
AnnotateUserInfo(ctx context.Context, previous Annotatable, ui *authenticationv1.UserInfo)
}
13 changes: 8 additions & 5 deletions testing/inner_default_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ limitations under the License.
package testing

import (
"context"

"github.com/knative/pkg/apis"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)
Expand Down Expand Up @@ -47,22 +49,23 @@ var _ apis.Validatable = (*InnerDefaultResource)(nil)
var _ apis.Defaultable = (*InnerDefaultResource)(nil)

// SetDefaults sets default values.
func (i *InnerDefaultResource) SetDefaults() {
i.Spec.SetDefaults()
func (i *InnerDefaultResource) SetDefaults(ctx context.Context) {
i.Spec.SetDefaults(ctx)
}

// SetDefaults sets default values.
func (cs *InnerDefaultSpec) SetDefaults() {
func (cs *InnerDefaultSpec) SetDefaults(ctx context.Context) {
if cs.FieldWithDefault == "" {
cs.FieldWithDefault = "I'm a default."
}
}

// Validate validates the resource.
func (*InnerDefaultResource) Validate() *apis.FieldError {
func (*InnerDefaultResource) Validate(ctx context.Context) *apis.FieldError {
return nil
}

// AnnotateUserInfo satisfies the Annotatable interface.
// For this type it is nop.
func (*InnerDefaultResource) AnnotateUserInfo(p apis.Annotatable, userName string) {}
func (*InnerDefaultResource) AnnotateUserInfo(ctx context.Context, p apis.Annotatable, userName string) {
}
9 changes: 6 additions & 3 deletions testing/inner_default_resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,21 @@ limitations under the License.

package testing

import "testing"
import (
"context"
"testing"
)

func TestInnerDefaultResource_Validate(t *testing.T) {
r := InnerDefaultResource{}
if err := r.Validate(); err != nil {
if err := r.Validate(context.TODO()); err != nil {
t.Fatalf("Expected no validation errors. Actual %v", err)
}
}

func TestInnerDefaultResource_SetDefaults(t *testing.T) {
r := InnerDefaultResource{}
r.SetDefaults()
r.SetDefaults(context.TODO())
if r.Spec.FieldWithDefault != "I'm a default." {
t.Fatalf("Unexpected defaulted value: %v", r.Spec.FieldWithDefault)
}
Expand Down
17 changes: 9 additions & 8 deletions testing/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package testing

import (
"context"
"fmt"

"github.com/knative/pkg/apis"
Expand Down Expand Up @@ -60,12 +61,12 @@ type ResourceSpec struct {
}

// SetDefaults sets the defaults on the object.
func (c *Resource) SetDefaults() {
c.Spec.SetDefaults()
func (c *Resource) SetDefaults(ctx context.Context) {
c.Spec.SetDefaults(ctx)
}

// SetDefaults sets the defaults on the spec.
func (cs *ResourceSpec) SetDefaults() {
func (cs *ResourceSpec) SetDefaults(ctx context.Context) {
if cs.FieldWithDefault == "" {
cs.FieldWithDefault = "I'm a default."
}
Expand All @@ -75,7 +76,7 @@ func (cs *ResourceSpec) SetDefaults() {
}

// AnnotateUserInfo satisfies the Annotatable interface.
func (c *Resource) AnnotateUserInfo(prev apis.Annotatable, ui *authenticationv1.UserInfo) {
func (c *Resource) AnnotateUserInfo(ctx context.Context, prev apis.Annotatable, ui *authenticationv1.UserInfo) {
a := c.ObjectMeta.GetAnnotations()
if a == nil {
a = map[string]string{}
Expand All @@ -102,18 +103,18 @@ func (c *Resource) AnnotateUserInfo(prev apis.Annotatable, ui *authenticationv1.
c.ObjectMeta.SetAnnotations(a)
}

func (c *Resource) Validate() *apis.FieldError {
return c.Spec.Validate().ViaField("spec")
func (c *Resource) Validate(ctx context.Context) *apis.FieldError {
return c.Spec.Validate(ctx).ViaField("spec")
}

func (cs *ResourceSpec) Validate() *apis.FieldError {
func (cs *ResourceSpec) Validate(ctx context.Context) *apis.FieldError {
if cs.FieldWithValidation != "magic value" {
return apis.ErrInvalidValue(cs.FieldWithValidation, "fieldWithValidation")
}
return nil
}

func (current *Resource) CheckImmutableFields(og apis.Immutable) *apis.FieldError {
func (current *Resource) CheckImmutableFields(ctx context.Context, og apis.Immutable) *apis.FieldError {
original, ok := og.(*Resource)
if !ok {
return &apis.FieldError{Message: "The provided original was not a Resource"}
Expand Down
15 changes: 10 additions & 5 deletions webhook/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -208,18 +208,21 @@ func validate(old GenericCRD, new GenericCRD) error {
// Copy the old object and set defaults so that we don't reject our own
// defaulting done earlier in the webhook.
old = old.DeepCopyObject().(GenericCRD)
old.SetDefaults()
// TODO(mattmoor): Plumb through a real context
old.SetDefaults(context.TODO())

immutableOld, ok := old.(apis.Immutable)
if !ok {
return fmt.Errorf("unexpected type mismatch %T vs. %T", old, new)
}
if err := immutableNew.CheckImmutableFields(immutableOld); err != nil {
// TODO(mattmoor): Plumb through a real context
if err := immutableNew.CheckImmutableFields(context.TODO(), immutableOld); err != nil {
return err
}
}
// Can't just `return new.Validate()` because it doesn't properly nil-check.
if err := new.Validate(); err != nil {
// TODO(mattmoor): Plumb through a real context
if err := new.Validate(context.TODO()); err != nil {
return err
}
return nil
Expand All @@ -240,7 +243,8 @@ func setAnnotations(patches duck.JSONPatch, new, old GenericCRD, ui *authenticat
}
b, a := new.DeepCopyObject().(apis.Annotatable), na

a.AnnotateUserInfo(oa, ui)
// TODO(mattmoor): Plumb through a real context
a.AnnotateUserInfo(context.TODO(), oa, ui)
patch, err := duck.CreatePatch(b, a)
if err != nil {
return nil, err
Expand All @@ -251,7 +255,8 @@ func setAnnotations(patches duck.JSONPatch, new, old GenericCRD, ui *authenticat
// setDefaults simply leverages apis.Defaultable to set defaults.
func setDefaults(patches duck.JSONPatch, crd GenericCRD) (duck.JSONPatch, error) {
before, after := crd.DeepCopyObject(), crd
after.SetDefaults()
// TODO(mattmoor): Plumb through a real context
after.SetDefaults(context.TODO())

patch, err := duck.CreatePatch(before, after)
if err != nil {
Expand Down
23 changes: 12 additions & 11 deletions webhook/webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package webhook

import (
"context"
"crypto/tls"
"encoding/json"
"encoding/pem"
Expand Down Expand Up @@ -138,7 +139,7 @@ func TestValidCreateResourceSucceeds(t *testing.T) {
r := createResource("a name")
for _, v := range []string{"v1alpha1", "v1beta1"} {
r.TypeMeta.APIVersion = v
r.SetDefaults() // Fill in defaults to check that there are no patches.
r.SetDefaults(context.TODO()) // Fill in defaults to check that there are no patches.
_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
resp := ac.admit(TestContextWithLogger(t), createCreateResource(r))
expectAllowed(t, resp)
Expand Down Expand Up @@ -234,9 +235,9 @@ func TestInvalidCreateResourceFails(t *testing.T) {

func TestNopUpdateResourceSucceeds(t *testing.T) {
r := createResource("a name")
r.SetDefaults() // Fill in defaults to check that there are no patches.
r.SetDefaults(context.TODO()) // Fill in defaults to check that there are no patches.
nr := r.DeepCopyObject().(*Resource)
r.AnnotateUserInfo(nil, &authenticationv1.UserInfo{Username: user1})
r.AnnotateUserInfo(context.TODO(), nil, &authenticationv1.UserInfo{Username: user1})
_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
resp := ac.admit(TestContextWithLogger(t), createUpdateResource(r, nr))
expectAllowed(t, resp)
Expand All @@ -245,10 +246,10 @@ func TestNopUpdateResourceSucceeds(t *testing.T) {

func TestValidUpdateResourcePreserveAnnotations(t *testing.T) {
old := createResource("a name")
old.SetDefaults() // Fill in defaults to check that there are no patches.
old.AnnotateUserInfo(nil, &authenticationv1.UserInfo{Username: user1})
old.SetDefaults(context.TODO()) // Fill in defaults to check that there are no patches.
old.AnnotateUserInfo(context.TODO(), nil, &authenticationv1.UserInfo{Username: user1})
new := createResource("a name")
new.SetDefaults()
new.SetDefaults(context.TODO())
// User set annotations on the resource.
new.ObjectMeta.SetAnnotations(map[string]string{
"key": "to-my-heart",
Expand All @@ -263,8 +264,8 @@ func TestValidUpdateResourcePreserveAnnotations(t *testing.T) {

func TestValidBigChangeResourceSucceeds(t *testing.T) {
old := createResource("a name")
old.SetDefaults() // Fill in defaults to check that there are no patches.
old.AnnotateUserInfo(nil, &authenticationv1.UserInfo{Username: user1})
old.SetDefaults(context.TODO()) // Fill in defaults to check that there are no patches.
old.AnnotateUserInfo(context.TODO(), nil, &authenticationv1.UserInfo{Username: user1})
new := createResource("a name")
new.Spec.FieldWithDefault = "melon collie and the infinite sadness"

Expand All @@ -281,8 +282,8 @@ func TestValidBigChangeResourceSucceeds(t *testing.T) {

func TestValidUpdateResourceSucceeds(t *testing.T) {
old := createResource("a name")
old.SetDefaults() // Fill in defaults to check that there are no patches.
old.AnnotateUserInfo(nil, &authenticationv1.UserInfo{Username: user1})
old.SetDefaults(context.TODO()) // Fill in defaults to check that there are no patches.
old.AnnotateUserInfo(context.TODO(), nil, &authenticationv1.UserInfo{Username: user1})
new := createResource("a name")

_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
Expand Down Expand Up @@ -326,7 +327,7 @@ func TestInvalidUpdateResourceFailsImmutability(t *testing.T) {

func TestDefaultingImmutableFields(t *testing.T) {
old := createResource("a name")
old.AnnotateUserInfo(nil, &authenticationv1.UserInfo{Username: user1})
old.AnnotateUserInfo(context.TODO(), nil, &authenticationv1.UserInfo{Username: user1})
new := createResource("a name")

// If we don't specify the new, but immutable field, we default it,
Expand Down

0 comments on commit 60fdcbc

Please sign in to comment.