Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -40,6 +41,7 @@ var AllCustomResourceValidators = []string{
oauth.PluginName,
project.PluginName,
config.PluginName,
operator.PluginName,
scheduler.PluginName,
clusterresourcequota.PluginName,
securitycontextconstraints.PluginName,
Expand Down Expand Up @@ -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)

Expand Down
Original file line number Diff line number Diff line change
@@ -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
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this is equivalent to the following, right?

if attributes.GetResource().Resource == "storages" && attributes.GetName() == "cluster" {
    return admission.NewForbidden(attributes, fmt.Errorf("deleting required %s.%s resource, named %s, is not allowed", attributes.GetResource().Resource, attributes.GetResource().Group, attributes.GetName()))
}

return nil

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it's equivalent, but I like the more verbose form because it's easier to add new objects that should not be deleted in the future, and it also matches the form in this admission plugin for config objects.

return admission.NewForbidden(attributes, fmt.Errorf("deleting required %s.%s resource, named %s, is not allowed", attributes.GetResource().Resource, attributes.GetResource().Group, attributes.GetName()))
}
Original file line number Diff line number Diff line change
@@ -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)
}
})
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion pkg/features/kube_features.go
Original file line number Diff line number Diff line change
Expand Up @@ -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},

Expand Down
30 changes: 30 additions & 0 deletions pkg/features/patch_kube_features.go
Original file line number Diff line number Diff line change
@@ -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))
}
2 changes: 1 addition & 1 deletion pkg/volume/csi/csi_plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
42 changes: 42 additions & 0 deletions pkg/volume/csimigration/patch_adc_plugin_manager.go
Original file line number Diff line number Diff line change
@@ -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)
}
8 changes: 4 additions & 4 deletions pkg/volume/csimigration/plugin_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ type PluginNameMapper interface {
type PluginManager struct {
PluginNameMapper
featureGate featuregate.FeatureGate

useADCPluginManagerFeatureGates bool
}

// NewPluginManager returns a new PluginManager instance
Expand Down Expand Up @@ -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.

Expand All @@ -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:
Expand Down
58 changes: 58 additions & 0 deletions pkg/volume/csimigration/plugin_manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package csimigration

import (
"fmt"
"os"
"testing"

v1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion pkg/volume/vsphere_volume/vsphere_volume.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down