Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

Fix: ingress rule lost podsrchost when neither pod and namespace selector is nil #3159

Closed
wants to merge 1 commit into from
Closed

Conversation

mainred
Copy link

@mainred mainred commented Nov 1, 2017

when we have ingress rule like this, this means traffict from pods of either condition
will be accepted:

  1. pods in the default namespace swith label "role=frontend"
  2. pods in the namespace with label "project=myproject"
metadata:
  name: test-network-policy
  namespace: default
spec:
  podSelector:
    matchLabels:
      role: db
  ingress:
  - from:
    - namespaceSelector:
        matchLabels:
          project: myproject
    - podSelector:
        matchLabels:
          role: frontend

But, weave-npc now only accepts NO.2 , and misses NO.1.
ref: https://kubernetes.io/docs/concepts/services-networking/network-policies/

…lector are not nil

when we have ingress rule like this, this means traffict from pods of either condition
will be accepted:
1. pods in the default namespace swith label "role=frontend"
2. pods in the namespace with label "project=myproject"

metadata:
  name: test-network-policy
  namespace: default
ingress:
- from:
  - namespaceSelector:
      matchLabels:
        project: myproject
  - podSelector:
      matchLabels:
        role: frontend

But, weave-npc now only accepts #2 , and misses #1.
ref: https://kubernetes.io/docs/concepts/services-networking/network-policies/
@mainred mainred changed the title Fix: ingress rule lost podsrchost when neither selector and namespace selector is nil Fix: ingress rule lost podsrchost when neither pod and namespace selector is nil Nov 2, 2017
@bboreham
Copy link
Contributor

bboreham commented Nov 3, 2017

Thanks for the contribution!

Let me see if I understand: the code will currently work for either a pod selector or a namespace selector, and you've fixed it to work in the case the policy has both.

I pushed this branch to the main repo so the full test suite will run.

Would probably put this on the 2.0 branch since it's a small change and a real bug.

@bboreham bboreham added this to the 2.0.6 milestone Nov 3, 2017
@mainred
Copy link
Author

mainred commented Nov 4, 2017

Yes, you got the point, bboreham.

@brb
Copy link
Contributor

brb commented Nov 6, 2017

@mainred Thanks for the contribution, but I don't see why the change is necessary. The current code does iterate over peers (in your given policy there are two peers), and for the policy creates two rules:

-A WEAVE-NPC-INGRESS -m set --match-set weave-%:FTKw;6Z~kKUkeo5F)k8K+kI src -m set --match-set weave-2ZiDW)3]+I}!oR#ru=*KAc{x1 dst -m comment --comment "namespaces: selector: project=myproject -> pods: namespace: default, selector: role=db" -j ACCEPT
-A WEAVE-NPC-INGRESS -m set --match-set weave-UbymzRD2/QjeNF/Kib4(cV*{P src -m set --match-set weave-2ZiDW)3]+I}!oR#ru=*KAc{x1 dst -m comment --comment "pods: namespace: default, selector: role=frontend -> pods: namespace: default, selector: role=db" -j ACCEPT

Just noting, that if you omit the dash before the second podSelector, it would make the policy invalid as "NetworkPolicyPeer describes a peer to allow traffic from. Exactly one of its fields must be specified." (src: https://kubernetes.io/docs/api-reference/v1.8/#networkpolicypeer-v1-networking)

@mainred
Copy link
Author

mainred commented Nov 6, 2017

@brb ,you are right, and thank you very much for your correcting my mistake. Maybe more testing should be made before I open another pr :) I'll close this pr.

@mainred mainred closed this Nov 6, 2017
@brb brb modified the milestones: 2.0.6, n/a Nov 6, 2017
@mainred mainred deleted the fix-missing-rule branch November 7, 2017 12:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants