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.

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

feat: add token requests client #805

Merged
merged 5 commits into from Feb 3, 2022
Merged

Conversation

aramase
Copy link
Member

@aramase aramase commented Nov 10, 2021

Signed-off-by: Anish Ramasekar anish.ramasekar@gmail.com

What this PR does / why we need it:

  • Adds token request client to the driver
    • This client will be used to generate token for configured audiences and expiration seconds. If this is enabled, providers no longer need to generate the token.
    • We're reusing the token generation logic from kubelet which caches the token until it expires and cache cleanup is explicitly called as part of NodeUnpublishVolume.
    • This change is added as backward compat to support kubernetes < 1.20. In 1.20+ by having token requests as part of CSI driver spec, kubelet will generate the token and pass to the driver as part of CSI volume mount context.
    • We're using the CSI driver spec to define the token audience and expiration seconds as mentioned here: https://kubernetes-csi.github.io/docs/token-requests.html
  • Added e2e test with e2e-provider

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #

Special notes for your reviewer:

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 10, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aramase

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:

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 10, 2021
@aramase aramase force-pushed the token branch 2 times, most recently from e9116d9 to f97a8ad Compare January 25, 2022 00:46
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 25, 2022
@aramase aramase force-pushed the token branch 2 times, most recently from f9c5e53 to 7b341eb Compare January 25, 2022 19:53
@aramase aramase marked this pull request as ready for review January 26, 2022 06:34
@k8s-ci-robot 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 Jan 26, 2022
@aramase
Copy link
Member Author

aramase commented Jan 26, 2022

/uncc @ritazh
/cc @nilekhc

This is ready for review!

@k8s-ci-robot k8s-ci-robot requested review from nilekhc and removed request for ritazh January 26, 2022 21:40
Copy link
Contributor

@tam7t tam7t left a comment

Choose a reason for hiding this comment

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

Looking great, gonna need a bit more time to think a few things through though!

config/rbac/role.yaml Outdated Show resolved Hide resolved
sigs.k8s.io/yaml v1.3.0 // indirect
)

replace (
Copy link
Contributor

Choose a reason for hiding this comment

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

i feel like there must be a way to do this without all these replace directives (I'm goin got checkout the branch and try playing around with some things to make go mod happy...)

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay so I think i understand this a bit better...

This PR introduces a dependency on https://pkg.go.dev/k8s.io/kubernetes/pkg/kubelet/token

which pulls in the k8s.io/kubernetes v1.23.0 module, but according to kubernetes/kubernetes#90358 (comment)

"depending on k8s.io/kubernetes directly as a library, which is not supported"

I guess what we're doing here is whats mentioned kubernetes/kubernetes#79384

Since this token dependency is not permanent (we have plan to remove and replace with RequiresRepublish once thats supposed by all K8s supported releases) I think I'm good with these replace directives - but we should definitely add a comment explaining that k8s.io/kubernetes dependency causes the problems linking to kubernetes/kubernetes#79384

pkg/k8s/token.go Outdated Show resolved Hide resolved
test/e2eprovider/server/server_test.go Outdated Show resolved Hide resolved
@aramase
Copy link
Member Author

aramase commented Jan 27, 2022

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jan 27, 2022
@aramase aramase requested a review from tam7t January 27, 2022 19:39
@aramase aramase force-pushed the token branch 2 times, most recently from b3a33b8 to 04970bc Compare January 27, 2022 19:43
@aramase
Copy link
Member Author

aramase commented Jan 27, 2022

/test pull-secrets-store-csi-driver-e2e-azure

@aramase aramase force-pushed the token branch 2 times, most recently from 2f66ddc to 7d53974 Compare February 1, 2022 21:58
pkg/k8s/token.go Outdated Show resolved Hide resolved
gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f // indirect
gopkg.in/yaml.v2 v2.4.0 // indirect
)

replace (
Copy link
Contributor

@tam7t tam7t Feb 2, 2022

Choose a reason for hiding this comment

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

oof i'm noticing that this doesnt directly depend on k8s.io/kubernetes but still needs all these replace directives...

I wonder if providers (which depend on sigs.k8s.io/secrets-store-csi-driver to pull in the protobuf interface) will also need the replace directives. If so we may wanna move the protobuf to its own go module so it doesnt pull in all the transitive dependency issues.

(we dont need to do that as part of this PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

opened an issue for it: #852

@tam7t
Copy link
Contributor

tam7t commented Feb 2, 2022

Looking good! just a few more comments

Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
@tam7t
Copy link
Contributor

tam7t commented Feb 3, 2022

/lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants