-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
Add blog article to announce Secret references in NodeExpandVolume #33979
Conversation
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is some questions and format fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting this draft in early! I hope this feedback helps.
/retitle Add blog article to announce Secret references in NodeExpandVolume |
e65390f
to
ce3e5f2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Here's a bit more feedback.
Hey @humblec, It's been a while since last update, would you like to address issues in the previous comments so that we can move forward? |
ce3e5f2
to
052f17f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I have a few suggestions for the article to tidy it up.
parameters: | ||
csi.storage.k8s.io/node-expand-secret-name: test-secret # the name of the Secret | ||
csi.storage.k8s.io/node-expand-secret-namespace: default # the namespace that the Secret is in | ||
provisioner: hostpath.csi.k8s.io |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this provisioner actually work with NodeExpandVolume secret references?
(if not, maybe make up a fictional provisioner, eg blockstorage.cloudprovider.example
?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah.. the hostpath output and logs are from real testing. should be fine. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the hostPath
provisioner actually use these secrets? If it ignores them, that's maybe not an ideal example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually this feature is from the kubernetes csi client to push the secrets to the CSI driver as part of the NodeExpandVolume call.. The hostPath provisioner is a CSI driver who get those as shown in the example.. in that sense, it looks fine to me keep the current version as an example .. @sftim
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if really needed I can replace the string hostpath
to hostpath-example
or something similar.. please let me know wdyt @sftim
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain in a bit more detail (to reviewers, not readers) how a cluster admin works out what they put into test-secret
? If I changed the contents of that Secret to have empty strings as the values, would it still work?
If so, then we're not really illustrating things end-to-end. Yes, the feature from the KEP is implemented, but the reader cares about the end-to-end story. They're less interested in seeing an integration test that proves that the secret reference gets to the CSI driver.
Is blockstorage.cloudprovider.example
a plausible CSI driver name / reference? If it is, how about using that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sftim the nodeExpandSecret follow the same convention or rules of other similar secrets ( for ex: node stage, publish, controller expand) in the SC as described here
https://kubernetes-csi.github.io/docs/secrets-and-credentials-storage-class.html. The secret values will be sent to the CSI driver and if the credentials are correct to connect or perform the requested operation it will work otherwise fail.
Is blockstorage.cloudprovider.example a plausible CSI driver name / reference? If it is, how about using that?
Sure, I have changed the provisioner name to above.. it sound fine to have above as a pluasible CSI driver name..
ptal.. thanks.
052f17f
to
6778b27
Compare
6778b27
to
56df3f0
Compare
Cc @xing-yang |
2c91269
to
40ae12a
Compare
It would be appreciated if we can get final reviews on this PR ! |
We (blog team) would like a lightweight tech review on this from SIG Storage (optionally also SIG Auth) folks, to make sure there's no obvious technical inaccuracy. The article LGTM from a content / writing perspective. @humblec could you find someone from SIG Storage to check that preview? |
Thanks @sftim 👍
Sure, let me find/get review from SIG Storage. |
I added one comment. Otherwise, it looks good to me. |
40ae12a
to
7d4e347
Compare
addressed the same .. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That last fix wasn't right, I'm afraid.
Signed-off-by: Humble Chirammal <[email protected]>
7d4e347
to
37b77be
Compare
I have reformatted as suggested.. thanks 👍 |
Thanks /lgtm |
LGTM label has been added. Git tree hash: b51ee65e1affc5d78012fc27552d7b2b78c241cb
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sftim 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 |
Ref# KEP: kubernetes/enhancements#3173
Implementation: kubernetes/kubernetes#105963
Signed-off-by: Humble Chirammal [email protected]