Merge pull request #84865 from deads2k/feature-gate-fail

panic in featuregate if a requested feature is unknown

pkg/volume/azure_file/azure_file_test.go

@@ -87,14 +87,10 @@ func getAzureTestCloud(t *testing.T) *azure.Cloud {
                 "aadClientSecret": "--aad-client-secret--"
         }`
 	configReader := strings.NewReader(config)
-	cloud, err := azure.NewCloud(configReader)
+	azureCloud, err := azure.NewCloudWithoutFeatureGates(configReader)
 	if err != nil {
 		t.Error(err)
 	}
-	azureCloud, ok := cloud.(*azure.Cloud)
-	if !ok {
-		t.Error("NewCloud returned incorrect type")
-	}
 	return azureCloud
 }
 

staging/src/k8s.io/component-base/featuregate/BUILD

@@ -7,6 +7,7 @@ go_library(
     importpath = "k8s.io/component-base/featuregate",
     visibility = ["//visibility:public"],
     deps = [
+        "//staging/src/k8s.io/apimachinery/pkg/util/naming:go_default_library",
         "//vendor/github.com/spf13/pflag:go_default_library",
         "//vendor/k8s.io/klog:go_default_library",
     ],

staging/src/k8s.io/component-base/featuregate/feature_gate.go

@@ -25,6 +25,8 @@ import (
 	"sync/atomic"
 
 	"github.com/spf13/pflag"
+
+	"k8s.io/apimachinery/pkg/util/naming"
 	"k8s.io/klog"
 )
 
@@ -103,6 +105,8 @@ type MutableFeatureGate interface {
 
 // featureGate implements FeatureGate as well as pflag.Value for flag parsing.
 type featureGate struct {
+	featureGateName string
+
 	special map[Feature]func(map[Feature]FeatureSpec, map[Feature]bool, bool)
 
 	// lock guards writes to known, enabled, and reads/writes of closed
@@ -128,6 +132,10 @@ func setUnsetAlphaGates(known map[Feature]FeatureSpec, enabled map[Feature]bool,
 // Set, String, and Type implement pflag.Value
 var _ pflag.Value = &featureGate{}
 
+// internalPackages are packages that ignored when creating a name for featureGates. These packages are in the common
+// call chains, so they'd be unhelpful as names.
+var internalPackages = []string{"k8s.io/component-base/featuregate/feature_gate.go"}
+
 func NewFeatureGate() *featureGate {
 	known := map[Feature]FeatureSpec{}
 	for k, v := range defaultFeatures {
@@ -142,9 +150,10 @@ func NewFeatureGate() *featureGate {
 	enabledValue.Store(enabled)
 
 	f := &featureGate{
-		known:   knownValue,
+		featureGateName: naming.GetNameFromCallsite(internalPackages...),
-		special: specialFeatures,
+		known:           knownValue,
-		enabled: enabledValue,
+		special:         specialFeatures,
+		enabled:         enabledValue,
 	}
 	return f
 }
@@ -263,12 +272,16 @@ func (f *featureGate) Add(features map[Feature]FeatureSpec) error {
 	return nil
 }
 
-// Enabled returns true if the key is enabled.
+// Enabled returns true if the key is enabled.  If the key is not known, this call will panic.
 func (f *featureGate) Enabled(key Feature) bool {
 	if v, ok := f.enabled.Load().(map[Feature]bool)[key]; ok {
 		return v
 	}
-	return f.known.Load().(map[Feature]FeatureSpec)[key].Default
+	if v, ok := f.known.Load().(map[Feature]FeatureSpec)[key]; ok {
+		return v.Default
+	}
+
+	panic(fmt.Errorf("feature %q is not registered in FeatureGate %q", key, f.featureGateName))
 }
 
 // AddFlag adds a flag for setting global feature gates to the specified FlagSet.

staging/src/k8s.io/legacy-cloud-providers/azure/azure.go

@@ -34,6 +34,7 @@ import (
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/wait"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/client-go/informers"
 	clientset "k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/scheme"
@@ -202,6 +203,8 @@ type Cloud struct {
 	metadata                *InstanceMetadataService
 	vmSet                   VMSet
 
+	// ipv6DualStack allows overriding for unit testing.  It's normally initialized from featuregates
+	ipv6DualStackEnabled bool
 	// Lock for access to node caches, includes nodeZones, nodeResourceGroups, and unmanagedNodes.
 	nodeCachesLock sync.Mutex
 	// nodeZones is a mapping from Zone to a sets.String of Node's names in the Zone
@@ -260,6 +263,18 @@ func init() {
 
 // NewCloud returns a Cloud with initialized clients
 func NewCloud(configReader io.Reader) (cloudprovider.Interface, error) {
+	az, err := NewCloudWithoutFeatureGates(configReader)
+	if err != nil {
+		return nil, err
+	}
+	az.ipv6DualStackEnabled = utilfeature.DefaultFeatureGate.Enabled(IPv6DualStack)
+
+	return az, nil
+}
+
+// NewCloudWithoutFeatureGates returns a Cloud without trying to wire the feature gates.  This is used by the unit tests
+// that don't load the actual features being used in the cluster.
+func NewCloudWithoutFeatureGates(configReader io.Reader) (*Cloud, error) {
 	config, err := parseConfig(configReader)
 	if err != nil {
 		return nil, err
@@ -271,6 +286,7 @@ func NewCloud(configReader io.Reader) (cloudprovider.Interface, error) {
 		unmanagedNodes:     sets.NewString(),
 		routeCIDRs:         map[string]string{},
 	}
+
 	err = az.InitializeCloudFromConfig(config, false)
 	if err != nil {
 		return nil, err

staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go

@@ -35,8 +35,6 @@ import (
 	cloudprovider "k8s.io/cloud-provider"
 	servicehelpers "k8s.io/cloud-provider/service/helpers"
 	"k8s.io/klog"
-
-	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	utilnet "k8s.io/utils/net"
 )
 
@@ -563,7 +561,7 @@ func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domai
 		}
 	}
 
-	if utilfeature.DefaultFeatureGate.Enabled(IPv6DualStack) {
+	if az.ipv6DualStackEnabled {
 		// TODO: (khenidak) if we ever enable IPv6 single stack, then we should
 		// not wrap the following in a feature gate
 		ipv6 := utilnet.IsIPv6String(service.Spec.ClusterIP)
@@ -697,7 +695,7 @@ func (az *Cloud) reconcileLoadBalancer(clusterName string, service *v1.Service,
 	klog.V(2).Infof("reconcileLoadBalancer for service(%s): lb(%s) wantLb(%t) resolved load balancer name", serviceName, lbName, wantLb)
 	lbFrontendIPConfigName := az.getFrontendIPConfigName(service, subnet(service))
 	lbFrontendIPConfigID := az.getFrontendIPConfigID(lbName, lbFrontendIPConfigName)
-	lbBackendPoolName := getBackendPoolName(clusterName, service)
+	lbBackendPoolName := getBackendPoolName(az.ipv6DualStackEnabled, clusterName, service)
 	lbBackendPoolID := az.getBackendPoolID(lbName, lbBackendPoolName)
 
 	lbIdleTimeout, err := getIdleTimeout(service)

staging/src/k8s.io/legacy-cloud-providers/azure/azure_routes.go

@@ -30,10 +30,6 @@ import (
 	cloudprovider "k8s.io/cloud-provider"
 	"k8s.io/klog"
 	utilnet "k8s.io/utils/net"
-
-	// Azure route controller changes behavior if ipv6dual stack feature is turned on
-	// remove this once the feature graduates
-	utilfeature "k8s.io/apiserver/pkg/util/feature"
 )
 
 // copied to minimize the number of cross reference
@@ -47,7 +43,7 @@ const (
 func (az *Cloud) ListRoutes(ctx context.Context, clusterName string) ([]*cloudprovider.Route, error) {
 	klog.V(10).Infof("ListRoutes: START clusterName=%q", clusterName)
 	routeTable, existsRouteTable, err := az.getRouteTable(cacheReadTypeDefault)
-	routes, err := processRoutes(routeTable, existsRouteTable, err)
+	routes, err := processRoutes(az.ipv6DualStackEnabled, routeTable, existsRouteTable, err)
 	if err != nil {
 		return nil, err
 	}
@@ -63,7 +59,7 @@ func (az *Cloud) ListRoutes(ctx context.Context, clusterName string) ([]*cloudpr
 		if cidr, ok := az.routeCIDRs[nodeName]; ok {
 			routes = append(routes, &cloudprovider.Route{
 				Name:            nodeName,
-				TargetNode:      mapRouteNameToNodeName(nodeName),
+				TargetNode:      mapRouteNameToNodeName(az.ipv6DualStackEnabled, nodeName),
 				DestinationCIDR: cidr,
 			})
 		}
@@ -73,7 +69,7 @@ func (az *Cloud) ListRoutes(ctx context.Context, clusterName string) ([]*cloudpr
 }
 
 // Injectable for testing
-func processRoutes(routeTable network.RouteTable, exists bool, err error) ([]*cloudprovider.Route, error) {
+func processRoutes(ipv6DualStackEnabled bool, routeTable network.RouteTable, exists bool, err error) ([]*cloudprovider.Route, error) {
 	if err != nil {
 		return nil, err
 	}
@@ -85,7 +81,7 @@ func processRoutes(routeTable network.RouteTable, exists bool, err error) ([]*cl
 	if routeTable.RouteTablePropertiesFormat != nil && routeTable.Routes != nil {
 		kubeRoutes = make([]*cloudprovider.Route, len(*routeTable.Routes))
 		for i, route := range *routeTable.Routes {
-			instance := mapRouteNameToNodeName(*route.Name)
+			instance := mapRouteNameToNodeName(ipv6DualStackEnabled, *route.Name)
 			cidr := *route.AddressPrefix
 			klog.V(10).Infof("ListRoutes: * instance=%q, cidr=%q", instance, cidr)
 
@@ -141,7 +137,7 @@ func (az *Cloud) CreateRoute(ctx context.Context, clusterName string, nameHint s
 		return err
 	}
 	if unmanaged {
-		if utilfeature.DefaultFeatureGate.Enabled(IPv6DualStack) {
+		if az.ipv6DualStackEnabled {
 			//TODO (khenidak) add support for unmanaged nodes when the feature reaches  beta
 			return fmt.Errorf("unmanaged nodes are not supported in dual stack mode")
 		}
@@ -156,7 +152,7 @@ func (az *Cloud) CreateRoute(ctx context.Context, clusterName string, nameHint s
 	if err := az.createRouteTableIfNotExists(clusterName, kubeRoute); err != nil {
 		return err
 	}
-	if !utilfeature.DefaultFeatureGate.Enabled(IPv6DualStack) {
+	if !az.ipv6DualStackEnabled {
 		targetIP, _, err = az.getIPForMachine(kubeRoute.TargetNode)
 		if err != nil {
 			return err
@@ -178,7 +174,7 @@ func (az *Cloud) CreateRoute(ctx context.Context, clusterName string, nameHint s
 			return err
 		}
 	}
-	routeName := mapNodeNameToRouteName(kubeRoute.TargetNode, string(kubeRoute.DestinationCIDR))
+	routeName := mapNodeNameToRouteName(az.ipv6DualStackEnabled, kubeRoute.TargetNode, string(kubeRoute.DestinationCIDR))
 	route := network.Route{
 		Name: to.StringPtr(routeName),
 		RoutePropertiesFormat: &network.RoutePropertiesFormat{
@@ -217,7 +213,7 @@ func (az *Cloud) DeleteRoute(ctx context.Context, clusterName string, kubeRoute
 
 	klog.V(2).Infof("DeleteRoute: deleting route. clusterName=%q instance=%q cidr=%q", clusterName, kubeRoute.TargetNode, kubeRoute.DestinationCIDR)
 
-	routeName := mapNodeNameToRouteName(kubeRoute.TargetNode, string(kubeRoute.DestinationCIDR))
+	routeName := mapNodeNameToRouteName(az.ipv6DualStackEnabled, kubeRoute.TargetNode, string(kubeRoute.DestinationCIDR))
 	err = az.DeleteRouteWithName(routeName)
 	if err != nil {
 		return err
@@ -231,16 +227,16 @@ func (az *Cloud) DeleteRoute(ctx context.Context, clusterName string, kubeRoute
 // These two functions enable stashing the instance name in the route
 // and then retrieving it later when listing. This is needed because
 // Azure does not let you put tags/descriptions on the Route itself.
-func mapNodeNameToRouteName(nodeName types.NodeName, cidr string) string {
+func mapNodeNameToRouteName(ipv6DualStackEnabled bool, nodeName types.NodeName, cidr string) string {
-	if !utilfeature.DefaultFeatureGate.Enabled(IPv6DualStack) {
+	if !ipv6DualStackEnabled {
 		return fmt.Sprintf("%s", nodeName)
 	}
 	return fmt.Sprintf(routeNameFmt, nodeName, cidrtoRfc1035(cidr))
 }
 
 // Used with mapNodeNameToRouteName. See comment on mapNodeNameToRouteName.
-func mapRouteNameToNodeName(routeName string) types.NodeName {
+func mapRouteNameToNodeName(ipv6DualStackEnabled bool, routeName string) types.NodeName {
-	if !utilfeature.DefaultFeatureGate.Enabled(IPv6DualStack) {
+	if !ipv6DualStackEnabled {
 		return types.NodeName(fmt.Sprintf("%s", routeName))
 	}
 	parts := strings.Split(routeName, routeNameSeparator)

staging/src/k8s.io/legacy-cloud-providers/azure/azure_routes_test.go

@@ -47,7 +47,7 @@ func TestDeleteRoute(t *testing.T) {
 		nodeInformerSynced: func() bool { return true },
 	}
 	route := cloudprovider.Route{TargetNode: "node", DestinationCIDR: "1.2.3.4/24"}
-	routeName := mapNodeNameToRouteName(route.TargetNode, route.DestinationCIDR)
+	routeName := mapNodeNameToRouteName(false, route.TargetNode, route.DestinationCIDR)
 
 	fakeRoutes.FakeStore = map[string]map[string]network.Route{
 		cloud.RouteTableName: {
@@ -80,7 +80,7 @@ func TestDeleteRoute(t *testing.T) {
 		nodeName: nodeCIDR,
 	}
 	route1 := cloudprovider.Route{
-		TargetNode:      mapRouteNameToNodeName(nodeName),
+		TargetNode:      mapRouteNameToNodeName(false, nodeName),
 		DestinationCIDR: nodeCIDR,
 	}
 	err = cloud.DeleteRoute(context.TODO(), "cluster", &route1)
@@ -138,7 +138,7 @@ func TestCreateRoute(t *testing.T) {
 		t.Errorf("unexpected calls create if not exists, exists: %v", fakeTable.Calls)
 	}
 
-	routeName := mapNodeNameToRouteName(route.TargetNode, string(route.DestinationCIDR))
+	routeName := mapNodeNameToRouteName(false, route.TargetNode, string(route.DestinationCIDR))
 	routeInfo, found := fakeRoutes.FakeStore[cloud.RouteTableName][routeName]
 	if !found {
 		t.Errorf("could not find route: %v in %v", routeName, fakeRoutes.FakeStore)
@@ -160,7 +160,7 @@ func TestCreateRoute(t *testing.T) {
 	cloud.unmanagedNodes.Insert(nodeName)
 	cloud.routeCIDRs = map[string]string{}
 	route1 := cloudprovider.Route{
-		TargetNode:      mapRouteNameToNodeName(nodeName),
+		TargetNode:      mapRouteNameToNodeName(false, nodeName),
 		DestinationCIDR: nodeCIDR,
 	}
 	err = cloud.CreateRoute(context.TODO(), "cluster", "unused", &route1)
@@ -326,7 +326,7 @@ func TestProcessRoutes(t *testing.T) {
 			expectedRoute: []cloudprovider.Route{
 				{
 					Name:            "name",
-					TargetNode:      mapRouteNameToNodeName("name"),
+					TargetNode:      mapRouteNameToNodeName(false, "name"),
 					DestinationCIDR: "1.2.3.4/16",
 				},
 			},
@@ -355,12 +355,12 @@ func TestProcessRoutes(t *testing.T) {
 			expectedRoute: []cloudprovider.Route{
 				{
 					Name:            "name",
-					TargetNode:      mapRouteNameToNodeName("name"),
+					TargetNode:      mapRouteNameToNodeName(false, "name"),
 					DestinationCIDR: "1.2.3.4/16",
 				},
 				{
 					Name:            "name2",
-					TargetNode:      mapRouteNameToNodeName("name2"),
+					TargetNode:      mapRouteNameToNodeName(false, "name2"),
 					DestinationCIDR: "5.6.7.8/16",
 				},
 			},
@@ -368,7 +368,7 @@ func TestProcessRoutes(t *testing.T) {
 		},
 	}
 	for _, test := range tests {
-		routes, err := processRoutes(test.rt, test.exists, test.err)
+		routes, err := processRoutes(false, test.rt, test.exists, test.err)
 		if test.expectErr {
 			if err == nil {
 				t.Errorf("%s: unexpected non-error", test.name)
@@ -423,11 +423,11 @@ func TestRouteNameFuncs(t *testing.T) {
 	v6CIDR := "fd3e:5f02:6ec0:30ba::/64"
 	nodeName := "thisNode"
 
-	routeName := mapNodeNameToRouteName(types.NodeName(nodeName), v4CIDR)
+	routeName := mapNodeNameToRouteName(false, types.NodeName(nodeName), v4CIDR)
-	outNodeName := mapRouteNameToNodeName(routeName)
+	outNodeName := mapRouteNameToNodeName(false, routeName)
 	assert.Equal(t, string(outNodeName), nodeName)
 
-	routeName = mapNodeNameToRouteName(types.NodeName(nodeName), v6CIDR)
+	routeName = mapNodeNameToRouteName(false, types.NodeName(nodeName), v6CIDR)
-	outNodeName = mapRouteNameToNodeName(routeName)
+	outNodeName = mapRouteNameToNodeName(false, routeName)
 	assert.Equal(t, string(outNodeName), nodeName)
 }

staging/src/k8s.io/legacy-cloud-providers/azure/azure_standard.go

@@ -40,7 +40,6 @@ import (
 	cloudprovider "k8s.io/cloud-provider"
 	"k8s.io/klog"
 
-	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/component-base/featuregate"
 	utilnet "k8s.io/utils/net"
 )
@@ -263,8 +262,8 @@ func isInternalLoadBalancer(lb *network.LoadBalancer) bool {
 // clusters moving from IPv4 to duakstack will require no changes
 // clusters moving from IPv6 (while not seen in the wild, we can not rule out their existence)
 // to dualstack will require deleting backend pools (the reconciler will take care of creating correct backendpools)
-func getBackendPoolName(clusterName string, service *v1.Service) string {
+func getBackendPoolName(ipv6DualStackEnabled bool, clusterName string, service *v1.Service) string {
-	if !utilfeature.DefaultFeatureGate.Enabled(IPv6DualStack) {
+	if !ipv6DualStackEnabled {
 		return clusterName
 	}
 	IPv6 := utilnet.IsIPv6String(service.Spec.ClusterIP)
@@ -721,7 +720,7 @@ func (as *availabilitySet) EnsureHostInPool(service *v1.Service, nodeName types.
 	}
 
 	var primaryIPConfig *network.InterfaceIPConfiguration
-	if !utilfeature.DefaultFeatureGate.Enabled(IPv6DualStack) {
+	if !as.Cloud.ipv6DualStackEnabled {
 		primaryIPConfig, err = getPrimaryIPConfig(nic)
 		if err != nil {
 			return err

staging/src/k8s.io/legacy-cloud-providers/azure/azure_test.go

@@ -1701,14 +1701,10 @@ func validateConfig(t *testing.T, config string) {
 
 func getCloudFromConfig(t *testing.T, config string) *Cloud {
 	configReader := strings.NewReader(config)
-	cloud, err := NewCloud(configReader)
+	azureCloud, err := NewCloudWithoutFeatureGates(configReader)
 	if err != nil {
 		t.Error(err)
 	}
-	azureCloud, ok := cloud.(*Cloud)
-	if !ok {
-		t.Error("NewCloud returned incorrect type")
-	}
 	return azureCloud
 }
 

staging/src/k8s.io/legacy-cloud-providers/azure/azure_vmss.go

@@ -37,7 +37,6 @@ import (
 	cloudprovider "k8s.io/cloud-provider"
 	"k8s.io/klog"
 
-	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	utilnet "k8s.io/utils/net"
 )
 
@@ -775,7 +774,7 @@ func (ss *scaleSet) EnsureHostInPool(service *v1.Service, nodeName types.NodeNam
 
 	var primaryIPConfiguration *compute.VirtualMachineScaleSetIPConfiguration
 	// Find primary network interface configuration.
-	if !utilfeature.DefaultFeatureGate.Enabled(IPv6DualStack) {
+	if !ss.Cloud.ipv6DualStackEnabled {
 		// Find primary IP configuration.
 		primaryIPConfiguration, err = getPrimaryIPConfigFromVMSSNetworkConfig(primaryNetworkInterfaceConfiguration)
 		if err != nil {