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

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

Merged

Conversation

tssurya
Copy link
Contributor

@tssurya tssurya commented Sep 24, 2023

TBD, This is a PoC to link to NPEP, until the implementable API design on the NPEP is not agreed upon: #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 Callout namespaces/pods peers do not include host-net pods #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
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 24, 2023
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 24, 2023
@tssurya
Copy link
Contributor Author

tssurya commented Sep 24, 2023

/label api-review

@k8s-ci-robot k8s-ci-robot added api-review Categorizes an issue or PR as actively needing an API review. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 24, 2023
@tssurya tssurya force-pushed the implement-egress-traffic-semantics branch from 3735cf6 to 3cd4701 Compare October 6, 2023 16:55
@netlify
Copy link

netlify bot commented Oct 6, 2023

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

Name Link
🔨 Latest commit 23d3882
🔍 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.

@tssurya tssurya marked this pull request as ready for review October 6, 2023 16:55
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 6, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2023
@tssurya tssurya force-pushed the implement-egress-traffic-semantics branch 2 times, most recently from 10a7267 to 234ed98 Compare October 7, 2023 13:00
@liggitt
Copy link

liggitt commented Oct 12, 2023

unless I'm missing something, it looks like the api-approved annotation on the CRD points at #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
Copy link
Member

astoycos commented Oct 12, 2023

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 #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 #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 :)

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2023
@tssurya
Copy link
Contributor Author

tssurya commented Oct 13, 2023

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
Copy link
Member

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.

@tssurya tssurya force-pushed the implement-egress-traffic-semantics branch from 234ed98 to f1c750c Compare October 16, 2023 10:45
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 16, 2023
@liggitt
Copy link

liggitt commented Oct 16, 2023

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.

@tssurya tssurya changed the title Implement Egress Traffic semantics (ANP&BANP NorthBound Support) Implement Cluster Egress Traffic semantics (ANP&BANP NorthBound Support) Oct 19, 2023
@tssurya tssurya force-pushed the implement-egress-traffic-semantics branch from f1c750c to 43ef2d3 Compare October 26, 2023 14:49
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 26, 2023
Copy link
Contributor

@Dyanngg Dyanngg left a comment

Choose a reason for hiding this comment

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

/lgtm

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

[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:

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 Nov 6, 2023
@tssurya tssurya force-pushed the implement-egress-traffic-semantics branch from 43ef2d3 to b0d14b2 Compare November 14, 2023 07:42
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 14, 2023
@tssurya tssurya changed the title Implement Cluster Egress Traffic semantics (ANP&BANP NorthBound Support) Implement Cluster Egress Traffic semantics (ANP&BANP NorthBound Support) - PART1 - Nodes Nov 14, 2023
This resulted from discussions in network-policy-api
meetings and after consulting apiserver team for best
practices.
So far ingress and egress peer expressions were symmetric.
However moving forward, since we are adding support for
egress (northbound) peers and fqdn which might have
differences compared to what we want to allow for ingress,
we have decided to split the peers into ingress and egress.

Signed-off-by: Surya Seetharaman <[email protected]>
Some FTR things:

1) As an egress peer a user can selector either namespaces, or pods or
   nodes.
In a given rule more than 1 type of selection is not allowed.
2) An empty node selector means it selects all nodes in the cluster.
3) nodes can be referred only from egress rule peers, since we only
support northbound use cases.

Signed-off-by: Surya Seetharaman <[email protected]>
@tssurya tssurya force-pushed the implement-egress-traffic-semantics branch from b0d14b2 to 23d3882 Compare November 14, 2023 09:25
@npinaeva
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2023
@k8s-ci-robot k8s-ci-robot merged commit f6c1cf2 into kubernetes-sigs:main Nov 14, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review Categorizes an issue or PR as actively needing an API review. 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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Development

Successfully merging this pull request may close these issues.

9 participants