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.

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

Documentation for "Immutable Secrets/ConfigMaps" feature #19297

Merged
merged 1 commit into from
Mar 13, 2020

Conversation

wojtek-t
Copy link
Member

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 25, 2020
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 25, 2020
@k8sio-netlify-preview-bot
Copy link
Collaborator

Deploy preview for kubernetes-io-vnext-staging failed.

Built with commit ddb7248

https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/5e54d3903bf8960009a34189

@k8sio-netlify-preview-bot
Copy link
Collaborator

k8sio-netlify-preview-bot commented Feb 25, 2020

Deploy preview for kubernetes-io-vnext-staging processing.

Building with commit 68c52ff

https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/5e6a445744df3600093e5d5b

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Hi @wojtek-t

Thanks for suggesting this documentation. Here's some feedback.

/sig apps

{{< feature-state for_k8s_version="v1.18" state="alpha" >}}

For clusters that are extensively using secrets (having at least tens of thousands of unique
secret to pod mounts), we designed the `Immutable Secrets/ConfigMaps` feature. It allows you
Copy link
Contributor

Choose a reason for hiding this comment

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

Immutable Secrets/ConfigMaps

For highlighting, I'd use:

Suggested change
secret to pod mounts), we designed the `Immutable Secrets/ConfigMaps` feature. It allows you
Secret to Pod mounts), Kubernetes has an _Immutable Secrets & ConfigMaps_ feature. It allows you

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@k8s-ci-robot k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Feb 26, 2020
@wojtek-t
Copy link
Member Author

/sig apps

This isn't sig apps - it's sig/storage with combination of sig/node

@wojtek-t
Copy link
Member Author

/sig node
/sig storage

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. labels Feb 27, 2020
@wojtek-t
Copy link
Member Author

/assign @saad-ali

@wojtek-t wojtek-t force-pushed the immutable_secrets_docs branch 2 times, most recently from 834d2ad to 1c22124 Compare February 27, 2020 13:13
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Some feedback


To use it, you need to enable `ImmutableEmphemeralVolumes`
[feature gate](/docs/reference/command-line-tools-reference/feature-gates/) and then
mark your Secret/ConfigMap by setting it `Immutable` field to true, e.g.:
Copy link
Contributor

Choose a reason for hiding this comment

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

nits:

Suggested change
mark your Secret/ConfigMap by setting it `Immutable` field to true, e.g.:
mark your Secret/ConfigMap by setting its `Immutable` field to true; for example:

Copy link
Member Author

Choose a reason for hiding this comment

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

done

