Skip to content

fix: support getting sasToken, msiSecret, SPN from secret #867

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
merged 2 commits into from
Apr 7, 2023
Merged

fix: support getting sasToken, msiSecret, SPN from secret #867

merged 2 commits into from
Apr 7, 2023

Conversation

andyzhangx
Copy link
Member

@andyzhangx andyzhangx commented Apr 4, 2023 •
edited
Loading

What type of PR is this?
/kind feature

What this PR does / why we need it:
feat: support getting sasToken, msiSecret, SPN from secret
This PR could support reading sasToken, msiSecret, SPN from inline volume

Which issue(s) this PR fixes:

Fixes #840, #863

Requirements:

Special notes for your reviewer:

Release note:

feat: support getting sasToken, msiSecret, SPN from secret

@k8s-ci-robot 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 Apr 4, 2023
@k8s-ci-robot k8s-ci-robot requested review from cvvz and ZeroMagic April 4, 2023 14:08
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 4, 2023
@andyzhangx andyzhangx force-pushed the get-sastoken-secret branch from 5432180 to 20598ec Compare April 4, 2023 14:17
@andyzhangx
Copy link
Member Author

/retest

1 similar comment
@andyzhangx
Copy link
Member Author

/retest

@andyzhangx andyzhangx force-pushed the get-sastoken-secret branch from 20598ec to 3599c2d Compare April 6, 2023 03:24
@andyzhangx
Copy link
Member Author

/retest

cvvz
cvvz reviewed Apr 6, 2023
View reviewed changes
hack/verify-examples.sh
@@ -21,9 +21,9 @@ rollout_and_wait() {

APPNAME=$(kubectl apply -f $1 | grep -E "^(:?daemonset|deployment|statefulset|pod)" | awk '{printf $1}')
if [[ -n $(expr "${APPNAME}" : "\(daemonset\|deployment\|statefulset\)" || true) ]]; then
kubectl rollout status $APPNAME --watch --timeout=5m
kubectl rollout status $APPNAME --watch --timeout=20m
Copy link
Member

Choose a reason for hiding this comment

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

Is making timeout longer helpful? Seems like e2e test still failed. If it was not, we should undo this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's helpful sometimes since after a while, the nfs connection is ok per my investigation, not sure why. let's keep it.

pkg/blob/blob.go Outdated
@@ -460,7 +465,7 @@ func (d *Driver) GetAuthEnv(ctx context.Context, volumeID, protocol string, attr
if secretName != "" && !strings.EqualFold(azureStorageAuthType, "msi") {
Copy link
Member

Choose a reason for hiding this comment

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

should remove !strings.EqualFold(azureStorageAuthType, "msi") condition since not only account key will be stored in secret.

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch, could you take a look at the modified code? thanks.

Copy link

Choose a reason for hiding this comment

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

Hello there. I think this change introduced the bug #926

@andyzhangx andyzhangx force-pushed the get-sastoken-secret branch from 4bf9d90 to 636a8e6 Compare April 6, 2023 14:47
@andyzhangx andyzhangx changed the title feat: support getting sasToken, msiSecret, SPN from secret fix: support getting sasToken, msiSecret, SPN from secret Apr 7, 2023
@andyzhangx
Copy link
Member Author

/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Apr 7, 2023
@andyzhangx andyzhangx merged commit 7084b29 into kubernetes-sigs:master Apr 7, 2023
@andyzhangx
Copy link
Member Author

/cherrypick release-1.20

@k8s-infra-cherrypick-robot
Copy link

@andyzhangx: #867 failed to apply on top of branch "release-1.20":

Applying: feat: support getting sasToken, msiSecret, SPN from secret
Using index info to reconstruct a base tree...
M	pkg/blob/blob.go
M	pkg/blob/blob_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/blob/blob_test.go
CONFLICT (content): Merge conflict in pkg/blob/blob_test.go
Auto-merging pkg/blob/blob.go
CONFLICT (content): Merge conflict in pkg/blob/blob.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 feat: support getting sasToken, msiSecret, SPN from secret
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-1.20

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.

@andyzhangx andyzhangx mentioned this pull request Apr 7, 2023
Merged
4 tasks
@andyzhangx andyzhangx mentioned this pull request May 12, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carvido1 carvido1 carvido1 left review comments

@cvvz cvvz cvvz left review comments

@ZeroMagic ZeroMagic Awaiting requested review from ZeroMagic

Assignees
No one assigned
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. kind/bug Categorizes issue or PR as related to a bug. kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

support AzureStorageAuthType SPN with Inline volume
5 participants
@andyzhangx @k8s-ci-robot @k8s-infra-cherrypick-robot @carvido1 @cvvz