implement ServiceAccountTokenProjection by mikedanese · Pull Request #62005 · kubernetes/kubernetes

mikedanese · 2018-04-02T04:25:48Z

design here: kubernetes/community#1973

Add a volume projection that is able to project service account tokens.

part of #48408

@kubernetes/sig-auth-pr-reviews @kubernetes/sig-storage-pr-reviews

k8s-ci-robot · 2018-04-02T04:25:49Z

@mikedanese: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

Details

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://git.k8s.io/community/contributors/devel/pull-requests.md#write-release-notes-if-needed.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

CaoShuFeng · 2018-04-02T12:08:42Z

/assign @CaoShuFeng

mikedanese · 2018-04-02T15:45:10Z

Currently working on testing.

example config:

kind: Deployment
apiVersion: apps/v1
metadata:
  labels:
    run: nginx
  name: nginx
spec:
  replicas: 30
  selector:
    matchLabels:
      run: nginx
  template:
    metadata:
      labels:
        run: nginx
    spec:
      containers:
      - image: nginx
        name: nginx
        volumeMounts:
        - mountPath: /var/run/secrets/tokens
          name: token
      volumes:
      - name: token
        projected:
          sources:
          - serviceAccountToken:
              path: token
              expirationSeconds: 60
              audiences:
                - hi

jbeda · 2018-04-02T15:55:18Z

/hold

I'd like to see an agreed upon and checked in design doc here. As this also impacts our API, I'd also like to see a heads up to the API review folks to make sure they are aware of the context. A checked in design doc will also help with release notes and documentation.

mikedanese · 2018-04-02T16:01:15Z

@jbeda this will go through the normal API review process. This is self assigned and and already labeled WIP. I'll be presenting the design (here) at the next sig-storage. Only opened for early feedback

awly · 2018-05-31T19:20:59Z

/lgtm

mikedanese · 2018-06-01T17:13:29Z

/retest

tallclair · 2018-06-01T19:14:27Z

/milestone v1.11
/kind feature
/priority important-soon

k8s-ci-robot · 2018-06-01T19:14:28Z

@tallclair: The provided milestone is not valid for this repository. Milestones in this repository: [next-candidate, v1.10, v1.11, v1.12, v1.13, v1.4, v1.5, v1.6, v1.7, v1.8, v1.9]

Use /milestone clear to clear the milestone.

Details

In response to this:

/milestone 1.11
/kind feature
/priority important-soon

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

saad-ali

Volume changes LGTM

/lgtm
/approve

mikedanese · 2018-06-04T18:07:52Z

/retest

tallclair

I think the service account admission controller needs to be updated too: https://github.com/kubernetes/kubernetes/blob/master/plugin/pkg/admission/serviceaccount/admission.go#L198

How do we prevent static pods from mounting serviceAccountTokenProjections?

tallclair · 2018-06-04T18:30:00Z

pkg/kubelet/kubelet.go

nit: move this down to where it's used

tallclair · 2018-06-04T18:31:01Z

pkg/kubelet/token/BUILD

Do you want to add an OWNERS file too?

tallclair · 2018-06-04T18:34:05Z

pkg/kubelet/token/token_manager.go

nit: prefer cacheLock

tallclair · 2018-06-04T18:37:30Z

pkg/kubelet/token/token_manager.go

Yeah, better safe than sorry. I don't think static pods go through defaulting.

tallclair · 2018-06-04T18:41:04Z

pkg/kubelet/token/token_manager.go

nit: what is iat? Use a more descriptive variable name.

iat is a common abbreviation in oauth/jose world https://tools.ietf.org/html/rfc7519#section-4.1.6

tallclair · 2018-06-04T18:45:58Z

pkg/kubelet/token/token_manager.go

nit: add a comment: // Require a refresh if within 20% of the TTL from the expiration time.

nit: move division to the outer expression for finer accuracy

This hurts readability because we have to cast back and forth between (duration,int64,float64) if the accuracy gain is +- half a second, I don't think it's worth it.

I mean -1 * time.Duration(*tr.Spec.ExpirationSeconds) * time.Second * 20 / 100, which resolves to a time.Duration. But yeah, the accuracy isn't worth the time it took me to write this comment ;-)

tallclair · 2018-06-04T18:50:27Z

pkg/kubelet/token/token_manager.go

Is name the service account name, or pod name? In either case, should this include a UID?

The name, namespace are of the service account. The actual TokenRequest has pod name and pod UID included in the object binding, which is validated on the apiserver. We can't include service account UID because that information is not present on the pod object.

tallclair · 2018-06-04T18:52:42Z

pkg/kubelet/token/token_manager.go

Does this need to include expiration time? If it's encoded in the token, then I think the answer is yes. If it just controls the cache, then maybe not?

It doesn't need an expiration time. The cache is essentially a map from TokenRequestSpec to TokenRequestStatus so an expiration timestamp is an attribute of the status.

Er, sorry, I meant ExpirationSeconds. I.e. I'm wondering if 2 volumes with the same spec but different expirations could share a cache entry. I expect this to be a pretty unlikely scenario, so it's probably not worth changing.

The ExpirationSeconds is in tr.Spec so the requested expiration is included in the cache key. Right now the requested expiration seconds will always be the expiration seconds, because we aren't implementing any sort of softmax. Two volume projections with the same spec should always project the same token.

tallclair · 2018-06-04T18:55:00Z

pkg/kubelet/token/token_manager_test.go

nit: use the assert/require library? require.NoError(t, err)

same below.

tallclair · 2018-06-04T18:57:05Z

pkg/kubelet/token/token_manager_test.go

