knative
/
pkg
mirror of https://github.com/knative/pkg.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

#457 Duck type user annotation logic (#467) 

* #457 Duck type user annotation logic

* #457 Duck type user annotation logic - tests

* #457 Revert updater annotation key from lastModifier to updater

* #457 Rename HasSpec#GetSpec() to HasSpec#GetUntypedSpec()

* #457 Fix some indentation

* #457 Get group for user info annotations from the request

* #457 Reduce confusuion in webhook testing by using same group
This commit is contained in:
Ali Ok 2019-06-25 04:20:05 +03:00 committed by Knative Prow Robot
parent d82505e6c5
commit 9f8e0692b7
8 changed files with 334 additions and 95 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
apis/interfaces.go
View File

@ -66,3 +66,10 @@ type Listable interface {
// The webhook functionality for this has been turned down, which is why this
// interface is empty.
type Annotatable interface{}
// HasSpec indicates that a particular type has a specification information
// and that information is retrievable.
type HasSpec interface {
// GetUntypedSpec returns the spec of the resource.
GetUntypedSpec() interface{}
}

53
testing/resource.go
View File

@ -21,9 +21,6 @@ import (
"fmt"
"github.com/knative/pkg/apis"
"github.com/knative/pkg/kmp"
authenticationv1 "k8s.io/api/authentication/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
)
@ -38,13 +35,6 @@ type Resource struct {
Spec ResourceSpec `json:"spec,omitempty"`
}
const (
// CreatorAnnotation is the annotation that denotes the user that created the resource.
CreatorAnnotation = "testing.knative.dev/creator"
// UpdaterAnnotation is the annotation that denotes the user that last updated the resource.
UpdaterAnnotation = "testing.knative.dev/updater"
)
// Check that Resource may be validated and defaulted.
var _ apis.Validatable = (*Resource)(nil)
var _ apis.Defaultable = (*Resource)(nil)
@ -60,47 +50,14 @@ type ResourceSpec struct {
FieldThatsImmutableWithDefault string `json:"fieldThatsImmutableWithDefault,omitempty"`
}
// GetUntypedSpec returns the spec of the resource.
func (r *Resource) GetUntypedSpec() interface{} {
return r.Spec
}
// SetDefaults sets the defaults on the object.
func (c *Resource) SetDefaults(ctx context.Context) {
c.Spec.SetDefaults(ctx)
if apis.IsInUpdate(ctx) {
old := apis.GetBaseline(ctx).(*Resource)
c.AnnotateUserInfo(ctx, old, apis.GetUserInfo(ctx))
} else {
c.AnnotateUserInfo(ctx, nil, apis.GetUserInfo(ctx))
}
}
// AnnotateUserInfo satisfies the Annotatable interface.
func (c *Resource) AnnotateUserInfo(ctx context.Context, prev *Resource, ui *authenticationv1.UserInfo) {
a := c.ObjectMeta.GetAnnotations()
if a == nil {
a = map[string]string{}
}
userName := ui.Username
// If previous is nil (i.e. this is `Create` operation),
// then we set both fields.
// Otherwise copy creator from the previous state.
if prev == nil {
a[CreatorAnnotation] = userName
} else {
// No spec update ==> bail out.
if ok, _ := kmp.SafeEqual(prev.Spec, c.Spec); ok {
if prev.ObjectMeta.GetAnnotations() != nil {
a[CreatorAnnotation] = prev.ObjectMeta.GetAnnotations()[CreatorAnnotation]
userName = prev.ObjectMeta.GetAnnotations()[UpdaterAnnotation]
}
} else {
if prev.ObjectMeta.GetAnnotations() != nil {
a[CreatorAnnotation] = prev.ObjectMeta.GetAnnotations()[CreatorAnnotation]
}
}
}
// Regardless of `old` set the updater.
a[UpdaterAnnotation] = userName
c.ObjectMeta.SetAnnotations(a)
}
func (c *Resource) Validate(ctx context.Context) *apis.FieldError {

13
webhook/helper_test.go
View File

@ -31,6 +31,7 @@ import (
"k8s.io/client-go/kubernetes"
. "github.com/knative/pkg/logging/testing"
. "github.com/knative/pkg/testing"
)
func waitForServerAvailable(t *testing.T, serverURL string, timeout time.Duration) error {
@ -117,3 +118,15 @@ func createSecureTLSClient(t *testing.T, kubeClient kubernetes.Interface, acOpts
},
}, nil
}
func createResource(name string) *Resource {
return &Resource{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: name,
},
Spec: ResourceSpec{
FieldWithValidation: "magic value",
},
}
}

61
webhook/user_info.go Normal file
View File

@ -0,0 +1,61 @@
/*
Copyright 2019 The Knative Authors
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
package webhook
import (
"context"
"github.com/knative/pkg/apis"
"k8s.io/apimachinery/pkg/api/equality"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)
const (
// CreatorAnnotationSuffix is the suffix of the annotation key to describe
// the user that created the resource.
CreatorAnnotationSuffix = "/creator"
// UpdaterAnnotationSuffix is the suffix of the annotation key to describe
// the user who last modified the resource.
UpdaterAnnotationSuffix = "/updater"
)
// SetUserInfoAnnotations sets creator and updater annotations on a resource.
func SetUserInfoAnnotations(resource apis.HasSpec, ctx context.Context, groupName string) {
if ui := apis.GetUserInfo(ctx); ui != nil {
objectMetaAccessor, ok := resource.(metav1.ObjectMetaAccessor)
if !ok {
return
}
annotations := objectMetaAccessor.GetObjectMeta().GetAnnotations()
if annotations == nil {
annotations = map[string]string{}
defer objectMetaAccessor.GetObjectMeta().SetAnnotations(annotations)
}
if apis.IsInUpdate(ctx) {
old := apis.GetBaseline(ctx).(apis.HasSpec)
if equality.Semantic.DeepEqual(old.GetUntypedSpec(), resource.GetUntypedSpec()) {
return
}
annotations[groupName+UpdaterAnnotationSuffix] = ui.Username
} else {
annotations[groupName+CreatorAnnotationSuffix] = ui.Username
annotations[groupName+UpdaterAnnotationSuffix] = ui.Username
}
}
}

179
webhook/user_info_test.go Normal file
View File

@ -0,0 +1,179 @@
/*
Copyright 2019 The Knative Authors
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
package webhook
import (
"context"
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/knative/pkg/apis"
. "github.com/knative/pkg/logging/testing"
. "github.com/knative/pkg/testing"
authenticationv1 "k8s.io/api/authentication/v1"
"reflect"
"testing"
)
func TestSetUserInfoAnnotationsWhenWithinCreate(t *testing.T) {
tests := []struct {
name string
configureContext func(context.Context) context.Context
setup func(context.Context, *Resource)
expectedAnnotations map[string]string
}{{
name: "test create",
configureContext: func(ctx context.Context) context.Context {
return apis.WithinCreate(apis.WithUserInfo(ctx, &authenticationv1.UserInfo{Username: user1}))
},
setup: func(ctx context.Context, r *Resource) {
r.Annotations = map[string]string{}
},
expectedAnnotations: map[string]string{
"pkg.knative.dev/creator": user1,
"pkg.knative.dev/updater": user1,
},
}, {
name: "test create (should override user info annotations when they are present)",
configureContext: func(ctx context.Context) context.Context {
return apis.WithinCreate(apis.WithUserInfo(ctx, &authenticationv1.UserInfo{Username: user1}))
},
setup: func(ctx context.Context, r *Resource) {
r.Annotations = map[string]string{
"pkg.knative.dev/creator": user2,
"pkg.knative.dev/updater": user2,
}
},
expectedAnnotations: map[string]string{
"pkg.knative.dev/creator": user1,
"pkg.knative.dev/updater": user1,
},
}, {
name: "test create (should not touch annotations when no user info available)",
configureContext: func(ctx context.Context) context.Context {
return apis.WithinCreate(ctx)
},
setup: func(ctx context.Context, r *Resource) {
},
expectedAnnotations: nil,
}}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
r := createResource("a name")
ctx := tc.configureContext(TestContextWithLogger(t))
tc.setup(ctx, r)
SetUserInfoAnnotations(r, ctx, "pkg.knative.dev")
if !reflect.DeepEqual(r.Annotations, tc.expectedAnnotations) {
t.Logf("Got : %#v", r.Annotations)
t.Logf("Want: %#v", tc.expectedAnnotations)
if diff := cmp.Diff(tc.expectedAnnotations, r.Annotations, cmpopts.EquateEmpty()); diff != "" {
t.Logf("diff: %v", diff)
}
t.Fatalf("Annotations don't match")
}
})
}
}
func TestSetUserInfoAnnotationsWhenWithinUpdate(t *testing.T) {
tests := []struct {
name string
configureContext func(context.Context, *Resource) context.Context
setup func(context.Context, *Resource)
expectedAnnotations map[string]string
}{{
name: "test update (should add updater annotation when it is not present)",
configureContext: func(ctx context.Context, r *Resource) context.Context {
return apis.WithinUpdate(apis.WithUserInfo(ctx, &authenticationv1.UserInfo{Username: user1}), r)
},
setup: func(ctx context.Context, r *Resource) {
r.Annotations = map[string]string{
"pkg.knative.dev/creator": user2,
}
r.Spec.FieldWithDefault = "changing this field"
},
expectedAnnotations: map[string]string{
"pkg.knative.dev/creator": user2,
"pkg.knative.dev/updater": user1,
},
}, {
name: "test update (should update updater annotation when it is present)",
configureContext: func(ctx context.Context, r *Resource) context.Context {
return apis.WithinUpdate(apis.WithUserInfo(ctx, &authenticationv1.UserInfo{Username: user1}), r)
},
setup: func(ctx context.Context, r *Resource) {
r.Annotations = map[string]string{
"pkg.knative.dev/creator": user2,
"pkg.knative.dev/updater": user2,
}
r.Spec.FieldWithDefault = "changing this field"
},
expectedAnnotations: map[string]string{
// should not change
"pkg.knative.dev/creator": user2,
"pkg.knative.dev/updater": user1,
},
}, {
name: "test update (should not touch annotations when no user info available)",
configureContext: func(ctx context.Context, r *Resource) context.Context {
return apis.WithinUpdate(ctx, r)
},
setup: func(ctx context.Context, r *Resource) {
// this is not necessary, but let's do this in case the execution flow changes
r.Spec.FieldWithDefault = "changing this field"
},
expectedAnnotations: nil,
}, {
name: "test update (should not touch annotations when nothing in spec is changed)",
configureContext: func(ctx context.Context, r *Resource) context.Context {
return apis.WithinUpdate(apis.WithUserInfo(ctx, &authenticationv1.UserInfo{Username: user1}), r)
},
setup: func(ctx context.Context, r *Resource) {
// change nothing
},
expectedAnnotations: map[string]string{},
}}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
r := createResource("a name")
ctx := tc.configureContext(TestContextWithLogger(t), r)
new := r.DeepCopy()
tc.setup(ctx, new)
SetUserInfoAnnotations(new, ctx, "pkg.knative.dev")
if !reflect.DeepEqual(new.Annotations, tc.expectedAnnotations) {
t.Logf("Got : %#v", new.Annotations)
t.Logf("Want: %#v", tc.expectedAnnotations)
if diff := cmp.Diff(tc.expectedAnnotations, new.Annotations, cmpopts.EquateEmpty()); diff != "" {
t.Logf("diff: %v", diff)
}
t.Fatalf("Annotations don't match")
}
})
}
}

30
webhook/webhook.go
View File

@ -550,6 +550,11 @@ func (ac *AdmissionController) mutate(ctx context.Context, req *admissionv1beta1
oldObj = oldObj.DeepCopyObject().(GenericCRD)
oldObj.SetDefaults(ctx)
s, ok := oldObj.(apis.HasSpec)
if ok {
SetUserInfoAnnotations(s, ctx, req.Resource.Group)
}
if req.SubResource == "" {
ctx = apis.WithinUpdate(ctx, oldObj)
} else {
@ -568,6 +573,11 @@ func (ac *AdmissionController) mutate(ctx context.Context, req *admissionv1beta1
return nil, err
}
if patches, err = ac.setUserInfoAnnotations(ctx, patches, newObj, req.Resource.Group); err != nil {
logger.Errorw("Failed the resource user info annotator", zap.Error(err))
return nil, err
}
// None of the validators will accept a nil value for newObj.
if newObj == nil {
return nil, errMissingNewObject
@ -582,6 +592,26 @@ func (ac *AdmissionController) mutate(ctx context.Context, req *admissionv1beta1
return json.Marshal(patches)
}
func (ac *AdmissionController) setUserInfoAnnotations(ctx context.Context, patches duck.JSONPatch, new GenericCRD, groupName string) (duck.JSONPatch, error) {
if new == nil {
return patches, nil
}
nh, ok := new.(apis.HasSpec)
if !ok {
return patches, nil
}
b, a := new.DeepCopyObject().(apis.HasSpec), nh
SetUserInfoAnnotations(nh, ctx, groupName)
patch, err := duck.CreatePatch(b, a)
if err != nil {
return nil, err
}
return append(patches, patch...), nil
}
// roundTripPatch generates the JSONPatch that corresponds to round tripping the given bytes through
// the Golang type (JSON -> Golang type -> JSON). Because it is not always true that
// bytes == json.Marshal(json.Unmarshal(bytes)).

3
webhook/webhook_integration_test.go
View File

@ -179,6 +179,7 @@ func TestValidResponseForResource(t *testing.T) {
t.Fatalf("Failed to marshal resource: %s", err)
}
admissionreq.Resource.Group = "pkg.knative.dev"
admissionreq.Object.Raw = marshaled
rev := &admissionv1beta1.AdmissionReview{
Request: admissionreq,
@ -262,6 +263,7 @@ func TestValidResponseForResourceWithContextDefault(t *testing.T) {
t.Fatalf("Failed to marshal resource: %s", err)
}
admissionreq.Resource.Group = "pkg.knative.dev"
admissionreq.Object.Raw = marshaled
rev := &admissionv1beta1.AdmissionReview{
Request: admissionreq,
@ -362,6 +364,7 @@ func TestInvalidResponseForResource(t *testing.T) {
},
}
admissionreq.Resource.Group = "pkg.knative.dev"
admissionreq.Object.Raw = marshaled
rev := &admissionv1beta1.AdmissionReview{

83
webhook/webhook_test.go
View File

@ -170,6 +170,10 @@ func TestAdmitCreates(t *testing.T) {
setup: func(ctx context.Context, r *Resource) {
r.TypeMeta.APIVersion = "v1alpha1"
r.SetDefaults(ctx)
r.Annotations = map[string]string{
"pkg.knative.dev/creator": user1,
"pkg.knative.dev/updater": user1,
}
},
patches: []jsonpatch.JsonPatchOperation{},
}, {
@ -177,6 +181,10 @@ func TestAdmitCreates(t *testing.T) {
setup: func(ctx context.Context, r *Resource) {
r.TypeMeta.APIVersion = "v1beta1"
r.SetDefaults(ctx)
r.Annotations = map[string]string{
"pkg.knative.dev/creator": user1,
"pkg.knative.dev/updater": user1,
}
},
patches: []jsonpatch.JsonPatchOperation{},
}, {
@ -187,8 +195,8 @@ func TestAdmitCreates(t *testing.T) {
Operation: "add",
Path: "/metadata/annotations",
Value: map[string]interface{}{
"testing.knative.dev/creator": user1,
"testing.knative.dev/updater": user1,
"pkg.knative.dev/creator": user1,
"pkg.knative.dev/updater": user1,
},
}, {
Operation: "add",
@ -208,11 +216,11 @@ func TestAdmitCreates(t *testing.T) {
},
patches: []jsonpatch.JsonPatchOperation{{
Operation: "add",
Path: "/metadata/annotations/testing.knative.dev~1creator",
Path: "/metadata/annotations/pkg.knative.dev~1creator",
Value: user1,
}, {
Operation: "add",
Path: "/metadata/annotations/testing.knative.dev~1updater",
Path: "/metadata/annotations/pkg.knative.dev~1updater",
Value: user1,
}, {
Operation: "add",
@ -232,8 +240,8 @@ func TestAdmitCreates(t *testing.T) {
Operation: "add",
Path: "/metadata/annotations",
Value: map[string]interface{}{
"testing.knative.dev/creator": user1,
"testing.knative.dev/updater": user1,
"pkg.knative.dev/creator": user1,
"pkg.knative.dev/updater": user1,
},
}, {
Operation: "add",
@ -243,17 +251,19 @@ func TestAdmitCreates(t *testing.T) {
}, {
name: "test simple creation (webhook corrects user annotation)",
setup: func(ctx context.Context, r *Resource) {
r.SetDefaults(apis.WithUserInfo(ctx,
// THIS IS NOT WHO IS CREATING IT, IT IS LIES!
&authenticationv1.UserInfo{Username: user2}))
r.SetDefaults(ctx)
// THIS IS NOT WHO IS CREATING IT, IT IS LIES!
r.Annotations = map[string]string{
"pkg.knative.dev/updater": user2,
}
},
patches: []jsonpatch.JsonPatchOperation{{
Operation: "replace",
Path: "/metadata/annotations/testing.knative.dev~1creator",
Path: "/metadata/annotations/pkg.knative.dev~1updater",
Value: user1,
}, {
Operation: "replace",
Path: "/metadata/annotations/testing.knative.dev~1updater",
Operation: "add",
Path: "/metadata/annotations/pkg.knative.dev~1creator",
Value: user1,
}},
}, {
@ -303,6 +313,7 @@ func createCreateResource(ctx context.Context, r *Resource) *admissionv1beta1.Ad
panic("failed to marshal resource")
}
req.Object.Raw = marshaled
req.Resource.Group = "pkg.knative.dev"
return req
}
@ -314,7 +325,7 @@ func TestAdmitUpdates(t *testing.T) {
rejection string
patches []jsonpatch.JsonPatchOperation
}{{
name: "test simple creation (no diff)",
name: "test simple update (no diff)",
setup: func(ctx context.Context, r *Resource) {
r.SetDefaults(ctx)
},
@ -324,7 +335,7 @@ func TestAdmitUpdates(t *testing.T) {
},
patches: []jsonpatch.JsonPatchOperation{},
}, {
name: "test simple creation (update updater annotation)",
name: "test simple update (update updater annotation)",
setup: func(ctx context.Context, r *Resource) {
r.SetDefaults(ctx)
},
@ -335,11 +346,11 @@ func TestAdmitUpdates(t *testing.T) {
},
patches: []jsonpatch.JsonPatchOperation{{
Operation: "replace",
Path: "/metadata/annotations/testing.knative.dev~1updater",
Path: "/metadata/annotations/pkg.knative.dev~1updater",
Value: user2,
}},
}, {
name: "test simple creation (annotation change doesn't change updater)",
name: "test simple update (annotation change doesn't change updater)",
setup: func(ctx context.Context, r *Resource) {
r.SetDefaults(ctx)
},
@ -358,10 +369,6 @@ func TestAdmitUpdates(t *testing.T) {
r.Spec.FieldThatsImmutableWithDefault = ""
},
patches: []jsonpatch.JsonPatchOperation{{
Operation: "replace",
Path: "/metadata/annotations/testing.knative.dev~1updater",
Value: user2,
}, {
Operation: "add",
Path: "/spec/fieldThatsImmutableWithDefault",
Value: "this is another default value",
@ -391,10 +398,12 @@ func TestAdmitUpdates(t *testing.T) {
old := createResource("a name")
ctx := TestContextWithLogger(t)
// Setup the resource using a creation context as user1
createCtx := apis.WithUserInfo(apis.WithinCreate(ctx),
&authenticationv1.UserInfo{Username: user1})
tc.setup(createCtx, old)
old.Annotations = map[string]string{
"pkg.knative.dev/creator": user1,
"pkg.knative.dev/updater": user1,
}
tc.setup(ctx, old)
new := old.DeepCopy()
@ -436,6 +445,7 @@ func createUpdateResource(ctx context.Context, old, new *Resource) *admissionv1b
panic("failed to marshal resource")
}
req.OldObject.Raw = marshaledOld
req.Resource.Group = "pkg.knative.dev"
return req
}
@ -709,18 +719,6 @@ func createDeployment(ac *AdmissionController) {
ac.Client.Apps().Deployments("knative-something").Create(deployment)
}
func createResource(name string) *Resource {
return &Resource{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: name,
},
Spec: ResourceSpec{
FieldWithValidation: "magic value",
},
}
}
func createWebhook(ac *AdmissionController, webhook *admissionregistrationv1beta1.MutatingWebhookConfiguration) {
client := ac.Client.AdmissionregistrationV1beta1().MutatingWebhookConfigurations()
_, err := client.Create(webhook)
@ -785,22 +783,13 @@ func expectPatches(t *testing.T, a []byte, e []jsonpatch.JsonPatchOperation) {
}
}
func updateAnnotationsWithUser(userU string) []jsonpatch.JsonPatchOperation {
// Just keys is being updated, so format is iffy.
return []jsonpatch.JsonPatchOperation{{
Operation: "replace",
Path: "/metadata/annotations/testing.knative.dev~1updater",
Value: userU,
}}
}
func setUserAnnotation(userC, userU string) jsonpatch.JsonPatchOperation {
return jsonpatch.JsonPatchOperation{
Operation: "add",
Path: "/metadata/annotations",
Value: map[string]interface{}{
CreatorAnnotation: userC,
UpdaterAnnotation: userU,
"pkg.knative.dev/creator": userC,
"pkg.knative.dev/updater": userU,
},
}
}