-
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 deprecation warnings for enforce-mountable-secrets annotation #48771
base: dev-1.32
Are you sure you want to change the base?
Add deprecation warnings for enforce-mountable-secrets annotation #48771
Conversation
👷 Deploy Preview for kubernetes-io-vnext-staging processing.
|
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
/lgtm |
LGTM label has been added. Git tree hash: 8b403a873f69216ce71d77033b385d7d24d13ce7
|
/lgtm |
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 the PR, but:
- please follow the style guide
- use
note
callouts, notwarning
- small but important detail: don't deprecate the annotation except in the well-known list. Deprecate setting it.
- It'll have been deprecated "since Kubernetes v1.32", not "in v1.32+"
- use
- we should also remove or reword all places where we recommend setting the annotation
{{< warning >}} | ||
`kubernetes.io/enforce-mountable-secrets` is deprecated in v1.32+. Use separate namespaces to isolate access to mounted secrets. | ||
{{< /warning >}} |
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.
Move this to below the "Used on" line
@@ -806,6 +806,10 @@ This annotation is used for describing specific behaviour of given object. | |||
|
|||
### kubernetes.io/enforce-mountable-secrets {#enforce-mountable-secrets} |
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.
### kubernetes.io/enforce-mountable-secrets {#enforce-mountable-secrets} | |
### kubernetes.io/enforce-mountable-secrets (deprecated) {#enforce-mountable-secrets} |
{{< warning >}} | ||
`kubernetes.io/enforce-mountable-secrets` is deprecated in v1.32+. Use separate namespaces to isolate access to mounted secrets. | ||
{{< /warning >}} |
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.
I'd use a feature-state
shortcode here.
@@ -674,6 +674,8 @@ For more information, you can refer to the [documentation about this annotation] | |||
{{< warning >}} | |||
Any containers that run with `privileged: true` on a node can access all | |||
Secrets used on that node. | |||
|
|||
`kubernetes.io/enforce-mountable-secrets` is deprecated in v1.32+. Use separate namespaces to isolate access to mounted secrets. |
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.
Omit this, and reword or remove the earlier text:
To enhance the security measures around Secrets, Kubernetes provides a mechanism: you can
annotate a ServiceAccount askubernetes.io/enforce-mountable-secrets: "true"
.
Because this PR adds warnings (as in: warning-level callouts), and we try to be really sparing with these, I suggest we don't merge this yet. |
This comment was marked as outdated.
This comment was marked as outdated.
Oops. Wrong PR. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
b91bcec
to
1fb2c74
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.
/lgtm
One nit; good to merge as is.
@@ -62,12 +62,16 @@ recommendations include: | |||
* Implement audit rules that alert on specific events, such as concurrent | |||
reading of multiple Secrets by a single user | |||
|
|||
#### Additional ServiceAccount annotations for Secret management | |||
#### Additional ServiceAccount annotations for Secret management (deprecated) |
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.
We can use a singular here.
#### Additional ServiceAccount annotations for Secret management (deprecated) | |
#### ServiceAccount annotation for Secret management (deprecated) {#additional-serviceaccount-annotations-for-secret-management} |
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.
The point of having a customized anchor ID is to make it easy to reference this section or subsection. In that spirit, I'd suggest we use a shorter ID which is
unique and clear enough in this page. For example, #additional-annotation
.
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.
I don't agree about the point. My intention here was for existing links to stay working.
LGTM label has been added. Git tree hash: 6fc7f116f79f151c01bec30aadc3d6fcfcc12b1a
|
@@ -62,12 +62,16 @@ recommendations include: | |||
* Implement audit rules that alert on specific events, such as concurrent | |||
reading of multiple Secrets by a single user | |||
|
|||
#### Additional ServiceAccount annotations for Secret management |
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, in a page about good practice, maybe we should simply be recommending using namespaces as trust boundaries?
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.
Yes, IMO this section should be replaced with the following but I’m not sure how this impacts users who may have bookmarked the old section.
Restrict Access for Secrets
Use separate namespaces to isolate access to mounted secrets.
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.
PTAL
Signed-off-by: Rita Zhang <[email protected]>
1fb2c74
to
3b8c927
Compare
New changes are detected. LGTM label has been removed. |
Description
Add warnings for deprecated
kubernetes.io/enforce-mountable-secrets
annotation. Encourage the use of separate namespaces when isolation is desired.Issue
Closes: #
/sig auth
/cc @liggitt