From 0f930bc759855647d6146ab5ce0e76bfd34cdb47 Mon Sep 17 00:00:00 2001 From: "Jay Vyas (jayunit100)" Date: Mon, 30 Nov 2020 20:13:15 -0500 Subject: [PATCH 01/23] namespace by name default labelling Co-authored-by: Jordan Liggitt Co-authored-by: Abhishek Raut --- pkg/apis/core/v1/defaults.go | 22 ++++++++++ pkg/apis/core/v1/defaults_test.go | 36 +++++++++++++++++ pkg/apis/core/v1/zz_generated.defaults.go | 1 + pkg/apis/core/validation/validation.go | 5 +++ pkg/apis/core/validation/validation_test.go | 40 +++++++++++++++++++ pkg/features/kube_features.go | 7 ++++ .../k8s.io/api/core/v1/well_known_labels.go | 2 + .../namespace/ns_conditions_test.go | 33 ++++++++++++++- 8 files changed, 144 insertions(+), 2 deletions(-) diff --git a/pkg/apis/core/v1/defaults.go b/pkg/apis/core/v1/defaults.go index 3381f652c0ec..4e780bafd323 100644 --- a/pkg/apis/core/v1/defaults.go +++ b/pkg/apis/core/v1/defaults.go @@ -323,6 +323,28 @@ func SetDefaults_HTTPGetAction(obj *v1.HTTPGetAction) { obj.Scheme = v1.URISchemeHTTP } } + +// SetDefaults_Namespace adds a default label for all namespaces +func SetDefaults_Namespace(obj *v1.Namespace) { + // TODO, remove this in 1.22 + // we cant SetDefaults for nameless namespaces (generateName), + // we don't need to overwrite the pre-existing labelMetadataName + if len(obj.Name) > 0 && obj.Labels[v1.LabelMetadataName] != obj.Name { + if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { + } + if !utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { + return + } + if obj.Labels == nil { + obj.Labels = map[string]string{} + } + // Default label that matches the name of the namespace, for selecting w/o apriori knowledge of labels. + // Update to this label is not allowed. Hence, the value of the label is intentionally overridden here + // with the namespace name. + obj.Labels[v1.LabelMetadataName] = obj.Name + } +} + func SetDefaults_NamespaceStatus(obj *v1.NamespaceStatus) { if obj.Phase == "" { obj.Phase = v1.NamespaceActive diff --git a/pkg/apis/core/v1/defaults_test.go b/pkg/apis/core/v1/defaults_test.go index b167110c0a83..43abe58e1f04 100644 --- a/pkg/apis/core/v1/defaults_test.go +++ b/pkg/apis/core/v1/defaults_test.go @@ -1416,6 +1416,42 @@ func TestSetDefaultNamespace(t *testing.T) { } } +func TestSetDefaultNamespaceLabels(t *testing.T) { + theNs := "default-ns-labels-are-great" + s := &v1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: theNs, + }, + } + obj2 := roundTrip(t, runtime.Object(s)) + s2 := obj2.(*v1.Namespace) + + if s2.Status.Phase != v1.NamespaceActive { + t.Errorf("Expected phase %v, got %v", v1.NamespaceActive, s2.Status.Phase) + } + if _, ok := s2.ObjectMeta.Labels[v1.LabelMetadataName]; !ok { + t.Errorf("Missing default namespace label key for %v", s) + } else { + if s2.ObjectMeta.Labels[v1.LabelMetadataName] != theNs { + t.Errorf("Expected default namespace label value of %v, but got %v", theNs, s2.ObjectMeta.Labels[v1.LabelMetadataName]) + } + } +} + +func TestSetDefaultNamespaceLabelsForGenerateName(t *testing.T) { + s := &v1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "default-ns-labels-are-great-generated", + }, + } + obj2 := roundTrip(t, runtime.Object(s)) + s2 := obj2.(*v1.Namespace) + + if _, ok := s2.ObjectMeta.Labels[v1.LabelMetadataName]; !ok { + t.Errorf("Missing default namespace label key for %v", s) + } +} + func TestSetDefaultPodSpecHostNetwork(t *testing.T) { portNum := int32(8080) s := v1.PodSpec{} diff --git a/pkg/apis/core/v1/zz_generated.defaults.go b/pkg/apis/core/v1/zz_generated.defaults.go index 493d9903e1b5..68e2a32aacc7 100644 --- a/pkg/apis/core/v1/zz_generated.defaults.go +++ b/pkg/apis/core/v1/zz_generated.defaults.go @@ -158,6 +158,7 @@ func SetObjectDefaults_LimitRangeList(in *v1.LimitRangeList) { } func SetObjectDefaults_Namespace(in *v1.Namespace) { + SetDefaults_Namespace(in) SetDefaults_NamespaceStatus(&in.Status) } diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 1b668083b3b3..33c6febc9c86 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -5656,6 +5656,11 @@ func ValidateNamespace(namespace *core.Namespace) field.ErrorList { for i := range namespace.Spec.Finalizers { allErrs = append(allErrs, validateFinalizerName(string(namespace.Spec.Finalizers[i]), field.NewPath("spec", "finalizers"))...) } + if !utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { + if namespace.Labels[v1.LabelMetadataName] != namespace.Name { + allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "labels").Key(v1.LabelMetadataName), namespace.Labels[v1.LabelMetadataName], fmt.Sprintf("must be %s", namespace.Name))) + } + } return allErrs } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 6ea9b35aa339..79633fba1eb9 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -14629,6 +14629,11 @@ func TestValidateNamespace(t *testing.T) { Finalizers: []core.FinalizerName{"example.com/something", "example.com/other"}, }, }, + // The following success case, when validated, confirms that GeneratedName implementations result in generated names that are + // additionally added as kubernetes.io/metadata.namespace labels + { + ObjectMeta: metav1.ObjectMeta{GenerateName: "abc"}, + }, } for _, successCase := range successCases { if errs := ValidateNamespace(&successCase); len(errs) != 0 { @@ -14658,6 +14663,41 @@ func TestValidateNamespace(t *testing.T) { t.Errorf("expected failure for %s", k) } } + labelsTestCases := map[string]struct { + ns *core.Namespace + expectError bool + }{ + "Valid: namespace name label exists": { + ns: &core.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "abc-xyz", Labels: map[string]string{"kubernetes.io/metadata.name": "abc-xyz"}}, + }, + expectError: false, + }, + "Invalid: namespace name label does not exist": { + ns: &core.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "abc-xyz"}, + }, + expectError: true, + }, + "Invalid: incorrect namespace name label exists": { + ns: &core.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "abc-xyz", Labels: map[string]string{"kubernetes.io/metadata.name": "i-be-wrong"}}, + }, + expectError: true, + }, + } + for tcName, tc := range labelsTestCases { + t.Run(tcName, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.NamespaceDefaultLabelName, true)() + errs := ValidateNamespace(tc.ns) + if tc.expectError && len(errs) == 0 { + t.Errorf("Unexpected success") + } + if !tc.expectError && len(errs) != 0 { + t.Errorf("Unexpected error(s): %v", errs) + } + }) + } } func TestValidateNamespaceFinalizeUpdate(t *testing.T) { diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 56c6cc6b81c0..5b2ce6fa433e 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -723,6 +723,12 @@ const ( // // Allows jobs to be created in the suspended state. SuspendJob featuregate.Feature = "SuspendJob" + + // owner: @jayunit100 @abhiraut @rikatz + // beta: v1.21 + // + // Labels all namespaces with a default label "kubernetes.io/metadata.name: " + NamespaceDefaultLabelName featuregate.Feature = "NamespaceDefaultLabelName" ) func init() { @@ -832,6 +838,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS IngressClassNamespacedParams: {Default: false, PreRelease: featuregate.Alpha}, ServiceInternalTrafficPolicy: {Default: false, PreRelease: featuregate.Alpha}, SuspendJob: {Default: false, PreRelease: featuregate.Alpha}, + NamespaceDefaultLabelName: {Default: true, PreRelease: featuregate.Beta}, // graduate to GA and lock to default in 1.22, remove in 1.24 // inherited features from generic apiserver, relisted here to get a conflict if it is changed // unintentionally on either side: diff --git a/staging/src/k8s.io/api/core/v1/well_known_labels.go b/staging/src/k8s.io/api/core/v1/well_known_labels.go index c623340d9380..5cf82a981755 100644 --- a/staging/src/k8s.io/api/core/v1/well_known_labels.go +++ b/staging/src/k8s.io/api/core/v1/well_known_labels.go @@ -65,4 +65,6 @@ const ( // any backends on excluded nodes are not reachable by those external load-balancers. // Implementations of this exclusion may vary based on provider. LabelNodeExcludeBalancers = "node.kubernetes.io/exclude-from-external-load-balancers" + // LabelMetadataName is the label name which, in-tree, is used to automatically label namespaces, so they can be selected easily by tools which require definitive labels + LabelMetadataName = "kubernetes.io/metadata.name" ) diff --git a/test/integration/namespace/ns_conditions_test.go b/test/integration/namespace/ns_conditions_test.go index da48d8c196ca..22892b002a79 100644 --- a/test/integration/namespace/ns_conditions_test.go +++ b/test/integration/namespace/ns_conditions_test.go @@ -19,11 +19,11 @@ package namespace import ( "context" "encoding/json" + "fmt" + "github.com/davecgh/go-spew/spew" "testing" "time" - "github.com/davecgh/go-spew/spew" - appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -115,6 +115,35 @@ func TestNamespaceCondition(t *testing.T) { } } +// TestNamespaceLabels tests for default labels added in https://github.com/kubernetes/kubernetes/pull/96968 +func TestNamespaceLabels(t *testing.T) { + closeFn, _, _, kubeClient, _ := namespaceLifecycleSetup(t) + defer closeFn() + nsName := "test-namespace-labels-generated" + // Create a new namespace w/ no name + ns, err := kubeClient.CoreV1().Namespaces().Create(context.TODO(), &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: nsName, + }, + }, metav1.CreateOptions{}) + + if _, ok := ns.ObjectMeta.Labels[corev1.LabelMetadataName]; !ok { + t.Fatal(fmt.Errorf("missing metadata name label from namespace (initial create) ")) + } + + if err != nil { + t.Fatal(err) + } + + if nsList, err := kubeClient.CoreV1().Namespaces().List(context.TODO(), metav1.ListOptions{}); err != nil { + for _, ns := range nsList.Items { + if _, ok := ns.ObjectMeta.Labels[corev1.LabelMetadataName]; !ok { + t.Fatal(fmt.Errorf("missing metadata name label from namespace (post create)")) + } + } + } +} + // JSONToUnstructured converts a JSON stub to unstructured.Unstructured and // returns a dynamic resource client that can be used to interact with it func jsonToUnstructured(stub, version, kind string) (*unstructured.Unstructured, error) { From aa51c7683c326707a4a9c895cdb4fa70daaf6b7c Mon Sep 17 00:00:00 2001 From: Ricardo Pchevuzinske Katz Date: Mon, 1 Mar 2021 09:02:39 -0300 Subject: [PATCH 02/23] Make some logic improvement into default namespace label --- pkg/apis/core/v1/defaults.go | 15 +++++---------- pkg/apis/core/validation/validation.go | 2 +- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/pkg/apis/core/v1/defaults.go b/pkg/apis/core/v1/defaults.go index 4e780bafd323..a5b81b2bf59b 100644 --- a/pkg/apis/core/v1/defaults.go +++ b/pkg/apis/core/v1/defaults.go @@ -331,18 +331,13 @@ func SetDefaults_Namespace(obj *v1.Namespace) { // we don't need to overwrite the pre-existing labelMetadataName if len(obj.Name) > 0 && obj.Labels[v1.LabelMetadataName] != obj.Name { if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { + if obj.Labels == nil { + obj.Labels = map[string]string{} + } + obj.Labels[v1.LabelMetadataName] = obj.Name } - if !utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { - return - } - if obj.Labels == nil { - obj.Labels = map[string]string{} - } - // Default label that matches the name of the namespace, for selecting w/o apriori knowledge of labels. - // Update to this label is not allowed. Hence, the value of the label is intentionally overridden here - // with the namespace name. - obj.Labels[v1.LabelMetadataName] = obj.Name } + } func SetDefaults_NamespaceStatus(obj *v1.NamespaceStatus) { diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 33c6febc9c86..3e0491db36a7 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -5656,7 +5656,7 @@ func ValidateNamespace(namespace *core.Namespace) field.ErrorList { for i := range namespace.Spec.Finalizers { allErrs = append(allErrs, validateFinalizerName(string(namespace.Spec.Finalizers[i]), field.NewPath("spec", "finalizers"))...) } - if !utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { + if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { if namespace.Labels[v1.LabelMetadataName] != namespace.Name { allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "labels").Key(v1.LabelMetadataName), namespace.Labels[v1.LabelMetadataName], fmt.Sprintf("must be %s", namespace.Name))) } From 4696c3ac13bfefbd23773190fdce279b712d0c0b Mon Sep 17 00:00:00 2001 From: Ricardo Pchevuzinske Katz Date: Mon, 1 Mar 2021 15:31:29 -0300 Subject: [PATCH 03/23] Fix unit tests --- pkg/apis/core/v1/defaults_test.go | 24 ++++++++++++++------- pkg/apis/core/validation/validation_test.go | 10 ++------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/pkg/apis/core/v1/defaults_test.go b/pkg/apis/core/v1/defaults_test.go index 43abe58e1f04..913f06014c09 100644 --- a/pkg/apis/core/v1/defaults_test.go +++ b/pkg/apis/core/v1/defaults_test.go @@ -1417,6 +1417,9 @@ func TestSetDefaultNamespace(t *testing.T) { } func TestSetDefaultNamespaceLabels(t *testing.T) { + // Although this is defaulted to true, it's still worth to enable the feature gate during the test + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.NamespaceDefaultLabelName, true)() + theNs := "default-ns-labels-are-great" s := &v1.Namespace{ ObjectMeta: metav1.ObjectMeta{ @@ -1436,19 +1439,24 @@ func TestSetDefaultNamespaceLabels(t *testing.T) { t.Errorf("Expected default namespace label value of %v, but got %v", theNs, s2.ObjectMeta.Labels[v1.LabelMetadataName]) } } -} -func TestSetDefaultNamespaceLabelsForGenerateName(t *testing.T) { - s := &v1.Namespace{ + // And let's disable the FG and check if it still defaults creating the labels + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.NamespaceDefaultLabelName, false)() + + theNs = "default-ns-labels-are-not-that-great" + s = &v1.Namespace{ ObjectMeta: metav1.ObjectMeta{ - GenerateName: "default-ns-labels-are-great-generated", + Name: theNs, }, } - obj2 := roundTrip(t, runtime.Object(s)) - s2 := obj2.(*v1.Namespace) + obj2 = roundTrip(t, runtime.Object(s)) + s2 = obj2.(*v1.Namespace) - if _, ok := s2.ObjectMeta.Labels[v1.LabelMetadataName]; !ok { - t.Errorf("Missing default namespace label key for %v", s) + if s2.Status.Phase != v1.NamespaceActive { + t.Errorf("Expected phase %v, got %v", v1.NamespaceActive, s2.Status.Phase) + } + if _, ok := s2.ObjectMeta.Labels[v1.LabelMetadataName]; ok { + t.Errorf("Default namespace shouldn't exist here, as the feature gate is disabled %v", s) } } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 79633fba1eb9..02981a32871c 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -14617,23 +14617,17 @@ func TestValidateResourceQuota(t *testing.T) { } func TestValidateNamespace(t *testing.T) { - validLabels := map[string]string{"a": "b"} invalidLabels := map[string]string{"NoUppercaseOrSpecialCharsLike=Equals": "b"} successCases := []core.Namespace{ { - ObjectMeta: metav1.ObjectMeta{Name: "abc", Labels: validLabels}, + ObjectMeta: metav1.ObjectMeta{Name: "abc", Labels: map[string]string{"a": "b", v1.LabelMetadataName: "abc"}}, }, { - ObjectMeta: metav1.ObjectMeta{Name: "abc-123"}, + ObjectMeta: metav1.ObjectMeta{Name: "abc-123", Labels: map[string]string{v1.LabelMetadataName: "abc-123"}}, Spec: core.NamespaceSpec{ Finalizers: []core.FinalizerName{"example.com/something", "example.com/other"}, }, }, - // The following success case, when validated, confirms that GeneratedName implementations result in generated names that are - // additionally added as kubernetes.io/metadata.namespace labels - { - ObjectMeta: metav1.ObjectMeta{GenerateName: "abc"}, - }, } for _, successCase := range successCases { if errs := ValidateNamespace(&successCase); len(errs) != 0 { From 9fbd555ddcd42d0f92839d7ff4cb3ef3a335f7b1 Mon Sep 17 00:00:00 2001 From: Ricardo Pchevuzinske Katz Date: Mon, 1 Mar 2021 16:29:11 -0300 Subject: [PATCH 04/23] minor change to trigger the CI --- pkg/apis/core/v1/defaults.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/apis/core/v1/defaults.go b/pkg/apis/core/v1/defaults.go index a5b81b2bf59b..6d0ee8c03eb3 100644 --- a/pkg/apis/core/v1/defaults.go +++ b/pkg/apis/core/v1/defaults.go @@ -337,7 +337,6 @@ func SetDefaults_Namespace(obj *v1.Namespace) { obj.Labels[v1.LabelMetadataName] = obj.Name } } - } func SetDefaults_NamespaceStatus(obj *v1.NamespaceStatus) { From 636ab68947a52f090aac2706250f1b56c5221acb Mon Sep 17 00:00:00 2001 From: Ricardo Pchevuzinske Katz Date: Tue, 2 Mar 2021 14:06:17 -0300 Subject: [PATCH 05/23] Correct some tests and validation behaviors --- pkg/apis/core/v1/defaults.go | 2 +- pkg/apis/core/v1/defaults_test.go | 28 +++++++++++-------- pkg/apis/core/validation/validation.go | 2 +- .../namespace/ns_conditions_test.go | 11 ++++---- 4 files changed, 24 insertions(+), 19 deletions(-) diff --git a/pkg/apis/core/v1/defaults.go b/pkg/apis/core/v1/defaults.go index 6d0ee8c03eb3..427ae2be81a2 100644 --- a/pkg/apis/core/v1/defaults.go +++ b/pkg/apis/core/v1/defaults.go @@ -326,7 +326,7 @@ func SetDefaults_HTTPGetAction(obj *v1.HTTPGetAction) { // SetDefaults_Namespace adds a default label for all namespaces func SetDefaults_Namespace(obj *v1.Namespace) { - // TODO, remove this in 1.22 + // TODO, remove the feature gate in 1.22 // we cant SetDefaults for nameless namespaces (generateName), // we don't need to overwrite the pre-existing labelMetadataName if len(obj.Name) > 0 && obj.Labels[v1.LabelMetadataName] != obj.Name { diff --git a/pkg/apis/core/v1/defaults_test.go b/pkg/apis/core/v1/defaults_test.go index 913f06014c09..ca7d655ad0a1 100644 --- a/pkg/apis/core/v1/defaults_test.go +++ b/pkg/apis/core/v1/defaults_test.go @@ -1429,15 +1429,8 @@ func TestSetDefaultNamespaceLabels(t *testing.T) { obj2 := roundTrip(t, runtime.Object(s)) s2 := obj2.(*v1.Namespace) - if s2.Status.Phase != v1.NamespaceActive { - t.Errorf("Expected phase %v, got %v", v1.NamespaceActive, s2.Status.Phase) - } - if _, ok := s2.ObjectMeta.Labels[v1.LabelMetadataName]; !ok { - t.Errorf("Missing default namespace label key for %v", s) - } else { - if s2.ObjectMeta.Labels[v1.LabelMetadataName] != theNs { - t.Errorf("Expected default namespace label value of %v, but got %v", theNs, s2.ObjectMeta.Labels[v1.LabelMetadataName]) - } + if s2.ObjectMeta.Labels[v1.LabelMetadataName] != theNs { + t.Errorf("Expected default namespace label value of %v, but got %v", theNs, s2.ObjectMeta.Labels[v1.LabelMetadataName]) } // And let's disable the FG and check if it still defaults creating the labels @@ -1452,14 +1445,25 @@ func TestSetDefaultNamespaceLabels(t *testing.T) { obj2 = roundTrip(t, runtime.Object(s)) s2 = obj2.(*v1.Namespace) - if s2.Status.Phase != v1.NamespaceActive { - t.Errorf("Expected phase %v, got %v", v1.NamespaceActive, s2.Status.Phase) - } if _, ok := s2.ObjectMeta.Labels[v1.LabelMetadataName]; ok { t.Errorf("Default namespace shouldn't exist here, as the feature gate is disabled %v", s) } } +func TestSetDefaultNamespaceLabelsForGenerateName(t *testing.T) { + + s := &v1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "default-ns-labels-are-great-generated", + }, + } + obj2 := roundTrip(t, runtime.Object(s)) + s2 := obj2.(*v1.Namespace) + if _, ok := s2.ObjectMeta.Labels[v1.LabelMetadataName]; !ok { + t.Errorf("Missing default namespace label key for %v", s) + } +} + func TestSetDefaultPodSpecHostNetwork(t *testing.T) { portNum := int32(8080) s := v1.PodSpec{} diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 3e0491db36a7..9da35c167520 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -5657,7 +5657,7 @@ func ValidateNamespace(namespace *core.Namespace) field.ErrorList { allErrs = append(allErrs, validateFinalizerName(string(namespace.Spec.Finalizers[i]), field.NewPath("spec", "finalizers"))...) } if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { - if namespace.Labels[v1.LabelMetadataName] != namespace.Name { + if len(namespace.Name) > 0 && namespace.Labels[v1.LabelMetadataName] != namespace.Name { allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "labels").Key(v1.LabelMetadataName), namespace.Labels[v1.LabelMetadataName], fmt.Sprintf("must be %s", namespace.Name))) } } diff --git a/test/integration/namespace/ns_conditions_test.go b/test/integration/namespace/ns_conditions_test.go index 22892b002a79..fee68afc1442 100644 --- a/test/integration/namespace/ns_conditions_test.go +++ b/test/integration/namespace/ns_conditions_test.go @@ -20,10 +20,11 @@ import ( "context" "encoding/json" "fmt" - "github.com/davecgh/go-spew/spew" "testing" "time" + "github.com/davecgh/go-spew/spew" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -127,8 +128,8 @@ func TestNamespaceLabels(t *testing.T) { }, }, metav1.CreateOptions{}) - if _, ok := ns.ObjectMeta.Labels[corev1.LabelMetadataName]; !ok { - t.Fatal(fmt.Errorf("missing metadata name label from namespace (initial create) ")) + if ns.Name != ns.Labels[corev1.LabelMetadataName] { + t.Fatal(fmt.Errorf("expected %q, got %q", ns.Name, ns.Labels[corev1.LabelMetadataName])) } if err != nil { @@ -137,8 +138,8 @@ func TestNamespaceLabels(t *testing.T) { if nsList, err := kubeClient.CoreV1().Namespaces().List(context.TODO(), metav1.ListOptions{}); err != nil { for _, ns := range nsList.Items { - if _, ok := ns.ObjectMeta.Labels[corev1.LabelMetadataName]; !ok { - t.Fatal(fmt.Errorf("missing metadata name label from namespace (post create)")) + if ns.Name != ns.Labels[corev1.LabelMetadataName] { + t.Fatal(fmt.Errorf("expected %q, got %q", ns.Name, ns.Labels[corev1.LabelMetadataName])) } } } From f2d756009865b882ec6af0cdca0378a5f1f3a8d4 Mon Sep 17 00:00:00 2001 From: Ricardo Pchevuzinske Katz Date: Tue, 2 Mar 2021 15:16:51 -0300 Subject: [PATCH 06/23] Add Canonicalize normalization and improve validation --- pkg/apis/core/validation/validation.go | 2 +- pkg/registry/core/namespace/strategy.go | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 9da35c167520..66f99183e809 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -5657,7 +5657,7 @@ func ValidateNamespace(namespace *core.Namespace) field.ErrorList { allErrs = append(allErrs, validateFinalizerName(string(namespace.Spec.Finalizers[i]), field.NewPath("spec", "finalizers"))...) } if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { - if len(namespace.Name) > 0 && namespace.Labels[v1.LabelMetadataName] != namespace.Name { + if label, ok := namespace.Labels[v1.LabelMetadataName]; len(namespace.Name) > 0 && ok && label != namespace.Name { allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "labels").Key(v1.LabelMetadataName), namespace.Labels[v1.LabelMetadataName], fmt.Sprintf("must be %s", namespace.Name))) } } diff --git a/pkg/registry/core/namespace/strategy.go b/pkg/registry/core/namespace/strategy.go index bb27621f2e01..f366fd026a97 100644 --- a/pkg/registry/core/namespace/strategy.go +++ b/pkg/registry/core/namespace/strategy.go @@ -20,6 +20,7 @@ import ( "context" "fmt" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" @@ -27,9 +28,11 @@ import ( "k8s.io/apiserver/pkg/registry/generic" apistorage "k8s.io/apiserver/pkg/storage" "k8s.io/apiserver/pkg/storage/names" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/kubernetes/pkg/api/legacyscheme" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/apis/core/validation" + "k8s.io/kubernetes/pkg/features" ) // namespaceStrategy implements behavior for Namespaces @@ -88,6 +91,15 @@ func (namespaceStrategy) Validate(ctx context.Context, obj runtime.Object) field // Canonicalize normalizes the object after validation. func (namespaceStrategy) Canonicalize(obj runtime.Object) { + ns := obj.(*api.Namespace) + if len(ns.Name) > 0 && ns.Labels[v1.LabelMetadataName] != ns.Name { + if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { + if ns.Labels == nil { + ns.Labels = map[string]string{} + } + ns.Labels[v1.LabelMetadataName] = ns.Name + } + } } // AllowCreateOnUpdate is false for namespaces. From 1a8cd99fd4bba21245e5e04535e6d92bc44562dd Mon Sep 17 00:00:00 2001 From: Ricardo Pchevuzinske Katz Date: Tue, 2 Mar 2021 16:43:28 -0300 Subject: [PATCH 07/23] Remove label validation that should be dealt by strategy --- pkg/apis/core/validation/validation_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 02981a32871c..d68085d9c8d9 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -14667,12 +14667,13 @@ func TestValidateNamespace(t *testing.T) { }, expectError: false, }, + /* Removing this, validation should probably just check if the label exists and is different from the expected "Invalid: namespace name label does not exist": { ns: &core.Namespace{ ObjectMeta: metav1.ObjectMeta{Name: "abc-xyz"}, }, expectError: true, - }, + },*/ "Invalid: incorrect namespace name label exists": { ns: &core.Namespace{ ObjectMeta: metav1.ObjectMeta{Name: "abc-xyz", Labels: map[string]string{"kubernetes.io/metadata.name": "i-be-wrong"}}, From a3391e0b99abd0653e74f524d572d82eb97fa402 Mon Sep 17 00:00:00 2001 From: jay vyas Date: Tue, 2 Mar 2021 15:34:45 -0500 Subject: [PATCH 08/23] Update defaults_test.go add fuzzer ns spec --- pkg/apis/core/fuzzer/fuzzer.go | 11 +++++++++++ pkg/apis/core/v1/defaults_test.go | 14 -------------- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/pkg/apis/core/fuzzer/fuzzer.go b/pkg/apis/core/fuzzer/fuzzer.go index aac80a1f3695..a0a7c38318ed 100644 --- a/pkg/apis/core/fuzzer/fuzzer.go +++ b/pkg/apis/core/fuzzer/fuzzer.go @@ -472,6 +472,17 @@ var Funcs = func(codecs runtimeserializer.CodecFactory) []interface{} { func(s *core.NamespaceSpec, c fuzz.Continue) { s.Finalizers = []core.FinalizerName{core.FinalizerKubernetes} }, + func(s *core.Namespace, c fuzz.Continue) { + s.Finalizers = []core.FinalizerName{core.FinalizerKubernetes} + c.FuzzNoCustom(s) // fuzz self without calling this function again + // Match name --> label defaulting + if len(s.Name) > 0 { + if s.Labels == nil { + s.Labels = map[string]string{} + } + s.Labels["kubernetes.io/metadata.name"] = s.Name + } + }, func(s *core.NamespaceStatus, c fuzz.Continue) { s.Phase = core.NamespaceActive }, diff --git a/pkg/apis/core/v1/defaults_test.go b/pkg/apis/core/v1/defaults_test.go index ca7d655ad0a1..b20fee58f612 100644 --- a/pkg/apis/core/v1/defaults_test.go +++ b/pkg/apis/core/v1/defaults_test.go @@ -1450,20 +1450,6 @@ func TestSetDefaultNamespaceLabels(t *testing.T) { } } -func TestSetDefaultNamespaceLabelsForGenerateName(t *testing.T) { - - s := &v1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "default-ns-labels-are-great-generated", - }, - } - obj2 := roundTrip(t, runtime.Object(s)) - s2 := obj2.(*v1.Namespace) - if _, ok := s2.ObjectMeta.Labels[v1.LabelMetadataName]; !ok { - t.Errorf("Missing default namespace label key for %v", s) - } -} - func TestSetDefaultPodSpecHostNetwork(t *testing.T) { portNum := int32(8080) s := v1.PodSpec{} From d46ae98ce3715373da6904b17a309ae5098221da Mon Sep 17 00:00:00 2001 From: "Jay Vyas (jayunit100)" Date: Wed, 3 Mar 2021 01:24:34 +0000 Subject: [PATCH 09/23] remove the finalizer thingy --- pkg/apis/core/fuzzer/fuzzer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/core/fuzzer/fuzzer.go b/pkg/apis/core/fuzzer/fuzzer.go index a0a7c38318ed..0600e57210b2 100644 --- a/pkg/apis/core/fuzzer/fuzzer.go +++ b/pkg/apis/core/fuzzer/fuzzer.go @@ -473,7 +473,7 @@ var Funcs = func(codecs runtimeserializer.CodecFactory) []interface{} { s.Finalizers = []core.FinalizerName{core.FinalizerKubernetes} }, func(s *core.Namespace, c fuzz.Continue) { - s.Finalizers = []core.FinalizerName{core.FinalizerKubernetes} + // s.Finalizers = []core.FinalizerName{core.FinalizerKubernetes} c.FuzzNoCustom(s) // fuzz self without calling this function again // Match name --> label defaulting if len(s.Name) > 0 { From 69bd09aabdd6200199a5a12f80fb5259b0363fba Mon Sep 17 00:00:00 2001 From: Ricardo Pchevuzinske Katz Date: Wed, 3 Mar 2021 17:54:18 -0300 Subject: [PATCH 10/23] Fix integration test --- pkg/apis/core/fuzzer/fuzzer.go | 1 - test/integration/namespace/ns_conditions_test.go | 14 +++++++++----- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/pkg/apis/core/fuzzer/fuzzer.go b/pkg/apis/core/fuzzer/fuzzer.go index 0600e57210b2..815d01791c11 100644 --- a/pkg/apis/core/fuzzer/fuzzer.go +++ b/pkg/apis/core/fuzzer/fuzzer.go @@ -473,7 +473,6 @@ var Funcs = func(codecs runtimeserializer.CodecFactory) []interface{} { s.Finalizers = []core.FinalizerName{core.FinalizerKubernetes} }, func(s *core.Namespace, c fuzz.Continue) { - // s.Finalizers = []core.FinalizerName{core.FinalizerKubernetes} c.FuzzNoCustom(s) // fuzz self without calling this function again // Match name --> label defaulting if len(s.Name) > 0 { diff --git a/test/integration/namespace/ns_conditions_test.go b/test/integration/namespace/ns_conditions_test.go index fee68afc1442..4e8832b9762b 100644 --- a/test/integration/namespace/ns_conditions_test.go +++ b/test/integration/namespace/ns_conditions_test.go @@ -128,19 +128,23 @@ func TestNamespaceLabels(t *testing.T) { }, }, metav1.CreateOptions{}) + if err != nil { + t.Fatal(err) + } + if ns.Name != ns.Labels[corev1.LabelMetadataName] { t.Fatal(fmt.Errorf("expected %q, got %q", ns.Name, ns.Labels[corev1.LabelMetadataName])) } + nsList, err := kubeClient.CoreV1().Namespaces().List(context.TODO(), metav1.ListOptions{}) + if err != nil { t.Fatal(err) } - if nsList, err := kubeClient.CoreV1().Namespaces().List(context.TODO(), metav1.ListOptions{}); err != nil { - for _, ns := range nsList.Items { - if ns.Name != ns.Labels[corev1.LabelMetadataName] { - t.Fatal(fmt.Errorf("expected %q, got %q", ns.Name, ns.Labels[corev1.LabelMetadataName])) - } + for _, ns := range nsList.Items { + if ns.Name != ns.Labels[corev1.LabelMetadataName] { + t.Fatal(fmt.Errorf("expected %q, got %q", ns.Name, ns.Labels[corev1.LabelMetadataName])) } } } From b39a87ddccd106dc18e62c5b5f42bf8b11e26225 Mon Sep 17 00:00:00 2001 From: Ricardo Pchevuzinske Katz Date: Thu, 4 Mar 2021 10:33:09 -0300 Subject: [PATCH 11/23] Add namespace canonicalize unit test --- pkg/registry/core/namespace/strategy_test.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/pkg/registry/core/namespace/strategy_test.go b/pkg/registry/core/namespace/strategy_test.go index 767077f5b1a4..2e6aebdb27b8 100644 --- a/pkg/registry/core/namespace/strategy_test.go +++ b/pkg/registry/core/namespace/strategy_test.go @@ -19,10 +19,14 @@ package namespace import ( "testing" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" + utilfeature "k8s.io/apiserver/pkg/util/feature" + featuregatetesting "k8s.io/component-base/featuregate/testing" apitesting "k8s.io/kubernetes/pkg/api/testing" api "k8s.io/kubernetes/pkg/apis/core" + "k8s.io/kubernetes/pkg/features" // ensure types are installed _ "k8s.io/kubernetes/pkg/apis/core/install" @@ -68,6 +72,19 @@ func TestNamespaceStrategy(t *testing.T) { } } +func TestNamespaceDefaultLabelCanonicalize(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.NamespaceDefaultLabelName, true)() + + namespace := &api.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "foo"}, + } + + Strategy.Canonicalize(namespace) + if label, ok := namespace.Labels[v1.LabelMetadataName]; len(namespace.Name) > 0 && !ok || label != namespace.Name { + t.Errorf("Invalid namespace, default label was not added") + } +} + func TestNamespaceStatusStrategy(t *testing.T) { ctx := genericapirequest.NewDefaultContext() if StatusStrategy.NamespaceScoped() { From 26db9fcec3bbff682d352da2abfceba7188278ce Mon Sep 17 00:00:00 2001 From: Ricardo Pchevuzinske Katz Date: Thu, 4 Mar 2021 18:14:14 -0300 Subject: [PATCH 12/23] Improve validation code and code comments --- pkg/apis/core/v1/defaults.go | 2 ++ pkg/apis/core/validation/validation.go | 2 +- pkg/registry/core/namespace/strategy.go | 4 ++++ pkg/registry/core/namespace/strategy_test.go | 4 ++-- 4 files changed, 9 insertions(+), 3 deletions(-) diff --git a/pkg/apis/core/v1/defaults.go b/pkg/apis/core/v1/defaults.go index 427ae2be81a2..1e3ca00fbc2c 100644 --- a/pkg/apis/core/v1/defaults.go +++ b/pkg/apis/core/v1/defaults.go @@ -329,6 +329,8 @@ func SetDefaults_Namespace(obj *v1.Namespace) { // TODO, remove the feature gate in 1.22 // we cant SetDefaults for nameless namespaces (generateName), // we don't need to overwrite the pre-existing labelMetadataName + // This code needs to be kept in sync with the implementation that exists + // in Namespace Canonicalize strategy (pkg/registry/core/namespace) if len(obj.Name) > 0 && obj.Labels[v1.LabelMetadataName] != obj.Name { if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { if obj.Labels == nil { diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 66f99183e809..9da35c167520 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -5657,7 +5657,7 @@ func ValidateNamespace(namespace *core.Namespace) field.ErrorList { allErrs = append(allErrs, validateFinalizerName(string(namespace.Spec.Finalizers[i]), field.NewPath("spec", "finalizers"))...) } if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { - if label, ok := namespace.Labels[v1.LabelMetadataName]; len(namespace.Name) > 0 && ok && label != namespace.Name { + if len(namespace.Name) > 0 && namespace.Labels[v1.LabelMetadataName] != namespace.Name { allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "labels").Key(v1.LabelMetadataName), namespace.Labels[v1.LabelMetadataName], fmt.Sprintf("must be %s", namespace.Name))) } } diff --git a/pkg/registry/core/namespace/strategy.go b/pkg/registry/core/namespace/strategy.go index f366fd026a97..b201b3dc20a5 100644 --- a/pkg/registry/core/namespace/strategy.go +++ b/pkg/registry/core/namespace/strategy.go @@ -91,6 +91,10 @@ func (namespaceStrategy) Validate(ctx context.Context, obj runtime.Object) field // Canonicalize normalizes the object after validation. func (namespaceStrategy) Canonicalize(obj runtime.Object) { + // Ensure the label matches the name for namespaces just created using GenerateName, + // where the final name wasn't available for defaulting to make this change. + // This code needs to be kept in sync with the implementation that exists + // in Namespace defaulting (pkg/apis/core/v1) ns := obj.(*api.Namespace) if len(ns.Name) > 0 && ns.Labels[v1.LabelMetadataName] != ns.Name { if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { diff --git a/pkg/registry/core/namespace/strategy_test.go b/pkg/registry/core/namespace/strategy_test.go index 2e6aebdb27b8..1900bc54ab70 100644 --- a/pkg/registry/core/namespace/strategy_test.go +++ b/pkg/registry/core/namespace/strategy_test.go @@ -41,7 +41,7 @@ func TestNamespaceStrategy(t *testing.T) { t.Errorf("Namespaces should not allow create on update") } namespace := &api.Namespace{ - ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "10"}, + ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "10", Labels: map[string]string{v1.LabelMetadataName: "foo"}}, Status: api.NamespaceStatus{Phase: api.NamespaceTerminating}, } Strategy.PrepareForCreate(ctx, namespace) @@ -80,7 +80,7 @@ func TestNamespaceDefaultLabelCanonicalize(t *testing.T) { } Strategy.Canonicalize(namespace) - if label, ok := namespace.Labels[v1.LabelMetadataName]; len(namespace.Name) > 0 && !ok || label != namespace.Name { + if namespace.Labels[v1.LabelMetadataName] != namespace.Name { t.Errorf("Invalid namespace, default label was not added") } } From 49e6cc9f73ecd347df4cc4d92d99423a09e1b675 Mon Sep 17 00:00:00 2001 From: Ricardo Pchevuzinske Katz Date: Thu, 4 Mar 2021 21:45:04 -0300 Subject: [PATCH 13/23] move validation of labels to validateupdate --- pkg/apis/core/validation/validation.go | 10 +-- pkg/apis/core/validation/validation_test.go | 88 +++++++++++---------- 2 files changed, 50 insertions(+), 48 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 9da35c167520..a9db71199ada 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -5656,11 +5656,6 @@ func ValidateNamespace(namespace *core.Namespace) field.ErrorList { for i := range namespace.Spec.Finalizers { allErrs = append(allErrs, validateFinalizerName(string(namespace.Spec.Finalizers[i]), field.NewPath("spec", "finalizers"))...) } - if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { - if len(namespace.Name) > 0 && namespace.Labels[v1.LabelMetadataName] != namespace.Name { - allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "labels").Key(v1.LabelMetadataName), namespace.Labels[v1.LabelMetadataName], fmt.Sprintf("must be %s", namespace.Name))) - } - } return allErrs } @@ -5687,6 +5682,11 @@ func validateKubeFinalizerName(stringValue string, fldPath *field.Path) field.Er // newNamespace is updated with fields that cannot be changed func ValidateNamespaceUpdate(newNamespace *core.Namespace, oldNamespace *core.Namespace) field.ErrorList { allErrs := ValidateObjectMetaUpdate(&newNamespace.ObjectMeta, &oldNamespace.ObjectMeta, field.NewPath("metadata")) + if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { + if len(newNamespace.Name) > 0 && newNamespace.Labels[v1.LabelMetadataName] != newNamespace.Name { + allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "labels").Key(v1.LabelMetadataName), newNamespace.Labels[v1.LabelMetadataName], fmt.Sprintf("must be %s", newNamespace.Name))) + } + } newNamespace.Spec.Finalizers = oldNamespace.Spec.Finalizers newNamespace.Status = oldNamespace.Status return allErrs diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index d68085d9c8d9..97e11fe8cf3c 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -14617,13 +14617,14 @@ func TestValidateResourceQuota(t *testing.T) { } func TestValidateNamespace(t *testing.T) { + validLabels := map[string]string{"a": "b"} invalidLabels := map[string]string{"NoUppercaseOrSpecialCharsLike=Equals": "b"} successCases := []core.Namespace{ { - ObjectMeta: metav1.ObjectMeta{Name: "abc", Labels: map[string]string{"a": "b", v1.LabelMetadataName: "abc"}}, + ObjectMeta: metav1.ObjectMeta{Name: "abc", Labels: validLabels}, }, { - ObjectMeta: metav1.ObjectMeta{Name: "abc-123", Labels: map[string]string{v1.LabelMetadataName: "abc-123"}}, + ObjectMeta: metav1.ObjectMeta{Name: "abc-123"}, Spec: core.NamespaceSpec{ Finalizers: []core.FinalizerName{"example.com/something", "example.com/other"}, }, @@ -14657,42 +14658,6 @@ func TestValidateNamespace(t *testing.T) { t.Errorf("expected failure for %s", k) } } - labelsTestCases := map[string]struct { - ns *core.Namespace - expectError bool - }{ - "Valid: namespace name label exists": { - ns: &core.Namespace{ - ObjectMeta: metav1.ObjectMeta{Name: "abc-xyz", Labels: map[string]string{"kubernetes.io/metadata.name": "abc-xyz"}}, - }, - expectError: false, - }, - /* Removing this, validation should probably just check if the label exists and is different from the expected - "Invalid: namespace name label does not exist": { - ns: &core.Namespace{ - ObjectMeta: metav1.ObjectMeta{Name: "abc-xyz"}, - }, - expectError: true, - },*/ - "Invalid: incorrect namespace name label exists": { - ns: &core.Namespace{ - ObjectMeta: metav1.ObjectMeta{Name: "abc-xyz", Labels: map[string]string{"kubernetes.io/metadata.name": "i-be-wrong"}}, - }, - expectError: true, - }, - } - for tcName, tc := range labelsTestCases { - t.Run(tcName, func(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.NamespaceDefaultLabelName, true)() - errs := ValidateNamespace(tc.ns) - if tc.expectError && len(errs) == 0 { - t.Errorf("Unexpected success") - } - if !tc.expectError && len(errs) != 0 { - t.Errorf("Unexpected error(s): %v", errs) - } - }) - } } func TestValidateNamespaceFinalizeUpdate(t *testing.T) { @@ -14850,7 +14815,7 @@ func TestValidateNamespaceUpdate(t *testing.T) { }, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "foo2", - Labels: map[string]string{"foo": "baz"}, + Labels: map[string]string{"foo": "baz", v1.LabelMetadataName: "foo2"}, }, }, true}, {core.Namespace{ @@ -14860,7 +14825,7 @@ func TestValidateNamespaceUpdate(t *testing.T) { }, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "foo3", - Labels: map[string]string{"foo": "baz"}, + Labels: map[string]string{"foo": "baz", v1.LabelMetadataName: "foo3"}, }, }, true}, {core.Namespace{ @@ -14871,7 +14836,7 @@ func TestValidateNamespaceUpdate(t *testing.T) { }, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "foo4", - Labels: map[string]string{"foo": "baz"}, + Labels: map[string]string{"foo": "baz", v1.LabelMetadataName: "foo4"}, }, }, true}, {core.Namespace{ @@ -14882,7 +14847,7 @@ func TestValidateNamespaceUpdate(t *testing.T) { }, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "foo5", - Labels: map[string]string{"Foo": "baz"}, + Labels: map[string]string{"Foo": "baz", v1.LabelMetadataName: "foo5"}, }, }, true}, {core.Namespace{ @@ -14893,7 +14858,7 @@ func TestValidateNamespaceUpdate(t *testing.T) { }, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "foo6", - Labels: map[string]string{"Foo": "baz"}, + Labels: map[string]string{"Foo": "baz", v1.LabelMetadataName: "foo6"}, }, Spec: core.NamespaceSpec{ Finalizers: []core.FinalizerName{"kubernetes"}, @@ -14902,7 +14867,44 @@ func TestValidateNamespaceUpdate(t *testing.T) { Phase: core.NamespaceTerminating, }, }, true}, + // The below tests validates the existence of v1.LabelMetadataName + {core.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "youshallnotremovelabel", + Labels: map[string]string{"foo": "baz", v1.LabelMetadataName: "youshallnotremovelabel"}, + }, + }, core.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "youshallnotremovelabel", + Labels: map[string]string{"Foo": "baz"}, + }, + }, false}, + {core.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "youshallnotchangelabel", + Labels: map[string]string{"foo": "baz", v1.LabelMetadataName: "youshallnotchangelabel"}, + }, + }, core.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "youshallnotremovelabel", + Labels: map[string]string{"Foo": "baz", v1.LabelMetadataName: "youshallnotchangelabel"}, + }, + }, false}, + {core.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "youshalladdrequiredlabel", + Labels: map[string]string{"foo": "baz"}, + }, + }, core.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "youshalladdrequiredlabel", + Labels: map[string]string{"Foo": "baz"}, + }, + }, false}, } + + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.NamespaceDefaultLabelName, true)() + for i, test := range tests { test.namespace.ObjectMeta.ResourceVersion = "1" test.oldNamespace.ObjectMeta.ResourceVersion = "1" From 22200eee7d212574d6bdb847e2f8056c59a2b81e Mon Sep 17 00:00:00 2001 From: jay vyas Date: Sat, 6 Mar 2021 12:28:28 -0700 Subject: [PATCH 14/23] spacex will save us all --- pkg/apis/core/v1/defaults.go | 7 +++++-- pkg/registry/core/namespace/storage/storage_test.go | 6 ++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/pkg/apis/core/v1/defaults.go b/pkg/apis/core/v1/defaults.go index 1e3ca00fbc2c..e92d365af182 100644 --- a/pkg/apis/core/v1/defaults.go +++ b/pkg/apis/core/v1/defaults.go @@ -331,8 +331,11 @@ func SetDefaults_Namespace(obj *v1.Namespace) { // we don't need to overwrite the pre-existing labelMetadataName // This code needs to be kept in sync with the implementation that exists // in Namespace Canonicalize strategy (pkg/registry/core/namespace) - if len(obj.Name) > 0 && obj.Labels[v1.LabelMetadataName] != obj.Name { - if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { + + // note that this can result in many calls to feature enablement in some cases, but + // we assume that theres no real cost there. + if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { + if len(obj.Name) > 0 && obj.Labels[v1.LabelMetadataName] != obj.Name { if obj.Labels == nil { obj.Labels = map[string]string{} } diff --git a/pkg/registry/core/namespace/storage/storage_test.go b/pkg/registry/core/namespace/storage/storage_test.go index ace116a406c1..bb9df6577eae 100644 --- a/pkg/registry/core/namespace/storage/storage_test.go +++ b/pkg/registry/core/namespace/storage/storage_test.go @@ -59,7 +59,6 @@ func TestCreate(t *testing.T) { test := genericregistrytest.New(t, storage.store).ClusterScope() namespace := validNewNamespace() namespace.ObjectMeta = metav1.ObjectMeta{GenerateName: "foo"} - test.TestCreate( // valid namespace, // invalid @@ -109,7 +108,10 @@ func TestGet(t *testing.T) { defer server.Terminate(t) defer storage.store.DestroyFunc() test := genericregistrytest.New(t, storage.store).ClusterScope() - test.TestGet(validNewNamespace()) + + test.TestGet(validNewNamespace()) // --> *api.Namespace object + // resttest.TestGet + // testGetFound } func TestList(t *testing.T) { From e673cabd8ce2b732dfdb40a2b1c6a761041eb2ee Mon Sep 17 00:00:00 2001 From: jay vyas Date: Sat, 6 Mar 2021 12:30:22 -0700 Subject: [PATCH 15/23] add comment to testget --- pkg/registry/core/namespace/storage/storage_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/registry/core/namespace/storage/storage_test.go b/pkg/registry/core/namespace/storage/storage_test.go index bb9df6577eae..984d938bd5a8 100644 --- a/pkg/registry/core/namespace/storage/storage_test.go +++ b/pkg/registry/core/namespace/storage/storage_test.go @@ -109,9 +109,8 @@ func TestGet(t *testing.T) { defer storage.store.DestroyFunc() test := genericregistrytest.New(t, storage.store).ClusterScope() - test.TestGet(validNewNamespace()) // --> *api.Namespace object - // resttest.TestGet - // testGetFound + // note that this ultimately may call validation + test.TestGet(validNewNamespace()) } func TestList(t *testing.T) { From d9a8645098f9038f969b4e4cb4ca989408273ddc Mon Sep 17 00:00:00 2001 From: jay vyas Date: Sat, 6 Mar 2021 12:31:27 -0700 Subject: [PATCH 16/23] readablility of canonicalize --- pkg/registry/core/namespace/strategy.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/registry/core/namespace/strategy.go b/pkg/registry/core/namespace/strategy.go index b201b3dc20a5..523c7de22584 100644 --- a/pkg/registry/core/namespace/strategy.go +++ b/pkg/registry/core/namespace/strategy.go @@ -96,8 +96,8 @@ func (namespaceStrategy) Canonicalize(obj runtime.Object) { // This code needs to be kept in sync with the implementation that exists // in Namespace defaulting (pkg/apis/core/v1) ns := obj.(*api.Namespace) - if len(ns.Name) > 0 && ns.Labels[v1.LabelMetadataName] != ns.Name { - if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { + if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { + if len(ns.Name) > 0 && ns.Labels[v1.LabelMetadataName] != ns.Name { if ns.Labels == nil { ns.Labels = map[string]string{} } From e2c76321684a21692122f437da58f870ad7f50e3 Mon Sep 17 00:00:00 2001 From: jay vyas Date: Sat, 6 Mar 2021 12:52:14 -0700 Subject: [PATCH 17/23] Added namespace finalize and status update validation --- pkg/apis/core/validation/validation.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index a9db71199ada..b3c639abb34c 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -5683,7 +5683,8 @@ func validateKubeFinalizerName(stringValue string, fldPath *field.Path) field.Er func ValidateNamespaceUpdate(newNamespace *core.Namespace, oldNamespace *core.Namespace) field.ErrorList { allErrs := ValidateObjectMetaUpdate(&newNamespace.ObjectMeta, &oldNamespace.ObjectMeta, field.NewPath("metadata")) if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { - if len(newNamespace.Name) > 0 && newNamespace.Labels[v1.LabelMetadataName] != newNamespace.Name { + // assuming generated names are resolved before arriving here + if newNamespace.Labels[v1.LabelMetadataName] != newNamespace.Name { allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "labels").Key(v1.LabelMetadataName), newNamespace.Labels[v1.LabelMetadataName], fmt.Sprintf("must be %s", newNamespace.Name))) } } @@ -5697,6 +5698,12 @@ func ValidateNamespaceUpdate(newNamespace *core.Namespace, oldNamespace *core.Na func ValidateNamespaceStatusUpdate(newNamespace, oldNamespace *core.Namespace) field.ErrorList { allErrs := ValidateObjectMetaUpdate(&newNamespace.ObjectMeta, &oldNamespace.ObjectMeta, field.NewPath("metadata")) newNamespace.Spec = oldNamespace.Spec + if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { + // assuming that any generate name has been called before we get here + if newNamespace.Labels[v1.LabelMetadataName] != newNamespace.Name { + allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "labels").Key(v1.LabelMetadataName), newNamespace.Labels[v1.LabelMetadataName], fmt.Sprintf("must be %s", newNamespace.Name))) + } + } if newNamespace.DeletionTimestamp.IsZero() { if newNamespace.Status.Phase != core.NamespaceActive { allErrs = append(allErrs, field.Invalid(field.NewPath("status", "Phase"), newNamespace.Status.Phase, "may only be 'Active' if `deletionTimestamp` is empty")) @@ -5713,7 +5720,12 @@ func ValidateNamespaceStatusUpdate(newNamespace, oldNamespace *core.Namespace) f // newNamespace is updated with fields that cannot be changed. func ValidateNamespaceFinalizeUpdate(newNamespace, oldNamespace *core.Namespace) field.ErrorList { allErrs := ValidateObjectMetaUpdate(&newNamespace.ObjectMeta, &oldNamespace.ObjectMeta, field.NewPath("metadata")) - + if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { + // assuming that any generate name has been called before we get here + if newNamespace.Labels[v1.LabelMetadataName] != newNamespace.Name { + allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "labels").Key(v1.LabelMetadataName), newNamespace.Labels[v1.LabelMetadataName], fmt.Sprintf("must be %s", newNamespace.Name))) + } + } fldPath := field.NewPath("spec", "finalizers") for i := range newNamespace.Spec.Finalizers { idxPath := fldPath.Index(i) From 90d56053d674df2fa4a6bbc516f831279ee1b818 Mon Sep 17 00:00:00 2001 From: jay vyas Date: Sat, 6 Mar 2021 13:01:07 -0700 Subject: [PATCH 18/23] comment about ungenerated names --- pkg/apis/core/validation/validation.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index b3c639abb34c..a84779b3d993 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -5652,6 +5652,7 @@ func ValidateResourceQuotaStatusUpdate(newResourceQuota, oldResourceQuota *core. // ValidateNamespace tests if required fields are set. func ValidateNamespace(namespace *core.Namespace) field.ErrorList { + // skip namespace default label validation, because theres no 'definition' for a default label on a namespace with an as of yet "ungenerated" name allErrs := ValidateObjectMeta(&namespace.ObjectMeta, false, ValidateNamespaceName, field.NewPath("metadata")) for i := range namespace.Spec.Finalizers { allErrs = append(allErrs, validateFinalizerName(string(namespace.Spec.Finalizers[i]), field.NewPath("spec", "finalizers"))...) From afe021e9df638e6c9d552a4f9ba4bbf3b62f69a1 Mon Sep 17 00:00:00 2001 From: Ricardo Pchevuzinske Katz Date: Sat, 6 Mar 2021 17:26:22 -0300 Subject: [PATCH 19/23] correcting a missing line on storage_test --- pkg/registry/core/namespace/storage/storage_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/registry/core/namespace/storage/storage_test.go b/pkg/registry/core/namespace/storage/storage_test.go index 984d938bd5a8..fc31c809cc29 100644 --- a/pkg/registry/core/namespace/storage/storage_test.go +++ b/pkg/registry/core/namespace/storage/storage_test.go @@ -59,6 +59,7 @@ func TestCreate(t *testing.T) { test := genericregistrytest.New(t, storage.store).ClusterScope() namespace := validNewNamespace() namespace.ObjectMeta = metav1.ObjectMeta{GenerateName: "foo"} + test.TestCreate( // valid namespace, // invalid From 8f2f100b12d324d14f5a701e1510bff323342179 Mon Sep 17 00:00:00 2001 From: Ricardo Pchevuzinske Katz Date: Sun, 7 Mar 2021 11:43:57 -0300 Subject: [PATCH 20/23] Update the namespace validation unit test --- pkg/apis/core/validation/validation_test.go | 108 +++++++++++++++++--- 1 file changed, 94 insertions(+), 14 deletions(-) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 97e11fe8cf3c..5d2b50013931 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -14669,42 +14669,80 @@ func TestValidateNamespaceFinalizeUpdate(t *testing.T) { {core.Namespace{}, core.Namespace{}, true}, {core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo"}}, + Name: "foo", + Labels: map[string]string{ + v1.LabelMetadataName: "foo", + }}}, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo"}, + Name: "foo", + Labels: map[string]string{ + v1.LabelMetadataName: "foo", + }}, Spec: core.NamespaceSpec{ Finalizers: []core.FinalizerName{"Foo"}, }, }, false}, {core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo"}, + Name: "foo", + Labels: map[string]string{ + v1.LabelMetadataName: "foo", + }}, Spec: core.NamespaceSpec{ Finalizers: []core.FinalizerName{"foo.com/bar"}, }, }, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo"}, + Name: "foo", + Labels: map[string]string{ + v1.LabelMetadataName: "foo", + }}, Spec: core.NamespaceSpec{ Finalizers: []core.FinalizerName{"foo.com/bar", "what.com/bar"}, }, }, true}, {core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "fooemptyfinalizer"}, + Name: "fooemptyfinalizer", + Labels: map[string]string{ + v1.LabelMetadataName: "fooemptyfinalizer", + }}, Spec: core.NamespaceSpec{ Finalizers: []core.FinalizerName{"foo.com/bar"}, }, }, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "fooemptyfinalizer"}, + Name: "fooemptyfinalizer", + Labels: map[string]string{ + v1.LabelMetadataName: "fooemptyfinalizer", + }}, Spec: core.NamespaceSpec{ Finalizers: []core.FinalizerName{"", "foo.com/bar", "what.com/bar"}, }, }, false}, + {core.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foowronglabel", + Labels: map[string]string{ + v1.LabelMetadataName: "foowronglabel", + }}, + Spec: core.NamespaceSpec{ + Finalizers: []core.FinalizerName{"foo.com/bar"}, + }, + }, + core.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foowronglabel", + Labels: map[string]string{ + v1.LabelMetadataName: "thisisreallywrong", + }}, + Spec: core.NamespaceSpec{ + Finalizers: []core.FinalizerName{"foo.com/bar", "what.com/bar"}, + }, + }, false}, } for i, test := range tests { test.namespace.ObjectMeta.ResourceVersion = "1" @@ -14736,10 +14774,16 @@ func TestValidateNamespaceStatusUpdate(t *testing.T) { // Cannot set deletionTimestamp via status update {core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo"}}, + Name: "foo", + Labels: map[string]string{ + v1.LabelMetadataName: "foo", + }}}, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo", + Name: "foo", + Labels: map[string]string{ + v1.LabelMetadataName: "foo", + }, DeletionTimestamp: &now}, Status: core.NamespaceStatus{ Phase: core.NamespaceTerminating, @@ -14748,11 +14792,17 @@ func TestValidateNamespaceStatusUpdate(t *testing.T) { // Can update phase via status update {core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo", + Name: "foo", + Labels: map[string]string{ + v1.LabelMetadataName: "foo", + }, DeletionTimestamp: &now}}, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo", + Name: "foo", + Labels: map[string]string{ + v1.LabelMetadataName: "foo", + }, DeletionTimestamp: &now}, Status: core.NamespaceStatus{ Phase: core.NamespaceTerminating, @@ -14760,20 +14810,50 @@ func TestValidateNamespaceStatusUpdate(t *testing.T) { }, true}, {core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo"}}, + Name: "foo", + Labels: map[string]string{ + v1.LabelMetadataName: "foo", + }}}, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo"}, + Name: "foo", + Labels: map[string]string{ + v1.LabelMetadataName: "foo", + }}, Status: core.NamespaceStatus{ Phase: core.NamespaceTerminating, }, }, false}, {core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo"}}, + Name: "foo", + Labels: map[string]string{ + v1.LabelMetadataName: "foo", + }}}, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "bar"}, + Name: "bar", + Labels: map[string]string{ + v1.LabelMetadataName: "bar", + }}, + Status: core.NamespaceStatus{ + Phase: core.NamespaceTerminating, + }, + }, false}, + {core.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Labels: map[string]string{ + v1.LabelMetadataName: "foo", + }, + DeletionTimestamp: &now}}, + core.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Labels: map[string]string{ + v1.LabelMetadataName: "foo123", + }, + DeletionTimestamp: &now}, Status: core.NamespaceStatus{ Phase: core.NamespaceTerminating, }, From 2bdfe9d162b58e7b06c53d43f6b93b033af9fa00 Mon Sep 17 00:00:00 2001 From: Ricardo Pchevuzinske Katz Date: Sun, 7 Mar 2021 12:31:40 -0300 Subject: [PATCH 21/23] Add more missing unit test changes --- pkg/registry/core/namespace/strategy_test.go | 26 +++++++++++--------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/pkg/registry/core/namespace/strategy_test.go b/pkg/registry/core/namespace/strategy_test.go index 1900bc54ab70..d4597accac74 100644 --- a/pkg/registry/core/namespace/strategy_test.go +++ b/pkg/registry/core/namespace/strategy_test.go @@ -95,13 +95,15 @@ func TestNamespaceStatusStrategy(t *testing.T) { } now := metav1.Now() oldNamespace := &api.Namespace{ - ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "10", DeletionTimestamp: &now}, - Spec: api.NamespaceSpec{Finalizers: []api.FinalizerName{"kubernetes"}}, - Status: api.NamespaceStatus{Phase: api.NamespaceActive}, + ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "10", DeletionTimestamp: &now, + Labels: map[string]string{v1.LabelMetadataName: "foo"}}, + Spec: api.NamespaceSpec{Finalizers: []api.FinalizerName{"kubernetes"}}, + Status: api.NamespaceStatus{Phase: api.NamespaceActive}, } namespace := &api.Namespace{ - ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "9", DeletionTimestamp: &now}, - Status: api.NamespaceStatus{Phase: api.NamespaceTerminating}, + ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "9", DeletionTimestamp: &now, + Labels: map[string]string{v1.LabelMetadataName: "foo"}}, + Status: api.NamespaceStatus{Phase: api.NamespaceTerminating}, } StatusStrategy.PrepareForUpdate(ctx, namespace, oldNamespace) if namespace.Status.Phase != api.NamespaceTerminating { @@ -128,14 +130,16 @@ func TestNamespaceFinalizeStrategy(t *testing.T) { t.Errorf("Namespaces should not allow create on update") } oldNamespace := &api.Namespace{ - ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "10"}, - Spec: api.NamespaceSpec{Finalizers: []api.FinalizerName{"kubernetes", "example.com/org"}}, - Status: api.NamespaceStatus{Phase: api.NamespaceActive}, + ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "10", + Labels: map[string]string{v1.LabelMetadataName: "foo"}}, + Spec: api.NamespaceSpec{Finalizers: []api.FinalizerName{"kubernetes", "example.com/org"}}, + Status: api.NamespaceStatus{Phase: api.NamespaceActive}, } namespace := &api.Namespace{ - ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "9"}, - Spec: api.NamespaceSpec{Finalizers: []api.FinalizerName{"example.com/foo"}}, - Status: api.NamespaceStatus{Phase: api.NamespaceTerminating}, + ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "9", + Labels: map[string]string{v1.LabelMetadataName: "foo"}}, + Spec: api.NamespaceSpec{Finalizers: []api.FinalizerName{"example.com/foo"}}, + Status: api.NamespaceStatus{Phase: api.NamespaceTerminating}, } FinalizeStrategy.PrepareForUpdate(ctx, namespace, oldNamespace) if namespace.Status.Phase != api.NamespaceActive { From 920096bc002f3b7f98c282016b2dbfc7db041250 Mon Sep 17 00:00:00 2001 From: Ricardo Pchevuzinske Katz Date: Mon, 8 Mar 2021 11:59:16 -0300 Subject: [PATCH 22/23] Let's just blast the value. Also documenting the workflow here --- pkg/apis/core/v1/defaults.go | 7 +++---- pkg/registry/core/namespace/strategy.go | 21 ++++++++++++++++----- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/pkg/apis/core/v1/defaults.go b/pkg/apis/core/v1/defaults.go index e92d365af182..dc936458708d 100644 --- a/pkg/apis/core/v1/defaults.go +++ b/pkg/apis/core/v1/defaults.go @@ -327,15 +327,14 @@ func SetDefaults_HTTPGetAction(obj *v1.HTTPGetAction) { // SetDefaults_Namespace adds a default label for all namespaces func SetDefaults_Namespace(obj *v1.Namespace) { // TODO, remove the feature gate in 1.22 - // we cant SetDefaults for nameless namespaces (generateName), - // we don't need to overwrite the pre-existing labelMetadataName + // we can't SetDefaults for nameless namespaces (generateName). // This code needs to be kept in sync with the implementation that exists // in Namespace Canonicalize strategy (pkg/registry/core/namespace) // note that this can result in many calls to feature enablement in some cases, but - // we assume that theres no real cost there. + // we assume that there's no real cost there. if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { - if len(obj.Name) > 0 && obj.Labels[v1.LabelMetadataName] != obj.Name { + if len(obj.Name) > 0 { if obj.Labels == nil { obj.Labels = map[string]string{} } diff --git a/pkg/registry/core/namespace/strategy.go b/pkg/registry/core/namespace/strategy.go index 523c7de22584..1bf63d95178b 100644 --- a/pkg/registry/core/namespace/strategy.go +++ b/pkg/registry/core/namespace/strategy.go @@ -95,14 +95,25 @@ func (namespaceStrategy) Canonicalize(obj runtime.Object) { // where the final name wasn't available for defaulting to make this change. // This code needs to be kept in sync with the implementation that exists // in Namespace defaulting (pkg/apis/core/v1) + // Why this hook *and* defaults.go? + // + // CREATE: + // Defaulting and PrepareForCreate happen before generateName is completed + // (i.e. the name is not yet known). Validation happens after generateName + // but should not modify objects. Canonicalize happens later, which makes + // it the best hook for setting the label. + // + // UPDATE: + // Defaulting and Canonicalize will both trigger with the full name. + // + // GET: + // Only defaulting will be applied. ns := obj.(*api.Namespace) if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { - if len(ns.Name) > 0 && ns.Labels[v1.LabelMetadataName] != ns.Name { - if ns.Labels == nil { - ns.Labels = map[string]string{} - } - ns.Labels[v1.LabelMetadataName] = ns.Name + if ns.Labels == nil { + ns.Labels = map[string]string{} } + ns.Labels[v1.LabelMetadataName] = ns.Name } } From 465474269f352c941eccea4dfdb255d4410b3634 Mon Sep 17 00:00:00 2001 From: Ricardo Pchevuzinske Katz Date: Mon, 8 Mar 2021 12:56:57 -0300 Subject: [PATCH 23/23] Remove unnecessary validations --- pkg/apis/core/validation/validation.go | 20 +-- pkg/apis/core/validation/validation_test.go | 155 +++----------------- 2 files changed, 20 insertions(+), 155 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index a84779b3d993..1b668083b3b3 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -5652,7 +5652,6 @@ func ValidateResourceQuotaStatusUpdate(newResourceQuota, oldResourceQuota *core. // ValidateNamespace tests if required fields are set. func ValidateNamespace(namespace *core.Namespace) field.ErrorList { - // skip namespace default label validation, because theres no 'definition' for a default label on a namespace with an as of yet "ungenerated" name allErrs := ValidateObjectMeta(&namespace.ObjectMeta, false, ValidateNamespaceName, field.NewPath("metadata")) for i := range namespace.Spec.Finalizers { allErrs = append(allErrs, validateFinalizerName(string(namespace.Spec.Finalizers[i]), field.NewPath("spec", "finalizers"))...) @@ -5683,12 +5682,6 @@ func validateKubeFinalizerName(stringValue string, fldPath *field.Path) field.Er // newNamespace is updated with fields that cannot be changed func ValidateNamespaceUpdate(newNamespace *core.Namespace, oldNamespace *core.Namespace) field.ErrorList { allErrs := ValidateObjectMetaUpdate(&newNamespace.ObjectMeta, &oldNamespace.ObjectMeta, field.NewPath("metadata")) - if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { - // assuming generated names are resolved before arriving here - if newNamespace.Labels[v1.LabelMetadataName] != newNamespace.Name { - allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "labels").Key(v1.LabelMetadataName), newNamespace.Labels[v1.LabelMetadataName], fmt.Sprintf("must be %s", newNamespace.Name))) - } - } newNamespace.Spec.Finalizers = oldNamespace.Spec.Finalizers newNamespace.Status = oldNamespace.Status return allErrs @@ -5699,12 +5692,6 @@ func ValidateNamespaceUpdate(newNamespace *core.Namespace, oldNamespace *core.Na func ValidateNamespaceStatusUpdate(newNamespace, oldNamespace *core.Namespace) field.ErrorList { allErrs := ValidateObjectMetaUpdate(&newNamespace.ObjectMeta, &oldNamespace.ObjectMeta, field.NewPath("metadata")) newNamespace.Spec = oldNamespace.Spec - if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { - // assuming that any generate name has been called before we get here - if newNamespace.Labels[v1.LabelMetadataName] != newNamespace.Name { - allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "labels").Key(v1.LabelMetadataName), newNamespace.Labels[v1.LabelMetadataName], fmt.Sprintf("must be %s", newNamespace.Name))) - } - } if newNamespace.DeletionTimestamp.IsZero() { if newNamespace.Status.Phase != core.NamespaceActive { allErrs = append(allErrs, field.Invalid(field.NewPath("status", "Phase"), newNamespace.Status.Phase, "may only be 'Active' if `deletionTimestamp` is empty")) @@ -5721,12 +5708,7 @@ func ValidateNamespaceStatusUpdate(newNamespace, oldNamespace *core.Namespace) f // newNamespace is updated with fields that cannot be changed. func ValidateNamespaceFinalizeUpdate(newNamespace, oldNamespace *core.Namespace) field.ErrorList { allErrs := ValidateObjectMetaUpdate(&newNamespace.ObjectMeta, &oldNamespace.ObjectMeta, field.NewPath("metadata")) - if utilfeature.DefaultFeatureGate.Enabled(features.NamespaceDefaultLabelName) { - // assuming that any generate name has been called before we get here - if newNamespace.Labels[v1.LabelMetadataName] != newNamespace.Name { - allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "labels").Key(v1.LabelMetadataName), newNamespace.Labels[v1.LabelMetadataName], fmt.Sprintf("must be %s", newNamespace.Name))) - } - } + fldPath := field.NewPath("spec", "finalizers") for i := range newNamespace.Spec.Finalizers { idxPath := fldPath.Index(i) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 5d2b50013931..6ea9b35aa339 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -14669,80 +14669,42 @@ func TestValidateNamespaceFinalizeUpdate(t *testing.T) { {core.Namespace{}, core.Namespace{}, true}, {core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Labels: map[string]string{ - v1.LabelMetadataName: "foo", - }}}, + Name: "foo"}}, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Labels: map[string]string{ - v1.LabelMetadataName: "foo", - }}, + Name: "foo"}, Spec: core.NamespaceSpec{ Finalizers: []core.FinalizerName{"Foo"}, }, }, false}, {core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Labels: map[string]string{ - v1.LabelMetadataName: "foo", - }}, + Name: "foo"}, Spec: core.NamespaceSpec{ Finalizers: []core.FinalizerName{"foo.com/bar"}, }, }, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Labels: map[string]string{ - v1.LabelMetadataName: "foo", - }}, + Name: "foo"}, Spec: core.NamespaceSpec{ Finalizers: []core.FinalizerName{"foo.com/bar", "what.com/bar"}, }, }, true}, {core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "fooemptyfinalizer", - Labels: map[string]string{ - v1.LabelMetadataName: "fooemptyfinalizer", - }}, + Name: "fooemptyfinalizer"}, Spec: core.NamespaceSpec{ Finalizers: []core.FinalizerName{"foo.com/bar"}, }, }, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "fooemptyfinalizer", - Labels: map[string]string{ - v1.LabelMetadataName: "fooemptyfinalizer", - }}, + Name: "fooemptyfinalizer"}, Spec: core.NamespaceSpec{ Finalizers: []core.FinalizerName{"", "foo.com/bar", "what.com/bar"}, }, }, false}, - {core.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foowronglabel", - Labels: map[string]string{ - v1.LabelMetadataName: "foowronglabel", - }}, - Spec: core.NamespaceSpec{ - Finalizers: []core.FinalizerName{"foo.com/bar"}, - }, - }, - core.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foowronglabel", - Labels: map[string]string{ - v1.LabelMetadataName: "thisisreallywrong", - }}, - Spec: core.NamespaceSpec{ - Finalizers: []core.FinalizerName{"foo.com/bar", "what.com/bar"}, - }, - }, false}, } for i, test := range tests { test.namespace.ObjectMeta.ResourceVersion = "1" @@ -14774,16 +14736,10 @@ func TestValidateNamespaceStatusUpdate(t *testing.T) { // Cannot set deletionTimestamp via status update {core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Labels: map[string]string{ - v1.LabelMetadataName: "foo", - }}}, + Name: "foo"}}, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Labels: map[string]string{ - v1.LabelMetadataName: "foo", - }, + Name: "foo", DeletionTimestamp: &now}, Status: core.NamespaceStatus{ Phase: core.NamespaceTerminating, @@ -14792,17 +14748,11 @@ func TestValidateNamespaceStatusUpdate(t *testing.T) { // Can update phase via status update {core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Labels: map[string]string{ - v1.LabelMetadataName: "foo", - }, + Name: "foo", DeletionTimestamp: &now}}, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Labels: map[string]string{ - v1.LabelMetadataName: "foo", - }, + Name: "foo", DeletionTimestamp: &now}, Status: core.NamespaceStatus{ Phase: core.NamespaceTerminating, @@ -14810,50 +14760,20 @@ func TestValidateNamespaceStatusUpdate(t *testing.T) { }, true}, {core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Labels: map[string]string{ - v1.LabelMetadataName: "foo", - }}}, - core.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Labels: map[string]string{ - v1.LabelMetadataName: "foo", - }}, - Status: core.NamespaceStatus{ - Phase: core.NamespaceTerminating, - }, - }, false}, - {core.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Labels: map[string]string{ - v1.LabelMetadataName: "foo", - }}}, + Name: "foo"}}, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "bar", - Labels: map[string]string{ - v1.LabelMetadataName: "bar", - }}, + Name: "foo"}, Status: core.NamespaceStatus{ Phase: core.NamespaceTerminating, }, }, false}, {core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Labels: map[string]string{ - v1.LabelMetadataName: "foo", - }, - DeletionTimestamp: &now}}, + Name: "foo"}}, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Labels: map[string]string{ - v1.LabelMetadataName: "foo123", - }, - DeletionTimestamp: &now}, + Name: "bar"}, Status: core.NamespaceStatus{ Phase: core.NamespaceTerminating, }, @@ -14895,7 +14815,7 @@ func TestValidateNamespaceUpdate(t *testing.T) { }, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "foo2", - Labels: map[string]string{"foo": "baz", v1.LabelMetadataName: "foo2"}, + Labels: map[string]string{"foo": "baz"}, }, }, true}, {core.Namespace{ @@ -14905,7 +14825,7 @@ func TestValidateNamespaceUpdate(t *testing.T) { }, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "foo3", - Labels: map[string]string{"foo": "baz", v1.LabelMetadataName: "foo3"}, + Labels: map[string]string{"foo": "baz"}, }, }, true}, {core.Namespace{ @@ -14916,7 +14836,7 @@ func TestValidateNamespaceUpdate(t *testing.T) { }, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "foo4", - Labels: map[string]string{"foo": "baz", v1.LabelMetadataName: "foo4"}, + Labels: map[string]string{"foo": "baz"}, }, }, true}, {core.Namespace{ @@ -14927,7 +14847,7 @@ func TestValidateNamespaceUpdate(t *testing.T) { }, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "foo5", - Labels: map[string]string{"Foo": "baz", v1.LabelMetadataName: "foo5"}, + Labels: map[string]string{"Foo": "baz"}, }, }, true}, {core.Namespace{ @@ -14938,7 +14858,7 @@ func TestValidateNamespaceUpdate(t *testing.T) { }, core.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "foo6", - Labels: map[string]string{"Foo": "baz", v1.LabelMetadataName: "foo6"}, + Labels: map[string]string{"Foo": "baz"}, }, Spec: core.NamespaceSpec{ Finalizers: []core.FinalizerName{"kubernetes"}, @@ -14947,44 +14867,7 @@ func TestValidateNamespaceUpdate(t *testing.T) { Phase: core.NamespaceTerminating, }, }, true}, - // The below tests validates the existence of v1.LabelMetadataName - {core.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "youshallnotremovelabel", - Labels: map[string]string{"foo": "baz", v1.LabelMetadataName: "youshallnotremovelabel"}, - }, - }, core.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "youshallnotremovelabel", - Labels: map[string]string{"Foo": "baz"}, - }, - }, false}, - {core.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "youshallnotchangelabel", - Labels: map[string]string{"foo": "baz", v1.LabelMetadataName: "youshallnotchangelabel"}, - }, - }, core.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "youshallnotremovelabel", - Labels: map[string]string{"Foo": "baz", v1.LabelMetadataName: "youshallnotchangelabel"}, - }, - }, false}, - {core.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "youshalladdrequiredlabel", - Labels: map[string]string{"foo": "baz"}, - }, - }, core.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "youshalladdrequiredlabel", - Labels: map[string]string{"Foo": "baz"}, - }, - }, false}, } - - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.NamespaceDefaultLabelName, true)() - for i, test := range tests { test.namespace.ObjectMeta.ResourceVersion = "1" test.oldNamespace.ObjectMeta.ResourceVersion = "1"