Skip to content

Commit

Permalink
Drop webhook logic to increment spec.generation
Browse files Browse the repository at this point in the history 
With Kubernetes 1.11+ metadata.generation now increments properly
when the status subresource is enabled on CRDs

For more details see: knative/serving#643
  • Loading branch information
@dprotaso
dprotaso committed Jan 21, 2019
1 parent a330baa commit 99cc227
Show file tree
Hide file tree
Showing 5 changed files with 16 additions and 192 deletions.
4 changes: 1 addition & 3 deletions testing/resource.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,8 @@ var _ apis.Defaultable = (*Resource)(nil)
var _ apis.Immutable = (*Resource)(nil)
var _ apis.Listable = (*Resource)(nil)

// Check that we implement the Generation duck type.
// ResourceSpec is structured with fields to test different webhook behaviour
type ResourceSpec struct {
Generation int64 `json:"generation,omitempty"`

FieldWithDefault string `json:"fieldWithDefault,omitempty"`
FieldWithValidation string `json:"fieldWithValidation,omitempty"`
FieldThatsImmutable string `json:"fieldThatsImmutable,omitempty"`
Expand Down
31 changes: 0 additions & 31 deletions testing/resource_test.go
View file

This file was deleted.

113 changes: 0 additions & 113 deletions webhook/webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ import (

"github.com/knative/pkg/apis"
"github.com/knative/pkg/apis/duck"
duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1"
"github.com/knative/pkg/logging"
"github.com/knative/pkg/logging/logkey"

Expand Down Expand Up @@ -284,15 +283,6 @@ func (ac *AdmissionController) Run(stop <-chan struct{}) error {
logger.Infof("Delaying admission webhook registration for %v", ac.Options.RegistrationDelay)
}

// Verify that each of the types we are given implements the Generation duck type.
for _, crd := range ac.Handlers {
cp := crd.DeepCopyObject()
var emptyGen duckv1alpha1.Generation
if err := duck.VerifyType(cp, &emptyGen); err != nil {
return err
}
}

select {
case <-time.After(ac.Options.RegistrationDelay):
cl := ac.Client.AdmissionregistrationV1beta1().MutatingWebhookConfigurations()
Expand Down Expand Up @@ -527,12 +517,6 @@ func (ac *AdmissionController) mutate(ctx context.Context, kind metav1.GroupVers

var patches []jsonpatch.JsonPatchOperation

err := updateGeneration(ctx, &patches, oldObj, newObj)
if err != nil {
logger.Error("Failed to update generation", zap.Error(err))
return nil, fmt.Errorf("Failed to update generation: %s", err)
}

if defaulter := SetDefaults(ctx); defaulter != nil {
if err := defaulter(&patches, newObj); err != nil {
logger.Error("Failed the resource specific defaulter", zap.Error(err))
Expand All @@ -558,103 +542,6 @@ func (ac *AdmissionController) mutate(ctx context.Context, kind metav1.GroupVers
return json.Marshal(patches)
}

// updateGeneration sets the generation by following this logic:
// if there's no old object, it's create, set generation to 1
// if there's an old object and spec has changed, set generation to oldGeneration + 1
// appends the patch to patches if changes are necessary.
// TODO: Generation does not work correctly with CRD. They are scrubbed
// by the APIserver (https://github.com/kubernetes/kubernetes/issues/58778)
// So, we add Generation here. Once that gets fixed, remove this and use
// ObjectMeta.Generation instead.
func updateGeneration(ctx context.Context, patches *[]jsonpatch.JsonPatchOperation, old GenericCRD, new GenericCRD) error {
logger := logging.FromContext(ctx)

if chg, err := hasChanged(ctx, old, new); err != nil {
return err
} else if !chg {
logger.Info("No changes in the spec, not bumping generation")
return nil
}

// Leverage Spec duck typing to bump the Generation of the resource.
before, err := asGenerational(ctx, new)
if err != nil {
return err
}
after := before.DeepCopyObject().(*duckv1alpha1.Generational)
after.Spec.Generation = after.Spec.Generation + 1

genBump, err := duck.CreatePatch(before, after)
if err != nil {
return err
}
*patches = append(*patches, genBump...)
return nil
}

// Not worth fully duck typing since there's no shared schema.
type hasSpec struct {
Spec json.RawMessage `json:"spec"`
}

func getSpecJSON(crd GenericCRD) ([]byte, error) {
b, err := json.Marshal(crd)
if err != nil {
return nil, err
}
hs := hasSpec{}
if err := json.Unmarshal(b, &hs); err != nil {
return nil, err
}
return []byte(hs.Spec), nil
}

func hasChanged(ctx context.Context, old, new GenericCRD) (bool, error) {
if old == nil {
return true, nil
}
logger := logging.FromContext(ctx)

oldSpecJSON, err := getSpecJSON(old)
if err != nil {
logger.Error("Failed to get Spec JSON for old", zap.Error(err))
return false, err
}
newSpecJSON, err := getSpecJSON(new)
if err != nil {
logger.Error("Failed to get Spec JSON for new", zap.Error(err))
return false, err
}

specPatches, err := jsonpatch.CreatePatch(oldSpecJSON, newSpecJSON)
if err != nil {
fmt.Printf("Error creating JSON patch:%v", err)
return false, err
}
if len(specPatches) == 0 {
return false, nil
}
specPatchesJSON, err := json.Marshal(specPatches)
if err != nil {
logger.Error("Failed to marshal spec patches", zap.Error(err))
return false, err
}
logger.Infof("Specs differ:\n%+v\n", string(specPatchesJSON))
return true, nil
}

func asGenerational(ctx context.Context, crd GenericCRD) (*duckv1alpha1.Generational, error) {
raw, err := json.Marshal(crd)
if err != nil {
return nil, err
}
kr := &duckv1alpha1.Generational{}
if err := json.Unmarshal(raw, kr); err != nil {
return nil, err
}
return kr, nil
}

func generateSecret(ctx context.Context, options *ControllerOptions) (*corev1.Secret, error) {
serverKey, serverCert, caCert, err := CreateCerts(ctx, options.ServiceName, options.Namespace)
if err != nil {
Expand Down
17 changes: 2 additions & 15 deletions webhook/webhook_integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (
"testing"
"time"

"github.com/google/go-cmp/cmp"
"github.com/mattbaird/jsonpatch"
admissionv1beta1 "k8s.io/api/admission/v1beta1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -171,7 +170,7 @@ func TestValidResponseForResource(t *testing.T) {
Kind: "Resource",
},
}
testRev := createResource(1234, "testrev")
testRev := createResource("testrev")
marshaled, err := json.Marshal(testRev)
if err != nil {
t.Fatalf("Failed to marshal resource: %s", err)
Expand Down Expand Up @@ -215,18 +214,6 @@ func TestValidResponseForResource(t *testing.T) {
if err != nil {
t.Fatalf("Failed to decode response: %v", err)
}

expectJsonPatch := incrementGenerationPatch(testRev.Spec.Generation)

var respPatch []jsonpatch.JsonPatchOperation
err = json.Unmarshal(reviewResponse.Response.Patch, &respPatch)
if err != nil {
t.Fatalf("Failed to unmarshal json patch %v", err)
}

if diff := cmp.Diff(respPatch[0], expectJsonPatch); diff != "" {
t.Errorf("Unexpected patch (-want, +got): %v", diff)
}
}

func TestInvalidResponseForResource(t *testing.T) {
Expand Down Expand Up @@ -254,7 +241,7 @@ func TestInvalidResponseForResource(t *testing.T) {
t.Fatalf("createSecureTLSClient() = %v", err)
}

resource := createResource(1, testResourceName)
resource := createResource(testResourceName)

resource.Spec.FieldWithValidation = "not the right value"
marshaled, err := json.Marshal(resource)
Expand Down
43 changes: 13 additions & 30 deletions webhook/webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,23 +115,19 @@ func TestUnknownKindFails(t *testing.T) {
}

func TestValidCreateResourceSucceeds(t *testing.T) {
r := createResource(1234, "a name")
r := createResource("a name")
r.SetDefaults() // Fill in defaults to check that there are no patches.
_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
resp := ac.admit(TestContextWithLogger(t), createCreateResource(&r))
expectAllowed(t, resp)
expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{
incrementGenerationPatch(r.Spec.Generation),
})
}

func TestValidCreateResourceSucceedsWithDefaultPatch(t *testing.T) {
r := createResource(1234, "a name")
r := createResource("a name")
_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
resp := ac.admit(TestContextWithLogger(t), createCreateResource(&r))
expectAllowed(t, resp)
expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{
incrementGenerationPatch(r.Spec.Generation),
{
Operation: "add",
Path: "/spec/fieldThatsImmutableWithDefault",
Expand All @@ -145,7 +141,7 @@ func TestValidCreateResourceSucceedsWithDefaultPatch(t *testing.T) {
}

func TestInvalidCreateResourceFails(t *testing.T) {
r := createResource(1234, "a name")
r := createResource("a name")

// Put a bad value in.
r.Spec.FieldWithValidation = "not what's expected"
Expand All @@ -156,7 +152,7 @@ func TestInvalidCreateResourceFails(t *testing.T) {
}

func TestNopUpdateResourceSucceeds(t *testing.T) {
r := createResource(1234, "a name")
r := createResource("a name")
r.SetDefaults() // Fill in defaults to check that there are no patches.
_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
resp := ac.admit(TestContextWithLogger(t), createUpdateResource(&r, &r))
Expand All @@ -165,19 +161,15 @@ func TestNopUpdateResourceSucceeds(t *testing.T) {
}

func TestValidUpdateResourceSucceeds(t *testing.T) {
old := createResource(1234, "a name")
old := createResource("a name")
old.SetDefaults() // Fill in defaults to check that there are no patches.
new := createResource(1234, "a name")
new := createResource("a name")
// We clear the field that has a default.

_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
resp := ac.admit(TestContextWithLogger(t), createUpdateResource(&old, &new))
expectAllowed(t, resp)
expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{{
Operation: "replace",
Path: "/spec/generation",
Value: 1235.0,
}, {
Operation: "add",
Path: "/spec/fieldThatsImmutableWithDefault",
Value: "this is another default value",
Expand All @@ -189,8 +181,8 @@ func TestValidUpdateResourceSucceeds(t *testing.T) {
}

func TestInvalidUpdateResourceFailsValidation(t *testing.T) {
old := createResource(1234, "a name")
new := createResource(1234, "a name")
old := createResource("a name")
new := createResource("a name")

// Try to update to a bad value.
new.Spec.FieldWithValidation = "not what's expected"
Expand All @@ -201,8 +193,8 @@ func TestInvalidUpdateResourceFailsValidation(t *testing.T) {
}

func TestInvalidUpdateResourceFailsImmutability(t *testing.T) {
old := createResource(1234, "a name")
new := createResource(1234, "a name")
old := createResource("a name")
new := createResource("a name")

// Try to change the value
new.Spec.FieldThatsImmutable = "a different value"
Expand All @@ -214,8 +206,8 @@ func TestInvalidUpdateResourceFailsImmutability(t *testing.T) {
}

func TestDefaultingImmutableFields(t *testing.T) {
old := createResource(1234, "a name")
new := createResource(1234, "a name")
old := createResource("a name")
new := createResource("a name")

// If we don't specify the new, but immutable field, we default it,
// and it is not rejected.
Expand Down Expand Up @@ -396,14 +388,13 @@ func createDeployment(ac *AdmissionController) {
ac.Client.ExtensionsV1beta1().Deployments("knative-something").Create(deployment)
}

func createResource(generation int64, name string) Resource {
func createResource(name string) Resource {
return Resource{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: name,
},
Spec: ResourceSpec{
Generation: generation,
FieldWithValidation: "magic value",
},
}
Expand Down Expand Up @@ -511,14 +502,6 @@ func expectPatches(t *testing.T, a []byte, e []jsonpatch.JsonPatchOperation) {
}
}

func incrementGenerationPatch(old int64) jsonpatch.JsonPatchOperation {
return jsonpatch.JsonPatchOperation{
Operation: "replace",
Path: "/spec/generation",
Value: float64(old) + 1.0,
}
}

func NewAdmissionController(client kubernetes.Interface, options ControllerOptions,
logger *zap.SugaredLogger) (*AdmissionController, error) {
return &AdmissionController{
Expand Down

0 comments on commit 99cc227

Please sign in to comment.