Conversation
Host policies are rejected on master with the following error:
$ kubectl describe ccnp | tail -n9
Status:
Nodes:
k8s1:
Error: Invalid CiliumNetworkPolicy spec: rule cannot have NodeSelector
Last Updated: 2020-08-10T12:52:51Z
k8s2:
Error: Invalid CiliumNetworkPolicy spec: rule cannot have NodeSelector
Last Updated: 2020-08-10T12:52:48Z
Events: <none>
This error is printed because CiliumNetworkPolicy.Parse() has some new
checks to prevent using NodeSelector in CNPs. It assumes
CiliumClusterwideNetworkPolicy.Parse() will be called in the case of CCNPs
with NodeSelectors. However, the k8s watcher for CCNPs calls
addCiliumNetworkPolicyV2 which takes a types.SlimCNP and therefore calls
CiliumNetworkPolicy.Parse().
This commit implements a temporary fix for this regression by manually
construction a CCNP object from CiliumNetworkPolicy.Parse() when the
namespace is empty (CNP is actually a CCNP) and parsing that instead of
the CNP object. It also adds a new unit test to check this behavior.
Related: #12834
Fixes: eafeb8f ("k8s: Disallow nodeSelector for CNP at agent level")
Signed-off-by: Paul Chaignon <paul@cilium.io>
9a63a88 to
1de9523
Compare
|
test-me-please |
There was a problem hiding this comment.
LGTM. Thanks for fixing this.
To answer your question about closing out the issue, I think yes we should close it because this fixes it. You asked about a "proper" fix--I spoke with @aanm offline about an approach that would allow us to "properly" fix this, however, it would require changes to the code that may not be worth the effort. In summary, we would need to split out the CNP and CCNP types from each other, which requires creating a new type for Rule (let's call it ClusterRule). ClusterRule would be what CCNP accepts as it contains a NodeSelector, while Rule would only contain an EndpointSelector. Unfortunately, the code is too heavily dependent on the Rule type.
I think we could revisit implementing a proper fix for #12834 if another need arises. Does that sound alright?
|
I think this is okay to merge. It has received two reviews from frequent contributors to this code and all required builds are passing. |
Host policies are rejected on master with the following error:
This error is printed because
CiliumNetworkPolicy.Parse()has some new checks to prevent usingNodeSelectorin CNPs. It assumesCiliumClusterwideNetworkPolicy.Parse()will be called in the case of CCNPs withNodeSelectors. However, the k8s watcher for CCNPs callsaddCiliumNetworkPolicyV2which takes atypes.SlimCNPand therefore callsCiliumNetworkPolicy.Parse().This commit implements a temporary fix for this regression by manually construction a CCNP object from
CiliumNetworkPolicy.Parse()when the namespace is empty (CNP is actually a CCNP) and parsing that instead of the CNP object. It also adds a new unit test to check this behavior.Fixes: #12834
Fixes: #11607
@christarazi Should we leave #12834 open until this is properly fixed, or is it okay to close it with this PR?