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

Add feature listsnapshots secrets #237

Closed
wants to merge 4 commits into from
Closed

Add feature listsnapshots secrets #237

wants to merge 4 commits into from

Conversation

bells17
Copy link
Contributor

@bells17 bells17 commented Jan 14, 2020 •
edited
Loading

I modified to be able to use secrets in ListSnapshotRequest.

What type of PR is this?
/kind feature

What this PR does / why we need it:
I want to use secrets in ListSnapshots.

Which issue(s) this PR fixes:
Fixes #236

Special notes for your reviewer:

  • This PR uses secret annotation of deletion, for ListSnapshots secrets. But should I split the annotation for ListSnapshots secrets?

Does this PR introduce a user-facing change?:

Support ListSnapshots secrets

@k8s-ci-robot
Copy link
Contributor

@bells17: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 14, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @bells17!

It looks like this is your first PR to kubernetes-csi/external-snapshotter 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-csi/external-snapshotter has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 14, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @bells17. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bells17
To complete the pull request process, please assign xing-yang
You can assign the PR to them by writing /assign @xing-yang in a comment when ready.

The full list of commands accepted by this bot can be found 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 size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 14, 2020
@bells17 bells17 mentioned this pull request Jan 14, 2020
Closed
@xing-yang
Copy link
Collaborator

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 14, 2020
@xing-yang
Copy link
Collaborator

/assign @xing-yang

@xing-yang
Copy link
Collaborator

This PR uses secret annotation of deletion, for ListSnapshots secrets. But should I split the annotation for ListSnapshots secrets?

Yes, we should use different parameter keys and different annotations for ListSnapshots and DeleteSnapshot secrets.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 22, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 2, 2020
@bells17
Copy link
Contributor Author

bells17 commented Feb 2, 2020

@xing-yang Thank you. I added listing secret as an annotation to content.
Please review it again.

xing-yang
xing-yang reviewed Feb 5, 2020
View reviewed changes
pkg/utils/util.go
// Annotation for secret name and namespace will be added to the content
// and used at snapshot content listing time.
AnnListingSecretRefName = "snapshot.storage.kubernetes.io/listing-secret-name"
AnnListingSecretRefNamespace = "snapshot.storage.kubernetes.io/listing-secret-namespace"
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/listing-secret-name/list-secret-name

On line 56-57, we have:
prefixedSnapshotterSecretNameKey = csiParameterPrefix + "snapshotter-secret-name"
prefixedSnapshotterSecretNamespaceKey = csiParameterPrefix + "snapshotter-secret-namespace"

Those are used for the deletion secrets. We need to add something similar for list snapshots, i.e., snapshotter-list-secret-name.

Take a look here: https://github.com/kubernetes-csi/external-provisioner/blob/master/pkg/controller/controller.go#L541
In the external-provisioner, there are multiple secrets being used. We need to add a parameter in getSecretReference() so that it can handle different secrets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xing-yang Thank you, and sorry I misunderstood about how to implement...
I created a new PR #252.
Could you review this PR?

@bells17 bells17 mentioned this pull request Feb 10, 2020
Merged
@xing-yang
Copy link
Collaborator

Closing this PR as a new PR #252 is submitted to replace this one.

@xing-yang xing-yang closed this Feb 10, 2020
xing-yang added a commit to xing-yang/external-snapshotter that referenced this pull request Nov 27, 2023
@xing-yang
Squashed 'release-tools/' changes from f9d5b9c..f8c8cc4
cf43916 
f8c8cc4 Merge pull request kubernetes-csi#237 from msau42/prow
b36b5bf Merge pull request kubernetes-csi#240 from dannawang0221/upgrade-go-version
adfddcc Merge pull request kubernetes-csi#243 from pohly/git-subtree-pull-fix
c465088 pull-test.sh: avoid "git subtree pull" error
7b175a1 Update csi-test version to v5.2.0
987c90c Update go version to 1.21 to match k/k
2c625d4 Add script to generate patch release notes

git-subtree-dir: release-tools
git-subtree-split: f8c8cc4
andyzhangx pushed a commit to andyzhangx/external-snapshotter that referenced this pull request Jun 5, 2024
@k8s-ci-robot
Merge pull request kubernetes-csi#237 from msau42/prow
f8c8cc4 
Add script to generate patch release notes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xing-yang xing-yang xing-yang left review comments

@saad-ali saad-ali Awaiting requested review from saad-ali

Assignees

@xing-yang xing-yang

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. kind/feature Categorizes issue or PR as related to a new feature. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support ListSnapshots secrets
3 participants
@bells17 @k8s-ci-robot @xing-yang