optional: use k8s.io/apimachinery/pkg/util/clock.FakeClock

mikedanese · 2018-06-04T20:28:19Z

How do we prevent static pods from mounting serviceAccountTokenProjections?

Added enforcement to the admission controller.

mikedanese · 2018-06-04T20:47:59Z

/retest

dims · 2018-06-04T22:18:11Z

/test pull-kubernetes-bazel-test

dims · 2018-06-04T22:18:30Z

/assign @tallclair
/assign @derekwaynecarr

k8s-github-robot · 2018-06-04T22:19:19Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@CaoShuFeng @awly @derekwaynecarr @liggitt @mikedanese @saad-ali @tallclair

Pull Request Labels

sig/auth sig/storage: Pull Request will be escalated to these SIGs if needed.
priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
kind/feature: New functionality.

Help

tallclair · 2018-06-04T22:32:24Z

pkg/kubelet/token/token_manager.go

I think this is growing unbounded? It should grow slowly, but you need some way of clearing out old tokens.

tallclair · 2018-06-04T22:39:40Z

pkg/kubelet/token/token_manager.go

I mean -1 * time.Duration(*tr.Spec.ExpirationSeconds) * time.Second * 20 / 100, which resolves to a time.Duration. But yeah, the accuracy isn't worth the time it took me to write this comment ;-)

tallclair · 2018-06-04T23:42:29Z

/lgtm

k8s-ci-robot · 2018-06-04T23:42:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awly, mikedanese, saad-ali, tallclair

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/controller/volume/attachdetach/OWNERS~~ [mikedanese,saad-ali]
~~pkg/controller/volume/expand/OWNERS~~ [mikedanese,saad-ali]
~~pkg/controller/volume/persistentvolume/OWNERS~~ [mikedanese,saad-ali]
~~pkg/kubelet/OWNERS~~ [tallclair]
~~pkg/volume/OWNERS~~ [saad-ali]
~~plugin/pkg/admission/serviceaccount/OWNERS~~ [mikedanese]

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

k8s-ci-robot · 2018-06-04T23:43:51Z

New changes are detected. LGTM label has been removed.

mikedanese · 2018-06-04T23:44:01Z

squashed

k8s-ci-robot · 2018-06-05T00:22:46Z

New changes are detected. LGTM label has been removed.

mikedanese · 2018-06-05T00:28:28Z

fixed govet in test.

mikedanese · 2018-06-05T02:20:17Z

/retest

dims · 2018-06-05T13:35:39Z

@mikedanese looks like this could use a good release note. that should be the last thing this needs to merge

k8s-github-robot · 2018-06-05T15:37:06Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-06-05T16:30:55Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

zparnold · 2018-06-13T15:04:47Z

Hello there! @mikedanese I'm Zach Arnold working on Docs for the 1.11 release. This PR was identified as one needing some documentation in the https://github.com/kubernetes/website repo around your contributions (thanks by the way!) When you have some time, could you please modify/add/remove the relevant content that needs changing in our documentation repo? Thanks! Please let me or my colleague Misty know (@zparnold/@misty on K8s Slack) if you need any assistance with the documentation.

mikedanese self-assigned this Apr 2, 2018

mikedanese added sig/auth Categorizes an issue or PR as relevant to SIG Auth. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Apr 2, 2018

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Apr 2, 2018

k8s-ci-robot requested review from janetkuo, jbeda, jingxu97, jsafrane, lavalamp, msau42, ncdc, rootfs, sttts, thockin, vishh and wojtek-t April 2, 2018 04:28

mikedanese force-pushed the svcacctproj branch 2 times, most recently from 5f43d74 to 06ece83 Compare April 2, 2018 04:47

k8s-ci-robot assigned CaoShuFeng Apr 2, 2018

mikedanese force-pushed the svcacctproj branch from 06ece83 to cdec713 Compare April 2, 2018 15:31

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 2, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 2, 2018

mikedanese force-pushed the svcacctproj branch 2 times, most recently from f5df59a to c20d417 Compare May 31, 2018 18:27

mikedanese assigned liggitt May 31, 2018

mikedanese force-pushed the svcacctproj branch from c20d417 to 1455a12 Compare May 31, 2018 18:54

saad-ali approved these changes Jun 4, 2018

View reviewed changes

tallclair reviewed Jun 4, 2018

View reviewed changes

implement service account token projection

91feb34

saad-ali mentioned this pull request Jun 5, 2018

[Failing Tests] All GKE related jobs are failing in release-master-blocking, upgrade, 1.11 release-blocking jobs #64789

Closed

Conversation

mikedanese commented Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Apr 2, 2018

Uh oh!

CaoShuFeng commented Apr 2, 2018

Uh oh!

mikedanese commented Apr 2, 2018

Uh oh!

jbeda commented Apr 2, 2018

Uh oh!

mikedanese commented Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

awly commented May 31, 2018

Uh oh!

mikedanese commented Jun 1, 2018

Uh oh!

tallclair commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jun 1, 2018

Uh oh!

saad-ali left a comment

Choose a reason for hiding this comment

Uh oh!

mikedanese commented Jun 4, 2018

Uh oh!

tallclair left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikedanese commented Jun 4, 2018

Uh oh!

mikedanese commented Jun 4, 2018

Uh oh!

dims commented Jun 4, 2018

Uh oh!

dims commented Jun 4, 2018

Uh oh!

k8s-github-robot commented Jun 4, 2018

Uh oh!

mikedanese commented Apr 2, 2018 •

edited

Loading

mikedanese commented Apr 2, 2018 •

edited

Loading

tallclair commented Jun 1, 2018 •

edited

Loading