Skip to content

Fix issue where pods could read stale immutable secrets/configmaps after re-creation #132046

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
Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix issue where pods could read stale immutable secrets/configmaps after re-creation #132046

wants to merge 4 commits into from
+80 −8

Conversation

kei01234kei
Copy link
Contributor

@kei01234kei kei01234kei commented May 31, 2025 •
edited
Loading

What type of PR is this?

/kind bug

What this PR does / why we need it:

To resolve the bug about the immutable secret and the configmap.

Which issue(s) this PR fixes:

Fixes #131589, #124701

Special notes for your reviewer:

Does this PR introduce a user-facing change?

To resolve the issue where newly created pods can continue to retrieve old immutable secrets/configmaps even after re-creating immutable resources with the same name, we have modified the system so that the reflector for immutable resources is no longer stopped.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. 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. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 31, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @kei01234kei. Thanks for your PR.

I'm waiting for a kubernetes 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-sigs/prow repository.

@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubelet area/test sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 31, 2025
@k8s-ci-robot k8s-ci-robot requested review from SataQiu and tallclair May 31, 2025 08:58
@kei01234kei
Copy link
Contributor Author

/cc @liggitt @wojtek-t

@k8s-ci-robot k8s-ci-robot requested review from liggitt and wojtek-t May 31, 2025 09:01
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 31, 2025
@kei01234kei
Copy link
Contributor Author

kei01234kei commented May 31, 2025 •
edited
Loading

In this PR, I prevent the reflector for immutable resources from being stopped.

A concern here is the negative impact on scalability, as @liggitt has pointed out (see #131589 (comment) ).
Still, since the reflector now supports watch lists, I think the adverse effects of keeping the reflector running have been reduced.

@kei01234kei
Copy link
Contributor Author

/kind bug

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels May 31, 2025
@kei01234kei kei01234kei marked this pull request as ready for review May 31, 2025 09:46
@kei01234kei kei01234kei changed the title WatchBasedManager does not stop watching immutable objects [WIP] WatchBasedManager does not stop watching immutable objects Jun 2, 2025
wojtek-t
wojtek-t reviewed Jun 2, 2025
View reviewed changes
pkg/kubelet/util/manager/watch_based_manager.go Outdated
// already have it from here
// - doing that would require significant refactoring to reflector
// we limit ourselves to just quickly stop the reflector here.
if c.isImmutable(object) {
Copy link
Member

Choose a reason for hiding this comment

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

This change would completely delete the whole gain of immutable secrets/configmaps and defeats the purpose of their existence.

There are other way of addressing the issue - basically what we need is to resync the cache when a new "usage" of secret/configmap is observed.

Copy link
Contributor Author

@kei01234kei kei01234kei Jun 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Is what you desire to change here into if !i.stopped { ?

kubernetes/pkg/kubelet/util/manager/watch_based_manager.go

Line 112 in 62f72ad

if i.immutable || !i.stopped {

If so, every time the Get method of the objectCache struct is executed, kubelet makes a request to the kube-apiserver. This can increase pressure on the kube-apiserver, especially during events such as rolling updates.
If this is acceptable to you, I will go ahead and apply the change.

Copy link
Member

Choose a reason for hiding this comment

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

This won't be enough on its own, because we need to ensure that it won't be overwritten later if watch is lagging. So it's more subtle than that - but it's roughly that direction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
make the reflector wait until it catches up to the latest resource version when it is restarted

I followed @serathius' comment specified here.

@wojtek-t wojtek-t self-assigned this Jun 2, 2025
@kei01234kei kei01234kei force-pushed the WatchBasedManager_does_not_stop_watching_immutable_objects branch from bc5ade5 to 382c4d8 Compare June 3, 2025 11:56
@kei01234kei kei01234kei changed the title [WIP] WatchBasedManager does not stop watching immutable objects Fix issue where pods could read stale immutable secrets/configmaps after re-creation Jun 3, 2025
@kei01234kei kei01234kei marked this pull request as ready for review June 3, 2025 13:23
@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 Jun 3, 2025
@SergeyKanzhelev SergeyKanzhelev moved this from Triage to Archive-it in SIG Node CI/Test Board Jun 4, 2025
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 6, 2025
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 10, 2025
@kei01234kei kei01234kei force-pushed the WatchBasedManager_does_not_stop_watching_immutable_objects branch from bad6b23 to 9e1373a Compare June 10, 2025 14:46
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 10, 2025
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 10, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kei01234kei
Once this PR has been reviewed and has the lgtm label, please ask for approval from wojtek-t. For more information see the Code Review Process.

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

@kei01234kei kei01234kei requested a review from wojtek-t June 10, 2025 14:54
@cici37
Copy link
Contributor

cici37 commented Jun 10, 2025

/remove-sig api-machinery

@k8s-ci-robot k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jun 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SataQiu SataQiu Awaiting requested review from SataQiu

@tallclair tallclair Awaiting requested review from tallclair

@liggitt liggitt Awaiting requested review from liggitt

@matthyx matthyx Awaiting requested review from matthyx

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

@aojea aojea Awaiting requested review from aojea

@haircommander haircommander Awaiting requested review from haircommander

@wojtek-t wojtek-t Awaiting requested review from wojtek-t

Assignees

@wojtek-t wojtek-t

Labels
area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
SIG Node CI/Test Board
Status: Archive-it
SIG Node: code and documentation PRs
Status: Triage
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

kubelet does not refresh immutable secrets after recreation as documentation says
4 participants
@kei01234kei @k8s-ci-robot @cici37 @wojtek-t