```

Once marked as immutable, it is NOT possible to revert this change nor to mutate the contents
of data field. You can only delete and recreate the secrete, but that will not be picked by
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
of data field. You can only delete and recreate the secrete, but that will not be picked by
of data field. You can only delete and recreate the Secret, but that will not be picked by

Also, I'd put this in a {{< note >}} or {{< caution >}} block to highlight the gotcha about Pod replacement.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@VineethReddy02
Copy link
Contributor

👋 Please rebase this PR on dev-1.18 in order to avoid merge conflicts.

Feel free to /hold cancel after you've rebased.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 7, 2020
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

@wojtek-t remember also update https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/

I've spotted a few other details that I recommend addressing.

Comment on lines 677 to 684
For clusters that are extensively using secrets (having at least tens of thousands of unique
Secret to Pod mounts), Kubernetes has an _Immutable Secrets/ConfigMaps_ feature. It allows you
to mark individual Secrets/ConfigMaps as `Immutable`, which blocks any changes to their data
and in return:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I use an immutable Secret if I only have a few hundred Secrets in my cluster?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - but I don't see where the text suggests you can't.

Copy link
Contributor

Choose a reason for hiding this comment

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

For clusters that are extensively using secrets (having at least tens of thousands of unique Secret to Pod mounts), Kubernetes has an Immutable Secrets/ConfigMaps feature.

This could read like a requirement.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you suggest? [I don't read it as a requirement]


To use it, you need to enable `ImmutableEmphemeralVolumes`
[feature gate](/docs/reference/command-line-tools-reference/feature-gates/) and then
mark your Secret/ConfigMap by setting its `Immutable` field to true, e.g.:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
mark your Secret/ConfigMap by setting its `Immutable` field to true, e.g.:
mark your Secret or ConfigMap, by setting its `immutable` field to `true`;
for example:

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2020
@wojtek-t
Copy link
Member Author

@kbhawkey - can you please take another pass (since we already have a technical lgtm now).

@kbhawkey
Copy link
Contributor

👀

@@ -105,6 +105,7 @@ different Kubernetes components.
| `HPAScaleToZero` | `false` | Alpha | 1.16 | |
| `HugePageStorageMediumSize` | `false` | Alpha | 1.18 | |
| `HyperVContainer` | `false` | Alpha | 1.10 | |
| `ImmutableEphemeralVolumes` | `false` | Alpha | 1.18 | |
Copy link
Contributor

Choose a reason for hiding this comment

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

@wojtek-t , If time, you probably want to add a brief description about this feature.
See the feature-gates section on this page, https://deploy-preview-19297--kubernetes-io-vnext-staging.netlify.com/docs/reference/command-line-tools-reference/feature-gates/#feature-gates

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -676,6 +676,35 @@ A container using a Secret as a
Secret updates.
{{< /note >}}

{{< feature-state for_k8s_version="v1.18" state="alpha" >}}
Copy link
Contributor

@kbhawkey kbhawkey Mar 11, 2020

Choose a reason for hiding this comment

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

@wojtek-t , Here are some suggestions.
Could you create a sub-heading to introduce this section?
Right now this content is under Using Secrets and mounted-secrets-are-updated-automatically.
Not sure if this makes sense?

#### Immutable Secrets and ConfigMaps

Possible edit to introduce feature:

The Kubernetes alpha feature Immutable Secrets and ConfigMaps provides an option to set individual Secrets and ConfigMaps as immutable. For clusters that extensively use Secrets (at least tens of thousands of unique Secret to Pod mounts), preventing changes to this data has the following advantages:

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually think that having it exactly in this section (mounted-secrets-are-updated-automatically) is exactly where it should be, because those secrets immutable secrets are the ones that won't be updated.

That said, I like the suggestion you have to introduce the feature - changing.

and in return:
- protects you from accidental (or unwanted) updates that could cause applications outages
- improves performance of your cluster by significantly reducing load on kube-apiserver, by
closing watches for secrets marked as immutable.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, Line 686, closing watches?

  • improves performance of your cluster by significantly reducing load on the kube-apiserver and closing watches for secrets marked as immutable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that "by" is better - because the performance is improved by the fact that we're closing those watches.

- improves performance of your cluster by significantly reducing load on kube-apiserver, by
closing watches for secrets marked as immutable.

To use it, you need to enable `ImmutableEmphemeralVolumes`
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible edit, something like:

To use this feature, enable the ImmutableEmphemeralVolumes feature gate
and set your Secret or ConfigMap immutable field to true. For example:

Copy link
Member Author

Choose a reason for hiding this comment

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

done


{{< note >}}
Once marked as immutable, it is NOT possible to revert this change nor to mutate the contents
of data field. You can only delete and recreate the Secret, but that will not be picked by
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible edit, pick and choose what makes sense:
Once a Secret is marked as immutable, it is not possible to revert this change nor to mutate the contents of the data field. You can only delete and recreate the Secret. Existing Pods maintain a mount point to the deleted Secret. It is recommended to recreate these Pods.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 12, 2020
Copy link
Member Author

@wojtek-t wojtek-t left a comment

Choose a reason for hiding this comment

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

@kbhawkey - thanks a lot for comments. PTAL

@@ -676,6 +676,35 @@ A container using a Secret as a
Secret updates.
{{< /note >}}

{{< feature-state for_k8s_version="v1.18" state="alpha" >}}
Copy link
Member Author

Choose a reason for hiding this comment

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

I actually think that having it exactly in this section (mounted-secrets-are-updated-automatically) is exactly where it should be, because those secrets immutable secrets are the ones that won't be updated.

That said, I like the suggestion you have to introduce the feature - changing.

- improves performance of your cluster by significantly reducing load on kube-apiserver, by
closing watches for secrets marked as immutable.

To use it, you need to enable `ImmutableEmphemeralVolumes`
Copy link
Member Author

Choose a reason for hiding this comment

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

done

and in return:
- protects you from accidental (or unwanted) updates that could cause applications outages
- improves performance of your cluster by significantly reducing load on kube-apiserver, by
closing watches for secrets marked as immutable.
Copy link
Member Author

Choose a reason for hiding this comment

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

I think that "by" is better - because the performance is improved by the fact that we're closing those watches.


{{< note >}}
Once marked as immutable, it is NOT possible to revert this change nor to mutate the contents
of data field. You can only delete and recreate the Secret, but that will not be picked by
Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -105,6 +105,7 @@ different Kubernetes components.
| `HPAScaleToZero` | `false` | Alpha | 1.16 | |
| `HugePageStorageMediumSize` | `false` | Alpha | 1.18 | |
| `HyperVContainer` | `false` | Alpha | 1.10 | |
| `ImmutableEphemeralVolumes` | `false` | Alpha | 1.18 | |
Copy link
Member Author

Choose a reason for hiding this comment

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

done

individual Secrets and ConfigMaps as immutable. For clusters that extensively use Secrets
(at least tens of thousands of unique Secret to Pod mounts), preventing changes to their
data has the following advantages:
- protects you from accidental (or unwanted) updates that could cause applications outages
Copy link
Contributor

Choose a reason for hiding this comment

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

@wojtek-t , Changes look good. One issue:
Add a newline at line 685 before the first list item (the list is not rendering)
Thanks!

@wojtek-t
Copy link
Member Author

@kbhawkey - done (hopefully will work this time)

@kbhawkey
Copy link
Contributor

/lgtm

@VineethReddy02 , Is the dev-1.18 branch accepting commits?

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

@kbhawkey yes! If the technical reviews are done, we can merge this into dev-1.18

@wojtek-t
Copy link
Member Author

@saad-ali approved it yesterday (there were only wording changes requested by @kbhawkey after that, so I believe we're fine).

@kbhawkey
Copy link
Contributor

👍

@wojtek-t
Copy link
Member Author

@kbhawkey - can you approve it then?

@kbhawkey
Copy link
Contributor

@wojtek-t , I assume from the changes that you have rebased this pull request a number of times (there was a mixup with the dev-1.18 branch early in the release). If so, @VineethReddy02, I am happy to approve this pull request or if you agree, you can as well.

@wojtek-t
Copy link
Member Author

@wojtek-t , I assume from the changes that you have rebased this pull request a number of times (there was a mixup with the dev-1.18 branch early in the release). If so, @VineethReddy02, I am happy to approve this pull request or if you agree, you can as well.

Yeah - I think it's ready. I would feel better if you could approve it.

@sftim
Copy link
Contributor

sftim commented Mar 13, 2020

Based on #19297 (comment) and #19297 (comment)
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: saad-ali, 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 /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 13, 2020
@k8s-ci-robot k8s-ci-robot merged commit d343728 into kubernetes:dev-1.18 Mar 13, 2020
@wojtek-t
Copy link
Member Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/docs Categorizes an issue or PR as relevant to SIG Docs. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants