panic in featuregate if a requested feature is unknown #84865

deads2k · 2019-11-06T15:52:04Z

featuregates.Enabled should not silently return false if a request feature is not registered. This PR updates the code to panic so that coding error is noticed at the point of usage instead of silently going down unexpected branches.

You get items like

	panic: feature "IPv6DualStack" is not registered in FeatureGate "k8s.io/apiserver/pkg/util/feature/feature_gate.go:28"

/kind bug
/priority important-soon
@kubernetes/sig-api-machinery-bugs

NONE

liggitt · 2019-11-06T15:53:17Z

Checking for enablement of a feature that is not registered is a programming error. I'm generally in favor of this. Will be interesting to see what in CI complains about this.

deads2k · 2019-11-06T15:55:48Z

Will be interesting to see what in CI complains about this.

Surely it will all work fine....

liggitt · 2019-11-06T15:58:03Z

/approve

needs a release note, since this is consumable by external components

deads2k · 2019-11-06T19:38:17Z

/retest

deads2k · 2019-11-06T20:41:11Z

needs a release note, since this is consumable by external components

we agree on slack that the message is very clear.

lavalamp · 2019-11-06T20:51:50Z

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

+			}
+		}()
+
+		ipv6DualStackEnabled = utilfeature.DefaultFeatureGate.Enabled(IPv6DualStack)


is it still possible to change the value? I don't understand how this can work, flags won't have been parsed while init functions are running?

right, this can't be initialized in init. could make it a member of the Cloud struct and set it in NewCloud

OK, yeah, I asked for that exact thing in my other comment.

where's @liggittlamp when you need him

lavalamp · 2019-11-06T20:57:28Z

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

@@ -104,6 +104,26 @@ const (
 	clusterNameKey = "kubernetes-cluster-name"
 )

+var (
+	// ipv6DualStack allows overriding for unit testing.  It's normally initialized from featuregates
+	ipv6DualStackEnabled bool


Please make it a member variable of something and set it on construction.

lavalamp · 2019-11-06T20:58:54Z

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

@@ -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


run-on sentence.

lavalamp · 2019-11-06T21:00:00Z

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

@@ -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 low entropy names for reflectors


s/low entropy/unhelpful/

... this way I don't have to debate with myself any more if "low" or "high" entropy is more technically correct

deads2k · 2019-11-06T21:13:06Z

comments addressed

liggitt · 2019-11-06T22:01:01Z

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

+	func() {
+		// this allows the code ot launch without featuregates defined.  It is currently required for unit tests where the
+		// featuregates are not registered.  This is effectively coding by side effect and an explicit register should
+		// be eventually created instead.


open a follow up issue for cloud-controller-manager/providers to resolve this?

lavalamp · 2019-11-06T22:10:16Z

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

+		// this allows the code ot launch without featuregates defined.  It is currently required for unit tests where the
+		// featuregates are not registered.  This is effectively coding by side effect and an explicit register should
+		// be eventually created instead.
+		defer func() {


If you split the NewCloud function so that test code doesn't have to execute this, do you still need to catch the panic?

If you split the NewCloud function so that test code doesn't have to execute this, do you still need to catch the panic?

And let it straight die instead? That's not consistent with existing behavior. I'm honestly unsure if this ever worked properly when it really ran. Are you sure you want to change behavior?

And let it straight die instead?

Yup, that's the goal of the change, isn't it? :)

k8s-ci-robot · 2019-11-07T15:59:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/volume/azure_file/OWNERS~~ [liggitt]
~~staging/src/k8s.io/component-base/OWNERS~~ [liggitt]
~~staging/src/k8s.io/legacy-cloud-providers/azure/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

deads2k · 2019-11-07T15:59:31Z

@khenidak This update makes the azure cloud provider require the definition of IPv6DualStack feature gate or it will fail with a message saying the featuregate is not defined. It can be defined as false (this is the default in the cloud-controller-manager).

deads2k · 2019-11-07T18:13:53Z

/retest

deads2k · 2019-11-07T18:48:16Z

/retest

lavalamp · 2019-11-07T18:55:32Z

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

+	return az, nil
+}
+
+// NewCloudWithoutFeatureGates returns a Cloud without trying to wire the feature gates.  This is used by the unit tests


Thanks. Looks like it can't be made private, or I would definitely ask for that. Oh well.

lavalamp · 2019-11-07T18:58:46Z

/lgtm

@khenidak If you run this cloud provider in alternative environments, you'll definitely want to take a look at this.

fejta-bot · 2019-11-07T22:18:01Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot requested review from stewart-yu and sttts November 6, 2019 15:54

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2019

liggitt mentioned this pull request Nov 6, 2019

remove global variable dependency from admission plugins #84813

Merged

deads2k force-pushed the feature-gate-fail branch from 38372ba to e8d91b3 Compare November 6, 2019 18:41

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. area/cloudprovider sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 6, 2019

deads2k force-pushed the feature-gate-fail branch from e8d91b3 to 5b9cb83 Compare November 6, 2019 20:40

k8s-ci-robot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 6, 2019

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 6, 2019

lavalamp reviewed Nov 6, 2019

View reviewed changes

deads2k force-pushed the feature-gate-fail branch from 5b9cb83 to 46c17bd Compare November 6, 2019 21:11

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 6, 2019

deads2k added 2 commits November 6, 2019 16:13

panic in featuregate if a requested feature is unknown

c6d2c37

remove featuregate hard requirement from azure legacy cloudprovider

20c956c

deads2k force-pushed the feature-gate-fail branch from 46c17bd to 20c956c Compare November 6, 2019 21:21

liggitt reviewed Nov 6, 2019

View reviewed changes

lavalamp reviewed Nov 6, 2019

View reviewed changes

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Nov 7, 2019

make azure fail if feature gates are not registered

7d897d5

deads2k force-pushed the feature-gate-fail branch from 80cee5c to 7d897d5 Compare November 7, 2019 17:42

lavalamp reviewed Nov 7, 2019

View reviewed changes

k8s-ci-robot assigned lavalamp Nov 7, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2019

k8s-ci-robot merged commit bae8f56 into kubernetes:master Nov 8, 2019

k8s-ci-robot added this to the v1.17 milestone Nov 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

panic in featuregate if a requested feature is unknown #84865

panic in featuregate if a requested feature is unknown #84865

deads2k commented Nov 6, 2019 •

edited

liggitt commented Nov 6, 2019

deads2k commented Nov 6, 2019

liggitt commented Nov 6, 2019 •

edited

deads2k commented Nov 6, 2019

deads2k commented Nov 6, 2019

lavalamp Nov 6, 2019

liggitt Nov 6, 2019

lavalamp Nov 6, 2019

liggitt Nov 6, 2019

lavalamp Nov 6, 2019

lavalamp Nov 6, 2019

lavalamp Nov 6, 2019

deads2k commented Nov 6, 2019

liggitt Nov 6, 2019

lavalamp Nov 6, 2019

deads2k Nov 7, 2019

lavalamp Nov 7, 2019

k8s-ci-robot commented Nov 7, 2019

deads2k commented Nov 7, 2019

deads2k commented Nov 7, 2019

deads2k commented Nov 7, 2019

lavalamp Nov 7, 2019

lavalamp commented Nov 7, 2019

fejta-bot commented Nov 7, 2019

panic in featuregate if a requested feature is unknown #84865

panic in featuregate if a requested feature is unknown #84865

Conversation

deads2k commented Nov 6, 2019 • edited

liggitt commented Nov 6, 2019

deads2k commented Nov 6, 2019

liggitt commented Nov 6, 2019 • edited

deads2k commented Nov 6, 2019

deads2k commented Nov 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Nov 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 7, 2019

deads2k commented Nov 7, 2019

deads2k commented Nov 7, 2019

deads2k commented Nov 7, 2019

Choose a reason for hiding this comment

lavalamp commented Nov 7, 2019

fejta-bot commented Nov 7, 2019

deads2k commented Nov 6, 2019 •

edited

liggitt commented Nov 6, 2019 •

edited