diff --git a/openshift-kube-apiserver/admission/customresourcevalidation/customresourcevalidationregistration/cr_validation_registration.go b/openshift-kube-apiserver/admission/customresourcevalidation/customresourcevalidationregistration/cr_validation_registration.go index cc1de149b3eac..1af9c4e71db3e 100644 --- a/openshift-kube-apiserver/admission/customresourcevalidation/customresourcevalidationregistration/cr_validation_registration.go +++ b/openshift-kube-apiserver/admission/customresourcevalidation/customresourcevalidationregistration/cr_validation_registration.go @@ -19,6 +19,7 @@ import ( "k8s.io/kubernetes/openshift-kube-apiserver/admission/customresourcevalidation/network" "k8s.io/kubernetes/openshift-kube-apiserver/admission/customresourcevalidation/node" "k8s.io/kubernetes/openshift-kube-apiserver/admission/customresourcevalidation/oauth" + "k8s.io/kubernetes/openshift-kube-apiserver/admission/customresourcevalidation/operator" "k8s.io/kubernetes/openshift-kube-apiserver/admission/customresourcevalidation/project" "k8s.io/kubernetes/openshift-kube-apiserver/admission/customresourcevalidation/rolebindingrestriction" "k8s.io/kubernetes/openshift-kube-apiserver/admission/customresourcevalidation/route" @@ -40,6 +41,7 @@ var AllCustomResourceValidators = []string{ oauth.PluginName, project.PluginName, config.PluginName, + operator.PluginName, scheduler.PluginName, clusterresourcequota.PluginName, securitycontextconstraints.PluginName, @@ -70,6 +72,7 @@ func RegisterCustomResourceValidation(plugins *admission.Plugins) { oauth.Register(plugins) project.Register(plugins) config.Register(plugins) + operator.Register(plugins) scheduler.Register(plugins) kubecontrollermanager.Register(plugins) diff --git a/openshift-kube-apiserver/admission/customresourcevalidation/operator/deny_delete_cluster_operator_resource.go b/openshift-kube-apiserver/admission/customresourcevalidation/operator/deny_delete_cluster_operator_resource.go new file mode 100644 index 0000000000000..f4cb78543ccef --- /dev/null +++ b/openshift-kube-apiserver/admission/customresourcevalidation/operator/deny_delete_cluster_operator_resource.go @@ -0,0 +1,52 @@ +package operator + +import ( + "context" + "fmt" + "io" + + "k8s.io/apiserver/pkg/admission" +) + +const PluginName = "operator.openshift.io/DenyDeleteClusterOperators" + +// Register registers an admission plugin factory whose plugin prevents the deletion of cluster operator resources. +func Register(plugins *admission.Plugins) { + plugins.Register(PluginName, func(config io.Reader) (admission.Interface, error) { + return newAdmissionPlugin(), nil + }) +} + +var _ admission.ValidationInterface = &admissionPlugin{} + +type admissionPlugin struct { + *admission.Handler +} + +func newAdmissionPlugin() *admissionPlugin { + return &admissionPlugin{Handler: admission.NewHandler(admission.Delete)} +} + +// Validate returns an error if there is an attempt to delete a cluster operator resource. +func (p *admissionPlugin) Validate(ctx context.Context, attributes admission.Attributes, _ admission.ObjectInterfaces) error { + if len(attributes.GetSubresource()) > 0 { + return nil + } + if attributes.GetResource().Group != "operator.openshift.io" { + return nil + } + switch attributes.GetResource().Resource { + // Deletion is denied for storages.operator.openshift.io objects named cluster, + // because MCO and KCM-O depend on this resource being present in order to + // correctly set environment variables on kubelet and kube-controller-manager. + case "storages": + if attributes.GetName() != "cluster" { + return nil + } + // Deletion is allowed for all other operator.openshift.io objects unless + // explicitly listed above. + default: + return nil + } + return admission.NewForbidden(attributes, fmt.Errorf("deleting required %s.%s resource, named %s, is not allowed", attributes.GetResource().Resource, attributes.GetResource().Group, attributes.GetName())) +} diff --git a/openshift-kube-apiserver/admission/customresourcevalidation/operator/deny_delete_cluster_operator_resource_test.go b/openshift-kube-apiserver/admission/customresourcevalidation/operator/deny_delete_cluster_operator_resource_test.go new file mode 100644 index 0000000000000..6b0eaa5cc911d --- /dev/null +++ b/openshift-kube-apiserver/admission/customresourcevalidation/operator/deny_delete_cluster_operator_resource_test.go @@ -0,0 +1,73 @@ +package operator + +import ( + "context" + "testing" + + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apiserver/pkg/admission" +) + +func TestAdmissionPlugin_Validate(t *testing.T) { + testCases := []struct { + tcName string + group string + resource string + name string + denyDelete bool + }{ + { + tcName: "NotBlackListedResourceNamedCluster", + group: "operator.openshift.io", + resource: "notBlacklisted", + name: "cluster", + denyDelete: false, + }, + { + tcName: "NotBlackListedResourceNamedNotCluster", + group: "operator.openshift.io", + resource: "notBlacklisted", + name: "notCluster", + denyDelete: false, + }, + { + tcName: "StorageResourceNamedCluster", + group: "operator.openshift.io", + resource: "storages", + name: "cluster", + denyDelete: true, + }, + { + tcName: "StorageResourceNamedNotCluster", + group: "operator.openshift.io", + resource: "storages", + name: "notCluster", + denyDelete: false, + }, + { + tcName: "ClusterVersionNotVersion", + group: "config.openshift.io", + resource: "clusterversions", + name: "instance", + denyDelete: false, + }, + { + tcName: "OtherGroup", + group: "not.operator.openshift.io", + resource: "notBlacklisted", + name: "cluster", + denyDelete: false, + }, + } + for _, tc := range testCases { + t.Run(tc.tcName, func(t *testing.T) { + err := newAdmissionPlugin().Validate(context.TODO(), admission.NewAttributesRecord( + nil, nil, schema.GroupVersionKind{}, "", + tc.name, schema.GroupVersionResource{Group: tc.group, Resource: tc.resource}, + "", admission.Delete, nil, false, nil), nil) + if tc.denyDelete != (err != nil) { + t.Error(tc.denyDelete, err) + } + }) + } +} diff --git a/pkg/controller/volume/attachdetach/attach_detach_controller.go b/pkg/controller/volume/attachdetach/attach_detach_controller.go index 1d628afbfc53d..3aa6c7bcf7a9c 100644 --- a/pkg/controller/volume/attachdetach/attach_detach_controller.go +++ b/pkg/controller/volume/attachdetach/attach_detach_controller.go @@ -180,7 +180,7 @@ func NewAttachDetachController( csiTranslator := csitrans.New() adc.intreeToCSITranslator = csiTranslator - adc.csiMigratedPluginManager = csimigration.NewPluginManager(csiTranslator, utilfeature.DefaultFeatureGate) + adc.csiMigratedPluginManager = csimigration.NewADCPluginManager(csiTranslator, utilfeature.DefaultFeatureGate) adc.desiredStateOfWorldPopulator = populator.NewDesiredStateOfWorldPopulator( timerConfig.DesiredStateOfWorldPopulatorLoopSleepPeriod, diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 8847b83094f61..2ad904d6f9cbe 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -955,7 +955,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS CSIMigrationRBD: {Default: false, PreRelease: featuregate.Alpha}, // Off by default (requires RBD CSI driver) - CSIMigrationvSphere: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.28 + CSIMigrationvSphere: {Default: true, PreRelease: featuregate.GA}, // LockToDefault when CSI driver with GA support for Windows, raw block and xfs features are available CSINodeExpandSecret: {Default: false, PreRelease: featuregate.Alpha}, diff --git a/pkg/features/patch_kube_features.go b/pkg/features/patch_kube_features.go new file mode 100644 index 0000000000000..79b4e3aa52c87 --- /dev/null +++ b/pkg/features/patch_kube_features.go @@ -0,0 +1,30 @@ +package features + +import ( + "os" + + "k8s.io/apimachinery/pkg/util/runtime" + utilfeature "k8s.io/apiserver/pkg/util/feature" + "k8s.io/component-base/featuregate" +) + +var ( + // owner: @fbertina + // beta: v1.23 + // + // Enables the vSphere CSI migration for the Attach/Detach controller (ADC) only. + ADCCSIMigrationVSphere featuregate.Feature = "ADC_CSIMigrationVSphere" +) + +var ocpDefaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ + ADCCSIMigrationVSphere: {Default: true, PreRelease: featuregate.GA}, +} + +func OpenShiftStartCSIMigrationVSphere() bool { + _, migrationEnabled := os.LookupEnv("OPENSHIFT_DO_VSPHERE_MIGRATION") + return migrationEnabled +} + +func init() { + runtime.Must(utilfeature.DefaultMutableFeatureGate.Add(ocpDefaultKubernetesFeatureGates)) +} diff --git a/pkg/volume/csi/csi_plugin.go b/pkg/volume/csi/csi_plugin.go index e00a4d6d097ae..aa34a79084ec4 100644 --- a/pkg/volume/csi/csi_plugin.go +++ b/pkg/volume/csi/csi_plugin.go @@ -232,7 +232,7 @@ func (p *csiPlugin) Init(host volume.VolumeHost) error { return utilfeature.DefaultFeatureGate.Enabled(features.CSIMigrationAzureFile) }, csitranslationplugins.VSphereInTreePluginName: func() bool { - return utilfeature.DefaultFeatureGate.Enabled(features.CSIMigrationvSphere) + return utilfeature.DefaultFeatureGate.Enabled(features.CSIMigrationvSphere) && features.OpenShiftStartCSIMigrationVSphere() }, csitranslationplugins.PortworxVolumePluginName: func() bool { return utilfeature.DefaultFeatureGate.Enabled(features.CSIMigrationPortworx) diff --git a/pkg/volume/csimigration/patch_adc_plugin_manager.go b/pkg/volume/csimigration/patch_adc_plugin_manager.go new file mode 100644 index 0000000000000..cdc0a1a2ae37d --- /dev/null +++ b/pkg/volume/csimigration/patch_adc_plugin_manager.go @@ -0,0 +1,42 @@ +package csimigration + +import ( + "k8s.io/component-base/featuregate" + csilibplugins "k8s.io/csi-translation-lib/plugins" + "k8s.io/kubernetes/pkg/features" +) + +// NewADCPluginManager returns a new PluginManager instance for the Attach Detach controller which uses different +// featuregates in openshift to control enablement/disablement which *DO NOT MATCH* the featuregates for the rest of the +// cluster. +func NewADCPluginManager(m PluginNameMapper, featureGate featuregate.FeatureGate) PluginManager { + ret := NewPluginManager(m, featureGate) + ret.useADCPluginManagerFeatureGates = true + return ret +} + +// adcIsMigrationEnabledForPlugin indicates whether CSI migration has been enabled +// for a particular storage plugin in Attach/Detach controller. +func (pm PluginManager) adcIsMigrationEnabledForPlugin(pluginName string) bool { + // CSIMigration feature should be enabled along with the plugin-specific one + if !pm.featureGate.Enabled(features.CSIMigration) { + return false + } + + switch pluginName { + case csilibplugins.VSphereInTreePluginName: + return pm.featureGate.Enabled(features.ADCCSIMigrationVSphere) + default: + return pm.isMigrationEnabledForPlugin(pluginName) + } +} + +// IsMigrationEnabledForPlugin indicates whether CSI migration has been enabled +// for a particular storage plugin +func (pm PluginManager) IsMigrationEnabledForPlugin(pluginName string) bool { + if pm.useADCPluginManagerFeatureGates { + return pm.adcIsMigrationEnabledForPlugin(pluginName) + } + + return pm.isMigrationEnabledForPlugin(pluginName) +} diff --git a/pkg/volume/csimigration/plugin_manager.go b/pkg/volume/csimigration/plugin_manager.go index b568e85ac1530..518114f6e728e 100644 --- a/pkg/volume/csimigration/plugin_manager.go +++ b/pkg/volume/csimigration/plugin_manager.go @@ -38,6 +38,8 @@ type PluginNameMapper interface { type PluginManager struct { PluginNameMapper featureGate featuregate.FeatureGate + + useADCPluginManagerFeatureGates bool } // NewPluginManager returns a new PluginManager instance @@ -81,9 +83,7 @@ func (pm PluginManager) IsMigrationCompleteForPlugin(pluginName string) bool { } } -// IsMigrationEnabledForPlugin indicates whether CSI migration has been enabled -// for a particular storage plugin -func (pm PluginManager) IsMigrationEnabledForPlugin(pluginName string) bool { +func (pm PluginManager) isMigrationEnabledForPlugin(pluginName string) bool { // CSIMigration feature should be enabled along with the plugin-specific one // CSIMigration has been GA. It will be enabled by default. @@ -99,7 +99,7 @@ func (pm PluginManager) IsMigrationEnabledForPlugin(pluginName string) bool { case csilibplugins.CinderInTreePluginName: return true case csilibplugins.VSphereInTreePluginName: - return pm.featureGate.Enabled(features.CSIMigrationvSphere) + return pm.featureGate.Enabled(features.CSIMigrationvSphere) && features.OpenShiftStartCSIMigrationVSphere() case csilibplugins.PortworxVolumePluginName: return pm.featureGate.Enabled(features.CSIMigrationPortworx) case csilibplugins.RBDVolumePluginName: diff --git a/pkg/volume/csimigration/plugin_manager_test.go b/pkg/volume/csimigration/plugin_manager_test.go index c1517075f4752..2c573990453eb 100644 --- a/pkg/volume/csimigration/plugin_manager_test.go +++ b/pkg/volume/csimigration/plugin_manager_test.go @@ -18,6 +18,7 @@ package csimigration import ( "fmt" + "os" "testing" v1 "k8s.io/api/core/v1" @@ -112,6 +113,62 @@ func TestIsMigratable(t *testing.T) { } } +func TestCheckMigrationFeatureFlags(t *testing.T) { + testCases := []struct { + name string + pluginFeature featuregate.Feature + pluginFeatureEnabled bool + pluginUnregsiterFeature featuregate.Feature + pluginUnregsiterEnabled bool + expectMigrationComplete bool + expectErr bool + }{ + { + name: "plugin specific migration feature enabled with plugin unregister disabled", + pluginFeature: features.CSIMigrationvSphere, + pluginFeatureEnabled: true, + pluginUnregsiterFeature: features.InTreePluginvSphereUnregister, + pluginUnregsiterEnabled: false, + expectMigrationComplete: false, + expectErr: false, + }, + { + name: "plugin specific migration feature and plugin unregister disabled", + pluginFeature: features.CSIMigrationvSphere, + pluginFeatureEnabled: false, + pluginUnregsiterFeature: features.InTreePluginvSphereUnregister, + pluginUnregsiterEnabled: false, + expectMigrationComplete: false, + expectErr: false, + }, + { + name: "all features enabled", + pluginFeature: features.CSIMigrationvSphere, + pluginFeatureEnabled: true, + pluginUnregsiterFeature: features.InTreePluginvSphereUnregister, + pluginUnregsiterEnabled: true, + expectMigrationComplete: true, + expectErr: false, + }, + } + for _, test := range testCases { + t.Run(fmt.Sprintf("Testing %v", test.name), func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, test.pluginFeature, test.pluginFeatureEnabled)() + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, test.pluginUnregsiterFeature, test.pluginUnregsiterEnabled)() + migrationComplete, err := CheckMigrationFeatureFlags(utilfeature.DefaultFeatureGate, test.pluginFeature, test.pluginUnregsiterFeature) + if err != nil && test.expectErr == false { + t.Errorf("Unexpected error: %v", err) + } + if err == nil && test.expectErr == true { + t.Errorf("Unexpected validation pass") + } + if migrationComplete != test.expectMigrationComplete { + t.Errorf("Unexpected migrationComplete result. Exp: %v, got %v", test.expectMigrationComplete, migrationComplete) + } + }) + } +} + func TestMigrationFeatureFlagStatus(t *testing.T) { testCases := []struct { name string @@ -191,6 +248,7 @@ func TestMigrationFeatureFlagStatus(t *testing.T) { csiMigrationCompleteResult: true, }, } + os.Setenv("OPENSHIFT_DO_VSPHERE_MIGRATION", "true") csiTranslator := csitrans.New() for _, test := range testCases { pm := NewPluginManager(csiTranslator, utilfeature.DefaultFeatureGate) diff --git a/pkg/volume/vsphere_volume/vsphere_volume.go b/pkg/volume/vsphere_volume/vsphere_volume.go index 6f7d02974c56d..329969b44d27a 100644 --- a/pkg/volume/vsphere_volume/vsphere_volume.go +++ b/pkg/volume/vsphere_volume/vsphere_volume.go @@ -75,7 +75,7 @@ func (plugin *vsphereVolumePlugin) GetPluginName() string { } func (plugin *vsphereVolumePlugin) IsMigratedToCSI() bool { - return utilfeature.DefaultFeatureGate.Enabled(features.CSIMigrationvSphere) + return utilfeature.DefaultFeatureGate.Enabled(features.CSIMigrationvSphere) && features.OpenShiftStartCSIMigrationVSphere() } func (plugin *vsphereVolumePlugin) GetVolumeName(spec *volume.Spec) (string, error) {