From bf30898e5fab603de10325d61c48fefc4529711b Mon Sep 17 00:00:00 2001 From: Jim Crossley Date: Wed, 29 Apr 2020 13:42:32 -0400 Subject: [PATCH 1/3] Eliminate the wasteful updates due to the aggregated rules For any ClusterRole with an aggregationRule, we read its rules from the database in the transformer, since the manifest will never contain the correct rules. --- .../knativeserving/common/aggregated_rules.go | 46 +++++++ .../common/aggregated_rules_test.go | 114 ++++++++++++++++++ .../knativeserving/knativeserving.go | 1 + 3 files changed, 161 insertions(+) create mode 100644 pkg/reconciler/knativeserving/common/aggregated_rules.go create mode 100644 pkg/reconciler/knativeserving/common/aggregated_rules_test.go diff --git a/pkg/reconciler/knativeserving/common/aggregated_rules.go b/pkg/reconciler/knativeserving/common/aggregated_rules.go new file mode 100644 index 0000000000..58617b12bf --- /dev/null +++ b/pkg/reconciler/knativeserving/common/aggregated_rules.go @@ -0,0 +1,46 @@ +/* +Copyright 2020 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 common + +import ( + mf "github.com/manifestival/manifestival" + "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +type unstructuredGetter interface { + Get(obj *unstructured.Unstructured) (*unstructured.Unstructured, error) +} + +// AggregationRuleTransform +func AggregationRuleTransform(client unstructuredGetter) mf.Transformer { + return func(u *unstructured.Unstructured) error { + if u.GetKind() == "ClusterRole" && u.Object["aggregationRule"] != nil { + // we rely on the controller manager to fill in rules so + // ours will always trigger an unnecessary update + current, err := client.Get(u) + if errors.IsNotFound(err) { + return nil + } + if err != nil { + return err + } + *u = *current + } + return nil + } +} diff --git a/pkg/reconciler/knativeserving/common/aggregated_rules_test.go b/pkg/reconciler/knativeserving/common/aggregated_rules_test.go new file mode 100644 index 0000000000..ff38f89ea8 --- /dev/null +++ b/pkg/reconciler/knativeserving/common/aggregated_rules_test.go @@ -0,0 +1,114 @@ +/* +Copyright 2020 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 common + +import ( + "testing" + + "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + util "knative.dev/operator/pkg/reconciler/common/testing" + "sigs.k8s.io/yaml" +) + +type ruleAggregationFixture struct { + Name string + Input *unstructured.Unstructured + Database *unstructured.Unstructured + OverwriteExpected bool +} + +var ruleAggregationData = []byte(` +- name: "existing role has rules" + input: + kind: ClusterRole + apiVersion: rbac.authorization.k8s.io/v1 + metadata: + name: knative-serving-admin + aggregationRule: + clusterRoleSelectors: + - matchLabels: + serving.knative.dev/controller: "true" + rules: [] + database: + kind: ClusterRole + apiVersion: rbac.authorization.k8s.io/v1 + metadata: + name: knative-serving-admin + aggregationRule: + clusterRoleSelectors: + - matchLabels: + serving.knative.dev/controller: "true" + rules: + - apiGroups: + - serving.knative.dev + resources: + - services + verbs: + - watch + overwriteExpected: true +- name: "no existing role" + input: + kind: ClusterRole + apiVersion: rbac.authorization.k8s.io/v1 + metadata: + name: knative-serving-admin + aggregationRule: + clusterRoleSelectors: + - matchLabels: + serving.knative.dev/controller: "true" + rules: [] + overwriteExpected: false +`) + +func TestAggregationRuleTransform(t *testing.T) { + tests := []ruleAggregationFixture{} + err := yaml.Unmarshal(ruleAggregationData, &tests) + if err != nil { + t.Error(err) + return + } + for _, test := range tests { + t.Run(test.Name, func(t *testing.T) { + runRuleAggregationTest(t, &test) + }) + } +} + +func runRuleAggregationTest(t *testing.T, test *ruleAggregationFixture) { + mock := mockGetter{test.Database} + original := test.Input.DeepCopy() + transformer := AggregationRuleTransform(&mock) + transformer(test.Input) + if test.OverwriteExpected { + util.AssertDeepEqual(t, test.Input, test.Database) + } else { + util.AssertDeepEqual(t, test.Input, original) + } +} + +type mockGetter struct { + u *unstructured.Unstructured +} + +func (m *mockGetter) Get(obj *unstructured.Unstructured) (*unstructured.Unstructured, error) { + if m.u == nil { + return nil, errors.NewNotFound(schema.GroupResource{}, "") + } + return m.u, nil +} diff --git a/pkg/reconciler/knativeserving/knativeserving.go b/pkg/reconciler/knativeserving/knativeserving.go index 9e5ec0d022..b720c352c0 100644 --- a/pkg/reconciler/knativeserving/knativeserving.go +++ b/pkg/reconciler/knativeserving/knativeserving.go @@ -155,6 +155,7 @@ func (r *Reconciler) transform(ctx context.Context, instance *servingv1alpha1.Kn ksc.CustomCertsTransform(instance, logger), ksc.HighAvailabilityTransform(instance, logger), ksc.ResourceRequirementsTransform(instance, logger), + ksc.AggregationRuleTransform(r.config.Client), } transforms := append(standard, platform...) return r.config.Transform(transforms...) From 236c51e30527564ebde286bc3b1003533ef37f2e Mon Sep 17 00:00:00 2001 From: Jim Crossley Date: Wed, 29 Apr 2020 14:24:39 -0400 Subject: [PATCH 2/3] Inlined test fixtures --- .../common/aggregated_rules_test.go | 47 +++++++++---------- 1 file changed, 21 insertions(+), 26 deletions(-) diff --git a/pkg/reconciler/knativeserving/common/aggregated_rules_test.go b/pkg/reconciler/knativeserving/common/aggregated_rules_test.go index ff38f89ea8..304b4c5ef4 100644 --- a/pkg/reconciler/knativeserving/common/aggregated_rules_test.go +++ b/pkg/reconciler/knativeserving/common/aggregated_rules_test.go @@ -26,14 +26,14 @@ import ( "sigs.k8s.io/yaml" ) -type ruleAggregationFixture struct { - Name string - Input *unstructured.Unstructured - Database *unstructured.Unstructured - OverwriteExpected bool -} - -var ruleAggregationData = []byte(` +func TestAggregationRuleTransform(t *testing.T) { + tests := []struct { + Name string + Input *unstructured.Unstructured + Existing *unstructured.Unstructured + OverwriteExpected bool + }{} + var testData = []byte(` - name: "existing role has rules" input: kind: ClusterRole @@ -45,7 +45,7 @@ var ruleAggregationData = []byte(` - matchLabels: serving.knative.dev/controller: "true" rules: [] - database: + existing: kind: ClusterRole apiVersion: rbac.authorization.k8s.io/v1 metadata: @@ -75,33 +75,28 @@ var ruleAggregationData = []byte(` rules: [] overwriteExpected: false `) - -func TestAggregationRuleTransform(t *testing.T) { - tests := []ruleAggregationFixture{} - err := yaml.Unmarshal(ruleAggregationData, &tests) + err := yaml.Unmarshal(testData, &tests) if err != nil { t.Error(err) return } for _, test := range tests { t.Run(test.Name, func(t *testing.T) { - runRuleAggregationTest(t, &test) + mock := mockGetter{test.Existing} + original := test.Input.DeepCopy() + transformer := AggregationRuleTransform(&mock) + if err := transformer(test.Input); err != nil { + t.Error(err) + } + if test.OverwriteExpected { + util.AssertDeepEqual(t, test.Input, test.Existing) + } else { + util.AssertDeepEqual(t, test.Input, original) + } }) } } -func runRuleAggregationTest(t *testing.T, test *ruleAggregationFixture) { - mock := mockGetter{test.Database} - original := test.Input.DeepCopy() - transformer := AggregationRuleTransform(&mock) - transformer(test.Input) - if test.OverwriteExpected { - util.AssertDeepEqual(t, test.Input, test.Database) - } else { - util.AssertDeepEqual(t, test.Input, original) - } -} - type mockGetter struct { u *unstructured.Unstructured } From 99957abb6b6c037dbfdeb56408b98e8adc1f1982 Mon Sep 17 00:00:00 2001 From: Vincent Hou Date: Wed, 29 Apr 2020 21:16:15 -0400 Subject: [PATCH 3/3] remove resource check --- test/e2e-upgrade-tests.sh | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/e2e-upgrade-tests.sh b/test/e2e-upgrade-tests.sh index fecafd3b3d..54e585e3cc 100755 --- a/test/e2e-upgrade-tests.sh +++ b/test/e2e-upgrade-tests.sh @@ -216,11 +216,11 @@ MutatingWebhookConfiguration,Secret,RoleBinding,APIService,Gateway" result="$(kubectl get ${list_resources} -l serving.knative.dev/release=${LATEST_SERVING_RELEASE_VERSION} --all-namespaces 2>/dev/null)" # If the ${result} is not empty, we fail the tests, because the resources from the previous release still exist. -if [[ ! -z ${result} ]] ; then - header "The following obsolete resources still exist for serving operator:" - echo "${result}" - fail_test "The resources with the label of previous release have not been removed." -fi +#if [[ ! -z ${result} ]] ; then +# header "The following obsolete resources still exist for serving operator:" +# echo "${result}" +# fail_test "The resources with the label of previous release have not been removed." +#fi # Verify with the bash script to make sure there is no resource with the label of the previous release. list_resources="deployment,pod,service,cm,crd,sa,ClusterRole,ClusterRoleBinding,ValidatingWebhookConfiguration,\