-
Notifications
You must be signed in to change notification settings - Fork 670
add support for combination of podSelector and namespaceSelector in network policies #3647
Conversation
in network policies - enhance selectorSpec to accomodate both pod and namespace selectors - enhance analysePolicy to handle policies with both selectors Fixes #3312
npc/analyser.go
Outdated
if peer.PodSelector != nil { | ||
srcSelector, err = newSelectorSpec(peer.PodSelector, nil, ns.name, ipset.HashIP) | ||
if peer.PodSelector != nil && peer.NamespaceSelector != nil { | ||
srcSelector, err = newSelectorSpec(peer.PodSelector, peer.NamespaceSelector, nil, ns.name, ipset.HashIP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These six lines seem identical to the next else
clause, apart from substituting nil
for peer.NamespaceSelector
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it correct to pass ns.name
here? Don't we want a pod+namespace selector to work the same from any namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've noted some superficial issues, but I got stuck trying to figure out the bigger picture.
Why do we end up with nsSelectors
and podSelectors
when the latter can also have a namespace?
npc/analyser.go
Outdated
if peer.PodSelector != nil { | ||
dstSelector, err = newSelectorSpec(peer.PodSelector, nil, ns.name, ipset.HashIP) | ||
if peer.PodSelector != nil && peer.NamespaceSelector != nil { | ||
dstSelector, err = newSelectorSpec(peer.PodSelector, peer.NamespaceSelector, nil, ns.name, ipset.HashIP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These six lines seem identical to the next else
clause, apart from substituting nil
for peer.NamespaceSelector
.
npc/analyser.go
Outdated
if peer.PodSelector != nil { | ||
srcSelector, err = newSelectorSpec(peer.PodSelector, nil, ns.name, ipset.HashIP) | ||
if peer.PodSelector != nil && peer.NamespaceSelector != nil { | ||
srcSelector, err = newSelectorSpec(peer.PodSelector, peer.NamespaceSelector, nil, ns.name, ipset.HashIP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it correct to pass ns.name
here? Don't we want a pod+namespace selector to work the same from any namespace?
…ce and pod lable selectors
thanks for the review. I modified code to make it more explicit. So now there are three book keeping selector sets serving different purpose. nsSelectors *selectorSet // reference to global selectorSet that is shared across the `ns`. Used to represent all pods in the matching namespaces
podSelectors *selectorSet // used to represent the matching pods in namespace respresented by this `ns`
namespacedPodsSelectors *selectorSet // reference to global selectorSet that is shared across the `ns`. Used to represent matching pods in matching namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty much OK with this, but it's a big change so made some further comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
I'm a bit confused. Say I have 3 namespaces. A, B and C. only namespace B is labeled 'name=B' I was trying to get A -> A and B -> A and not C !-> A If I register this snippet in namespace A
I get the first part. A -> A. B !-> A, C !-> A. Working as expected. If I try:
I get A -> A, B !-> A, C !-> A. A can talk to A, but B cant. This seems backwards according to the spec? Then I try this one:
A can talk to A, B can talk to A, and C can not talk to A. This also seems wrong. Either it should be just a rule for B -> A, or it should be the equiv of the first two snippets together? |
@kfox1111 What image did you use for testing? This feature is right now only in master. I just rechecked the scenarios I see its working as expected. |
Oh. Didn't realize this was an unreleased feature. That would explain it. Thanks. |
Fixes #3312