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

controller: add pv secret annotation support to forget SC #713

Merged
merged 3 commits into from
Mar 31, 2022
Merged

controller: add pv secret annotation support to forget SC #713

merged 3 commits into from
Mar 31, 2022

Conversation

humblec
Copy link
Contributor

@humblec humblec commented Mar 10, 2022 •
edited
Loading

Signed-off-by: Humble Chirammal [email protected]

/kind feature
Fixes #330

Special notes for your reviewer:

-->

Previous to this release, if the SC does not exist at time of PVC deletion where provisioner need credentials to delete volume which was passed as SC parameter at creation time, the PV deletion will fail as it can not fetch credentials by reading the SC from the API server. However this workflow is improved from this release which allows the PV deletion to succeed even if SC was deleted or does not exist at time of PV deletion.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. 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 Mar 10, 2022
@humblec humblec changed the title controller: add pv secret annotation support to forget SC [wip]controller: add pv secret annotation support to forget SC Mar 10, 2022
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 10, 2022
@mkimuram
Copy link
Contributor

@humblec I've created unit tests for this PR mkimuram@f0b88ab on top of this PR's commit. Please check it.
Also, please let me know your idea on how we collaborate, could you merge my test codes to this PR or should I create another PR?

@humblec humblec changed the title [wip]controller: add pv secret annotation support to forget SC controller: add pv secret annotation support to forget SC Mar 16, 2022
@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 Mar 16, 2022
@humblec
Copy link
Contributor Author

humblec commented Mar 16, 2022 •
edited
Loading

@humblec I've created unit tests for this PR mkimuram@f0b88ab on top of this PR's commit. Please check it. Also, please let me know your idea on how we collaborate, could you merge my test codes to this PR or should I create another PR?

@mkimuram Thats cool!. I have added your commit on top of this PR 👍 , lets see how it goes!

@humblec
Copy link
Contributor Author

humblec commented Mar 16, 2022

/assign @jsafrane
/assign @msau42
/assign @xing-yang

jsafrane
jsafrane reviewed Mar 17, 2022
View reviewed changes
pkg/controller/controller.go Outdated Show resolved Hide resolved
pkg/controller/controller.go Outdated Show resolved Hide resolved
pkg/controller/controller.go Outdated Show resolved Hide resolved
pkg/controller/controller.go Show resolved Hide resolved
pkg/controller/controller_test.go
Comment on lines +3998 to +3999
prefixedProvisionerSecretNameKey: "provisionersecret",
prefixedProvisionerSecretNamespaceKey: defaultSecretNsName,
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a different key-value par than getDefaultProvisinerSecrets() to see where the final secret comes from. This applies to all tests below.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jsafrane Thank you for your review.

Non-empty provisioner secret is set as PV annotation and modified StorageClass exists and Non-existent provisioner secret is set as PV annotation already checks where the final secret comes from.
Former one has invalid secret specified in the StorageClass and verifies that the right secret is taken from annotations (we may still need to discuss if it should be the expected behavior as commented here).
Latter one has invalid secret specified in the annotations and verifies that it fails to get the secret.

However, we may be able to improve getDefaultProvisinerSecrets() to prepare two secrets and check that either of the expected secret is set, to make it clear.
(Also, getDefaultProvisinerSecrets() should be renamed to something like getProvisinerSecretCandidates()?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsafrane could you please confirm/share your thought on above , so that the unit test can also be adjusted..

@humblec humblec force-pushed the new-sc-delete branch 2 times, most recently from 150cb7d to 6fcffe1 Compare March 18, 2022 07:14
ggriffiths
ggriffiths reviewed Mar 22, 2022
View reviewed changes
Copy link
Member

@ggriffiths ggriffiths left a comment

Choose a reason for hiding this comment

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

Mostly looks good, just one question for the provision changes

@humblec
Copy link
Contributor Author

humblec commented Mar 30, 2022

@jsafrane all comments except #713 (comment) has been addressed. ptal.. thanks !

@ggriffiths
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 30, 2022
@jsafrane
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: humblec, jsafrane

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 Mar 31, 2022
@k8s-ci-robot k8s-ci-robot merged commit 1c600a9 into kubernetes-csi:master Mar 31, 2022
@humblec humblec mentioned this pull request Jun 10, 2022
Merged
@humblec humblec mentioned this pull request Nov 10, 2022
Closed
@hoyho hoyho mentioned this pull request Apr 17, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkimuram mkimuram mkimuram left review comments

@jsafrane jsafrane jsafrane left review comments

@ggriffiths ggriffiths ggriffiths left review comments

@chrishenzie chrishenzie Awaiting requested review from chrishenzie

Assignees

@xing-yang xing-yang

@ggriffiths ggriffiths

@jsafrane jsafrane

@msau42 msau42

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/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.

Improve Deletion Secret Handling
7 participants
@humblec @mkimuram @ggriffiths @jsafrane @k8s-ci-robot @xing-yang @msau42