Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify certain NetworkPolicy behavior #10151

Merged

Conversation

danwinship
Copy link
Contributor

  1. We never documented podSelector+namespaceSelector.
  2. We never clearly documented ipBlock's semantics.

I don't know if the formatting is ideal.

@kubernetes/sig-network-pr-reviews

@k8s-ci-robot k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 30, 2018
@k8sio-netlify-preview-bot
Copy link
Collaborator

k8sio-netlify-preview-bot commented Aug 30, 2018

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 370ad2d

https://deploy-preview-10151--kubernetes-io-master-staging.netlify.com

@k8sio-netlify-preview-bot
Copy link
Collaborator

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 3cd0001

https://deploy-preview-10151--kubernetes-io-master-staging.netlify.com


Likewise, `ipBlock` `egress` rules generally select traffic based on the destination IP
provided by the source pod, and in particular, connections to a `Service` IP that then
gets redirected to a cluster-external IP may not be handled the same way as connections
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the ingress section above makes reasonable sense to me. This part is perhaps less obvious to me than the ingress case.

For egress podSelector / namespaceSelector, the enforcement is typically done post service DNAT, and so it's perhaps intuitive that the same would be true for ipBlock as well. I'm not 100% convinced either way.

For both cases, I'm slightly concerned that we may be proscribing a bit too heavily, and a general "Some plugins provide subtly different behaviors, so refer to each plugin's documentation for more information" type statement might be more appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying it should work this way, but it seems like it probably inevitably will work this way for any plugins that don't have their own integrated kube-proxy; they can't enforce the rule before the connection passes through kube-proxy because they don't know what the final destination IP will be, but they probably can't enforce the rule after the connection passes through kube-proxy either, because they can no longer tell what pod the connection came from at that point (and there may not be any hooks left to let the plugin grab the packet again after it has gone through POSTROUTING anyway).

I'm fine with trying to avoid proscribing. That's what the "in general" was about but I can waffle even more than that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the other question is how ipBlock egress rules interact with the service CIDR. I was assuming that if you are allowed to egress to a pod directly, you are also allowed to egress to the pod via any service IP that points to it, without needing to have an ipBlock allowing egress to that service IP or to the service CIDR in general. And in fact, I was assuming that you couldn't block access to service IPs even by doing

egress:
- to:
  - ipBlock:
      cidr: 0.0.0.0/0
      except:
      - 172.30.0.0/16

but maybe that's either wrong or implementation-defined?

@danwinship
Copy link
Contributor Author

Updated the text:

Cluster ingress and egress mechanisms often require rewriting the source or destination IP
of packets. In cases where this happens, it is not defined whether this happens before or
after NetworkPolicy processing, and the behavior may be different for different
combinations of network plugin, cloud provider, Service implementation, etc.

In the case of ingress, this means that in some cases you may be able to filter incoming
packets based on the actual original source IP, while in other cases, the "source IP" that
the NetworkPolicy acts on may be the IP of a LoadBalancer or of the pod's node, etc.

For egress, this means that connections from pods to Service IPs that get rewritten to
cluster-external IPs may or may not be subject to ipBlock-based policies.


When in doubt, use `kubectl describe` to see how Kubernetes has interpreted the policy.

__ipBlock__: This selects particular IP addresses to allow as ingress sources or egress destinations. Normally these would be cluster-external IPs, since pod IPs are ephemeral and unpredictable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you highlight that this should be in the form of a CIDR .

We validate the CIDR here and leverage the net pkg ParseCIDR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably need to talk more about syntax in general... the document is currently more focused on semantics...

I also noticed that we no longer have the canonical description of how connections are matched against policies anywhere. (eg, I don't think any current documents mention the fact that healthchecks are always allowed through even when pods are isolated)

@zparnold zparnold changed the base branch from master to release-1.12 September 10, 2018 12:43
@zparnold
Copy link
Member

/milestone 1.12

@zparnold zparnold added this to the 1.12 milestone Sep 10, 2018
@danwinship
Copy link
Contributor Author

@zparnold The changes here are not 1.12 specific; the features discussed have existed (in beta) for several releases, and the clarifications here apply to the beta implementations. So according to https://kubernetes.io/docs/contribute/start#choose-which-git-branch-to-use this should be against master, not release-1.12

@zparnold zparnold changed the base branch from release-1.12 to master September 10, 2018 14:38
@zparnold
Copy link
Member

Ok

@danwinship
Copy link
Contributor Author

OK, changed the description of ipBlock to say that it selects "particular IP CIDR ranges" rather than "particular IP addresses". Other than that, there isn't really a lot to say about syntax, since most of the other fields are more obviously constrained.

I also rewrote the description of the sample NetworkPolicy to more clearly emphasize what the ipBlock section was doing (and to remove some redundancy that I think hurt readability).

@zparnold
Copy link
Member

/retest

@zparnold
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 25, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zparnold

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 25, 2018
@k8s-ci-robot k8s-ci-robot merged commit c6a4252 into kubernetes:master Sep 25, 2018
@danwinship danwinship deleted the networkpolicy-clarifications branch September 25, 2018 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/network Categorizes an issue or PR as relevant to SIG Network. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants