From 0f88bbe9b1fcb2138cd668948d75358c1a1d9858 Mon Sep 17 00:00:00 2001 From: wojtekt Date: Mon, 7 Dec 2020 13:16:40 +0100 Subject: [PATCH 1/3] Promote Immutable Secrets/ConfigMaps feature to GA --- pkg/apis/core/types.go | 2 -- pkg/features/kube_features.go | 5 +++-- pkg/kubelet/util/manager/watch_based_manager.go | 4 +--- .../util/manager/watch_based_manager_test.go | 5 ----- pkg/registry/core/configmap/strategy.go | 9 --------- pkg/registry/core/secret/strategy.go | 9 --------- staging/src/k8s.io/api/core/v1/types.go | 2 -- test/e2e/common/configmap_volume.go | 13 ++++++++++--- test/e2e/common/secrets_volume.go | 13 ++++++++++--- 9 files changed, 24 insertions(+), 38 deletions(-) diff --git a/pkg/apis/core/types.go b/pkg/apis/core/types.go index 72d4fb6db66..094366379d0 100644 --- a/pkg/apis/core/types.go +++ b/pkg/apis/core/types.go @@ -4945,7 +4945,6 @@ type Secret struct { // Immutable field, if set, ensures that data stored in the Secret cannot // be updated (only object metadata can be modified). - // This is a beta field enabled by ImmutableEphemeralVolumes feature gate. // +optional Immutable *bool @@ -5073,7 +5072,6 @@ type ConfigMap struct { // Immutable field, if set, ensures that data stored in the ConfigMap cannot // be updated (only object metadata can be modified). - // This is a beta field enabled by ImmutableEphemeralVolumes feature gate. // +optional Immutable *bool diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index fedddd75f2a..32b5823f7dd 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -595,7 +595,8 @@ const ( // owner: @wojtek-t // alpha: v1.18 - // beta: v1.19 + // beta: v1.19 + // ga: v1.21 // // Enables a feature to make secrets and configmaps data immutable. ImmutableEphemeralVolumes featuregate.Feature = "ImmutableEphemeralVolumes" @@ -817,7 +818,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS DaemonSetUpdateSurge: {Default: false, PreRelease: featuregate.Alpha}, ServiceTopology: {Default: false, PreRelease: featuregate.Alpha}, ServiceAppProtocol: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, - ImmutableEphemeralVolumes: {Default: true, PreRelease: featuregate.Beta}, + ImmutableEphemeralVolumes: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.24 HugePageStorageMediumSize: {Default: true, PreRelease: featuregate.Beta}, DownwardAPIHugePages: {Default: false, PreRelease: featuregate.Alpha}, ExternalPolicyForExternalIP: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.22 diff --git a/pkg/kubelet/util/manager/watch_based_manager.go b/pkg/kubelet/util/manager/watch_based_manager.go index 4dfe1037539..30f25b405bc 100644 --- a/pkg/kubelet/util/manager/watch_based_manager.go +++ b/pkg/kubelet/util/manager/watch_based_manager.go @@ -34,8 +34,6 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apimachinery/pkg/watch" - utilfeature "k8s.io/apiserver/pkg/util/feature" - "k8s.io/kubernetes/pkg/features" ) type listObjectFunc func(string, metav1.ListOptions) (runtime.Object, error) @@ -208,7 +206,7 @@ func (c *objectCache) Get(namespace, name string) (runtime.Object, error) { // already have it from here // - doing that would require significant refactoring to reflector // we limit ourselves to just quickly stop the reflector here. - if utilfeature.DefaultFeatureGate.Enabled(features.ImmutableEphemeralVolumes) && c.isImmutable(object) { + if c.isImmutable(object) { if item.stop() { klog.V(4).Infof("Stopped watching for changes of %q/%q - object is immutable", namespace, name) } diff --git a/pkg/kubelet/util/manager/watch_based_manager_test.go b/pkg/kubelet/util/manager/watch_based_manager_test.go index 1f103404a9a..d81c4901106 100644 --- a/pkg/kubelet/util/manager/watch_based_manager_test.go +++ b/pkg/kubelet/util/manager/watch_based_manager_test.go @@ -31,14 +31,11 @@ import ( "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apimachinery/pkg/watch" - utilfeature "k8s.io/apiserver/pkg/util/feature" clientset "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/fake" core "k8s.io/client-go/testing" - featuregatetesting "k8s.io/component-base/featuregate/testing" corev1 "k8s.io/kubernetes/pkg/apis/core/v1" - "k8s.io/kubernetes/pkg/features" "github.com/stretchr/testify/assert" ) @@ -197,8 +194,6 @@ func TestSecretCacheMultipleRegistrations(t *testing.T) { } func TestImmutableSecretStopsTheReflector(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ImmutableEphemeralVolumes, true)() - secret := func(rv string, immutable bool) *v1.Secret { result := &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/registry/core/configmap/strategy.go b/pkg/registry/core/configmap/strategy.go index d592c181c0c..12f021ce40c 100644 --- a/pkg/registry/core/configmap/strategy.go +++ b/pkg/registry/core/configmap/strategy.go @@ -28,11 +28,9 @@ import ( "k8s.io/apiserver/pkg/registry/rest" pkgstorage "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" ) // strategy implements behavior for ConfigMap objects @@ -86,14 +84,7 @@ func (strategy) ValidateUpdate(ctx context.Context, newObj, oldObj runtime.Objec return validation.ValidateConfigMapUpdate(newCfg, oldCfg) } -func isImmutableInUse(configMap *api.ConfigMap) bool { - return configMap != nil && configMap.Immutable != nil -} - func dropDisabledFields(configMap *api.ConfigMap, oldConfigMap *api.ConfigMap) { - if !utilfeature.DefaultFeatureGate.Enabled(features.ImmutableEphemeralVolumes) && !isImmutableInUse(oldConfigMap) { - configMap.Immutable = nil - } } func (strategy) AllowUnconditionalUpdate() bool { diff --git a/pkg/registry/core/secret/strategy.go b/pkg/registry/core/secret/strategy.go index 0d5908d8975..ce2ea2b4d77 100644 --- a/pkg/registry/core/secret/strategy.go +++ b/pkg/registry/core/secret/strategy.go @@ -29,11 +29,9 @@ import ( "k8s.io/apiserver/pkg/registry/rest" pkgstorage "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" ) // strategy implements behavior for Secret objects @@ -80,14 +78,7 @@ func (strategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) fie return validation.ValidateSecretUpdate(obj.(*api.Secret), old.(*api.Secret)) } -func isImmutableInUse(secret *api.Secret) bool { - return secret != nil && secret.Immutable != nil -} - func dropDisabledFields(secret *api.Secret, oldSecret *api.Secret) { - if !utilfeature.DefaultFeatureGate.Enabled(features.ImmutableEphemeralVolumes) && !isImmutableInUse(oldSecret) { - secret.Immutable = nil - } } func (strategy) AllowUnconditionalUpdate() bool { diff --git a/staging/src/k8s.io/api/core/v1/types.go b/staging/src/k8s.io/api/core/v1/types.go index 86e91c987c0..e047885411b 100644 --- a/staging/src/k8s.io/api/core/v1/types.go +++ b/staging/src/k8s.io/api/core/v1/types.go @@ -5742,7 +5742,6 @@ type Secret struct { // be updated (only object metadata can be modified). // If not set to true, the field can be modified at any time. // Defaulted to nil. - // This is a beta field enabled by ImmutableEphemeralVolumes feature gate. // +optional Immutable *bool `json:"immutable,omitempty" protobuf:"varint,5,opt,name=immutable"` @@ -5883,7 +5882,6 @@ type ConfigMap struct { // be updated (only object metadata can be modified). // If not set to true, the field can be modified at any time. // Defaulted to nil. - // This is a beta field enabled by ImmutableEphemeralVolumes feature gate. // +optional Immutable *bool `json:"immutable,omitempty" protobuf:"varint,4,opt,name=immutable"` diff --git a/test/e2e/common/configmap_volume.go b/test/e2e/common/configmap_volume.go index 468960f8487..64700f37686 100644 --- a/test/e2e/common/configmap_volume.go +++ b/test/e2e/common/configmap_volume.go @@ -488,9 +488,16 @@ var _ = ginkgo.Describe("[sig-storage] ConfigMap", func() { }) - // It should be forbidden to change data for configmaps marked as immutable, but - // allowed to modify its metadata independently of its state. - ginkgo.It("should be immutable if `immutable` field is set", func() { + /* + Release: v1.21 + Testname: ConfigMap Volume, immutability + Description: Create a ConfigMap. Update it's data field, the update MUST succeed. + Mark the ConfigMap as immutable, the update MUST succeed. Try to update its data, the update MUST fail. + Try to mark the ConfigMap back as not immutable, the update MUST fail. + Try to update the ConfigMap`s metadata (labels), the update must succeed. + Try to delete the ConfigMap, the deletion must succeed. + */ + framework.ConformanceIt("should be immutable if `immutable` field is set", func() { name := "immutable" configMap := newConfigMap(f, name) diff --git a/test/e2e/common/secrets_volume.go b/test/e2e/common/secrets_volume.go index b6b9e32c16c..4bf10122f11 100644 --- a/test/e2e/common/secrets_volume.go +++ b/test/e2e/common/secrets_volume.go @@ -371,9 +371,16 @@ var _ = ginkgo.Describe("[sig-storage] Secrets", func() { gomega.Eventually(pollDeleteLogs, podLogTimeout, framework.Poll).Should(gomega.ContainSubstring("Error reading file /etc/secret-volumes/delete/data-1")) }) - // It should be forbidden to change data for secrets marked as immutable, but - // allowed to modify its metadata independently of its state. - ginkgo.It("should be immutable if `immutable` field is set", func() { + /* + Release: v1.21 + Testname: Secrets Volume, immutability + Description: Create a secret. Update it's data field, the update MUST succeed. + Mark the secret as immutable, the update MUST succeed. Try to update its data, the update MUST fail. + Try to mark the secret back as not immutable, the update MUST fail. + Try to update the secret`s metadata (labels), the update must succeed. + Try to delete the secret, the deletion must succeed. + */ + framework.ConformanceIt("should be immutable if `immutable` field is set", func() { name := "immutable" secret := secretForTest(f.Namespace.Name, name) From 8ce71fbae2d757fb61cf1f70d3f16e12aac9b1c6 Mon Sep 17 00:00:00 2001 From: wojtekt Date: Mon, 7 Dec 2020 13:50:59 +0100 Subject: [PATCH 2/3] Autogenerated --- api/openapi-spec/swagger.json | 4 ++-- pkg/kubelet/util/manager/BUILD | 5 ----- pkg/registry/core/configmap/BUILD | 2 -- pkg/registry/core/secret/BUILD | 2 -- staging/src/k8s.io/api/core/v1/generated.proto | 2 -- .../src/k8s.io/api/core/v1/types_swagger_doc_generated.go | 4 ++-- 6 files changed, 4 insertions(+), 15 deletions(-) diff --git a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json index bc851dc6d2a..8468a316ff4 100644 --- a/api/openapi-spec/swagger.json +++ b/api/openapi-spec/swagger.json @@ -5661,7 +5661,7 @@ "type": "object" }, "immutable": { - "description": "Immutable, if set to true, ensures that data stored in the ConfigMap cannot be updated (only object metadata can be modified). If not set to true, the field can be modified at any time. Defaulted to nil. This is a beta field enabled by ImmutableEphemeralVolumes feature gate.", + "description": "Immutable, if set to true, ensures that data stored in the ConfigMap cannot be updated (only object metadata can be modified). If not set to true, the field can be modified at any time. Defaulted to nil.", "type": "boolean" }, "kind": { @@ -9900,7 +9900,7 @@ "type": "object" }, "immutable": { - "description": "Immutable, if set to true, ensures that data stored in the Secret cannot be updated (only object metadata can be modified). If not set to true, the field can be modified at any time. Defaulted to nil. This is a beta field enabled by ImmutableEphemeralVolumes feature gate.", + "description": "Immutable, if set to true, ensures that data stored in the Secret cannot be updated (only object metadata can be modified). If not set to true, the field can be modified at any time. Defaulted to nil.", "type": "boolean" }, "kind": { diff --git a/pkg/kubelet/util/manager/BUILD b/pkg/kubelet/util/manager/BUILD index 2b569138a75..27aac298b20 100644 --- a/pkg/kubelet/util/manager/BUILD +++ b/pkg/kubelet/util/manager/BUILD @@ -10,7 +10,6 @@ go_library( importpath = "k8s.io/kubernetes/pkg/kubelet/util/manager", visibility = ["//visibility:public"], deps = [ - "//pkg/features:go_default_library", "//pkg/kubelet/util:go_default_library", "//staging/src/k8s.io/api/core/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library", @@ -23,7 +22,6 @@ go_library( "//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/watch:go_default_library", "//staging/src/k8s.io/apiserver/pkg/storage/etcd3:go_default_library", - "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", "//staging/src/k8s.io/client-go/tools/cache:go_default_library", "//vendor/k8s.io/klog/v2:go_default_library", ], @@ -39,7 +37,6 @@ go_test( deps = [ "//pkg/api/v1/pod:go_default_library", "//pkg/apis/core/v1:go_default_library", - "//pkg/features:go_default_library", "//staging/src/k8s.io/api/core/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/equality:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library", @@ -49,11 +46,9 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/watch:go_default_library", - "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", "//staging/src/k8s.io/client-go/kubernetes:go_default_library", "//staging/src/k8s.io/client-go/kubernetes/fake:go_default_library", "//staging/src/k8s.io/client-go/testing:go_default_library", - "//staging/src/k8s.io/component-base/featuregate/testing:go_default_library", "//vendor/github.com/stretchr/testify/assert:go_default_library", ], ) diff --git a/pkg/registry/core/configmap/BUILD b/pkg/registry/core/configmap/BUILD index 855b4083df2..9cafd0ebc91 100644 --- a/pkg/registry/core/configmap/BUILD +++ b/pkg/registry/core/configmap/BUILD @@ -17,7 +17,6 @@ go_library( "//pkg/api/legacyscheme:go_default_library", "//pkg/apis/core:go_default_library", "//pkg/apis/core/validation:go_default_library", - "//pkg/features:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/fields:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", @@ -26,7 +25,6 @@ go_library( "//staging/src/k8s.io/apiserver/pkg/registry/rest:go_default_library", "//staging/src/k8s.io/apiserver/pkg/storage:go_default_library", "//staging/src/k8s.io/apiserver/pkg/storage/names:go_default_library", - "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", ], ) diff --git a/pkg/registry/core/secret/BUILD b/pkg/registry/core/secret/BUILD index c600ddb1aa1..4c88ba54abd 100644 --- a/pkg/registry/core/secret/BUILD +++ b/pkg/registry/core/secret/BUILD @@ -17,7 +17,6 @@ go_library( "//pkg/api/legacyscheme:go_default_library", "//pkg/apis/core:go_default_library", "//pkg/apis/core/validation:go_default_library", - "//pkg/features:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/fields:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library", @@ -27,7 +26,6 @@ go_library( "//staging/src/k8s.io/apiserver/pkg/registry/rest:go_default_library", "//staging/src/k8s.io/apiserver/pkg/storage:go_default_library", "//staging/src/k8s.io/apiserver/pkg/storage/names:go_default_library", - "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", ], ) diff --git a/staging/src/k8s.io/api/core/v1/generated.proto b/staging/src/k8s.io/api/core/v1/generated.proto index 320d0c90650..b0a4431bda5 100644 --- a/staging/src/k8s.io/api/core/v1/generated.proto +++ b/staging/src/k8s.io/api/core/v1/generated.proto @@ -461,7 +461,6 @@ message ConfigMap { // be updated (only object metadata can be modified). // If not set to true, the field can be modified at any time. // Defaulted to nil. - // This is a beta field enabled by ImmutableEphemeralVolumes feature gate. // +optional optional bool immutable = 4; @@ -4419,7 +4418,6 @@ message Secret { // be updated (only object metadata can be modified). // If not set to true, the field can be modified at any time. // Defaulted to nil. - // This is a beta field enabled by ImmutableEphemeralVolumes feature gate. // +optional optional bool immutable = 5; diff --git a/staging/src/k8s.io/api/core/v1/types_swagger_doc_generated.go b/staging/src/k8s.io/api/core/v1/types_swagger_doc_generated.go index d8275910963..605605fdc6b 100644 --- a/staging/src/k8s.io/api/core/v1/types_swagger_doc_generated.go +++ b/staging/src/k8s.io/api/core/v1/types_swagger_doc_generated.go @@ -252,7 +252,7 @@ func (ComponentStatusList) SwaggerDoc() map[string]string { var map_ConfigMap = map[string]string{ "": "ConfigMap holds configuration data for pods to consume.", "metadata": "Standard object's metadata. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata", - "immutable": "Immutable, if set to true, ensures that data stored in the ConfigMap cannot be updated (only object metadata can be modified). If not set to true, the field can be modified at any time. Defaulted to nil. This is a beta field enabled by ImmutableEphemeralVolumes feature gate.", + "immutable": "Immutable, if set to true, ensures that data stored in the ConfigMap cannot be updated (only object metadata can be modified). If not set to true, the field can be modified at any time. Defaulted to nil.", "data": "Data contains the configuration data. Each key must consist of alphanumeric characters, '-', '_' or '.'. Values with non-UTF-8 byte sequences must use the BinaryData field. The keys stored in Data must not overlap with the keys in the BinaryData field, this is enforced during validation process.", "binaryData": "BinaryData contains the binary data. Each key must consist of alphanumeric characters, '-', '_' or '.'. BinaryData can contain byte sequences that are not in the UTF-8 range. The keys stored in BinaryData must not overlap with the ones in the Data field, this is enforced during validation process. Using this field will require 1.10+ apiserver and kubelet.", } @@ -2060,7 +2060,7 @@ func (SeccompProfile) SwaggerDoc() map[string]string { var map_Secret = map[string]string{ "": "Secret holds secret data of a certain type. The total bytes of the values in the Data field must be less than MaxSecretSize bytes.", "metadata": "Standard object's metadata. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata", - "immutable": "Immutable, if set to true, ensures that data stored in the Secret cannot be updated (only object metadata can be modified). If not set to true, the field can be modified at any time. Defaulted to nil. This is a beta field enabled by ImmutableEphemeralVolumes feature gate.", + "immutable": "Immutable, if set to true, ensures that data stored in the Secret cannot be updated (only object metadata can be modified). If not set to true, the field can be modified at any time. Defaulted to nil.", "data": "Data contains the secret data. Each key must consist of alphanumeric characters, '-', '_' or '.'. The serialized form of the secret data is a base64 encoded string, representing the arbitrary (possibly non-string) data value here. Described in https://tools.ietf.org/html/rfc4648#section-4", "stringData": "stringData allows specifying non-binary secret data in string form. It is provided as a write-only convenience method. All keys and values are merged into the data field on write, overwriting any existing values. It is never output when reading from the API.", "type": "Used to facilitate programmatic handling of secret data.", From 60828f36f89f9998c9928cba04c35d844019931b Mon Sep 17 00:00:00 2001 From: wojtekt Date: Wed, 30 Dec 2020 16:56:08 +0100 Subject: [PATCH 3/3] Update conformance tests list --- test/conformance/testdata/conformance.yaml | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test/conformance/testdata/conformance.yaml b/test/conformance/testdata/conformance.yaml index af99cf94677..a6a728b4669 100755 --- a/test/conformance/testdata/conformance.yaml +++ b/test/conformance/testdata/conformance.yaml @@ -2162,6 +2162,16 @@ The content MUST be accessible from all the mapped volume mounts. release: v1.9 file: test/e2e/common/configmap_volume.go +- testname: ConfigMap Volume, immutability + codename: '[sig-storage] ConfigMap should be immutable if `immutable` field is set + [Conformance]' + description: Create a ConfigMap. Update it's data field, the update MUST succeed. + Mark the ConfigMap as immutable, the update MUST succeed. Try to update its data, + the update MUST fail. Try to mark the ConfigMap back as not immutable, the update + MUST fail. Try to update the ConfigMap`s metadata (labels), the update must succeed. + Try to delete the ConfigMap, the deletion must succeed. + release: v1.21 + file: test/e2e/common/configmap_volume.go - testname: ConfigMap Volume, update codename: '[sig-storage] ConfigMap updates should be reflected in volume [NodeConformance] [Conformance]' @@ -2771,6 +2781,16 @@ the secret from the both the mounted volumes from the two specified custom paths. release: v1.9 file: test/e2e/common/secrets_volume.go +- testname: Secrets Volume, immutability + codename: '[sig-storage] Secrets should be immutable if `immutable` field is set + [Conformance]' + description: Create a secret. Update it's data field, the update MUST succeed. Mark + the secret as immutable, the update MUST succeed. Try to update its data, the + update MUST fail. Try to mark the secret back as not immutable, the update MUST + fail. Try to update the secret`s metadata (labels), the update must succeed. Try + to delete the secret, the deletion must succeed. + release: v1.21 + file: test/e2e/common/secrets_volume.go - testname: 'SubPath: Reading content from a configmap volume.' codename: '[sig-storage] Subpath Atomic writer volumes should support subpaths with configmap pod [LinuxOnly] [Conformance]'