🌱 Support for kube-api-access volume mount #348

wondywang · 2023-08-03T10:41:46Z

What this PR does / why we need it:
This PR continue to improve the feature gated support for VirtualCluster to work with any Kubernetes version 1.21+ and when the RootCACertConfigMap feature is enabled in 1.20 clusters. To achieve this, before creating the pPod, the content in the kube-api-access volume is changed. Change it to directly mount the tenant cluster secret instead of applying the token of the super cluster.

Before:

  volumes:
  - name: kube-api-access-sjz6l
    projected:
      defaultMode: 420
      sources:
      - serviceAccountToken:
          expirationSeconds: 3607
          path: token
      - configMap:
          items:
          - key: ca.crt
            path: ca.crt
          name: kube-root-ca.crt
      - downwardAPI:
          items:
          - fieldRef:
              apiVersion: v1
              fieldPath: metadata.namespace
            path: namespace

After:

  volumes:
  - name: kube-api-access-f7ww9
    projected:
      defaultMode: 420
      sources:
      - secret:
          name: default-token-v2wlt

Here is the reference to the implementation in the Kubernetes： https://github.com/kubernetes/kubernetes/blob/master/plugin/pkg/admission/serviceaccount/admission.go

Follow-up needs to be considered: After Kubernetes version 1.24+, KCM will no longer distribute ServiceAccount tokens. I plan to add a token manager in the ServiceAccount sync controller to create ServiceAccount tokens and keep them in the validity period.

Which issue(s) this PR fixes:

#282

/kind feature

christopherhein · 2023-08-03T23:35:26Z

This is looking great so far, this doesn’t do the TokenManagement aspects but I see your note that this can come later.

Fei-Guo · 2023-08-04T00:27:32Z

@wondywang.

Please fix lint/test failures.
Please add UTs for the change.
Please add a feature gate to guard your change entirely.

wondywang · 2023-08-04T03:02:37Z

@wondywang.

Please fix lint/test failures.

Please add UTs for the change.

Please add a feature gate to guard your change entirely.

Ok, I will do it. If adding a new gate, I will prefer separate out a new file. I actually thought about doing that.

wondywang · 2023-08-09T03:40:03Z

@Fei-Guo @christopherhein

PTAL, thanks.

Fei-Guo

Overall LG. Some nits.

Fei-Guo · 2023-08-09T18:23:46Z

virtualcluster/pkg/syncer/resources/pod/mutatorplugin/podkubeapiaccessmutator.go

