search

kubernetes-sigs / network-policy-api

This repo addresses further work involving Kubernetes network security beyond the initial NetworkPolicy resource
Apache License 2.0
50 stars 28 forks source link

NPEP: Iron out Cluster Egress Support API Design #144

Closed tssurya closed 5 months ago

tssurya commented 9 months ago

See POC: https://github.com/kubernetes-sigs/network-policy-api/pull/143

A few things were discussed in the sig-network-policy-api meeting that happened on 10th October 2023:

  1. Should we diverge on our ingress and egress peer types - since we are adding only egress support initially and FQDN is also an egress peer thing? As per apiserver team recommendations that seems to be the best way forward? CONSENSUS: Yes let's have AdminNetworkPolicyPeerIngress and AdminNetworkPolicyPeerEgress types -> this will be commit1 of POC PR: https://github.com/kubernetes-sigs/network-policy-api/pull/143.
  2. Should we openly callout that Namespace and Pods peer types do NOT account for host-networked pods ? CONSENSUS: Yes, makes sense; let's do that. Opened https://github.com/kubernetes-sigs/network-policy-api/pull/156
  3. Should we have CIDRs and FQDNs inside the main API for ANP and BANP OR should we go with ExternalNetworks CRD like mentioned in the NPEP? Both have advantages and disadvantages and during the meeting no-one had strong preferences and nobody opposed either OR which is good. But we wanted to check back with @rahulkjoshi on what would make sense for FQDN egress peers. So for now going ahead with whatever is present in the NPEP and then we can narrow in on that.
  4. What about node2pod healthchecks that NetworkPolicy API allows implicitly: https://kubernetes.io/docs/concepts/services-networking/network-policies/ (When a pod is isolated for ingress, the only allowed connections into the pod are those from the pod's node)? For ANP we don't want that, we want admins to be able to explicitly specify that relation. This can be the ability to express isSameNode relation for a pod dynamically in addition to giving out NodeSelectors but this is an ingress traffic user story. CONSENSUS: Since we will split the peers as ingress/egress as per (1), this API design detail can be covered as part of ingress NPEP. As of now we don't have user stories that ask of this for egress traffic (I want pods to be able to talk to only my node).
  5. What about "internalDestinations?" -> NetPol defined "ipBlock" as a type of peer which can have podCIDRs/nodeCIDRs whatever in it, in ANP we are trying to explicitly define a peer as "outside the cluster" sort of like egressIPs where intra cluster traffic is not considered as egress. However some implementations cannot distinguish between internal and external... so how strong should this definition of "northbound destinations" be?
k8s-ci-robot commented 9 months ago

Skipping CI for Draft Pull Request. If you want CI signal for your change, please convert it to an actual PR. You can still manually trigger a test run with /test all

tssurya commented 9 months ago

/label api-review

netlify[bot] commented 9 months ago

Deploy Preview for kubernetes-sigs-network-policy-api ready!

Name Link
Latest commit 5182ef25ce8ff0605654123346fc66d668852391
Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-network-policy-api/deploys/65ae3fb78bf3560008befa79
Deploy Preview https://deploy-preview-144--kubernetes-sigs-network-policy-api.netlify.app/npeps/npep-126-egress-traffic-control
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

astoycos commented 7 months ago

/hold

After discussion in Sig-Network today I think we should have @thockin, @aojea and @kal from the core sig-network side put eyes on this as well before agreeing on the API design, basically they should have a look once the NPEP enters the "implementable" state which I will add to our docs as well.

tssurya commented 6 months ago

@danwinship @astoycos PTAL. This NPEP's changes are ready for merge. Nodes PR is already merged: https://github.com/kubernetes-sigs/network-policy-api/pull/143 CIDR PR is waiting for this one to merge: https://github.com/kubernetes-sigs/network-policy-api/pull/185

We have started a different NPEP for the new CDIR object use cases so we could just merge this one first asap.

tssurya commented 5 months ago

thanks @danwinship ! I think I have addressed all your comments, PTAL.

tssurya commented 5 months ago

I worry that we designed a "Cluster Egress" API (as the title states) and then at the last minute said "ok fine it's cluster-internal too" without fully considering the ramifications of that...

I have changed the terminologies to match what's reflected from previous discussions. I think title itself is ok because we do now will start allowing cluster egress support just that its not "exclusively ONLY cluster egress support" has an additional internal support too. OR more like its just a CIDR peer block :)

tssurya commented 5 months ago

I am happy with how this looks. Removing hold. Let's merge this?

tssurya commented 5 months ago

/hold cancel

tssurya commented 5 months ago

/assign @danwinship

astoycos commented 5 months ago

/lgtm /approve

Congrats @tssurya!!

k8s-ci-robot commented 5 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: astoycos, Dyanngg, tssurya

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: - ~~[OWNERS](https://github.com/kubernetes-sigs/network-policy-api/blob/main/OWNERS)~~ [Dyanngg,astoycos] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment