Fix cleaning up identities #1021
Conversation
howardjohn
added
the
release-notes-none
istio-testing
added
the
size/L
| // Only track local nodes to avoid overhead | ||
| if track_identity { | ||
| self.by_identity | ||
| .entry(w.identity()) | ||
| .or_default() | ||
| .insert(w.uid.clone()); | ||
| } |
There was a problem hiding this comment.
I think this comment describes the intended use for track_identity rather than what this code does. Should we consider adding this comment to the insert method itself and/or changing track_identity to be is_node_local perhaps?
There was a problem hiding this comment.
should_track_certificates_for_removal() probably belongs in workload and should be checked as part of insert, and not in cert_mgr - it's being called before workload.insert pretty much everywhere.
There was a problem hiding this comment.
The reason I did it this way is workload doesn't have access to the proxy mode, local node, etc at all, so it cannot make the call itself. We could plumb it down, but then its inconsistent with the should_prefetch_certificate and requires even more plumbing around
There was a problem hiding this comment.
The reason I did it this way is workload doesn't have access to the proxy mode, local node, etc at all,
Figured.
We could plumb it down, but then its inconsistent with the should_prefetch_certificate and requires even more plumbing around
should_prefetch and should_track are almost the same check and it feels like both should probably go in workload but yeah that's getting OOS for this.
| // TODO: this should be vec![], but our testing setup doesn't exercise the real codepath | ||
| check( | ||
| vec![id1s.clone()], | ||
| "removing final workload should clear things out", | ||
| ) | ||
| .await; |
There was a problem hiding this comment.
Is this something we're looking to address before merging? Is it something covered elsewhere so we're not that worried?
Kind of odd to assert a final state that isn't actually the state it should be
There was a problem hiding this comment.
I would love to before merging though it will take a lot of work I may not have time to in the timelines this would be good to merge in (1.22.0). I think asserting the final state in the meantime is better than no assertion so if/when we fix it, it is remembered
There was a problem hiding this comment.
yeah, odd but probably the least bad option I suppose
|
|
|
/cherrypick release-1.22 |
|
@howardjohn: new pull request created: #1027 In response to this:
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. |
This fixes a bug where identities were being dropped wrongly. Basically we didnt track the identity properly, so ANY pod removal caused all ztunnels to forget that identity. This wasnt detected because ztunnel will on-demand fetch certificates its missing.