+	}
+	for _, secret := range secrets.Items {
+		if secret.Type != corev1.SecretTypeOpaque {
+			klog.Infof("type")


Do you need the log?

virtualcluster/pkg/syncer/resources/pod/mutatorplugin/podkubeapiaccessmutator.go

Fei-Guo · 2023-08-09T18:29:44Z

virtualcluster/pkg/syncer/resources/pod/mutatorplugin/podkubeapiaccessmutator.go

+	for i, container := range pod.Spec.Containers {
+		for j := 0; j < len(container.VolumeMounts); j++ {
+			if container.VolumeMounts[j].Name == old {
+				klog.V(6).Infof("mutate containers %s volumeMount.name from %s to %s", container.Name, old, new)


V(6) -> V(4).

virtualcluster/pkg/syncer/resources/pod/mutatorplugin/podrootcacertmutator.go

Fei-Guo · 2023-08-10T18:03:39Z

LGTM. @christopherhein, take another look?

ravisantoshgudimetla

Mostly LGTM. Thanks for working on this @wondywang

ravisantoshgudimetla · 2023-08-11T19:56:01Z

virtualcluster/pkg/syncer/resources/pod/mutatorplugin/podkubeapiaccessmutator.go

+	}
+
+	// Could not find in cache, attempt to look up directly
+	numAttempts := 1


nit: You can use something like this here: https://github.com/kubernetes/apimachinery/blob/v0.27.4/pkg/util/wait/poll.go#L45

thx @ravisantoshgudimetla. I suddenly found that the retry logic here is unnecessary. So it was removed directly.

ravisantoshgudimetla · 2023-08-11T20:00:48Z

virtualcluster/pkg/syncer/resources/pod/mutatorplugin/podkubeapiaccessmutator.go

+			return fmt.Errorf("error looking up secret of serviceAccount %s/%s: %v", targetNamespace, p.PPod.Spec.ServiceAccountName, err)
+		}
+
+		if shouldAutomount(serviceAccount, p.PPod) {


Can we use automountServiceAccountToken on the pod spec?

We explicitly override this from vc-syncer so the value in the pPod should always be false, seems like if we have the vPod object we could get that value.

ravisantoshgudimetla · 2023-08-11T20:09:35Z

virtualcluster/pkg/syncer/resources/pod/mutatorplugin/podkubeapiaccessmutator_test.go

+	tests := []struct {
+		name                  string
+		pPod                  *corev1.Pod
+		existingObjectInSuper []runtime.Object


It'd be easier if you add an expected Pod after mutation as a field in this struct.

hi, @ravisantoshgudimetla. I thought about it. This is not easy to implement, because the names kube-api-access-xxxx here are randomly generated by names.SimpleNameGenerator.GenerateName.

I see. No issues then. I was actually referring to the contents not the name.

ravisantoshgudimetla · 2023-08-11T20:12:48Z

virtualcluster/pkg/syncer/resources/pod/mutatorplugin/podkubeapiaccessmutator_test.go

+func TestPodKubeAPIAccessMutatorPlugin_Mutator(t *testing.T) {
+	defer util.SetFeatureGateDuringTest(t, featuregate.DefaultFeatureGate, featuregate.KubeAPIAccessSupport, true)()
+
+	tests := []struct {


Can you add 2 more test cases with auto mount enable and disabled and if possible can we check the content of this secret(token matches what we expect it to be?)

https://github.com/kubernetes-sigs/cluster-api-provider-nested/pull/348/files#r1291799079

christopherhein · 2023-08-11T22:35:12Z

Overall looks great, @Fei-Guo & @ravisantoshgudimetla added the same feedback I would. Thank you @wondywang for taking the lead on this.

wondywang · 2023-08-14T06:53:01Z

Overall looks great, @Fei-Guo & @ravisantoshgudimetla added the same feedback I would. Thank you @wondywang for taking the lead on this.

@christopherhein @ravisantoshgudimetla That's great, thanks! I will make some change later.

ravisantoshgudimetla · 2023-08-21T19:53:03Z

@wondywang - go-imports are failing. Can you fix and push the commit. LGTM from code standpoint.

wondywang · 2023-08-28T09:37:27Z

@wondywang - go-imports are failing. Can you fix and push the commit. LGTM from code standpoint.

Sorry, I've been a little busy lately. I will get it done as soon as possible. @ravisantoshgudimetla

wondywang · 2023-08-29T14:27:49Z

@Fei-Guo @christopherhein PTAL.

And I also update serviceAccount token manager in this commit. This part of the logic is an idea of mine (maybe not complete). Can be used as a reference. I plan not to mention PR for now. Wait for @ravisantoshgudimetla to finish it. What do you think?

wondywang@ad8545a

christopherhein

/lgtm

@Fei-Guo I’ll give you the chance to approve this.

cc @ravisantoshgudimetla as well, thanks!

Fei-Guo · 2023-08-30T21:28:24Z

/lgtm
/approve

k8s-ci-robot · 2023-08-30T21:28:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Fei-Guo, wondywang

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:

~~virtualcluster/OWNERS~~ [Fei-Guo]

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

feat: support for kube-api-access volume mount

7705057

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 3, 2023

k8s-ci-robot requested review from christopherhein and Fei-Guo August 3, 2023 10:41

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 3, 2023

Fei-Guo reviewed Aug 9, 2023

View reviewed changes

add featuregate KubeApiAccessSupport

f1813b1

wondywang force-pushed the kube-root-ca branch from 70b7d65 to f1813b1 Compare August 10, 2023 14:48

ravisantoshgudimetla reviewed Aug 11, 2023

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2023

wondywang added 2 commits August 19, 2023 14:52

some code optimize

d40233d

Empty-Commit

7e6d8f2

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2023

fix goimports error

265b8a2

christopherhein reviewed Aug 30, 2023

View reviewed changes

k8s-ci-robot assigned christopherhein Aug 30, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2023

k8s-ci-robot assigned Fei-Guo Aug 30, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2023

k8s-ci-robot merged commit 1e31f2d into kubernetes-sigs:main Aug 30, 2023
6 of 7 checks passed

yoonmac mentioned this pull request Sep 12, 2023

🐛 fix pod kube-api-access sa not found #352

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Support for kube-api-access volume mount #348

🌱 Support for kube-api-access volume mount #348

wondywang commented Aug 3, 2023

christopherhein commented Aug 3, 2023

Fei-Guo commented Aug 4, 2023

wondywang commented Aug 4, 2023

wondywang commented Aug 9, 2023

Fei-Guo left a comment

Fei-Guo Aug 9, 2023

Fei-Guo Aug 9, 2023

Fei-Guo commented Aug 10, 2023

ravisantoshgudimetla left a comment

ravisantoshgudimetla Aug 11, 2023 •

edited

wondywang Aug 19, 2023

ravisantoshgudimetla Aug 11, 2023

christopherhein Aug 11, 2023

ravisantoshgudimetla Aug 11, 2023

wondywang Aug 19, 2023

ravisantoshgudimetla Aug 21, 2023

ravisantoshgudimetla Aug 11, 2023

christopherhein Aug 11, 2023

christopherhein commented Aug 11, 2023

wondywang commented Aug 14, 2023

ravisantoshgudimetla commented Aug 21, 2023

wondywang commented Aug 28, 2023

wondywang commented Aug 29, 2023 •

edited

christopherhein left a comment

Fei-Guo commented Aug 30, 2023

k8s-ci-robot commented Aug 30, 2023

🌱 Support for kube-api-access volume mount #348

🌱 Support for kube-api-access volume mount #348

Conversation

wondywang commented Aug 3, 2023

christopherhein commented Aug 3, 2023

Fei-Guo commented Aug 4, 2023

wondywang commented Aug 4, 2023

wondywang commented Aug 9, 2023

Fei-Guo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fei-Guo commented Aug 10, 2023

ravisantoshgudimetla left a comment

Choose a reason for hiding this comment

ravisantoshgudimetla Aug 11, 2023 • 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

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

christopherhein commented Aug 11, 2023

wondywang commented Aug 14, 2023

ravisantoshgudimetla commented Aug 21, 2023

wondywang commented Aug 28, 2023

wondywang commented Aug 29, 2023 • edited

christopherhein left a comment

Choose a reason for hiding this comment

Fei-Guo commented Aug 30, 2023

k8s-ci-robot commented Aug 30, 2023

ravisantoshgudimetla Aug 11, 2023 •

edited

wondywang commented Aug 29, 2023 •

edited