k8s: include namespace in EndpointSliceName#43999
Conversation
2eb772a to
37721e8
Compare
|
Was there an actual issue caused by this? What would be awesome would be if you'd be able to show with a test case in Or is there a problematic use of I wouldn't change this just for the sake of it if there are no issues due to this as this does increase the number of allocations we need to perform to process an endpoint slice change. |
Yes, apologies, I should have given more context:
This bug caused real negative behaviour in our cluster, which does have EndpointSlices of the same name across namespaces, as each would clobber each other in the
This is reasonable - I will try to formulate a testcase to demonstrate this bug. |
382e990 to
e5dc380
Compare
|
@joamaki Thank you to the pointer to the testcases - I have added a testcase now demonstrating how this issue arises. Without 4bd9521 this test always fails, with it the test always passes. If you could point me in the right direction of the correct commit order etiquette for this test, that would be very helpful. Normally I'd add a fail-proving test before the commit that fixes it, but cilium's Contribution Guide mentions that every commit needs to be able to build so I wasn't clear if that means "every commit needs to compile + all tests pass" or just "every commit needs to compile; fail proving tests are acceptable". Happy to reorder commits as suggested. (Also rebased to latest |
|
If we are concerned about memory usage, we could look at changing If we parse the uuid into a 128-bit value this will create a constant, and relatively short 16-byte binary string, or if we take the uid as-is a 36 byte string without any extra processing / allocations. I would be happy to do this if you think it would have value. |
Awesome thanks for the test case! I would add the failing test case first and then the fix. The CI only checks that every commit builds (e.g. I think the test case would be slightly easier to follow if it wouldn't look at the BPF maps and would just look at the tables. I'll see if I can come up with a minimal version of it. |
|
Here's a suggestion for how to write the test case: |
Ta. Yes this is far more readable. I'll update. |
I quickly checked with |
|
"fix endpointslices with the same name cause changes to be missed" as release note is a bit vague. Could you reword it a bit so it's clear what was fixed? |
This commit adds a test that when we have two EndpointSlices in different namespaces with the same name, that these do not cause any collisions or problems. Specifically, we test the scenario where two EndpointSlices are setup, then one is modified by removing an address. This should be correctly picked up and the backend removed from the BPF maps. This test is a regression test for cilium#43999. Signed-off-by: Emily Shepherd <emily@redcoat.dev>
We previously did not include the Namespace in EndpointSliceNames, relying on the fact that EndpointSlices that use generateName- are unlikely to have name collisions, even across namespaces. While this is usually the case, there is no requirement for EndpointSlice managers to use generateName, and there are examples of controllers that do not (for example the master kubernetes service's EndpointSlice is always called "kubernetes"). Including the namespace in EndpointSliceNames guarantees collisions cannot occur. See cilium#43999 for further discussion of this bug. Signed-off-by: Emily Shepherd <emily@redcoat.dev>
e5dc380 to
f07f3d5
Compare
|
/test |
|
Commit 3954c69 does not match "(?m)^Signed-off-by:". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
This commit adds a test that when we have two EndpointSlices in different namespaces with the same name, that these do not cause any collisions or problems. Specifically, we test the scenario where two EndpointSlices are setup, then one is modified by removing an address. This should be correctly picked up and the backend removed from the BPF maps. This test is a regression test for cilium#43999. Signed-off-by: Emily Shepherd <emily@redcoat.dev>
We previously did not include the Namespace in EndpointSliceNames, relying on the fact that EndpointSlices that use generateName- are unlikely to have name collisions, even across namespaces. While this is usually the case, there is no requirement for EndpointSlice managers to use generateName, and there are examples of controllers that do not (for example the master kubernetes service's EndpointSlice is always called "kubernetes"). Including the namespace in EndpointSliceNames guarantees collisions cannot occur. See cilium#43999 for further discussion of this bug. Signed-off-by: Emily Shepherd <emily@redcoat.dev>
This commit adds a test that when we have two EndpointSlices in different namespaces with the same name, that these do not cause any collisions or problems. Specifically, we test the scenario where two EndpointSlices are setup, then one is modified by removing an address. This should be correctly picked up and the backend removed from the BPF maps. This test is a regression test for #43999. Signed-off-by: Emily Shepherd <emily@redcoat.dev>
[ upstream commit e69da93 ] This commit adds a test that when we have two EndpointSlices in different namespaces with the same name, that these do not cause any collisions or problems. Specifically, we test the scenario where two EndpointSlices are setup, then one is modified by removing an address. This should be correctly picked up and the backend removed from the BPF maps. This test is a regression test for cilium#43999. Signed-off-by: Emily Shepherd <emily@redcoat.dev>
[ upstream commit 3132f0c ] We previously did not include the Namespace in EndpointSliceNames, relying on the fact that EndpointSlices that use generateName- are unlikely to have name collisions, even across namespaces. While this is usually the case, there is no requirement for EndpointSlice managers to use generateName, and there are examples of controllers that do not (for example the master kubernetes service's EndpointSlice is always called "kubernetes"). Including the namespace in EndpointSliceNames guarantees collisions cannot occur. See cilium#43999 for further discussion of this bug. Signed-off-by: Emily Shepherd <emily@redcoat.dev>
[ upstream commit 74c1bde ] This commit adds a test that when we have two EndpointSlices in different namespaces with the same name, that these do not cause any collisions or problems. Specifically, we test the scenario where two EndpointSlices are setup, then one is modified by removing an address. This should be correctly picked up and the backend removed from the BPF maps. This test is a regression test for cilium#43999. Signed-off-by: Emily Shepherd <emily@redcoat.dev> Signed-off-by: Joe Stringer <joe@cilium.io>
[ upstream commit 5c146c3 ] We previously did not include the Namespace in EndpointSliceNames, relying on the fact that EndpointSlices that use generateName- are unlikely to have name collisions, even across namespaces. While this is usually the case, there is no requirement for EndpointSlice managers to use generateName, and there are examples of controllers that do not (for example the master kubernetes service's EndpointSlice is always called "kubernetes"). Including the namespace in EndpointSliceNames guarantees collisions cannot occur. See cilium#43999 for further discussion of this bug. Signed-off-by: Emily Shepherd <emily@redcoat.dev> Signed-off-by: Joe Stringer <joe@cilium.io>
[ upstream commit 3132f0c ] We previously did not include the Namespace in EndpointSliceNames, relying on the fact that EndpointSlices that use generateName- are unlikely to have name collisions, even across namespaces. While this is usually the case, there is no requirement for EndpointSlice managers to use generateName, and there are examples of controllers that do not (for example the master kubernetes service's EndpointSlice is always called "kubernetes"). Including the namespace in EndpointSliceNames guarantees collisions cannot occur. See cilium#43999 for further discussion of this bug. Signed-off-by: Emily Shepherd <emily@redcoat.dev>
[ upstream commit 74c1bde ] This commit adds a test that when we have two EndpointSlices in different namespaces with the same name, that these do not cause any collisions or problems. Specifically, we test the scenario where two EndpointSlices are setup, then one is modified by removing an address. This should be correctly picked up and the backend removed from the BPF maps. This test is a regression test for cilium#43999. Signed-off-by: Emily Shepherd <emily@redcoat.dev>
[ upstream commit 5c146c3 ] We previously did not include the Namespace in EndpointSliceNames, relying on the fact that EndpointSlices that use generateName- are unlikely to have name collisions, even across namespaces. While this is usually the case, there is no requirement for EndpointSlice managers to use generateName, and there are examples of controllers that do not (for example the master kubernetes service's EndpointSlice is always called "kubernetes"). Including the namespace in EndpointSliceNames guarantees collisions cannot occur. See cilium#43999 for further discussion of this bug. Signed-off-by: Emily Shepherd <emily@redcoat.dev>
[ upstream commit 74c1bde ] This commit adds a test that when we have two EndpointSlices in different namespaces with the same name, that these do not cause any collisions or problems. Specifically, we test the scenario where two EndpointSlices are setup, then one is modified by removing an address. This should be correctly picked up and the backend removed from the BPF maps. This test is a regression test for #43999. Signed-off-by: Emily Shepherd <emily@redcoat.dev> Signed-off-by: Joe Stringer <joe@cilium.io>
[ upstream commit 5c146c3 ] We previously did not include the Namespace in EndpointSliceNames, relying on the fact that EndpointSlices that use generateName- are unlikely to have name collisions, even across namespaces. While this is usually the case, there is no requirement for EndpointSlice managers to use generateName, and there are examples of controllers that do not (for example the master kubernetes service's EndpointSlice is always called "kubernetes"). Including the namespace in EndpointSliceNames guarantees collisions cannot occur. See #43999 for further discussion of this bug. Signed-off-by: Emily Shepherd <emily@redcoat.dev> Signed-off-by: Joe Stringer <joe@cilium.io>
[ upstream commit 74c1bde ] This commit adds a test that when we have two EndpointSlices in different namespaces with the same name, that these do not cause any collisions or problems. Specifically, we test the scenario where two EndpointSlices are setup, then one is modified by removing an address. This should be correctly picked up and the backend removed from the BPF maps. This test is a regression test for cilium#43999. Signed-off-by: Emily Shepherd <emily@redcoat.dev>
[ upstream commit 5c146c3 ] We previously did not include the Namespace in EndpointSliceNames, relying on the fact that EndpointSlices that use generateName- are unlikely to have name collisions, even across namespaces. While this is usually the case, there is no requirement for EndpointSlice managers to use generateName, and there are examples of controllers that do not (for example the master kubernetes service's EndpointSlice is always called "kubernetes"). Including the namespace in EndpointSliceNames guarantees collisions cannot occur. See cilium#43999 for further discussion of this bug. Signed-off-by: Emily Shepherd <emily@redcoat.dev>
[ upstream commit 74c1bde ] This commit adds a test that when we have two EndpointSlices in different namespaces with the same name, that these do not cause any collisions or problems. Specifically, we test the scenario where two EndpointSlices are setup, then one is modified by removing an address. This should be correctly picked up and the backend removed from the BPF maps. This test is a regression test for #43999. Signed-off-by: Emily Shepherd <emily@redcoat.dev>
[ upstream commit 74c1bde ] This commit adds a test that when we have two EndpointSlices in different namespaces with the same name, that these do not cause any collisions or problems. Specifically, we test the scenario where two EndpointSlices are setup, then one is modified by removing an address. This should be correctly picked up and the backend removed from the BPF maps. This test is a regression test for #43999. Signed-off-by: Emily Shepherd <emily@redcoat.dev>
[ upstream commit 5c146c3 ] We previously did not include the Namespace in EndpointSliceNames, relying on the fact that EndpointSlices that use generateName- are unlikely to have name collisions, even across namespaces. While this is usually the case, there is no requirement for EndpointSlice managers to use generateName, and there are examples of controllers that do not (for example the master kubernetes service's EndpointSlice is always called "kubernetes"). Including the namespace in EndpointSliceNames guarantees collisions cannot occur. See #43999 for further discussion of this bug. Signed-off-by: Emily Shepherd <emily@redcoat.dev>
We previously did not include the Namespace in EndpointSliceNames, relying on the fact that EndpointSlices that use generateName- are unlikely to have name collisions, even across namespaces. While this is usually the case, there is no requirement for EndpointSlice managers to use generateName, and there are examples of controllers that do not (for example the master kubernetes service's EndpointSlice is always called "kubernetes").
Including the namespace in EndpointSliceNames guarantees collisions cannot occur.
Warning: If this bugfix is to be merged into a branch that does not yet contain d003678, the old code that that commit cleaned up suffers from the same bug. Happy to provide a separate PR for that, if required, although it looks very out of date.
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXXline if the commit addresses a particularGitHub issue.
Fixes: <commit-id>tag, thenplease add the commit author[s] as reviewer[s] to this issue.