separate RootCAConfigMap from BoundServiceAccountTokenVolume #96197

zshihang · 2020-11-03T22:43:52Z

What type of PR is this?

/kind feature

What this PR does / why we need it:
before BoundServiceAccountTokenVolume graduate to Beta, kube-controller-manager needs to publish kube-root-ca.crt to every namespace. In the multi-server cluster upgrade, a leading old kube-controller-manager might not publish the configmap for a short period of time. However, during the period, new pods may have the new projected tokens and as a result, those pods will fail to start.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:
-->

The beta `RootCAConfigMap` feature gate is enabled by default and causes kube-controller-manager to publish a "kube-root-ca.crt" ConfigMap to every namespace. This ConfigMap contains a CA bundle used for verifying connections to the kube-apiserver.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-auth/1205-bound-service-account-tokens

zshihang · 2020-11-03T22:44:10Z

/cc @liggitt @mikedanese

zshihang · 2020-11-03T22:44:22Z

/sig auth
/priority important-soon
/triage accepted

zshihang · 2020-11-03T23:10:29Z

/cc @neolit123

neolit123 · 2020-11-03T23:26:00Z

suggesting minor reword of the release note:

separate a new feature gate RootCAConfigMap from the BoundServiceAccountTokenVolume feature gate and graduate it to Beta. kube-controller-manager will now publish a "kube-root-ca.crt" ConfigMap to every namespace by default.

neolit123 · 2020-11-03T23:30:02Z

pkg/features/kube_features.go

+	//
+	// Allows kube-controller-manager to publish kube-root-ca.crt configmap to
+	// every namespace. This feature is separated from BoundServiceAccountTokenVolume
+	// to solve multi-server cluster upgrade issue.


potentially we can omit the second sentence here that explains the upgrade issue and instead include a mention that BoundServiceAccountTokenVolume requires RootCAConfigMap.

deferring to sig-auth, though.

agree the separation is not important, just that it is a prereq of BoundServiceAccountTokenVolume

neolit123 · 2020-11-03T23:38:13Z

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/controller-roles.yaml

+    - events
+    verbs:
+    - create
+    - patch


does rootcacertpublisher need "patch" for events?

yes, that's part of the standard event permissions used by the event recorder:

kubernetes/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go

Lines 53 to 55 in 71fea80

func eventsRule() rbacv1.PolicyRule {

return rbacv1helpers.NewRule("create", "update", "patch").Groups(legacyGroup, eventsGroup).Resources("events").RuleOrDie()

}

liggitt

add validation that makes sure RootCAConfigMap is enabled if BoundServiceAccountTokenVolume is enabled, and update the doc on RootCAConfigMap as noted

lgtm otherwise

liggitt · 2020-11-04T14:55:00Z

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/controller-roles.yaml

+    - events
+    verbs:
+    - create
+    - patch


yes, that's part of the standard event permissions used by the event recorder:

kubernetes/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go

Lines 53 to 55 in 71fea80

func eventsRule() rbacv1.PolicyRule {

return rbacv1helpers.NewRule("create", "update", "patch").Groups(legacyGroup, eventsGroup).Resources("events").RuleOrDie()

}

zshihang · 2020-11-04T21:25:17Z

/retest

liggitt · 2020-11-04T21:22:25Z

test/e2e/auth/service_accounts.go

+	ginkgo.It("should guarantee kube-root-ca.crt exist in any namespace", func() {
+		const rootCAConfigMapName = "kube-root-ca.crt"
+
+		_, err := f.ClientSet.CoreV1().ConfigMaps(f.Namespace.Name).Get(context.TODO(), rootCAConfigMapName, metav1.GetOptions{})


the configmap might not exist instantly in the new test namespace (it's created by a controller), so this should be a wait.PollImmediate with a wait.ForeverTestTimeout

framework.ExpectNoError(wait.PollImmediate(time.Second, wait.ForeverTestTimetout, func() (bool, error) { _, err := f.ClientSet.CoreV1().ConfigMaps(f.Namespace.Name).Get(context.TODO(), rootCAConfigMapName, metav1.GetOptions{}) if err == nil { return true, nil } if errors.IsNotFound(err) { framework.Logf("root ca configmap not found, retrying") return false, nil } return false, err }))

test/e2e/auth/service_accounts.go

zshihang · 2020-11-04T22:42:32Z

/retest

zshihang · 2020-11-05T00:37:03Z

/retest

liggitt · 2020-11-05T00:47:46Z

Both e2e failures look related. The config map quota test might need to be adjusted. I'm surprised we have a conformance test that fails if anything else creates a config map in the name space. That does not seem right

zshihang · 2020-11-05T01:39:01Z

the config map quota test is just flaky.

kubernetes/test/e2e/apimachinery/resource_quota.go

Line 313 in f75316e

defaultConfigMaps := fmt.Sprintf("%d", found)

if found does count the kube-root-ca.crt, then the test will pass. otherwise not.

liggitt · 2020-11-05T04:45:08Z

bazel failure is #96243
/retest

liggitt · 2020-11-05T04:56:12Z

/lgtm
/approve

please open a doc update against the dev-1.20 branch of the website, adding details about this feature gate and beta status

k8s-ci-robot · 2020-11-05T04:56:43Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~cmd/kube-controller-manager/OWNERS~~ [liggitt]
~~pkg/controller/certificates/OWNERS~~ [liggitt]
~~pkg/features/OWNERS~~ [liggitt]
~~pkg/kubeapiserver/OWNERS~~ [liggitt]
~~plugin/pkg/auth/authorizer/OWNERS~~ [liggitt]
~~test/cmd/OWNERS~~ [liggitt]
~~test/e2e/auth/OWNERS~~ [liggitt]

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

liggitt · 2020-11-05T13:16:37Z

the config map quota test is just flaky.

this definitely makes the flake worse.

previous flakes in the alpha job:

https://storage.googleapis.com/k8s-gubernator/triage/index.html?pr=1&job=alpha&test=ResourceQuota%20should%20create%20a%20ResourceQuota%20and%20capture%20the%20life%20of%20a%20configMap

new flakes in the main job:

https://storage.googleapis.com/k8s-gubernator/triage/index.html?pr=1&test=ResourceQuota%20should%20create%20a%20ResourceQuota%20and%20capture%20the%20life%20of%20a%20configMap&xjob=alpha

liggitt · 2020-11-05T13:27:13Z

opened #96265 to reduce the test flake, but the mechanism that test uses to detect existing configmaps is questionable

k8s-ci-robot requested review from liggitt and mikedanese November 3, 2020 22:44

k8s-ci-robot requested a review from deads2k November 3, 2020 22:45

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Nov 3, 2020

zshihang force-pushed the token branch from 680688b to ad05fd4 Compare November 3, 2020 23:08

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Nov 3, 2020

k8s-ci-robot requested a review from neolit123 November 3, 2020 23:10

neolit123 reviewed Nov 3, 2020

View reviewed changes

mikedanese assigned liggitt Nov 4, 2020

liggitt reviewed Nov 4, 2020

View reviewed changes

liggitt mentioned this pull request Nov 4, 2020

TokenRequest API and Kubelet integration kubernetes/enhancements#542

Closed

zshihang force-pushed the token branch from ad05fd4 to 0a0cf54 Compare November 4, 2020 19:56

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

zshihang force-pushed the token branch from 0a0cf54 to 3909ca1 Compare November 4, 2020 20:00

liggitt reviewed Nov 4, 2020

View reviewed changes

zshihang force-pushed the token branch 2 times, most recently from 7f379cd to cc5de7e Compare November 4, 2020 22:04

liggitt reviewed Nov 4, 2020

View reviewed changes

test/e2e/auth/service_accounts.go Show resolved Hide resolved

zshihang force-pushed the token branch from cc5de7e to 1afda50 Compare November 4, 2020 22:34

k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Nov 4, 2020

separate RootCAConfigMap from BoundServiceAccountTokenVolume

d40f0c4

zshihang force-pushed the token branch from 1afda50 to d40f0c4 Compare November 5, 2020 01:10

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

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

k8s-ci-robot merged commit 8bdd10b into kubernetes:master Nov 5, 2020

liggitt mentioned this pull request Nov 5, 2020

Revert "add e2e test for Service ExternalIPs" #96257

Merged

zshihang mentioned this pull request Nov 5, 2020

separate RootCAConfigMap from BoundServiceAccountToken and Beta kubernetes/website#24909

Merged

github-actions bot mentioned this pull request Nov 10, 2020

Week Ending November 8, 2020 dev-obs/actus#265

Open

zshihang added this to 1.20 - Done in SIG-Auth: Projected Service Account Tokens Aug 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

separate RootCAConfigMap from BoundServiceAccountTokenVolume #96197

separate RootCAConfigMap from BoundServiceAccountTokenVolume #96197

zshihang commented Nov 3, 2020 •

edited by liggitt

zshihang commented Nov 3, 2020

zshihang commented Nov 3, 2020

zshihang commented Nov 3, 2020

neolit123 commented Nov 3, 2020 •

edited

neolit123 Nov 3, 2020

liggitt Nov 4, 2020

zshihang Nov 4, 2020

neolit123 Nov 3, 2020

liggitt Nov 4, 2020

liggitt left a comment

liggitt Nov 4, 2020

zshihang commented Nov 4, 2020

liggitt Nov 4, 2020

zshihang Nov 4, 2020

zshihang commented Nov 4, 2020

zshihang commented Nov 5, 2020

liggitt commented Nov 5, 2020

zshihang commented Nov 5, 2020

liggitt commented Nov 5, 2020

liggitt commented Nov 5, 2020

k8s-ci-robot commented Nov 5, 2020

liggitt commented Nov 5, 2020

liggitt commented Nov 5, 2020

	func eventsRule() rbacv1.PolicyRule {
	return rbacv1helpers.NewRule("create", "update", "patch").Groups(legacyGroup, eventsGroup).Resources("events").RuleOrDie()
	}

separate RootCAConfigMap from BoundServiceAccountTokenVolume #96197

separate RootCAConfigMap from BoundServiceAccountTokenVolume #96197

Conversation

zshihang commented Nov 3, 2020 • edited by liggitt

zshihang commented Nov 3, 2020

zshihang commented Nov 3, 2020

zshihang commented Nov 3, 2020

neolit123 commented Nov 3, 2020 • edited

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

liggitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zshihang commented Nov 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zshihang commented Nov 4, 2020

zshihang commented Nov 5, 2020

liggitt commented Nov 5, 2020

zshihang commented Nov 5, 2020

liggitt commented Nov 5, 2020

liggitt commented Nov 5, 2020

k8s-ci-robot commented Nov 5, 2020

liggitt commented Nov 5, 2020

liggitt commented Nov 5, 2020

zshihang commented Nov 3, 2020 •

edited by liggitt

neolit123 commented Nov 3, 2020 •

edited