Skip to content

migrate service account volume to a projected volume when BoundServiceAccountTokenVolumes are enabled #69848

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
k8s-ci-robot merged 3 commits into from
Nov 17, 2018
Merged

migrate service account volume to a projected volume when BoundServiceAccountTokenVolumes are enabled #69848

k8s-ci-robot merged 3 commits into from
Nov 17, 2018

Conversation

@mikedanese
Copy link
Member

@mikedanese mikedanese commented Oct 16, 2018 •
edited
Loading

This should be safe now that projected volumes have been around since 1.6 and it allows us to migrate the ca.crt to a configmap and the token to a service account token volume projection once these features are ready.

Projected volumes were added in 8fb1b71

Part of #70679

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

When the BoundServiceAccountTokenVolumes Alpha feature is enabled, ServiceAccount volumes now use a projected volume source and their names have the prefix "kube-api-access".

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 16, 2018
@k8s-ci-robot k8s-ci-robot requested review from enj and liggitt October 16, 2018 04:51
@mikedanese
Copy link
Member Author

mikedanese commented Oct 16, 2018 •
edited
Loading

Will add a backwards compatibility test but we are covered by the (mount over default token path) hack already. I would like to do this without a feature flag since it is fully backwards compatible and built on GA APIs.

@mikedanese mikedanese requested a review from deads2k October 16, 2018 04:59
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 16, 2018
@mikedanese
Copy link
Member Author

mikedanese commented Oct 16, 2018

/retest

awly
awly reviewed Oct 16, 2018
View reviewed changes
test/integration/serviceaccount/service_account_test.go Outdated
Copy link
Contributor

@awly awly Oct 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use createVolume here instead, this is just maintenance burden and won't catch any failures

Copy link
Member Author

@mikedanese mikedanese Oct 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a v1 type and is post defaulting. Admission works on the internal type pre-defaulting.

Copy link
Member

@liggitt liggitt Oct 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Admission works on the internal type pre-defaulting.

Not sure it matters for this discussion, but by the time an object gets to admission, it was read in external form, defaulted, and converted to internal form, so defaulting was already done

Copy link
Member Author

@mikedanese mikedanese Oct 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, it's nuanced. Objects passed in via the request have undergone defaulting and conversion. In this specific instance, the internal volume type is created in the admission controller, not passed in from the request, so it has not undergone defaulting.

Copy link
Member

@liggitt liggitt Oct 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, got it

@awly
Copy link
Contributor

awly commented Oct 17, 2018

/lgtm
but would love to see a backwards-compat test

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2018
@awly
Copy link
Contributor

awly commented Oct 17, 2018

/retest

@mikedanese
Copy link
Member Author

mikedanese commented Oct 17, 2018

Need to add back compat tests.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 17, 2018
@liggitt
Copy link
Member

liggitt commented Oct 17, 2018

cc @wojtek-t for visibility on switching to projected volumes. looks like secrets via projected volume make use of the same caching secretmanager bits, which is good.

cc @kubernetes/sig-storage-pr-reviews - are there any behavior differences with projected volumes around updating live values, files being read-only, etc?

@k8s-ci-robot k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Oct 17, 2018
@wojtek-t
Copy link
Member

wojtek-t commented Oct 17, 2018

cc @wojtek-t for visibility on switching to projected volumes. looks like secrets via projected volume make use of the same caching secretmanager bits, which is good.

@liggitt @mikedanese - can you point me where do use the manager for projected volumes?

Also - we switched secrets/configmaps to WatchingSecretManager in 1.12, so going back to CachingManager, would be a regression.
Adding hold to understand stuff.

/hold

@liggitt
Copy link
Member

liggitt commented Oct 17, 2018

sorry... I meant it uses the kubelet's secret manager, whichever one is configured:

https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/projected/projected.go#L74-L76

@wojtek-t
Copy link
Member

wojtek-t commented Oct 18, 2018

sorry... I meant it uses the kubelet's secret manager, whichever one is configured:

ok - thanks for the pointer Jordan!

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 22, 2018
@mikedanese mikedanese removed the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Oct 22, 2018
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 14, 2018
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Nov 14, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikedanese

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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 14, 2018
@mikedanese mikedanese removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 15, 2018
@mikedanese
Copy link
Member Author

mikedanese commented Nov 15, 2018

/retest

@mikedanese
Copy link
Member Author

mikedanese commented Nov 16, 2018

/retest

@cjcullen
Copy link
Member

cjcullen commented Nov 16, 2018 •
edited
Loading

Change the title and the description + release note so this doesn't scare everybody. E.g.:
"migrate service account volume to a projected volume when BoundServiceAccountTokenVolumes are enabled" etc.

@mikedanese mikedanese changed the title migrate service account volume to a projected volume migrate service account volume to a projected volume when BoundServiceAccountTokenVolumes are enabled Nov 16, 2018
@cjcullen
Copy link
Member

cjcullen commented Nov 16, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2018
@cjwagner
Copy link
Member

cjwagner commented Nov 17, 2018

Please add the appropriate priority label.

@mikedanese mikedanese added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Nov 17, 2018
@mikedanese
Copy link
Member Author

mikedanese commented Nov 17, 2018

Adding labels per https://kubernetes.slack.com/archives/C2C40FMNF/p1542418440380400

@AishSundar
Copy link
Contributor

AishSundar commented Nov 17, 2018

/test pull-kubernetes-e2e-gce-100-performance

@mikedanese
Copy link
Member Author

mikedanese commented Nov 17, 2018

/retest

@k8s-ci-robot k8s-ci-robot merged commit ca696fe into kubernetes:master Nov 17, 2018
@mikedanese mikedanese deleted the projadmission branch November 17, 2018 07:41
@WanLinghao WanLinghao mentioned this pull request Mar 4, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liggitt liggitt liggitt left review comments

@deads2k deads2k Awaiting requested review from deads2k

+2 more reviewers

@awly awly awly left review comments

@enj enj enj left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@awly awly

@cjcullen cjcullen

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

v1.13

Development

Successfully merging this pull request may close these issues.

10 participants

@mikedanese @awly @liggitt @wojtek-t @tallclair @enj @k8s-ci-robot @cjcullen @cjwagner @AishSundar