Minor endpoint/ipcache/test refactoring for CIDR+L4 work#3884
Minor endpoint/ipcache/test refactoring for CIDR+L4 work#3884tgraf merged 5 commits intocilium:masterfrom
Conversation
9bb8af3 to
99d323b
Compare
|
test-me-please |
raybejjani
left a comment
There was a problem hiding this comment.
a small naming change. Don't block merging on me.
There was a problem hiding this comment.
This name really confused me. I thought it would touch the IPCache (I hadn't realised it was package function). Could we name it UpsertToKVStore or something equally verbose? (same for DeleteIP below)
Since we mention how this change propagates, could we add a similar statement to Upsert? (It's available instantly with Upsert but there can be a delay with UpsertIP).
There was a problem hiding this comment.
funny, in the full PR I ended up doing this anyway, but it wasn't part of the regular refactoring. I'll do this.
Soon, it might be worth splitting the ipcache kvstore push stuff from the in-memory / kvstore event handling piece. That would clarify things a bit.
99d323b to
f554929
Compare
|
test-me-please |
ianvernon
left a comment
There was a problem hiding this comment.
LGTM apart from one requested change that doesn't need re-review.
There was a problem hiding this comment.
Can you make DeleteIP mirror UpsertIPToKVStore so that it is clear that it is being deleted from the key-value store, and not the ipcache itself directly?
raybejjani
left a comment
There was a problem hiding this comment.
Thanks for switching it up! I'll second @ianvernon on also changing ipcache.DeleteIP but otherwise, ship it!
There was a problem hiding this comment.
Nit: Can we make this Failed to clear policy after egress test to make it more obvious in logs that something failed.
Commit 6c35fbe ("pkg/policy: fix merging of L4-related policy") introduced similar logic to this expansion of the empty endpointselector slice, so this could should be unused. Remove it. Signed-off-by: Joe Stringer <joe@covalent.io>
This makes the two lookup functions diverge less. Signed-off-by: Joe Stringer <joe@covalent.io>
This will improve the code reuse for when IP prefixes need to be pushed into the kvstore. Signed-off-by: Joe Stringer <joe@covalent.io>
Signed-off-by: Joe Stringer <joe@covalent.io>
Refactor some of the calls in TestEgressToHost() so that a new test in the same context can share the same setup / teardown logic. Signed-off-by: Joe Stringer <joe@covalent.io>
f554929 to
c4f4f47
Compare
|
test-me-please |
|
Fixed up the minor nits, thanks for the review everyone. |
[Split out from #3835 for easier review]
This is a short series of misc refactoring patches I picked up on my way towards #1685, split out for easier review.
This change is