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

Implement Cluster Egress Traffic semantics (ANP&BANP NorthBound Support) - PART1 - Nodes #143

Closed tssurya closed 7 months ago

tssurya commented 9 months ago

TBD, This is a PoC to link to NPEP, until the implementable API design on the NPEP is not agreed upon: https://github.com/kubernetes-sigs/network-policy-api/pull/144 and merged, this PR shall remain in Draft state. It's just much easier to review/iterate on this PR and then copy the semantics into NPEP so that things don't go stale.

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 this PR.
  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?

Relates #126

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 23d3882d5c8bc280c2f1c7b590c11e1d3509a6b0
Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-network-policy-api/deploys/65533d1eafeb1d0007445668
Deploy Preview https://deploy-preview-143--kubernetes-sigs-network-policy-api.netlify.app
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.

liggitt commented 9 months ago

unless I'm missing something, it looks like the api-approved annotation on the CRD points at https://github.com/kubernetes-sigs/network-policy-api/pull/135 which did not get a K8s API review (https://go.k8s.io/api-review).

/assign @thockin @smarterclayton for visibility and review - https://github.com/kubernetes/kubernetes/blob/bae6911b112f38c2a429fe672bd3dd6ec3d11f7b/OWNERS_ALIASES#L353-L355

astoycos commented 9 months ago

Hiya @liggitt 👋

V0.1.0 and V0.1.1

Was our very first release + patch release with almost the exact v1alpha1 API that was implemented with https://github.com/kubernetes-sigs/network-policy-api/pull/30 (which was reviewed by @thockin and @danwinship )

This release was mainly to get that API into an official release so that our group could make sure that the release tooling and support we've implemented was in fact working and up to date, hence https://github.com/kubernetes-sigs/network-policy-api/pull/135 being the "approval link".

All of our tooling was inspired by the gateway-api group (which looking back now at its very beginning didn't even add the "api-approved" annotations to their CRD , see GWAPI v0.2.0 for reference, and didn't even add that annotation until https://github.com/kubernetes-sigs/gateway-api/releases/tag/v0.5.0-rc1 which when their first beta release )

If it's alright with you (for now) I propose we stop annotating our CRDs with api-approved.kubernetes.io? (Or at least until we're trying to release a beta API or something like that). Only because I feel like while this API is still very young (alpha) we're going to be needing to cut releases at a bit faster pace, even if most the time it's just to update/test our tooling + release related mechanisms.

Please LMK what you think :)

tssurya commented 8 months ago

If it's alright with you (for now) I propose we stop annotating our CRDs with api-approved.kubernetes.io?

We kind of had to do this to get successful CRD installs on clusters, we cannot stop that annotation IIUC. See https://github.com/kubernetes-sigs/network-policy-api/pull/40/files#diff-963e5daaaefbb96ed736d966ac4b1dcb7d9cf72414faf299ec69d3263404850cR6

We will start running into:

metadata.annotations[api-approved.kubernetes.io]: Required value: protected groups must have approval annotation "api-approved.kubernetes.io", see https://github.com/kubernetes/enhancements/pull/1111]
astoycos commented 8 months ago

We kind of had to do this to get successful CRD installs on clusters, we cannot stop that annotation IIUC.

Good point... Looks like GWAPI made use of group: networking.x-k8s.io for their first few releases which is why they didn't encounter this issue.

liggitt commented 8 months ago

Right... if you want to use an official k8s.io API group, you need an API review. SIGs can use the experimental group (*.x-k8s.io) to be Kubernetes adjacent without blocking on official API reviews.

k8s-ci-robot commented 8 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
npinaeva commented 7 months ago

/lgtm