Skip to content
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

Pass pod metadata to flex plugin #39488

Merged
merged 1 commit into from
May 18, 2017
Merged

Pass pod metadata to flex plugin #39488

merged 1 commit into from
May 18, 2017
+49 −11

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Jan 5, 2017 •
edited
Loading

Normal volume plugins get the pod spec to pull information from when setting up their volume, but flex plugins do not.

If a flex volume wants to set up things unique to the pod, or limited in permission based on the service account, the pod namespace, name, uid, and service account name are needed.

This PR adds pod uid, name, namespace, and service account name to the options passed to the plugin available during mounting

The options passed to a flexvolume plugin's mount command now contains the pod name (`kubernetes.io/pod.name`), namespace (`kubernetes.io/pod.namespace`), uid (`kubernetes.io/pod.uid`), and service account name (`kubernetes.io/serviceAccount.name`).

@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 5, 2017
@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 5, 2017
@liggitt
Copy link
Member Author

liggitt commented Jan 5, 2017

cc @kubernetes/sig-storage-misc @childsb

@childsb
Copy link
Contributor

childsb commented Jan 6, 2017

Pending the build + test failures this seems legit. I'm goign to ask @chakri-nelluri to review.

@liggitt
Copy link
Member Author

liggitt commented Jan 11, 2017

@childsb @chakri-nelluri any feedback?

@k8s-github-robot k8s-github-robot assigned erictune and unassigned pmorie Jan 30, 2017
@liggitt
Copy link
Member Author

liggitt commented Feb 20, 2017

@childsb @chakri-nelluri friendly ping

@liggitt liggitt mentioned this pull request Feb 21, 2017
Merged
@chakri-nelluri
Copy link
Contributor

chakri-nelluri commented Feb 27, 2017 •
edited
Loading

@liggitt changes to support attach/detach in flex volume went in today. PTAL.
As per Pod metadata, we do not have access to pod info from the attacher/detacher framework in attach/detach calls. It will only work for filesystem drivers, who will implement mount/unmount.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2017
@thockin thockin removed their assignment Feb 28, 2017
k8s-github-robot pushed a commit that referenced this pull request Feb 28, 2017
Merge pull request #41837 from liggitt/storageclass-param-validation
d33f6b8 
Automatic merge from submit-queue

Reserve kubernetes.io and k8s.io namespace for flex volume options

Split from #39488.

Flex volume already stuffs system information into the options map, and assumes it is free to do so:
```
	optionFSType    = "kubernetes.io/fsType"
	optionReadWrite = "kubernetes.io/readwrite"
	optionKeySecret = "kubernetes.io/secret"
```

this formalizes that by reserving the `kubernetes.io` and `k8s.io` namespaces so that user-specified options are never stomped by the system, and flex plugins can know that options with those namespaces came from the system, not user-options.

```release-note
Parameter keys in a StorageClass `parameters` map may not use the `kubernetes.io` or `k8s.io` namespaces.
```
@liggitt liggitt changed the title Pass pod metadata to flex plugin WIP - Pass pod metadata to flex plugin Mar 3, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 13, 2017
@liggitt liggitt changed the title WIP - Pass pod metadata to flex plugin Pass pod metadata to flex plugin Apr 13, 2017
@pmorie
Copy link
Member

pmorie commented Apr 25, 2017

@liggitt is this ready for review now?

jeffvance
jeffvance reviewed Apr 25, 2017
View reviewed changes
pkg/volume/flexvolume/mounter.go
extraOptions[optionKeyPodNamespace] = f.podNamespace
extraOptions[optionKeyPodUID] = string(f.podUID)
// service account metadata
extraOptions[optionKeyServiceAccountName] = f.podServiceAccountName
Copy link
Contributor

Choose a reason for hiding this comment

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

I have not studied this pr closely but am curious about what if in addition to, or instead of a service-account, a pod defines a securityContext? Does that need to be stored here?

Copy link
Member Author

Choose a reason for hiding this comment

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

fsGroup is already passed in. I'm not sure what else a flexvolume plugin would want/need, but my primary interest is enabling flex volumes to inject pod identity types of things, for which we'd want pod name, namespace, uid, and possibly service account name.

Copy link
Contributor

Choose a reason for hiding this comment

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

For now, volume drivers don't use service account (though in the future they could). But if we pass service account, should we also pass the secret too?

Copy link
Member Author

Choose a reason for hiding this comment

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

if we pass service account, should we also pass the secret too?

No, the point is not to let the volume plugin act as the service account, it is to inform the volume plugin which service account the pod is using. Think of a vault plugin that injects credentials from vault for that service account.

@liggitt
Copy link
Member Author

liggitt commented May 4, 2017

@liggitt is this ready for review now?

Yes

@k8s-github-robot k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 4, 2017
@pmorie pmorie added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 4, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 5, 2017
@phsiao
Copy link
Contributor

phsiao commented May 9, 2017

Interface changes in flexVolume in 1.6 broke several of our home-grown plugins because the volume name must be computed ahead of attach time and our plugin can't do that with little information. We use flexVolume for creating database LVM snapshots for development, creating scratch space from LVM thinpool, and we also use it to clone rbd snapshots for distributing code and data. These plugins can be patched to work by adding unique volume name in flexvolume.options but that is very limiting, and don't support deployment where multiple pods can run on the same node.

This PR addresses the challenge by providing pod uid, name, namespace in the options, so the volume name can be derived.

@chakri-nelluri
Copy link
Contributor

Review status: 0 of 5 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

pkg/volume/flexvolume/mounter.go, line 69 at r1 (raw file):


	// pod metadata
	extraOptions[optionKeyPodName] = f.podName

Thanks @liggitt
These options are only passed to plugins which implement mount call.
For plugins which implement "attach/detach" calls, please add "PV Name" information. That can be use as the metadata to construct the volumeName or use it as a tag for the detach call.

Comments from Reviewable

@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 17, 2017
@liggitt
Copy link
Member Author

liggitt commented May 18, 2017

@k8s-bot kops aws e2e test this

@liggitt
Copy link
Member Author

liggitt commented May 18, 2017

For plugins which implement "attach/detach" calls, please add "PV Name" information. That can be use as the metadata to construct the volumeName or use it as a tag for the detach call.

I'm specifically interested in this for prototyping flexvolume plugins to inject pod identity. I'm less familiar with the way flexvolume interacts with PV and attach/detach, so I'd rather let someone else add that if it makes sense.

@liggitt
Copy link
Member Author

liggitt commented May 18, 2017

rebased, retagging

@liggitt liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 18, 2017
@smarterclayton
Copy link
Contributor

Yeah I think this can stand on its own - it makes flex volumes more useful for the work sig-auth needs to prototype.

@smarterclayton
Copy link
Contributor

@k8s-bot kops aws e2e test this

@chakri-nelluri chakri-nelluri mentioned this pull request May 18, 2017
Closed
@liggitt
Copy link
Member Author

liggitt commented May 18, 2017

tagging @chakri-nelluri @saad-ali @MikaelCluseau for approval

@chakri-nelluri
Copy link
Contributor

Ok. Sounds good to me. I will make those changes in subsequent PR.

Review status: 0 of 5 files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

@chakri-nelluri
Copy link
Contributor

/lgtm

Reviewed 3 of 5 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chakri-nelluri, liggitt

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 18, 2017
@k8s-ci-robot
Copy link
Contributor

@chakri-nelluri: changing LGTM is restricted to assignees, and only kubernetes org members may be assigned issues.

In response to this:

/lgtm

Reviewed 3 of 5 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

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. I understand the commands that are listed here.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 5f3f170 into kubernetes:master May 18, 2017
@liggitt liggitt deleted the flex-metadata branch May 19, 2017 04:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rootfs rootfs rootfs left review comments

@jeffvance jeffvance jeffvance left review comments

Assignees

@pmorie pmorie

@erictune erictune

@saad-ali saad-ali

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.