Repository: knative/pkg
Pull request #2302 (2021-09-30 19:22:55)

introduce new case convention for leader election config map
Part of https://github.com/knative/serving/issues/7300

Majority of serving uses lowercase-hyphens - ie. some-property over
someProperty

This changes the leader election config map parsing to support formats
and to prioritize the lowercase-hyphen case

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2302#" title="Author self-approved">dprotaso</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative/pkg/blob/main/OWNERS)~~ [dprotaso]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
dprotaso: /assign @julz 
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/pkg/pull/2302?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) Report
> Merging [#2302](https://codecov.io/gh/knative/pkg/pull/2302?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (b3b7ccd) into [main](https://codecov.io/gh/knative/pkg/commit/07ea512d3bf010db0799f66c03ddeff645835bfb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (07ea512) will **decrease** coverage by `0.02%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative/pkg/pull/2302/graphs/tree.svg?width=650&height=150&src=pr&token=rlqpDnfxbO&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)](https://codecov.io/gh/knative/pkg/pull/2302?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)

```diff
@@            Coverage Diff             @@
##             main    #2302      +/-   ##
==========================================
- Coverage   64.40%   64.37%   -0.03%     
==========================================
  Files         223      226       +3     
  Lines        9599     9631      +32     
==========================================
+ Hits         6182     6200      +18     
- Misses       3142     3156      +14     
  Partials      275      275              
```


| [Impacted Files](https://codecov.io/gh/knative/pkg/pull/2302?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) | Coverage Δ | |
|---|---|---|
| [leaderelection/config.go](https://codecov.io/gh/knative/pkg/pull/2302/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-bGVhZGVyZWxlY3Rpb24vY29uZmlnLmdv) | `78.72% <100.00%> (+1.45%)` | :arrow_up: |
| [apis/duck/v1/cronjob\_defaults.go](https://codecov.io/gh/knative/pkg/pull/2302/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-YXBpcy9kdWNrL3YxL2Nyb25qb2JfZGVmYXVsdHMuZ28=) | `100.00% <0.00%> (ø)` | |
| [apis/duck/v1/cronjob\_types.go](https://codecov.io/gh/knative/pkg/pull/2302/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-YXBpcy9kdWNrL3YxL2Nyb25qb2JfdHlwZXMuZ28=) | `0.00% <0.00%> (ø)` | |
| [apis/duck/v1/cronjob\_validation.go](https://codecov.io/gh/knative/pkg/pull/2302/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-YXBpcy9kdWNrL3YxL2Nyb25qb2JfdmFsaWRhdGlvbi5nbw==) | `100.00% <0.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/pkg/pull/2302?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative/pkg/pull/2302?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Last update [07ea512...b3b7ccd](https://codecov.io/gh/knative/pkg/pull/2302?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).


Patch:
diff --git a/apis/contexts.go b/apis/contexts.go
index a8c7683a4..b9835fc00 100644
--- a/apis/contexts.go
+++ b/apis/contexts.go
@@ -18,6 +18,7 @@ package apis
 
 import (
 	"context"
+	"net/http"
 
 	authenticationv1 "k8s.io/api/authentication/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -233,3 +234,22 @@ func WithDryRun(ctx context.Context) context.Context {
 func IsDryRun(ctx context.Context) bool {
 	return ctx.Value(isDryRun{}) != nil
 }
+
+// This is attached to contexts passed to webhook interfaces with
+// additional context from the HTTP request.
+type httpReq struct{}
+
+// WithHTTPRequest associated the HTTP request object the webhook
+// received with the context.
+func WithHTTPRequest(ctx context.Context, r *http.Request) context.Context {
+	return context.WithValue(ctx, httpReq{}, r)
+}
+
+// GetHTTPRequest fetches the raw HTTP request received by the webhook.
+func GetHTTPRequest(ctx context.Context) *http.Request {
+	v := ctx.Value(httpReq{})
+	if v == nil {
+		return nil
+	}
+	return v.(*http.Request)
+}
diff --git a/leaderelection/config.go b/leaderelection/config.go
index 8d785c522..0694395bf 100644
--- a/leaderelection/config.go
+++ b/leaderelection/config.go
@@ -46,10 +46,15 @@ func NewConfigFromMap(data map[string]string) (*Config, error) {
 	config := defaultConfig()
 
 	if err := cm.Parse(data,
+		// Parse legacy keys first
 		cm.AsDuration("leaseDuration", &config.LeaseDuration),
 		cm.AsDuration("renewDeadline", &config.RenewDeadline),
 		cm.AsDuration("retryPeriod", &config.RetryPeriod),
 
+		cm.AsDuration("lease-duration", &config.LeaseDuration),
+		cm.AsDuration("renew-deadline", &config.RenewDeadline),
+		cm.AsDuration("retry-period", &config.RetryPeriod),
+
 		cm.AsUint32("buckets", &config.Buckets),
 	); err != nil {
 		return nil, err
diff --git a/tracker/enqueue.go b/tracker/enqueue.go
index 70fbe0e46..88d0254b3 100644
--- a/tracker/enqueue.go
+++ b/tracker/enqueue.go
@@ -95,7 +95,10 @@ func (i *impl) TrackReference(ref Reference, obj interface{}) error {
 	invalidFields := map[string][]string{
 		"APIVersion": validation.IsQualifiedName(ref.APIVersion),
 		"Kind":       validation.IsCIdentifier(ref.Kind),
-		"Namespace":  validation.IsDNS1123Label(ref.Namespace),
+	}
+	// Allow namespace to be empty for cluster-scoped references.
+	if ref.Namespace != "" {
+		invalidFields["Namespace"] = validation.IsDNS1123Label(ref.Namespace)
 	}
 	var selector labels.Selector
 	fieldErrors := []string{}
diff --git a/webhook/admission.go b/webhook/admission.go
index 4d00c746b..ff14585f7 100644
--- a/webhook/admission.go
+++ b/webhook/admission.go
@@ -26,6 +26,7 @@ import (
 	"go.uber.org/zap"
 	admissionv1 "k8s.io/api/admission/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"knative.dev/pkg/apis"
 	"knative.dev/pkg/logging"
 	"knative.dev/pkg/logging/logkey"
 )
@@ -104,6 +105,7 @@ func admissionHandler(rootLogger *zap.SugaredLogger, stats StatsReporter, c Admi
 			logkey.UserInfo, fmt.Sprint(review.Request.UserInfo))
 
 		ctx := logging.WithLogger(r.Context(), logger)
+		ctx = apis.WithHTTPRequest(ctx, r)
 
 		response := admissionv1.AdmissionReview{
 			// Use the same type meta as the request - this is required by the K8s API
diff --git a/webhook/conversion.go b/webhook/conversion.go
index 3ee306671..201ff41f4 100644
--- a/webhook/conversion.go
+++ b/webhook/conversion.go
@@ -24,6 +24,7 @@ import (
 
 	"go.uber.org/zap"
 	apixv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	"knative.dev/pkg/apis"
 	"knative.dev/pkg/logging"
 )
 
@@ -53,6 +54,8 @@ func conversionHandler(rootLogger *zap.SugaredLogger, _ StatsReporter, c Convers
 		)
 
 		ctx := logging.WithLogger(r.Context(), logger)
+		ctx = apis.WithHTTPRequest(ctx, r)
+
 		response := apixv1.ConversionReview{
 			// Use the same type meta as the request - this is required by the K8s API
 			// note: v1beta1 & v1 ConversionReview shapes are identical so even though


Test patch:
diff --git a/apis/contexts_test.go b/apis/contexts_test.go
index e55fddbbb..469ada614 100644
--- a/apis/contexts_test.go
+++ b/apis/contexts_test.go
@@ -18,6 +18,7 @@ package apis
 
 import (
 	"context"
+	"net/http"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -222,3 +223,21 @@ func TestParentMeta(t *testing.T) {
 		t.Errorf("ParentMeta() = %v, wanted %v", got, want)
 	}
 }
+
+func TestGetHTTPRequest(t *testing.T) {
+	ctx := context.Background()
+
+	if got := GetHTTPRequest(ctx); got != nil {
+		t.Errorf("GetHTTPRequest() = %v, wanted %v", got, nil)
+	}
+
+	req, err := http.NewRequest("GET", "https://google.com", nil)
+	if err != nil {
+		t.Fatalf("NewRequest() = %v", err)
+	}
+	ctx = WithHTTPRequest(ctx, req)
+
+	if want, got := req, GetHTTPRequest(ctx); got != want {
+		t.Errorf("GetHTTPRequest() = %v, wanted %v", got, want)
+	}
+}
diff --git a/leaderelection/config_test.go b/leaderelection/config_test.go
index 0cfa4b3cd..e3359bf63 100644
--- a/leaderelection/config_test.go
+++ b/leaderelection/config_test.go
@@ -51,9 +51,9 @@ func okData() map[string]string {
 		// values in this data come from the defaults suggested in the
 		// code:
 		// https://github.com/kubernetes/client-go/blob/kubernetes-1.16.0/tools/leaderelection/leaderelection.go
-		"leaseDuration": "15s",
-		"renewDeadline": "10s",
-		"retryPeriod":   "2s",
+		"lease-duration": "15s",
+		"renew-deadline": "10s",
+		"retry-period":   "2s",
 	}
 }
 
@@ -78,23 +78,23 @@ func TestNewConfigMapFromData(t *testing.T) {
 			return config
 		}(),
 	}, {
-		name: "invalid leaseDuration",
+		name: "invalid lease-duration",
 		data: kmeta.UnionMaps(okData(), map[string]string{
-			"leaseDuration": "flops",
+			"lease-duration": "flops",
 		}),
-		err: `failed to parse "leaseDuration": time: invalid duration`,
+		err: `failed to parse "lease-duration": time: invalid duration`,
 	}, {
-		name: "invalid renewDeadline",
+		name: "invalid renew-deadline",
 		data: kmeta.UnionMaps(okData(), map[string]string{
-			"renewDeadline": "flops",
+			"renew-deadline": "flops",
 		}),
-		err: `failed to parse "renewDeadline": time: invalid duration`,
+		err: `failed to parse "renew-deadline": time: invalid duration`,
 	}, {
-		name: "invalid retryPeriod",
+		name: "invalid retry-period",
 		data: kmeta.UnionMaps(okData(), map[string]string{
-			"retryPeriod": "flops",
+			"retry-period": "flops",
 		}),
-		err: `failed to parse "retryPeriod": time: invalid duration`,
+		err: `failed to parse "retry-period": time: invalid duration`,
 	}, {
 		name: "invalid buckets - not an int",
 		data: kmeta.UnionMaps(okData(), map[string]string{
@@ -113,6 +113,37 @@ func TestNewConfigMapFromData(t *testing.T) {
 			"buckets": strconv.Itoa(int(MaxBuckets + 1)),
 		}),
 		err: fmt.Sprintf("buckets: value must be between 1 <= %d <= %d", MaxBuckets+1, MaxBuckets),
+	}, {
+		name: "legacy keys",
+		data: map[string]string{
+			"leaseDuration": "2s",
+			"renewDeadline": "3s",
+			"retryPeriod":   "4s",
+			"buckets":       "5",
+		},
+		expected: &Config{
+			Buckets:       5,
+			LeaseDuration: 2 * time.Second,
+			RenewDeadline: 3 * time.Second,
+			RetryPeriod:   4 * time.Second,
+		},
+	}, {
+		name: "prioritize new keys",
+		data: map[string]string{
+			"lease-duration": "1s",
+			"renew-deadline": "2s",
+			"retry-period":   "3s",
+			"leaseDuration":  "4s",
+			"renewDeadline":  "5s",
+			"retryPeriod":    "6s",
+			"buckets":        "7",
+		},
+		expected: &Config{
+			Buckets:       7,
+			LeaseDuration: 1 * time.Second,
+			RenewDeadline: 2 * time.Second,
+			RetryPeriod:   3 * time.Second,
+		},
 	}}
 
 	for _, tc := range cases {
diff --git a/test/migrate/checks.go b/test/migrate/checks.go
new file mode 100644
index 000000000..3778743fe
--- /dev/null
+++ b/test/migrate/checks.go
@@ -0,0 +1,53 @@
+package migrate
+
+/*
+Copyright 2021 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.
+*/
+
+import (
+	"context"
+	"strings"
+	"testing"
+
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+// ExpectSingleStoredVersion verifies that status.storedVersions on specific CRDs has only one version
+// and the version is listed in spec.Versions with storage: true. It means the CRDs
+// have been migrated and previous/unused API versions can be safely removed from the spec.
+func ExpectSingleStoredVersion(t *testing.T, crdClient apiextensionsv1.CustomResourceDefinitionInterface, crdGroup string) {
+	t.Helper()
+
+	crdList, err := crdClient.List(context.Background(), metav1.ListOptions{})
+	if err != nil {
+		t.Fatal("Unable to fetch crd list:", err)
+	}
+
+	for _, crd := range crdList.Items {
+		if strings.Contains(crd.Name, crdGroup) {
+			if len(crd.Status.StoredVersions) != 1 {
+				t.Errorf("%q does not have a single stored version: %+v", crd.Name, crd)
+			}
+			stored := crd.Status.StoredVersions[0]
+			for _, v := range crd.Spec.Versions {
+				if stored == v.Name && !v.Storage {
+					t.Errorf("%q is invalid: spec.versions.storage must be true for %q or "+
+						"version %q must be removed from status.storageVersions: %+v", crd.Name, v.Name, v.Name, crd)
+				}
+			}
+		}
+	}
+}
diff --git a/tracker/enqueue_test.go b/tracker/enqueue_test.go
index 18f7651ff..1703b12e6 100644
--- a/tracker/enqueue_test.go
+++ b/tracker/enqueue_test.go
@@ -228,6 +228,13 @@ func TestAllowedObjectReferences(t *testing.T) {
 			Namespace:  "default",
 			Name:       "test",
 		},
+	}, {
+		name: "Namespace",
+		objRef: corev1.ObjectReference{
+			APIVersion: "v1",
+			Kind:       "Namespace",
+			Name:       "default",
+		},
 	}, {
 		name: "Non-core resource",
 		objRef: corev1.ObjectReference{
@@ -333,15 +340,6 @@ func TestBadObjectReferences(t *testing.T) {
 			Name:       "kaniko",
 		},
 		match: "Kind",
-	}, {
-		name: "Missing Namespace",
-		objRef: corev1.ObjectReference{
-			APIVersion: "build.knative.dev/v1alpha1",
-			Kind:       "Build",
-			// Namespace: "default",
-			Name: "kaniko",
-		},
-		match: "Namespace",
 	}, {
 		name: "Capital in Namespace",
 		objRef: corev1.ObjectReference{
@@ -395,7 +393,7 @@ func TestBadObjectReferences(t *testing.T) {
 			// Namespace:  "default",
 			// Name:      "kaniko",
 		},
-		match: "\nAPIVersion:.*\nKind:.*\nNamespace:",
+		match: "\nAPIVersion:.*\nKind:",
 	}}
 
 	for _, test := range tests {
@@ -434,7 +432,7 @@ func TestBadReferences(t *testing.T) {
 	}{{
 		name:   "Missing All",
 		objRef: Reference{},
-		match:  "\nAPIVersion:.*\nKind:.*\nNamespace:",
+		match:  "\nAPIVersion:.*\nKind:",
 	}, {
 		name: "Name and Selector",
 		objRef: Reference{
diff --git a/webhook/admission_integration_test.go b/webhook/admission_integration_test.go
index 0ce307c7a..66a73c483 100644
--- a/webhook/admission_integration_test.go
+++ b/webhook/admission_integration_test.go
@@ -33,6 +33,7 @@ import (
 	admissionv1 "k8s.io/api/admission/v1"
 	authenticationv1 "k8s.io/api/authentication/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"knative.dev/pkg/apis"
 	kubeclient "knative.dev/pkg/client/injection/kube/client/fake"
 	"knative.dev/pkg/metrics/metricstest"
 	_ "knative.dev/pkg/metrics/testing"
@@ -50,6 +51,12 @@ func (fac *fixedAdmissionController) Path() string {
 }
 
 func (fac *fixedAdmissionController) Admit(ctx context.Context, req *admissionv1.AdmissionRequest) *admissionv1.AdmissionResponse {
+	r := apis.GetHTTPRequest(ctx)
+	if r == nil {
+		panic("nil request!")
+	} else if r.URL.Path != fac.path {
+		panic("wrong path!")
+	}
 	return fac.response
 }