NPEP: FQDN Selector for Egress, User stories by rahulkjoshi · Pull Request #134 · kubernetes-sigs/network-policy-api

rahulkjoshi · 2023-08-08T21:56:28Z

Issue-Tracker: #133

netlify · 2023-08-08T21:56:33Z

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

Name	Link
🔨 Latest commit	`2ef73d4`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-network-policy-api/deploys/65b8302ae76dd500080ddf42
😎 Deploy Preview	https://deploy-preview-134--kubernetes-sigs-network-policy-api.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

k8s-ci-robot · 2023-08-08T21:56:37Z

Hi @rahulkjoshi. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

npinaeva

thanks for putting this together @rahulkjoshi !

astoycos

Good start, thanks for opening this! This NPEP is narrow and I'm alright with that :)

I think in the case where we may not have a bunch of proper user stories to add, but still want to specify the explicit desired use of a single user story examples can be used to add explicit color.

i.e in this case there's really only one real desire .... add a FQDN selector to ANP + BANP however if we want to specify that the selector will include both matching by full name AND by wildcard pattern let's use examples on that single user story to do so.

rahulkjoshi · 2023-08-15T16:53:26Z

Restored branch

shaneutt · 2023-08-29T16:07:14Z

/cc

Dyanngg

LGTM overall

astoycos · 2023-11-28T15:23:27Z

/lgtm for the user stories!

I'll let @danwinship Add the approval here when he's ready

astoycos

Oh an additionally @rahulkjoshi Please add this to NPEP to the website https://github.com/kubernetes-sigs/network-policy-api/blob/main/mkdocs.yml#L58

danwinship · 2023-12-05T20:30:42Z

/lgtm
/ok-to-test

danehans · 2023-12-05T21:14:03Z

+resolve to. Keeping up with changing IP addresses is a maintenance burden, and
+hampers the readability of the network policies.


Is the implementation of this spec expected to maintain the list of resolved IPs?

Yes that's the expectation.

Will the API(s) that support this NPEP define the expectations for managing the list?

@rahulkjoshi do you have any feedback regarding ^?

We will probably do some work to define expected behaviors alongside the API. I will try to flesh those out in a follow-up as well, but as some prior art of what I'm thinking: https://docs.google.com/document/d/1wOO6fgY0PRToJ85yC5WFDywAhwkK0m-ynLrHW-EmuGM/edit#bookmark=id.61oxh3gwesyg

danehans · 2023-12-05T21:33:21Z

+  (for example `kubernetes.io`).
+* Support basic wildcard matching capabilities when specifying FQDNs (for
+  example `*.cloud-provider.io`)
+* Currently only `ALLOW` type rules are proposed.


Is all other egress denied, including in-cluster?

Not exactly -- AdminNetworkPolicy doesn't operate on default deny semantics like that.

These rules would be paired with either a lower priority DENY rule, or use the default deny behavior of Kubernetes NetworkPolicy.

@rahulkjoshi ^ is what I thought but the NPEP is not explicit in this regard. Might be worth adding a condensed form of ^ to be explicit.

These rules would be paired with either a lower priority DENY rule...

However, the NPEP states that DENY rules are not supported.

Yeah I can see how that's confusing -- I hope long-term it's clearer when we provide the API but, I'll try to explain and I'm open to suggestions on how to phrase this so it's easier to understand for other readers:

Quick Note, API details will be affected by what we decide in #144

Roughly speaking, the new API will extend the AdminNetworkPolicyEgressPeer by adding a new selector. AdminNetworkPolicyEgressPeer is specified inside the AdminNetworkPolicyEgressRule struct (in the repeated field To).

When we say DENY rules are not supported, more specifically what we're saying is: If a AdminNetworkPolicyEgressRule has Action: "Deny", then none of the AdminNetworkPolicyEgressPeers in that rule can specify FQDN Selectors.

You can however, specify another AdminNetworkPolicyEgressRule with Action: "Deny" (you can either make another entry in AdminNetworkPolicySpec.Egress or create a separate ANP object) and include other selectors in that rule.

astoycos · 2024-01-08T21:50:36Z

Small poke @rahulkjoshi This seems pretty close!

astoycos

/lgtm
/approve

Thanks for all the hard work @rahulkjoshi!!!

k8s-ci-robot · 2024-01-30T02:43:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: astoycos, rahulkjoshi

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [astoycos]

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 added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 8, 2023

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 8, 2023

k8s-ci-robot requested review from astoycos and danwinship August 8, 2023 21:56

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 8, 2023

Dyanngg reviewed Aug 8, 2023

View reviewed changes

Comment thread npep/npep-133.md Outdated

npinaeva reviewed Aug 14, 2023

View reviewed changes

Comment thread npep/npep-133.md Outdated

Comment thread npep/npep-133.md Outdated

astoycos reviewed Aug 15, 2023

View reviewed changes

Comment thread npep/npep-133.md Outdated

Comment thread npep/npep-133.md Outdated

Comment thread npep/npep-133.md Outdated

Comment thread npep/npep-133.md Outdated

Comment thread npep/npep-133.md Outdated

rahulkjoshi closed this Aug 15, 2023

rahulkjoshi deleted the master branch August 15, 2023 16:31

rahulkjoshi restored the master branch August 15, 2023 16:52

rahulkjoshi reopened this Aug 15, 2023

k8s-ci-robot requested a review from shaneutt August 29, 2023 16:07

danwinship reviewed Sep 6, 2023

View reviewed changes

Comment thread npep/npep-133.md Outdated

Comment thread npep/npep-133.md Outdated

Comment thread npep/npep-133.md Outdated

Comment thread npep/npep-133.md Outdated

Comment thread npep/npep-133.md Outdated

rahulkjoshi force-pushed the master branch from cca1cf0 to 12eceea Compare September 25, 2023 17:30

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 25, 2023

rahulkjoshi force-pushed the master branch from 12eceea to 5e0ca8a Compare September 25, 2023 21:07

rahulkjoshi force-pushed the master branch from 5e0ca8a to f0dfa80 Compare October 24, 2023 15:44

npinaeva reviewed Oct 25, 2023

View reviewed changes

Comment thread npep/npep-133.md Outdated

danwinship reviewed Oct 25, 2023

View reviewed changes

Comment thread npep/npep-133.md Outdated

Comment thread npep/npep-133.md Outdated

rahulkjoshi force-pushed the master branch 2 times, most recently from 319cec0 to 5bcb5b5 Compare October 30, 2023 20:45

Dyanngg reviewed Nov 23, 2023

View reviewed changes

Comment thread npep/npep-133.md

astoycos requested a review from danwinship November 28, 2023 15:23

astoycos requested a review from npinaeva November 28, 2023 15:23

astoycos suggested changes Nov 28, 2023

View reviewed changes

rahulkjoshi force-pushed the master branch 3 times, most recently from eaf7f9a to afdec78 Compare November 30, 2023 20:10

danwinship reviewed Dec 1, 2023

View reviewed changes

Comment thread npeps/npep-133.md Outdated

Comment thread npeps/npep-133.md Outdated

Comment thread npeps/npep-133.md

Comment thread npeps/npep-133.md Outdated

rahulkjoshi force-pushed the master branch 2 times, most recently from dacf43d to 970076e Compare December 4, 2023 19:43

astoycos requested a review from danwinship December 5, 2023 17:33

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 5, 2023

k8s-ci-robot assigned danwinship Dec 5, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 5, 2023

danehans reviewed Dec 5, 2023

View reviewed changes

rahulkjoshi force-pushed the master branch from 970076e to 88bd57d Compare December 5, 2023 21:58

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 5, 2023

rahulkjoshi force-pushed the master branch from 88bd57d to 021317f Compare December 5, 2023 21:59

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2023

Initial FQDN Selector NPEP with User stories

2ef73d4

rahulkjoshi force-pushed the master branch from 021317f to 2ef73d4 Compare January 29, 2024 23:09

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2024

k8s-ci-robot assigned astoycos Jan 30, 2024

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 30, 2024

astoycos approved these changes Jan 30, 2024

View reviewed changes

k8s-ci-robot merged commit 9967e86 into kubernetes-sigs:main Jan 30, 2024

rahulkjoshi deleted the master branch February 1, 2024 16:00

		resolve to. Keeping up with changing IP addresses is a maintenance burden, and
		hampers the readability of the network policies.

Uh oh!

Conversation

rahulkjoshi commented Aug 8, 2023

Uh oh!

netlify Bot commented Aug 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

k8s-ci-robot commented Aug 8, 2023

Uh oh!

Uh oh!

npinaeva left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

astoycos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rahulkjoshi commented Aug 15, 2023

Uh oh!

shaneutt commented Aug 29, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Dyanngg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

astoycos commented Nov 28, 2023

Uh oh!

astoycos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danwinship commented Dec 5, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astoycos commented Jan 8, 2024

Uh oh!

astoycos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

netlify Bot commented Aug 8, 2023 •

edited

Loading

astoycos left a comment •

edited

Loading