From f92165ca30a2b886184da575f113586b32140aa9 Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Wed, 2 May 2018 14:59:43 -0400 Subject: [PATCH 1/4] Enables status subresource in order to fix generation bumping * This requires k8s 1.10 with CustomResourceSubresources feature gate enabled * Patching is used instead of update when adding labels - prevents bumping the generation since nested specs have a k8s Time struct which, during serialization, are not omitted when empty https://github.com/elafros/elafros/issues/642 Signed-off-by: Brenda Chan --- Gopkg.lock | 1 + config/300-configuration.yaml | 2 + config/300-revision.yaml | 2 + config/300-route.yaml | 2 + config/300-service.yaml | 2 + .../serving/v1alpha1/configuration_types.go | 14 - .../v1alpha1/configuration_types_test.go | 12 - pkg/apis/serving/v1alpha1/revision_types.go | 14 - .../serving/v1alpha1/revision_types_test.go | 13 - pkg/apis/serving/v1alpha1/route_types.go | 14 - pkg/apis/serving/v1alpha1/service_types.go | 16 +- .../serving/v1alpha1/service_types_test.go | 299 +++++++++--------- pkg/controller/configuration/configuration.go | 26 +- .../configuration/configuration_test.go | 11 +- pkg/controller/label_helpers.go | 108 +++++++ pkg/controller/revision/revision.go | 10 +- pkg/controller/route/route.go | 43 ++- pkg/controller/route/route_test.go | 191 ++++++----- pkg/controller/service/service.go | 31 +- pkg/webhook/configuration_test.go | 8 +- pkg/webhook/route_test.go | 3 +- pkg/webhook/service_test.go | 8 +- pkg/webhook/webhook.go | 85 +---- pkg/webhook/webhook_test.go | 121 +++---- test/conformance/route_test.go | 4 +- .../apimachinery/pkg/api/equality/BUILD.bazel | 16 + .../apimachinery/pkg/api/equality/semantic.go | 49 +++ 27 files changed, 528 insertions(+), 577 deletions(-) create mode 100644 pkg/controller/label_helpers.go create mode 100644 vendor/k8s.io/apimachinery/pkg/api/equality/BUILD.bazel create mode 100644 vendor/k8s.io/apimachinery/pkg/api/equality/semantic.go diff --git a/Gopkg.lock b/Gopkg.lock index 4292dbaabebe..90dda64ccdf4 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -681,6 +681,7 @@ [[projects]] name = "k8s.io/apimachinery" packages = [ + "pkg/api/equality", "pkg/api/errors", "pkg/api/meta", "pkg/api/resource", diff --git a/config/300-configuration.yaml b/config/300-configuration.yaml index ce4906900bac..2a82e9bd607a 100644 --- a/config/300-configuration.yaml +++ b/config/300-configuration.yaml @@ -23,3 +23,5 @@ spec: kind: Configuration plural: configurations scope: Namespaced + subresources: + status: {} diff --git a/config/300-revision.yaml b/config/300-revision.yaml index 3d45a9ff756b..26845cfcf05d 100644 --- a/config/300-revision.yaml +++ b/config/300-revision.yaml @@ -23,3 +23,5 @@ spec: kind: Revision plural: revisions scope: Namespaced + subresources: + status: {} diff --git a/config/300-route.yaml b/config/300-route.yaml index 2d5697d873bf..d853be45c943 100644 --- a/config/300-route.yaml +++ b/config/300-route.yaml @@ -23,3 +23,5 @@ spec: kind: Route plural: routes scope: Namespaced + subresources: + status: {} diff --git a/config/300-service.yaml b/config/300-service.yaml index d94643c85a45..a786fa75402c 100644 --- a/config/300-service.yaml +++ b/config/300-service.yaml @@ -23,3 +23,5 @@ spec: kind: Service plural: services scope: Namespaced + subresources: + status: {} diff --git a/pkg/apis/serving/v1alpha1/configuration_types.go b/pkg/apis/serving/v1alpha1/configuration_types.go index 76923be6d297..c9216319348c 100644 --- a/pkg/apis/serving/v1alpha1/configuration_types.go +++ b/pkg/apis/serving/v1alpha1/configuration_types.go @@ -47,12 +47,6 @@ type Configuration struct { // ConfigurationSpec holds the desired state of the Configuration (from the client). type ConfigurationSpec struct { - // 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. - Generation int64 `json:"generation,omitempty"` - // Build optionally holds the specification for the build to // perform to produce the Revision's container image. Build *build.BuildSpec `json:"build,omitempty"` @@ -122,14 +116,6 @@ type ConfigurationList struct { Items []Configuration `json:"items"` } -func (r *Configuration) GetGeneration() int64 { - return r.Spec.Generation -} - -func (r *Configuration) SetGeneration(generation int64) { - r.Spec.Generation = generation -} - func (r *Configuration) GetSpecJSON() ([]byte, error) { return json.Marshal(r.Spec) } diff --git a/pkg/apis/serving/v1alpha1/configuration_types_test.go b/pkg/apis/serving/v1alpha1/configuration_types_test.go index 4d97a1cdb7d3..fcf4befe04a4 100644 --- a/pkg/apis/serving/v1alpha1/configuration_types_test.go +++ b/pkg/apis/serving/v1alpha1/configuration_types_test.go @@ -18,18 +18,6 @@ import ( corev1 "k8s.io/api/core/v1" ) -func TestConfigurationGeneration(t *testing.T) { - config := Configuration{} - if e, a := int64(0), config.GetGeneration(); e != a { - t.Errorf("empty revision generation should be 0 was: %d", a) - } - - config.SetGeneration(5) - if e, a := int64(5), config.GetGeneration(); e != a { - t.Errorf("getgeneration mismatch expected: %d got: %d", e, a) - } -} - func TestConfigurationIsReady(t *testing.T) { cases := []struct { name string diff --git a/pkg/apis/serving/v1alpha1/revision_types.go b/pkg/apis/serving/v1alpha1/revision_types.go index 4b21668ffa47..c559d2410d99 100644 --- a/pkg/apis/serving/v1alpha1/revision_types.go +++ b/pkg/apis/serving/v1alpha1/revision_types.go @@ -85,12 +85,6 @@ const ( // RevisionSpec holds the desired state of the Revision (from the client). type RevisionSpec struct { - // 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. - Generation int64 `json:"generation,omitempty"` - // ServingState holds a value describing the desired state the Kubernetes // resources should be in for this Revision. // Users must not specify this when creating a revision. It is expected @@ -191,14 +185,6 @@ type RevisionList struct { Items []Revision `json:"items"` } -func (r *Revision) GetGeneration() int64 { - return r.Spec.Generation -} - -func (r *Revision) SetGeneration(generation int64) { - r.Spec.Generation = generation -} - func (r *Revision) GetSpecJSON() ([]byte, error) { return json.Marshal(r.Spec) } diff --git a/pkg/apis/serving/v1alpha1/revision_types_test.go b/pkg/apis/serving/v1alpha1/revision_types_test.go index 7620d0dcf5d7..778f64e7dec8 100644 --- a/pkg/apis/serving/v1alpha1/revision_types_test.go +++ b/pkg/apis/serving/v1alpha1/revision_types_test.go @@ -19,19 +19,6 @@ import ( corev1 "k8s.io/api/core/v1" ) -func TestGeneration(t *testing.T) { - r := Revision{} - if a := r.GetGeneration(); a != 0 { - t.Errorf("empty revision generation should be 0 was: %d", a) - } - - r.SetGeneration(5) - if e, a := int64(5), r.GetGeneration(); e != a { - t.Errorf("getgeneration mismatch expected: %d got: %d", e, a) - } - -} - func TestIsReady(t *testing.T) { cases := []struct { name string diff --git a/pkg/apis/serving/v1alpha1/route_types.go b/pkg/apis/serving/v1alpha1/route_types.go index 8cd548650d5c..2d11ea84cc1c 100644 --- a/pkg/apis/serving/v1alpha1/route_types.go +++ b/pkg/apis/serving/v1alpha1/route_types.go @@ -71,12 +71,6 @@ type TrafficTarget struct { // RouteSpec holds the desired state of the Route (from the client). type RouteSpec struct { - // 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. - Generation int64 `json:"generation,omitempty"` - // Traffic specifies how to distribute traffic over a collection of Knative Serving Revisions and Configurations. Traffic []TrafficTarget `json:"traffic,omitempty"` } @@ -141,14 +135,6 @@ type RouteList struct { Items []Route `json:"items"` } -func (r *Route) GetGeneration() int64 { - return r.Spec.Generation -} - -func (r *Route) SetGeneration(generation int64) { - r.Spec.Generation = generation -} - func (r *Route) GetSpecJSON() ([]byte, error) { return json.Marshal(r.Spec) } diff --git a/pkg/apis/serving/v1alpha1/service_types.go b/pkg/apis/serving/v1alpha1/service_types.go index a76fbf1b7bcf..607d542ffecb 100644 --- a/pkg/apis/serving/v1alpha1/service_types.go +++ b/pkg/apis/serving/v1alpha1/service_types.go @@ -36,14 +36,8 @@ type Service struct { } // ServiceSpec represents the configuration for the Service object. -// Exactly one of its members (other than Generation) must be specified. +// Exactly one of its members must be specified. type ServiceSpec struct { - // 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. - Generation int64 `json:"generation,omitempty"` - // RunLatest defines a simple Service. It will automatically // configure a route that keeps the latest ready revision // from the supplied configuration running. @@ -108,14 +102,6 @@ type ServiceList struct { Items []Service `json:"items"` } -func (s *Service) GetGeneration() int64 { - return s.Spec.Generation -} - -func (s *Service) SetGeneration(generation int64) { - s.Spec.Generation = generation -} - func (s *Service) GetSpecJSON() ([]byte, error) { return json.Marshal(s.Spec) } diff --git a/pkg/apis/serving/v1alpha1/service_types_test.go b/pkg/apis/serving/v1alpha1/service_types_test.go index d712aeec09c7..f2960d52bd73 100644 --- a/pkg/apis/serving/v1alpha1/service_types_test.go +++ b/pkg/apis/serving/v1alpha1/service_types_test.go @@ -13,173 +13,160 @@ limitations under the License. package v1alpha1 import ( - "testing" + "testing" - corev1 "k8s.io/api/core/v1" + corev1 "k8s.io/api/core/v1" ) -func TestServiceGeneration(t *testing.T) { - service := Service{} - if got, want := service.GetGeneration(), int64(0); got != want { - t.Errorf("Empty Service generation should be %d, was %d", want, got) - } - - answer := int64(42) - service.SetGeneration(answer) - if got := service.GetGeneration(); got != answer { - t.Errorf("GetGeneration mismatch; got %d, want %d", got, answer) - } -} - func TestServiceIsReady(t *testing.T) { - cases := []struct { - name string - status ServiceStatus - isReady bool - }{ - { - name: "empty status should not be ready", - status: ServiceStatus{}, - isReady: false, - }, - { - name: "Different condition type should not be ready", - status: ServiceStatus{ - Conditions: []ServiceCondition{ - { - Type: "Foo", - Status: corev1.ConditionTrue, - }, - }, - }, - isReady: false, - }, - { - name: "False condition status should not be ready", - status: ServiceStatus{ - Conditions: []ServiceCondition{ - { - Type: ServiceConditionReady, - Status: corev1.ConditionFalse, - }, - }, - }, - isReady: false, - }, - { - name: "Unknown condition status should not be ready", - status: ServiceStatus{ - Conditions: []ServiceCondition{ - { - Type: ServiceConditionReady, - Status: corev1.ConditionUnknown, - }, - }, - }, - isReady: false, - }, - { - name: "Missing condition status should not be ready", - status: ServiceStatus{ - Conditions: []ServiceCondition{ - { - Type: ServiceConditionReady, - }, - }, - }, - isReady: false, - }, - { - name: "True condition status should be ready", - status: ServiceStatus{ - Conditions: []ServiceCondition{ - { - Type: ServiceConditionReady, - Status: corev1.ConditionTrue, - }, - }, - }, - isReady: true, - }, - { - name: "Multiple conditions with ready status should be ready", - status: ServiceStatus{ - Conditions: []ServiceCondition{ - { - Type: "Foo", - Status: corev1.ConditionTrue, - }, - { - Type: ServiceConditionReady, - Status: corev1.ConditionTrue, - }, - }, - }, - isReady: true, - }, - { - name: "Multiple conditions with ready status false should not be ready", - status: ServiceStatus{ - Conditions: []ServiceCondition{ - { - Type: "Foo", - Status: corev1.ConditionTrue, - }, - { - Type: ServiceConditionReady, - Status: corev1.ConditionFalse, - }, - }, - }, - isReady: false, - }, - } + cases := []struct { + name string + status ServiceStatus + isReady bool + }{ + { + name: "empty status should not be ready", + status: ServiceStatus{}, + isReady: false, + }, + { + name: "Different condition type should not be ready", + status: ServiceStatus{ + Conditions: []ServiceCondition{ + { + Type: "Foo", + Status: corev1.ConditionTrue, + }, + }, + }, + isReady: false, + }, + { + name: "False condition status should not be ready", + status: ServiceStatus{ + Conditions: []ServiceCondition{ + { + Type: ServiceConditionReady, + Status: corev1.ConditionFalse, + }, + }, + }, + isReady: false, + }, + { + name: "Unknown condition status should not be ready", + status: ServiceStatus{ + Conditions: []ServiceCondition{ + { + Type: ServiceConditionReady, + Status: corev1.ConditionUnknown, + }, + }, + }, + isReady: false, + }, + { + name: "Missing condition status should not be ready", + status: ServiceStatus{ + Conditions: []ServiceCondition{ + { + Type: ServiceConditionReady, + }, + }, + }, + isReady: false, + }, + { + name: "True condition status should be ready", + status: ServiceStatus{ + Conditions: []ServiceCondition{ + { + Type: ServiceConditionReady, + Status: corev1.ConditionTrue, + }, + }, + }, + isReady: true, + }, + { + name: "Multiple conditions with ready status should be ready", + status: ServiceStatus{ + Conditions: []ServiceCondition{ + { + Type: "Foo", + Status: corev1.ConditionTrue, + }, + { + Type: ServiceConditionReady, + Status: corev1.ConditionTrue, + }, + }, + }, + isReady: true, + }, + { + name: "Multiple conditions with ready status false should not be ready", + status: ServiceStatus{ + Conditions: []ServiceCondition{ + { + Type: "Foo", + Status: corev1.ConditionTrue, + }, + { + Type: ServiceConditionReady, + Status: corev1.ConditionFalse, + }, + }, + }, + isReady: false, + }, + } - for _, tc := range cases { - if e, a := tc.isReady, tc.status.IsReady(); e != a { - t.Errorf("%q expected: %v got: %v", tc.name, e, a) - } - } + for _, tc := range cases { + if e, a := tc.isReady, tc.status.IsReady(); e != a { + t.Errorf("%q expected: %v got: %v", tc.name, e, a) + } + } } func TestServiceConditions(t *testing.T) { - svc := &Service{} - foo := &ServiceCondition { - Type: "Foo", - Status: "True", - } - bar := &ServiceCondition { - Type: "Bar", - Status: "True", - } + svc := &Service{} + foo := &ServiceCondition{ + Type: "Foo", + Status: "True", + } + bar := &ServiceCondition{ + Type: "Bar", + Status: "True", + } - // Add a single condition. - svc.Status.SetCondition(foo) - if got, want := len(svc.Status.Conditions), 1; got != want { - t.Fatalf("Unexpected Condition length; got %d, want %d", got, want) - } + // Add a single condition. + svc.Status.SetCondition(foo) + if got, want := len(svc.Status.Conditions), 1; got != want { + t.Fatalf("Unexpected Condition length; got %d, want %d", got, want) + } - // Remove non-existent condition. - svc.Status.RemoveCondition(bar.Type) - if got, want := len(svc.Status.Conditions), 1; got != want { - t.Fatalf("Unexpected Condition length; got %d, want %d", got, want) - } + // Remove non-existent condition. + svc.Status.RemoveCondition(bar.Type) + if got, want := len(svc.Status.Conditions), 1; got != want { + t.Fatalf("Unexpected Condition length; got %d, want %d", got, want) + } - // Add a second Condition. - svc.Status.SetCondition(bar) - if got, want := len(svc.Status.Conditions), 2; got != want { - t.Fatalf("Unexpected Condition length; got %d, want %d", got, want) - } + // Add a second Condition. + svc.Status.SetCondition(bar) + if got, want := len(svc.Status.Conditions), 2; got != want { + t.Fatalf("Unexpected Condition length; got %d, want %d", got, want) + } - // Remove the first Condition. - svc.Status.RemoveCondition(foo.Type) - if got, want := len(svc.Status.Conditions), 1; got != want { - t.Fatalf("Unexpected condition length; got %d, want %d", got, want) - } + // Remove the first Condition. + svc.Status.RemoveCondition(foo.Type) + if got, want := len(svc.Status.Conditions), 1; got != want { + t.Fatalf("Unexpected condition length; got %d, want %d", got, want) + } - // Test Add nil condition. - svc.Status.SetCondition(nil) - if got, want := len(svc.Status.Conditions), 1; got != want { - t.Fatal("Error, nil condition was allowed to be added.") - } + // Test Add nil condition. + svc.Status.SetCondition(nil) + if got, want := len(svc.Status.Conditions), 1; got != want { + t.Fatal("Error, nil condition was allowed to be added.") + } } diff --git a/pkg/controller/configuration/configuration.go b/pkg/controller/configuration/configuration.go index b4bbee4e7363..bc600e6c2ab4 100644 --- a/pkg/controller/configuration/configuration.go +++ b/pkg/controller/configuration/configuration.go @@ -20,6 +20,7 @@ import ( "context" "fmt" + "github.com/golang/glog" buildv1alpha1 "github.com/knative/build/pkg/apis/build/v1alpha1" buildclientset "github.com/knative/build/pkg/client/clientset/versioned" "github.com/knative/serving/pkg/apis/serving" @@ -135,8 +136,8 @@ func (c *Controller) syncHandler(key string) error { // Configuration business logic if config.GetGeneration() == config.Status.ObservedGeneration { // TODO(vaikas): Check to see if Status.LatestCreatedRevisionName is ready and update Status.LatestReady - logger.Infof("Skipping reconcile since already reconciled %d == %d", - config.Spec.Generation, config.Status.ObservedGeneration) + glog.Infof("Skipping reconcile since already reconciled %d == %d", + config.Generation, config.Status.ObservedGeneration) return nil } @@ -198,7 +199,7 @@ func (c *Controller) syncHandler(key string) error { if rev.ObjectMeta.Annotations == nil { rev.ObjectMeta.Annotations = make(map[string]string) } - rev.ObjectMeta.Annotations[serving.ConfigurationGenerationAnnotationKey] = fmt.Sprintf("%v", config.Spec.Generation) + rev.ObjectMeta.Annotations[serving.ConfigurationGenerationAnnotationKey] = fmt.Sprintf("%v", config.Generation) // Delete revisions when the parent Configuration is deleted. rev.OwnerReferences = append(rev.OwnerReferences, *controllerRef) @@ -219,7 +220,7 @@ func (c *Controller) syncHandler(key string) error { // Also update the LatestCreatedRevisionName so that we'll know revision to check // for ready state so that when ready, we can make it Latest. config.Status.LatestCreatedRevisionName = created.ObjectMeta.Name - config.Status.ObservedGeneration = config.Spec.Generation + config.Status.ObservedGeneration = config.Generation logger.Infof("Updating the configuration status:\n%+v", config) @@ -232,25 +233,12 @@ func (c *Controller) syncHandler(key string) error { } func generateRevisionName(u *v1alpha1.Configuration) (string, error) { - // TODO: consider making sure the length of the - // string will not cause problems down the stack - if u.Spec.Generation == 0 { - return "", fmt.Errorf("configuration generation cannot be 0") - } - return fmt.Sprintf("%s-%05d", u.Name, u.Spec.Generation), nil + return fmt.Sprintf("%s-%05d", u.Name, u.Generation), nil } func (c *Controller) updateStatus(u *v1alpha1.Configuration) (*v1alpha1.Configuration, error) { configClient := c.ElaClientSet.ServingV1alpha1().Configurations(u.Namespace) - newu, err := configClient.Get(u.Name, metav1.GetOptions{}) - if err != nil { - return nil, err - } - newu.Status = u.Status - - // TODO: for CRD there's no updatestatus, so use normal update - return configClient.Update(newu) - // return configClient.UpdateStatus(newu) + return configClient.UpdateStatus(u) } func (c *Controller) addRevisionEvent(obj interface{}) { diff --git a/pkg/controller/configuration/configuration_test.go b/pkg/controller/configuration/configuration_test.go index e6218f684977..1c2db4791682 100644 --- a/pkg/controller/configuration/configuration_test.go +++ b/pkg/controller/configuration/configuration_test.go @@ -62,13 +62,12 @@ const ( func getTestConfiguration() *v1alpha1.Configuration { return &v1alpha1.Configuration{ ObjectMeta: metav1.ObjectMeta{ - SelfLink: "/apis/serving/v1alpha1/namespaces/test/configurations/test-config", - Name: "test-config", - Namespace: testNamespace, + SelfLink: "/apis/serving/v1alpha1/namespaces/test/configurations/test-config", + Name: "test-config", + Namespace: testNamespace, + Generation: 1, }, Spec: v1alpha1.ConfigurationSpec{ - //TODO(grantr): This is a workaround for generation initialization - Generation: 1, RevisionTemplate: v1alpha1.RevisionTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ @@ -225,7 +224,7 @@ func TestCreateConfigurationsCreatesRevision(t *testing.T) { t.Errorf("rev does not have configuration label <%s:%s>", serving.ConfigurationLabelKey, config.Name) } - if rev.Annotations[serving.ConfigurationGenerationAnnotationKey] != fmt.Sprintf("%v", config.Spec.Generation) { + if rev.Annotations[serving.ConfigurationGenerationAnnotationKey] != fmt.Sprintf("%v", config.Generation) { t.Errorf("rev does not have generation annotation <%s:%s>", serving.ConfigurationGenerationAnnotationKey, config.Name) } diff --git a/pkg/controller/label_helpers.go b/pkg/controller/label_helpers.go new file mode 100644 index 000000000000..777c314dc256 --- /dev/null +++ b/pkg/controller/label_helpers.go @@ -0,0 +1,108 @@ +package controller + +import ( + "encoding/json" + "fmt" + "strings" + + "github.com/knative/serving/pkg/apis/serving/v1alpha1" + "github.com/mattbaird/jsonpatch" + "k8s.io/apimachinery/pkg/types" +) + +type configurationClient interface { + Patch(string, types.PatchType, []byte, ...string) (result *v1alpha1.Configuration, err error) +} + +type revisionClient interface { + Patch(string, types.PatchType, []byte, ...string) (result *v1alpha1.Revision, err error) +} + +func SetConfigLabel( + client configurationClient, + configName, + labelKey, + labelValue string) error { + + patch, err := createAddLabelPatch(labelKey, labelValue) + if err != nil { + return err + } + + _, err = client.Patch(configName, types.JSONPatchType, patch) + return err +} + +func RemoveConfigLabel( + client configurationClient, + configName, + labelKey string) error { + + patch, err := createRemoveLabelPatch(labelKey) + if err != nil { + return err + } + + _, err = client.Patch(configName, types.JSONPatchType, patch) + return err +} + +func SetRevisionLabel( + client revisionClient, + revisionName, + labelKey, + labelValue string) error { + + patch, err := createAddLabelPatch(labelKey, labelValue) + if err != nil { + return err + } + + _, err = client.Patch(revisionName, types.JSONPatchType, patch) + return err + +} + +func RemoveRevisionLabel( + client revisionClient, + revisionName, + labelKey string) error { + + patch, err := createRemoveLabelPatch(labelKey) + if err != nil { + return err + } + + _, err = client.Patch(revisionName, types.JSONPatchType, patch) + return err +} + +func createAddLabelPatch(key, value string) ([]byte, error) { + patch := []jsonpatch.JsonPatchOperation{ + { + Operation: "add", + Path: makePath("/metadata/labels", key), + Value: value, + }, + } + + return json.Marshal(patch) +} + +func createRemoveLabelPatch(key string) ([]byte, error) { + patch := []jsonpatch.JsonPatchOperation{ + { + Operation: "remove", + Path: makePath("/metadata/labels", key), + }, + } + + return json.Marshal(patch) +} + +// TODO(bsnchan) switch to jsonpatch.MakePath when merged +// https://github.com/mattbaird/jsonpatch/pull/15 +func makePath(path, part string) string { + pathPart := strings.Replace(part, "/", "~1", -1) + return fmt.Sprintf("%s/%s", path, pathPart) +} diff --git a/pkg/controller/revision/revision.go b/pkg/controller/revision/revision.go index ed46bddf31db..c2ee2e3279ec 100644 --- a/pkg/controller/revision/revision.go +++ b/pkg/controller/revision/revision.go @@ -1014,15 +1014,7 @@ func (c *Controller) removeFinalizers(ctx context.Context, rev *v1alpha1.Revisio func (c *Controller) updateStatus(rev *v1alpha1.Revision) (*v1alpha1.Revision, error) { prClient := c.ElaClientSet.ServingV1alpha1().Revisions(rev.Namespace) - newRev, err := prClient.Get(rev.Name, metav1.GetOptions{}) - if err != nil { - return nil, err - } - newRev.Status = rev.Status - - // TODO: for CRD there's no updatestatus, so use normal update - return prClient.Update(newRev) - // return prClient.UpdateStatus(newRev) + return prClient.UpdateStatus(rev) } // Given an endpoint see if it's managed by us and return the diff --git a/pkg/controller/route/route.go b/pkg/controller/route/route.go index fe0c0d439a4e..2cdc74dc7bdc 100644 --- a/pkg/controller/route/route.go +++ b/pkg/controller/route/route.go @@ -22,6 +22,7 @@ import ( "fmt" "reflect" + "github.com/golang/glog" "github.com/josephburnett/k8sflag/pkg/k8sflag" corev1 "k8s.io/api/core/v1" v1beta1 "k8s.io/api/extensions/v1beta1" @@ -41,6 +42,7 @@ import ( "github.com/knative/serving/pkg/controller" "github.com/knative/serving/pkg/logging" "github.com/knative/serving/pkg/logging/logkey" + "go.opencensus.io/stats" "go.opencensus.io/tag" "go.uber.org/zap" @@ -234,7 +236,6 @@ func (c *Controller) syncTrafficTargetsAndUpdateRouteStatus(ctx context.Context, if err := c.extendRevisionsWithIndirectTrafficTargets(ctx, route, configMap, revMap); err != nil { return nil, err } - if err := c.deleteLabelForOutsideOfGivenConfigurations(ctx, route, configMap); err != nil { return nil, err } @@ -440,18 +441,16 @@ func (c *Controller) setLabelForGivenConfigurations( // Set label for newly added configurations as traffic target. for _, config := range configMap { - if config.Labels == nil { - config.Labels = make(map[string]string) - } else if _, ok := config.Labels[serving.RouteLabelKey]; ok { + if _, ok := config.Labels[serving.RouteLabelKey]; ok { continue } - config.Labels[serving.RouteLabelKey] = route.Name - if _, err := configClient.Update(config); err != nil { - logger.Errorf("Failed to update Configuration %s: %s", config.Name, err) + + err := controller.SetConfigLabel(configClient, config.Name, serving.RouteLabelKey, route.Name) + if err != nil { + logger.Errorf("Failed add route label to Configuration %q: %q", config.Name, err) return err } } - return nil } @@ -474,18 +473,15 @@ func (c *Controller) setLabelForGivenRevisions( } for _, rev := range revMap { - if rev.Labels == nil { - rev.Labels = make(map[string]string) - } else if _, ok := rev.Labels[serving.RouteLabelKey]; ok { + if _, ok := rev.Labels[serving.RouteLabelKey]; ok { continue } - rev.Labels[serving.RouteLabelKey] = route.Name - if _, err := revisionClient.Update(rev); err != nil { - logger.Errorf("Failed to add route label to Revision %s: %s", rev.Name, err) + err := controller.SetRevisionLabel(revisionClient, rev.Name, serving.RouteLabelKey, route.Name) + if err != nil { + glog.Errorf("Failed to add route label to Revision %q: %q", rev.Name, err) return err } } - return nil } @@ -508,14 +504,13 @@ func (c *Controller) deleteLabelForOutsideOfGivenConfigurations( // Delete label for newly removed configurations as traffic target. for _, config := range oldConfigsList.Items { if _, ok := configMap[config.Name]; !ok { - delete(config.Labels, serving.RouteLabelKey) - if _, err := configClient.Update(&config); err != nil { - logger.Errorf("Failed to update Configuration %s: %s", config.Name, err) + err := controller.RemoveConfigLabel(configClient, config.Name, serving.RouteLabelKey) + if err != nil { + logger.Errorf("Failed to delete route label from Configuration %q: %q", config.Name, err) return err } } } - return nil } @@ -538,9 +533,8 @@ func (c *Controller) deleteLabelForOutsideOfGivenRevisions( // Delete label for newly removed revisions as traffic target. for _, rev := range oldRevList.Items { if _, ok := revMap[rev.Name]; !ok { - delete(rev.Labels, serving.RouteLabelKey) - if _, err := revClient.Update(&rev); err != nil { - logger.Errorf("Failed to remove route label from Revision %s: %s", rev.Name, err) + if err := controller.RemoveRevisionLabel(revClient, rev.Name, serving.RouteLabelKey); err != nil { + logger.Errorf("Failed to delete route label from Revision %q: %q", rev.Name, err) return err } } @@ -775,8 +769,7 @@ func (c *Controller) updateStatus(route *v1alpha1.Route) (*v1alpha1.Route, error // Check if there is anything to update. if !reflect.DeepEqual(existing.Status, route.Status) { existing.Status = route.Status - // TODO: for CRD there's no updatestatus, so use normal update. - return routeClient.Update(existing) + return routeClient.UpdateStatus(existing) } return existing, nil } @@ -945,7 +938,7 @@ func (c *Controller) updateIngressEvent(old, new interface{}) { Status: corev1.ConditionTrue, }) - if _, err = routeClient.Update(route); err != nil { + if _, err = routeClient.UpdateStatus(route); err != nil { c.Logger.Error( "Error updating readiness of route upon ingress becoming", zap.Error(err), diff --git a/pkg/controller/route/route_test.go b/pkg/controller/route/route_test.go index aa9e2d541fa5..fc24b1e6f7ab 100644 --- a/pkg/controller/route/route_test.go +++ b/pkg/controller/route/route_test.go @@ -121,8 +121,6 @@ func getTestConfiguration() *v1alpha1.Configuration { Namespace: testNamespace, }, Spec: v1alpha1.ConfigurationSpec{ - // This is a workaround for generation initialization - Generation: 1, RevisionTemplate: v1alpha1.RevisionTemplateSpec{ Spec: v1alpha1.RevisionSpec{ Container: corev1.Container{ @@ -1030,19 +1028,22 @@ func TestCreateRouteDeletesOutdatedRouteRules(t *testing.T) { } } -func TestSetLabelToConfigurationDirectlyConfigured(t *testing.T) { +func TestRouteLabelIsSetWhenTrafficTargetIsAConfiguration(t *testing.T) { _, elaClient, controller, _, elaInformer := newTestController(t) config := getTestConfiguration() rev := getTestRevisionForConfig(config) route := getTestRouteWithTrafficTargets( - []v1alpha1.TrafficTarget{ - v1alpha1.TrafficTarget{ - ConfigurationName: config.Name, - Percent: 100, - }, - }, + []v1alpha1.TrafficTarget{{ConfigurationName: config.Name, Percent: 100}}, ) + var actions []kubetesting.PatchAction + reactionFunc := func(action kubetesting.Action) (bool, runtime.Object, error) { + actions = append(actions, action.(kubetesting.PatchAction)) + return false, nil, nil + } + + elaClient.AddReactor("patch", "configurations", reactionFunc) + elaClient.AddReactor("patch", "revisions", reactionFunc) elaClient.ServingV1alpha1().Configurations(testNamespace).Create(config) elaClient.ServingV1alpha1().Revisions(testNamespace).Create(rev) elaClient.ServingV1alpha1().Routes(testNamespace).Create(route) @@ -1050,37 +1051,23 @@ func TestSetLabelToConfigurationDirectlyConfigured(t *testing.T) { elaInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) controller.updateRouteEvent(KeyOrDie(route)) - config, err := elaClient.ServingV1alpha1().Configurations(testNamespace).Get(config.Name, metav1.GetOptions{}) - if err != nil { - t.Fatalf("error getting config: %v", err) + if len(actions) != 1 { + t.Fatalf("Expected the route label to produce only one route label patch: %+v", + actions) } - // Configuration should be labeled for this route - expectedLabels := map[string]string{serving.RouteLabelKey: route.Name} - if diff := cmp.Diff(expectedLabels, config.Labels); diff != "" { - t.Errorf("Unexpected label diff (-want +got): %v", diff) + if actions[0].GetResource().Resource != "configurations" { + t.Fatalf("Expected only the configuration to have an updated route label: %+v", + formatPatches(actions)) } -} -func TestSetLabelToRevisionDirectlyConfigured(t *testing.T) { - _, elaClient, controller, _, elaInformer := newTestController(t) - config := getTestConfiguration() - rev := getTestRevisionForConfig(config) - route := getTestRouteWithTrafficTargets( - []v1alpha1.TrafficTarget{ - v1alpha1.TrafficTarget{ - ConfigurationName: config.Name, - Percent: 100, - }, - }, - ) + // Configuration should have been patched to have the route label + want := `[{"op":"add","path":"/metadata/labels/serving.knative.dev~1route","value":"test-route"}]` + got := string(actions[0].GetPatch()) - elaClient.ServingV1alpha1().Configurations(testNamespace).Create(config) - elaClient.ServingV1alpha1().Revisions(testNamespace).Create(rev) - elaClient.ServingV1alpha1().Routes(testNamespace).Create(route) - // Since updateRouteEvent looks in the lister, we need to add it to the informer - elaInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) - controller.updateRouteEvent(KeyOrDie(route)) + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("Unexpected label diff (-want +got): %v", diff) + } rev, err := elaClient.ServingV1alpha1().Revisions(testNamespace).Get(rev.Name, metav1.GetOptions{}) if err != nil { @@ -1097,40 +1084,47 @@ func TestSetLabelToRevisionDirectlyConfigured(t *testing.T) { } // Mark the revision as the Config's latest ready revision + // Also since we've patched the config it should have the route label + // FYI: the client doesn't do this for the fakes config.Status.LatestReadyRevisionName = rev.Name - - elaClient.ServingV1alpha1().Configurations(testNamespace).Update(config) + config.Labels = map[string]string{serving.RouteLabelKey: route.Name} + elaClient.ServingV1alpha1().Configurations(testNamespace).UpdateStatus(config) controller.updateRouteEvent(KeyOrDie(route)) - rev, err = elaClient.ServingV1alpha1().Revisions(testNamespace).Get(rev.Name, metav1.GetOptions{}) - if err != nil { - t.Fatalf("error getting revision: %v", err) + if len(actions) != 2 { + t.Fatalf("Expected the route label to produce only one additional route label patch: %+v", + formatPatches(actions)) } - // Revision should have the route label - expectedLabels = map[string]string{ - serving.ConfigurationLabelKey: config.Name, - serving.RouteLabelKey: route.Name, + if actions[1].GetResource().Resource != "revisions" { + t.Fatalf("Expected only the revision to have an updated route label: %+v", + actions) } - if diff := cmp.Diff(expectedLabels, rev.Labels); diff != "" { + // Revision should have been patched to have the route label + got = string(actions[1].GetPatch()) + + if diff := cmp.Diff(want, got); diff != "" { t.Errorf("Unexpected label diff (-want +got): %v", diff) } } -func TestSetLabelToConfigurationAndRevisionIndirectlyConfigured(t *testing.T) { +func TestRouteLabelIsSetWhenTrafficTargetIsARevision(t *testing.T) { _, elaClient, controller, _, elaInformer := newTestController(t) config := getTestConfiguration() rev := getTestRevisionForConfig(config) route := getTestRouteWithTrafficTargets( - []v1alpha1.TrafficTarget{ - v1alpha1.TrafficTarget{ - RevisionName: rev.Name, - Percent: 100, - }, - }, + []v1alpha1.TrafficTarget{{RevisionName: rev.Name, Percent: 100}}, ) + var actions []kubetesting.PatchAction + reactionFunc := func(action kubetesting.Action) (bool, runtime.Object, error) { + actions = append(actions, action.(kubetesting.PatchAction)) + return false, nil, nil + } + + elaClient.AddReactor("patch", "configurations", reactionFunc) + elaClient.AddReactor("patch", "revisions", reactionFunc) elaClient.ServingV1alpha1().Configurations(testNamespace).Create(config) elaClient.ServingV1alpha1().Revisions(testNamespace).Create(rev) elaClient.ServingV1alpha1().Routes(testNamespace).Create(route) @@ -1138,30 +1132,26 @@ func TestSetLabelToConfigurationAndRevisionIndirectlyConfigured(t *testing.T) { elaInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) controller.updateRouteEvent(KeyOrDie(route)) - config, err := elaClient.ServingV1alpha1().Configurations(testNamespace).Get(config.Name, metav1.GetOptions{}) - if err != nil { - t.Fatalf("error getting config: %v", err) + if len(actions) != 2 { + t.Fatalf("Expected to have a route label patch for the revision and configuration: %+v", + formatPatches(actions)) } - // Configuration should be labeled for this route - expectedLabels := map[string]string{serving.RouteLabelKey: route.Name} - if diff := cmp.Diff(expectedLabels, config.Labels); diff != "" { - t.Errorf("Unexpected label in configuration diff (-want +got): %v", diff) + if actions[0].GetResource().Resource != "configurations" { + t.Fatalf("Expected the configuration to have an updated route label: %+v", actions) } - rev, err = elaClient.ServingV1alpha1().Revisions(testNamespace).Get(rev.Name, metav1.GetOptions{}) - if err != nil { - t.Fatalf("error getting revision: %v", err) - } - - // Revision should have the route label - expectedLabels = map[string]string{ - serving.ConfigurationLabelKey: config.Name, - serving.RouteLabelKey: route.Name, + if actions[1].GetResource().Resource != "revisions" { + t.Fatalf("Expected the revision to have an updated route label: %+v", actions) } - if diff := cmp.Diff(expectedLabels, rev.Labels); diff != "" { - t.Errorf("Unexpected label in revision diff (-want +got): %v", diff) + for _, a := range actions { + // The configuration and revision should have the route label + want := `[{"op":"add","path":"/metadata/labels/serving.knative.dev~1route","value":"test-route"}]` + got := string(a.GetPatch()) + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("Unexpected label diff (-want +got): %v", diff) + } } } @@ -1265,38 +1255,44 @@ func TestDeleteLabelOfConfigurationAndRevisionWhenUnconfigured(t *testing.T) { // Set a route label in revision which is expected to be deleted. rev.Labels[serving.RouteLabelKey] = route.Name + var actions []kubetesting.PatchAction + reactionFunc := func(action kubetesting.Action) (bool, runtime.Object, error) { + actions = append(actions, action.(kubetesting.PatchAction)) + return false, nil, nil + } + + elaClient.AddReactor("patch", "configurations", reactionFunc) + elaClient.AddReactor("patch", "revisions", reactionFunc) elaClient.ServingV1alpha1().Configurations(testNamespace).Create(config) elaClient.ServingV1alpha1().Revisions(testNamespace).Create(rev) + // Since updateRouteEvent looks in the lister, we need to add it to the informer elaInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) elaClient.ServingV1alpha1().Routes(testNamespace).Create(route) controller.updateRouteEvent(KeyOrDie(route)) - config, err := elaClient.ServingV1alpha1().Configurations(testNamespace).Get(config.Name, metav1.GetOptions{}) - if err != nil { - t.Fatalf("error getting config: %v", err) - } + if len(actions) != 2 { + t.Fatalf("Expected to have a route label patch for the revision and configuration: %+v", + formatPatches(actions)) - // Check labels in configuration, should be empty. - expectedLabels := map[string]string{} - if diff := cmp.Diff(expectedLabels, config.Labels); diff != "" { - t.Errorf("Unexpected label in Configuration diff (-want +got): %v", diff) } - rev, err = elaClient.ServingV1alpha1().Revisions(testNamespace).Get(rev.Name, metav1.GetOptions{}) - if err != nil { - t.Fatalf("error getting revision: %v", err) + if actions[0].GetResource().Resource != "configurations" { + t.Fatalf("Expected the configuration route label to be deleted: %+v", actions) } - expectedLabels = map[string]string{ - serving.ConfigurationLabelKey: config.Name, + if actions[1].GetResource().Resource != "revisions" { + t.Fatalf("Expected the revision route label to be deleted: %+v", actions) } - // Check labels in revision, should be empty. - if diff := cmp.Diff(expectedLabels, rev.Labels); diff != "" { - t.Errorf("Unexpected label in revision diff (-want +got): %v", diff) + for _, a := range actions { + // The configuration and revision's route label should be deleted + want := `[{"op":"remove","path":"/metadata/labels/serving.knative.dev~1route"}]` + got := string(a.GetPatch()) + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("Unexpected label diff (-want +got): %v", diff) + } } - } func TestUpdateRouteDomainWhenRouteLabelChanges(t *testing.T) { @@ -1504,3 +1500,24 @@ func TestUpdateIngressEventUpdateRouteStatus(t *testing.T) { t.Errorf("Unexpected condition diff (-want +got): %v", diff) } } + +func formatPatches(actions []kubetesting.PatchAction) []interface{} { + result := make([]interface{}, 0, len(actions)) + + for _, a := range actions { + result = append(result, prettyPatch{ + patch: string(a.GetPatch()), + name: a.GetName(), + resource: a.GetResource().Resource, + }) + + } + + return result +} + +type prettyPatch struct { + name string + resource string + patch string +} diff --git a/pkg/controller/service/service.go b/pkg/controller/service/service.go index f5b8a908a9b8..e85aea2d05d4 100644 --- a/pkg/controller/service/service.go +++ b/pkg/controller/service/service.go @@ -18,9 +18,10 @@ package service import ( "fmt" - "reflect" "go.uber.org/zap" + + apiequality "k8s.io/apimachinery/pkg/api/equality" apierrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/runtime" @@ -147,7 +148,7 @@ func (c *Controller) updateServiceEvent(key string) error { // TODO(#642): Remove this. if service.GetGeneration() == service.Status.ObservedGeneration { logger.Infof("Skipping reconcile since already reconciled %d == %d", - service.Spec.Generation, service.Status.ObservedGeneration) + service.Generation, service.Status.ObservedGeneration) return nil } @@ -171,7 +172,7 @@ func (c *Controller) updateServiceEvent(key string) error { // Update the Status of the Service with the latest generation that // we just reconciled against so we don't keep generating Revisions. // TODO(#642): Remove this. - service.Status.ObservedGeneration = service.Spec.Generation + service.Status.ObservedGeneration = service.Generation logger.Infof("Updating the Service status:\n%+v", service) @@ -185,17 +186,7 @@ func (c *Controller) updateServiceEvent(key string) error { func (c *Controller) updateStatus(service *v1alpha1.Service) (*v1alpha1.Service, error) { serviceClient := c.ElaClientSet.ServingV1alpha1().Services(service.Namespace) - existing, err := serviceClient.Get(service.Name, metav1.GetOptions{}) - if err != nil { - return nil, err - } - // Check if there is anything to update. - if !reflect.DeepEqual(existing.Status, service.Status) { - existing.Status = service.Status - // TODO: for CRD there's no updatestatus, so use normal update. - return serviceClient.Update(existing) - } - return existing, nil + return serviceClient.UpdateStatus(service) } func (c *Controller) reconcileConfiguration(config *v1alpha1.Configuration) error { @@ -209,7 +200,11 @@ func (c *Controller) reconcileConfiguration(config *v1alpha1.Configuration) erro } return err } - // TODO(vaikas): Perhaps only update if there are actual changes. + + if apiequality.Semantic.DeepEqual(existing.Spec, config.Spec) { + return nil + } + copy := existing.DeepCopy() copy.Spec = config.Spec _, err = configClient.Update(copy) @@ -227,7 +222,11 @@ func (c *Controller) reconcileRoute(route *v1alpha1.Route) error { } return err } - // TODO(vaikas): Perhaps only update if there are actual changes. + + if apiequality.Semantic.DeepEqual(existing.Spec, route.Spec) { + return nil + } + copy := existing.DeepCopy() copy.Spec = route.Spec _, err = routeClient.Update(copy) diff --git a/pkg/webhook/configuration_test.go b/pkg/webhook/configuration_test.go index 81ff39c9f886..08038467c1a2 100644 --- a/pkg/webhook/configuration_test.go +++ b/pkg/webhook/configuration_test.go @@ -20,6 +20,7 @@ import ( "strings" "testing" + build "github.com/knative/build/pkg/apis/build/v1alpha1" "github.com/knative/serving/pkg/apis/serving/v1alpha1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" @@ -27,7 +28,7 @@ import ( ) func TestValidConfigurationAllowed(t *testing.T) { - configuration := createConfiguration(testGeneration, testConfigurationName) + configuration := createConfiguration(testConfigurationName) if err := ValidateConfiguration(testCtx)(nil, &configuration, &configuration); err != nil { t.Fatalf("Expected allowed. Failed with %s", err) @@ -61,7 +62,7 @@ func TestEmptyTemplateInSpecNotAllowed(t *testing.T) { Name: testConfigurationName, }, Spec: v1alpha1.ConfigurationSpec{ - Generation: testGeneration, + Build: new(build.BuildSpec), RevisionTemplate: v1alpha1.RevisionTemplateSpec{}, }, } @@ -78,7 +79,6 @@ func TestEmptyContainerNotAllowed(t *testing.T) { Name: testConfigurationName, }, Spec: v1alpha1.ConfigurationSpec{ - Generation: testGeneration, RevisionTemplate: v1alpha1.RevisionTemplateSpec{ Spec: v1alpha1.RevisionSpec{ ServiceAccountName: "Fred", @@ -102,7 +102,6 @@ func TestServingStateNotAllowed(t *testing.T) { Name: testConfigurationName, }, Spec: v1alpha1.ConfigurationSpec{ - Generation: testGeneration, RevisionTemplate: v1alpha1.RevisionTemplateSpec{ Spec: v1alpha1.RevisionSpec{ ServingState: v1alpha1.RevisionServingStateActive, @@ -140,7 +139,6 @@ func TestUnwantedFieldInContainerNotAllowed(t *testing.T) { Name: testConfigurationName, }, Spec: v1alpha1.ConfigurationSpec{ - Generation: testGeneration, RevisionTemplate: v1alpha1.RevisionTemplateSpec{ Spec: v1alpha1.RevisionSpec{ Container: container, diff --git a/pkg/webhook/route_test.go b/pkg/webhook/route_test.go index d8be13c457e2..d313b987fe21 100644 --- a/pkg/webhook/route_test.go +++ b/pkg/webhook/route_test.go @@ -29,8 +29,7 @@ func createRouteWithTraffic(trafficTargets []v1alpha1.TrafficTarget) v1alpha1.Ro Name: testRouteName, }, Spec: v1alpha1.RouteSpec{ - Generation: testGeneration, - Traffic: trafficTargets, + Traffic: trafficTargets, }, } } diff --git a/pkg/webhook/service_test.go b/pkg/webhook/service_test.go index 4afdbf975abe..1c32987ff808 100644 --- a/pkg/webhook/service_test.go +++ b/pkg/webhook/service_test.go @@ -39,7 +39,7 @@ func TestRunLatest(t *testing.T) { s := v1alpha1.Service{ Spec: v1alpha1.ServiceSpec{ RunLatest: &v1alpha1.RunLatestType{ - Configuration: createConfiguration(1, "config").Spec, + Configuration: createConfiguration("config").Spec, }, }, } @@ -69,7 +69,7 @@ func TestPinned(t *testing.T) { Spec: v1alpha1.ServiceSpec{ Pinned: &v1alpha1.PinnedType{ RevisionName: "revision", - Configuration: createConfiguration(1, "config").Spec, + Configuration: createConfiguration("config").Spec, }, }, } @@ -117,7 +117,7 @@ func TestPinnedSetsDefaults(t *testing.T) { s := v1alpha1.Service{ Spec: v1alpha1.ServiceSpec{ Pinned: &v1alpha1.PinnedType{ - Configuration: createConfiguration(1, "config").Spec, + Configuration: createConfiguration("config").Spec, }, }, } @@ -147,7 +147,7 @@ func TestLatestSetsDefaults(t *testing.T) { s := v1alpha1.Service{ Spec: v1alpha1.ServiceSpec{ RunLatest: &v1alpha1.RunLatestType{ - Configuration: createConfiguration(1, "config").Spec, + Configuration: createConfiguration("config").Spec, }, }, } diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index 4542aa78a898..1143e8509539 100644 --- a/pkg/webhook/webhook.go +++ b/pkg/webhook/webhook.go @@ -447,13 +447,11 @@ func (ac *AdmissionController) admit(ctx context.Context, request *admissionv1be } logger.Infof("Kind: %q PatchBytes: %v", request.Kind, string(patchBytes)) + patchType := admissionv1beta1.PatchTypeJSONPatch return &admissionv1beta1.AdmissionResponse{ - Patch: patchBytes, - Allowed: true, - PatchType: func() *admissionv1beta1.PatchType { - pt := admissionv1beta1.PatchTypeJSONPatch - return &pt - }(), + Patch: patchBytes, + Allowed: true, + PatchType: &patchType, } } @@ -492,12 +490,6 @@ func (ac *AdmissionController) mutate(ctx context.Context, kind string, oldBytes 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 := handler.Defaulter; defaulter != nil { if err := defaulter(&patches, newObj); err != nil { logger.Error("Failed the resource specific defaulter", zap.Error(err)) @@ -518,6 +510,7 @@ func (ac *AdmissionController) mutate(ctx context.Context, kind string, oldBytes logger.Error("Failed to validate", zap.Error(err)) return nil, fmt.Errorf("Failed to validate: %s", err) } + return json.Marshal(patches) } @@ -534,74 +527,6 @@ func validateMetadata(new GenericCRD) error { return nil } -// 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) - var oldGeneration int64 - if old == nil { - logger.Info("Old is nil") - } else { - oldGeneration = old.GetGeneration() - } - if oldGeneration == 0 { - logger.Info("Creating an object, setting generation to 1") - *patches = append(*patches, jsonpatch.JsonPatchOperation{ - Operation: "add", - Path: "/spec/generation", - Value: 1, - }) - return nil - } - - oldSpecJSON, err := old.GetSpecJSON() - if err != nil { - logger.Error("Failed to get Spec JSON for old", zap.Error(err)) - } - newSpecJSON, err := new.GetSpecJSON() - if err != nil { - logger.Error("Failed to get Spec JSON for new", zap.Error(err)) - } - - specPatches, err := jsonpatch.CreatePatch(oldSpecJSON, newSpecJSON) - if err != nil { - fmt.Printf("Error creating JSON patch:%v", err) - return err - } - if len(specPatches) > 0 { - specPatchesJSON, err := json.Marshal(specPatches) - if err != nil { - logger.Error("Failed to marshal spec patches", zap.Error(err)) - return err - } - logger.Infof("Specs differ:\n%+v\n", string(specPatchesJSON)) - - operation := "replace" - if newGeneration := new.GetGeneration(); newGeneration == 0 { - // If new is missing Generation, we need to "add" instead of "replace". - // We see this for Service resources because the initial generation is - // added to the managed Configuration and Route, but not the Service - // that manages them. - // TODO(#642): Remove this. - operation = "add" - } - *patches = append(*patches, jsonpatch.JsonPatchOperation{ - Operation: operation, - Path: "/spec/generation", - Value: oldGeneration + 1, - }) - return nil - } - logger.Info("No changes in the spec, not bumping generation") - return nil -} - func generateSecret(ctx context.Context, name, namespace string) (*corev1.Secret, error) { serverKey, serverCert, caCert, err := CreateCerts(ctx) if err != nil { diff --git a/pkg/webhook/webhook_test.go b/pkg/webhook/webhook_test.go index 2abc61cc7355..59cbf5e64ebd 100644 --- a/pkg/webhook/webhook_test.go +++ b/pkg/webhook/webhook_test.go @@ -52,7 +52,6 @@ const ( imageName = "test-container-image" envVarName = "envname" envVarValue = "envvalue" - testGeneration = 1 testRouteName = "test-route-name" testRevisionName = "test-revision" testServiceName = "test-service-name" @@ -141,7 +140,7 @@ func TestInvalidNewConfigurationNameFails(t *testing.T) { Kind: metav1.GroupVersionKind{Kind: "Configuration"}, } invalidName := "configuration.example" - config := createConfiguration(0, invalidName) + config := createConfiguration(invalidName) marshaled, err := json.Marshal(config) if err != nil { t.Fatalf("Failed to marshal configuration: %s", err) @@ -150,7 +149,7 @@ func TestInvalidNewConfigurationNameFails(t *testing.T) { expectFailsWith(t, ac.admit(testCtx, req), "Invalid resource name") invalidName = strings.Repeat("a", 64) - config = createConfiguration(0, invalidName) + config = createConfiguration(invalidName) marshaled, err = json.Marshal(config) if err != nil { t.Fatalf("Failed to marshal configuration: %s", err) @@ -163,14 +162,13 @@ func TestValidNewConfigurationObject(t *testing.T) { _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) resp := ac.admit(testCtx, createValidCreateConfiguration()) expectAllowed(t, resp) - p := incrementGenerationPatch(0) - expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{p}) + expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{}) } func TestValidConfigurationNoChanges(t *testing.T) { _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) - old := createConfiguration(testGeneration, testConfigurationName) - new := createConfiguration(testGeneration, testConfigurationName) + old := createConfiguration(testConfigurationName) + new := createConfiguration(testConfigurationName) resp := ac.admit(testCtx, createUpdateConfiguration(&old, &new)) expectAllowed(t, resp) expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{}) @@ -178,8 +176,8 @@ func TestValidConfigurationNoChanges(t *testing.T) { func TestValidConfigurationEnvChanges(t *testing.T) { _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) - old := createConfiguration(testGeneration, testConfigurationName) - new := createConfiguration(testGeneration, testConfigurationName) + old := createConfiguration(testConfigurationName) + new := createConfiguration(testConfigurationName) new.Spec.RevisionTemplate.Spec.Container.Env = []corev1.EnvVar{ corev1.EnvVar{ Name: envVarName, @@ -188,13 +186,7 @@ func TestValidConfigurationEnvChanges(t *testing.T) { } resp := ac.admit(testCtx, createUpdateConfiguration(&old, &new)) expectAllowed(t, resp) - expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{ - jsonpatch.JsonPatchOperation{ - Operation: "replace", - Path: "/spec/generation", - Value: 2, - }, - }) + expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{}) } func TestInvalidNewRouteNameFails(t *testing.T) { @@ -204,7 +196,7 @@ func TestInvalidNewRouteNameFails(t *testing.T) { Kind: metav1.GroupVersionKind{Kind: "Route"}, } invalidName := "route.example" - config := createRoute(0, invalidName) + config := createRoute(invalidName) marshaled, err := json.Marshal(config) if err != nil { t.Fatalf("Failed to marshal route: %s", err) @@ -213,7 +205,7 @@ func TestInvalidNewRouteNameFails(t *testing.T) { expectFailsWith(t, ac.admit(testCtx, req), "Invalid resource name") invalidName = strings.Repeat("a", 64) - config = createRoute(0, invalidName) + config = createRoute(invalidName) marshaled, err = json.Marshal(config) if err != nil { t.Fatalf("Failed to marshal route: %s", err) @@ -226,14 +218,13 @@ func TestValidNewRouteObject(t *testing.T) { _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) resp := ac.admit(testCtx, createValidCreateRoute()) expectAllowed(t, resp) - p := incrementGenerationPatch(0) - expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{p}) + expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{}) } func TestValidRouteNoChanges(t *testing.T) { _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) - old := createRoute(1, testRouteName) - new := createRoute(1, testRouteName) + old := createRoute(testRouteName) + new := createRoute(testRouteName) resp := ac.admit(testCtx, createUpdateRoute(&old, &new)) expectAllowed(t, resp) expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{}) @@ -241,7 +232,7 @@ func TestValidRouteNoChanges(t *testing.T) { func TestInvalidOldRoute(t *testing.T) { _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) - new := createRoute(1, testRouteName) + new := createRoute(testRouteName) newBytes, err := json.Marshal(new) if err != nil { t.Errorf("Marshal(%v) = %v", new, err) @@ -253,7 +244,7 @@ func TestInvalidOldRoute(t *testing.T) { func TestInvalidNewRoute(t *testing.T) { _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) - old := createRoute(1, testRouteName) + old := createRoute(testRouteName) oldBytes, err := json.Marshal(old) if err != nil { t.Errorf("Marshal(%v) = %v", old, err) @@ -265,8 +256,8 @@ func TestInvalidNewRoute(t *testing.T) { func TestValidRouteChanges(t *testing.T) { _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) - old := createRoute(1, testRouteName) - new := createRoute(1, testRouteName) + old := createRoute(testRouteName) + new := createRoute(testRouteName) new.Spec.Traffic = []v1alpha1.TrafficTarget{ v1alpha1.TrafficTarget{ RevisionName: testRevisionName, @@ -275,13 +266,7 @@ func TestValidRouteChanges(t *testing.T) { } resp := ac.admit(testCtx, createUpdateRoute(&old, &new)) expectAllowed(t, resp) - expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{ - jsonpatch.JsonPatchOperation{ - Operation: "replace", - Path: "/spec/generation", - Value: 2, - }, - }) + expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{}) } func TestValidNewRevisionObject(t *testing.T) { @@ -300,11 +285,6 @@ func TestValidNewRevisionObject(t *testing.T) { resp := ac.admit(testCtx, req) expectAllowed(t, resp) expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{ - jsonpatch.JsonPatchOperation{ - Operation: "add", - Path: "/spec/generation", - Value: 1, - }, jsonpatch.JsonPatchOperation{ Operation: "add", Path: "/spec/servingState", @@ -337,13 +317,7 @@ func TestValidRevisionUpdates(t *testing.T) { req.Object.Raw = marshaled resp := ac.admit(testCtx, req) expectAllowed(t, resp) - expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{ - jsonpatch.JsonPatchOperation{ - Operation: "add", - Path: "/spec/generation", - Value: 1, - }, - }) + expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{}) } func TestInvalidRevisionUpdate(t *testing.T) { @@ -403,36 +377,34 @@ func TestValidNewServicePinned(t *testing.T) { _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) resp := ac.admit(testCtx, createValidCreateServicePinned()) expectAllowed(t, resp) - p := incrementGenerationPatch(0) - expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{p}) + expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{}) } func TestValidNewServiceRunLatest(t *testing.T) { _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) resp := ac.admit(testCtx, createValidCreateServiceRunLatest()) expectAllowed(t, resp) - p := incrementGenerationPatch(0) - expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{p}) + expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{}) } func TestInvalidNewServiceNoSpecs(t *testing.T) { _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) - svc := createServicePinned(0, testServiceName) + svc := createServicePinned(testServiceName) svc.Spec.Pinned = nil expectFailsWith(t, ac.admit(testCtx, createCreateService(svc)), "exactly one of runLatest or pinned") } func TestInvalidNewServiceNoRevisionNameInPinned(t *testing.T) { _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) - svc := createServicePinned(0, testServiceName) + svc := createServicePinned(testServiceName) svc.Spec.Pinned.RevisionName = "" expectFailsWith(t, ac.admit(testCtx, createCreateService(svc)), "spec.pinned.revisionName") } func TestValidServiceEnvChanges(t *testing.T) { _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) - old := createServicePinned(testGeneration, testServiceName) - new := createServicePinned(testGeneration, testServiceName) + old := createServicePinned(testServiceName) + new := createServicePinned(testServiceName) new.Spec.Pinned.Configuration.RevisionTemplate.Spec.Container.Env = []corev1.EnvVar{ corev1.EnvVar{ Name: envVarName, @@ -441,13 +413,6 @@ func TestValidServiceEnvChanges(t *testing.T) { } resp := ac.admit(testCtx, createUpdateService(&old, &new)) expectAllowed(t, resp) - expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{ - jsonpatch.JsonPatchOperation{ - Operation: "replace", - Path: "/spec/generation", - Value: 2, - }, - }) } func TestValidWebhook(t *testing.T) { @@ -504,7 +469,7 @@ func createValidCreateConfiguration() *admissionv1beta1.AdmissionRequest { Operation: admissionv1beta1.Create, Kind: metav1.GroupVersionKind{Kind: "Configuration"}, } - config := createConfiguration(0, testConfigurationName) + config := createConfiguration(testConfigurationName) marshaled, err := json.Marshal(config) if err != nil { panic("failed to marshal configuration") @@ -525,7 +490,7 @@ func createValidCreateRoute() *admissionv1beta1.AdmissionRequest { Operation: admissionv1beta1.Create, Kind: metav1.GroupVersionKind{Kind: "Route"}, } - route := createRoute(0, testRouteName) + route := createRoute(testRouteName) marshaled, err := json.Marshal(route) if err != nil { panic("failed to marshal route") @@ -579,12 +544,12 @@ func createBaseUpdateService() *admissionv1beta1.AdmissionRequest { func createUpdateService(old, new *v1alpha1.Service) *admissionv1beta1.AdmissionRequest { req := createBaseUpdateService() - marshaled, err := json.Marshal(old) + marshaled, err := json.Marshal(new) if err != nil { panic("failed to marshal service") } req.Object.Raw = marshaled - marshaledOld, err := json.Marshal(new) + marshaledOld, err := json.Marshal(old) if err != nil { panic("failed to marshal service") } @@ -606,11 +571,11 @@ func createCreateService(service v1alpha1.Service) *admissionv1beta1.AdmissionRe } func createValidCreateServicePinned() *admissionv1beta1.AdmissionRequest { - return createCreateService(createServicePinned(0, testServiceName)) + return createCreateService(createServicePinned(testServiceName)) } func createValidCreateServiceRunLatest() *admissionv1beta1.AdmissionRequest { - return createCreateService(createServiceRunLatest(0, testServiceName)) + return createCreateService(createServiceRunLatest(testServiceName)) } func createWebhook(ac *AdmissionController, webhook *admissionregistrationv1beta1.MutatingWebhookConfiguration) { @@ -672,14 +637,13 @@ func expectPatches(t *testing.T, a []byte, e []jsonpatch.JsonPatchOperation) { } } -func createConfiguration(generation int64, configurationName string) v1alpha1.Configuration { +func createConfiguration(configurationName string) v1alpha1.Configuration { return v1alpha1.Configuration{ ObjectMeta: metav1.ObjectMeta{ Namespace: testNamespace, Name: configurationName, }, Spec: v1alpha1.ConfigurationSpec{ - Generation: generation, RevisionTemplate: v1alpha1.RevisionTemplateSpec{ Spec: v1alpha1.RevisionSpec{ Container: corev1.Container{ @@ -696,14 +660,13 @@ func createConfiguration(generation int64, configurationName string) v1alpha1.Co } } -func createRoute(generation int64, routeName string) v1alpha1.Route { +func createRoute(routeName string) v1alpha1.Route { return v1alpha1.Route{ ObjectMeta: metav1.ObjectMeta{ Namespace: testNamespace, Name: routeName, }, Spec: v1alpha1.RouteSpec{ - Generation: generation, Traffic: []v1alpha1.TrafficTarget{ v1alpha1.TrafficTarget{ Name: "test-traffic-target", @@ -730,41 +693,31 @@ func createRevision(revName string) v1alpha1.Revision { } } -func createServicePinned(generation int64, serviceName string) v1alpha1.Service { +func createServicePinned(serviceName string) v1alpha1.Service { return v1alpha1.Service{ ObjectMeta: metav1.ObjectMeta{ Namespace: testNamespace, Name: serviceName, }, Spec: v1alpha1.ServiceSpec{ - Generation: generation, Pinned: &v1alpha1.PinnedType{ RevisionName: testRevisionName, - Configuration: createConfiguration(generation, "config").Spec, + Configuration: createConfiguration("config").Spec, }, }, } } -func createServiceRunLatest(generation int64, serviceName string) v1alpha1.Service { +func createServiceRunLatest(serviceName string) v1alpha1.Service { return v1alpha1.Service{ ObjectMeta: metav1.ObjectMeta{ Namespace: testNamespace, Name: serviceName, }, Spec: v1alpha1.ServiceSpec{ - Generation: generation, RunLatest: &v1alpha1.RunLatestType{ - Configuration: createConfiguration(generation, "config").Spec, + Configuration: createConfiguration("config").Spec, }, }, } } - -func incrementGenerationPatch(old int64) jsonpatch.JsonPatchOperation { - return jsonpatch.JsonPatchOperation{ - Operation: "add", - Path: "/spec/generation", - Value: old + 1, - } -} diff --git a/test/conformance/route_test.go b/test/conformance/route_test.go index 57fbaa76b454..a6de5b88cfb2 100644 --- a/test/conformance/route_test.go +++ b/test/conformance/route_test.go @@ -76,8 +76,8 @@ func updateConfigWithImage(clients *test.Clients, imagePaths []string) error { if err != nil { return err } - if newConfig.Spec.Generation != int64(2) { - return fmt.Errorf("The spec was updated so the Generation should be 2 but it was actually %d", newConfig.Spec.Generation) + if newConfig.Generation != int64(2) { + return fmt.Errorf("The spec was updated so the Generation should be 2 but it was actually %d", newConfig.Generation) } return nil } diff --git a/vendor/k8s.io/apimachinery/pkg/api/equality/BUILD.bazel b/vendor/k8s.io/apimachinery/pkg/api/equality/BUILD.bazel new file mode 100644 index 000000000000..1024fe65c50b --- /dev/null +++ b/vendor/k8s.io/apimachinery/pkg/api/equality/BUILD.bazel @@ -0,0 +1,16 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library") + +go_library( + name = "go_default_library", + srcs = ["semantic.go"], + importmap = "elafros/vendor/k8s.io/apimachinery/pkg/api/equality", + importpath = "k8s.io/apimachinery/pkg/api/equality", + visibility = ["//visibility:public"], + deps = [ + "//vendor/k8s.io/apimachinery/pkg/api/resource:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/conversion:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/fields:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/labels:go_default_library", + ], +) diff --git a/vendor/k8s.io/apimachinery/pkg/api/equality/semantic.go b/vendor/k8s.io/apimachinery/pkg/api/equality/semantic.go new file mode 100644 index 000000000000..f02fa8e4340e --- /dev/null +++ b/vendor/k8s.io/apimachinery/pkg/api/equality/semantic.go @@ -0,0 +1,49 @@ +/* +Copyright 2014 The Kubernetes 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 equality + +import ( + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/conversion" + "k8s.io/apimachinery/pkg/fields" + "k8s.io/apimachinery/pkg/labels" +) + +// Semantic can do semantic deep equality checks for api objects. +// Example: apiequality.Semantic.DeepEqual(aPod, aPodWithNonNilButEmptyMaps) == true +var Semantic = conversion.EqualitiesOrDie( + func(a, b resource.Quantity) bool { + // Ignore formatting, only care that numeric value stayed the same. + // TODO: if we decide it's important, it should be safe to start comparing the format. + // + // Uninitialized quantities are equivalent to 0 quantities. + return a.Cmp(b) == 0 + }, + func(a, b metav1.MicroTime) bool { + return a.UTC() == b.UTC() + }, + func(a, b metav1.Time) bool { + return a.UTC() == b.UTC() + }, + func(a, b labels.Selector) bool { + return a.String() == b.String() + }, + func(a, b fields.Selector) bool { + return a.String() == b.String() + }, +) From 5a375589ea0c0f6009a6e79723ac14a20a8021a5 Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Mon, 7 May 2018 15:34:09 -0400 Subject: [PATCH 2/4] Refactor adding/removing labels to a helper Signed-off-by: Brenda Chan --- pkg/controller/configuration/configuration.go | 3 ++- pkg/controller/configuration/configuration_test.go | 4 ++-- pkg/controller/route/route.go | 1 - 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/controller/configuration/configuration.go b/pkg/controller/configuration/configuration.go index bc600e6c2ab4..ba44c9fea3e9 100644 --- a/pkg/controller/configuration/configuration.go +++ b/pkg/controller/configuration/configuration.go @@ -19,6 +19,7 @@ package configuration import ( "context" "fmt" + "strconv" "github.com/golang/glog" buildv1alpha1 "github.com/knative/build/pkg/apis/build/v1alpha1" @@ -199,7 +200,7 @@ func (c *Controller) syncHandler(key string) error { if rev.ObjectMeta.Annotations == nil { rev.ObjectMeta.Annotations = make(map[string]string) } - rev.ObjectMeta.Annotations[serving.ConfigurationGenerationAnnotationKey] = fmt.Sprintf("%v", config.Generation) + rev.ObjectMeta.Annotations[serving.ConfigurationGenerationAnnotationKey] = strconv.Itoa(int(config.Generation)) // Delete revisions when the parent Configuration is deleted. rev.OwnerReferences = append(rev.OwnerReferences, *controllerRef) diff --git a/pkg/controller/configuration/configuration_test.go b/pkg/controller/configuration/configuration_test.go index 1c2db4791682..2f2172a854b4 100644 --- a/pkg/controller/configuration/configuration_test.go +++ b/pkg/controller/configuration/configuration_test.go @@ -27,7 +27,7 @@ package configuration Congfiguration. */ import ( - "fmt" + "strconv" "testing" "time" @@ -224,7 +224,7 @@ func TestCreateConfigurationsCreatesRevision(t *testing.T) { t.Errorf("rev does not have configuration label <%s:%s>", serving.ConfigurationLabelKey, config.Name) } - if rev.Annotations[serving.ConfigurationGenerationAnnotationKey] != fmt.Sprintf("%v", config.Generation) { + if rev.Annotations[serving.ConfigurationGenerationAnnotationKey] != strconv.Itoa(int(config.Generation)) { t.Errorf("rev does not have generation annotation <%s:%s>", serving.ConfigurationGenerationAnnotationKey, config.Name) } diff --git a/pkg/controller/route/route.go b/pkg/controller/route/route.go index 2cdc74dc7bdc..7be5fb7db34a 100644 --- a/pkg/controller/route/route.go +++ b/pkg/controller/route/route.go @@ -444,7 +444,6 @@ func (c *Controller) setLabelForGivenConfigurations( if _, ok := config.Labels[serving.RouteLabelKey]; ok { continue } - err := controller.SetConfigLabel(configClient, config.Name, serving.RouteLabelKey, route.Name) if err != nil { logger.Errorf("Failed add route label to Configuration %q: %q", config.Name, err) From dabed476b2c75981f121b0d084986e2f51deeb19 Mon Sep 17 00:00:00 2001 From: Brenda Chan Date: Mon, 7 May 2018 15:39:33 -0400 Subject: [PATCH 3/4] Use gke 1.10 and enable alpha features for conformance tests Signed-off-by: Dave Protasowski --- test/e2e-tests.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e-tests.sh b/test/e2e-tests.sh index 175b3b5b4f78..812f2a96ede3 100755 --- a/test/e2e-tests.sh +++ b/test/e2e-tests.sh @@ -137,7 +137,7 @@ if [[ -z $1 ]]; then header "Creating test cluster" # Smallest cluster required to run the end-to-end-tests CLUSTER_CREATION_ARGS=( - --gke-create-args="--enable-autoscaling --min-nodes=1 --max-nodes=${E2E_CLUSTER_NODES} --scopes=cloud-platform" + --gke-create-args="--enable-autoscaling --enable-kubernetes-alpha --min-nodes=1 --max-nodes=${E2E_CLUSTER_NODES} --scopes=cloud-platform" --gke-shape={\"default\":{\"Nodes\":${E2E_CLUSTER_NODES}\,\"MachineType\":\"${E2E_CLUSTER_MACHINE}\"}} --provider=gke --deployment=gke From 642ee1cd0210746af01abcc9c30c39745e48f5b1 Mon Sep 17 00:00:00 2001 From: Brenda Chan Date: Wed, 6 Jun 2018 10:34:11 -0400 Subject: [PATCH 4/4] Enable Kubernetes alpha features for GKE 1.10 * This is required to enable CustomResourceSubresources https://github.com/knative/serving/issues/642 --- docs/creating-a-kubernetes-cluster.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/creating-a-kubernetes-cluster.md b/docs/creating-a-kubernetes-cluster.md index 10cbd73156d4..e28fdb7eab0c 100644 --- a/docs/creating-a-kubernetes-cluster.md +++ b/docs/creating-a-kubernetes-cluster.md @@ -36,6 +36,7 @@ To use a k8s cluster running in GKE: --scopes=cloud-platform \ --machine-type=n1-standard-4 \ --enable-autoscaling --min-nodes=1 --max-nodes=3 \ + --enable-kubernetes-alpha \ knative-demo ``` @@ -45,6 +46,7 @@ To use a k8s cluster running in GKE: * Knative Serving currently requires 4-cpu nodes to run conformance tests. Changing the machine type from the default may cause failures. * Autoscale from 1 to 3 nodes. Adjust this for your use case + * Kubernetes alpha features are required to enable the `CustomResourceSubresources` feature. * Change this to your preferred cluster name You can see the list of supported cluster versions in a particular zone by