v0.1.6 api review #235

astoycos · 2024-06-18T16:01:16Z

What type of PR is this?
/kind api-change

What this PR does / why we need it:
This PR is a diff of /apis from v0.1.6 (main branch) to v0.1.1 (release-v0.1.1 branch).

Note: This PR is purely to facilitate review, it is not intended to merge.

Note2: This does not include FQDN support from from @rahulkjoshi in #233, which will come in our next 0.2.0 release

aojea · 2024-07-02T21:51:03Z

apis/v1alpha1/adminnetworkpolicy_types.go

+	// implementation can apply any of the matching policies to the connection, and
+	// there is no way for the user to reliably determine which one it will choose.


There are some implementations that are able to do this deterministically, I did it sorting by name here https://github.com/kubernetes-sigs/kube-network-policies/blob/db089bc6ed6c17a654ef0ea3f3a93d9122041c4e/pkg/networkpolicy/networkpolicyapi.go#L189-L196

I think we can guide users, and say that from the API point of view is difficult to define the exact behavior as it may depend on implementation details, but they should refer to the project implementing this API to understand what is the expected behavior

This was hashed out in #216 and #229.

Individual implementations may do something deterministic, but they are not required to. The point of the explanation here is that if you want an "expected behavior", then you have to avoid having two ANPs with the same priority that both match the same packet. (But also, if you do have two ANPs with the same priority that both match the same packet, the "undefined-ness" is much more restricted than it was before; the implementation must obey one of the matching policies. It can't just "always deny", or ignore both policies, etc.)

I was trying to point out that we can guide the users to ask their implementations about the behaviors, the way is worded now seems that "is impossible to know", when it really means "it is impossible to define a general behavior, but implementations may have a deterministic one"

What it really means is "don't do that".

The only scenario that is not well-defined is when you have two ANPs at the same priority that attempt to take different actions on the same packet. No matter what the implementation does, it will fail to obey the administrator's intent, because the administrator has requested that it do two contradictory things. Don't do that. Don't ask which policy it's actually going to use in that case, just fix your policies.

as a developer is clear, but as a user you can do it if the implementation you are using is deterministic

aojea · 2024-07-02T21:53:04Z

apis/v1alpha1/adminnetworkpolicy_types.go

@@ -130,11 +162,16 @@ type AdminNetworkPolicyIngressRule struct {
 // AdminNetworkPolicyEgressRule describes an action to take on a particular
 // set of traffic originating from pods selected by a AdminNetworkPolicy's
 // Subject field.
+// <network-policy-api:experimental:validation>
+// +kubebuilder:validation:XValidation:rule="!(self.to.exists(peer, has(peer.networks) || has(peer.nodes)) && has(self.ports) && self.ports.exists(port, has(port.namedPort)))",message="networks/nodes peer cannot be set with namedPorts since there are no namedPorts for networks/nodes"


@thockin I think that you mention this to me at one point, about having a way to unit test or validate these expressions

aojea · 2024-07-02T21:56:06Z

apis/v1alpha1/baselineadminnetworkpolicy_types.go

+	//
+	// Support: Core
+	//
+	To []AdminNetworkPolicyEgressPeer `json:"to"`


why now the differentiation between Ingress and Egress?

The network selector was added to egress (egress controls by CIDR block). We didn't include Ingress yet due to the difficulties reliably determining the source IP of ingress traffic.

https://network-policy-api.sigs.k8s.io/npeps/npep-126-egress-traffic-control/#non-goals

aojea · 2024-07-02T21:58:38Z

apis/v1alpha1/shared_types.go

-	// To be selected a Namespace must have all of the labels defined in SameLabels,
-	// AND they must all have the same value as the subject of this policy.
-	// If Samelabels is Empty then nothing is selected.
+	Namespaces *metav1.LabelSelector `json:"namespaces,omitempty"`


much simpler

k8s-triage-robot · 2024-10-16T13:59:52Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

tssurya · 2024-10-21T09:04:32Z

@astoycos should I help with this? I know last time we spoke we wanted to setup a meeting with going through the PR and hashing out API changes?

tssurya · 2024-10-21T09:04:46Z

/remove-lifecycle stale

Signed-off-by: astoycos <[email protected]>

k8s-ci-robot · 2024-10-29T03:33:10Z

@astoycos: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-network-policy-api-verify	`b5ff96c`	link	true	`/test pull-network-policy-api-verify`
pull-network-policy-api-crd-e2e	`b5ff96c`	link	true	`/test pull-network-policy-api-crd-e2e`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

astoycos · 2024-10-29T03:34:15Z

/reopen

k8s-ci-robot · 2024-10-29T03:34:19Z

@astoycos: Failed to re-open PR: state cannot be changed. The review-v0.1.6 branch was force-pushed or recreated.

In response to this:

/reopen

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-sigs/prow repository.

astoycos · 2024-10-29T03:38:20Z

For some reason this won't let me re-open now 🤔

astoycos · 2024-10-29T04:15:47Z

/reopen

k8s-ci-robot · 2024-10-29T04:15:51Z

@astoycos: Failed to re-open PR: state cannot be changed. The review-v0.1.6 branch was force-pushed or recreated.

In response to this:

/reopen

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-sigs/prow repository.

astoycos · 2024-10-29T04:19:09Z

Opened #260 to replace this 🤔 Unless someone can figure out how to re-open we can continue the discussion there

mattfenwick added 30 commits January 7, 2021 10:37

initial commit

30de3d9

cleanup; working end-to-end example

6dc8f56

add traffic query command

0d613f1

read policies from file(s)

48d3916

add connectivity probe

af5bbe3

add example netpols

b53abd4

add examples in readme

8ebbab7

add ports, protocols to probe

700cc13

add queryTargets command

696530e

rearrange functions

84fcb6b

try out revamped model/builder/explainer

ea6c571

rearrange examples

4cad52e

fix readme, add comment

59226ea

Merge branch 'master' into dedupe-model

5536295

get most testcases passing

4140100

minor fixes

d845afa

renamings

adef648

restore IPBlock test case

86a718b

expand test coverage

5635b56

extend test coverage

d4775c2

add explainer tests

ed21625

fix up explainer tests

13ac6a1

fix up README examples

7973d03

sort pod names

d0905ff

indent output for clarity

f263d7d

remove unused field

2f4fc11

start generator

4ed7262

create generator and comparator

0c73549

fix cidrs, compare real to synthetic

bf90c07

add ip test cases

f946d65

k8s-ci-robot assigned danwinship and thockin Jun 27, 2024

astoycos changed the title ~~v0.1.6 api review~~ v0.2.0 api review Jul 2, 2024

astoycos force-pushed the review-v0.1.6 branch from 6a48e8b to 65d12fe Compare July 2, 2024 15:41

astoycos changed the title ~~v0.2.0 api review~~ v0.1.6 api review Jul 2, 2024

astoycos marked this pull request as ready for review July 2, 2024 16:21

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 Jul 2, 2024

aojea reviewed Jul 2, 2024

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 16, 2024

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 21, 2024

tssurya mentioned this pull request Oct 21, 2024

[RELEASE] Cut v1Beta1 of the AdminNetwork policy API #172

Open

v0.1.6 api review

b5ff96c

Signed-off-by: astoycos <[email protected]>

astoycos closed this Oct 29, 2024

astoycos force-pushed the review-v0.1.6 branch from 65d12fe to b5ff96c Compare October 29, 2024 03:32

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 29, 2024

astoycos mentioned this pull request Oct 29, 2024

v0.1.6-api review #260

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v0.1.6 api review #235

v0.1.6 api review #235

astoycos commented Jun 18, 2024 •

edited

Loading

aojea Jul 2, 2024

danwinship Jul 8, 2024

aojea Jul 8, 2024

danwinship Jul 9, 2024

aojea Jul 18, 2024 •

edited

Loading

aojea Jul 2, 2024

aojea Jul 2, 2024

rahulkjoshi Jul 3, 2024

aojea Jul 2, 2024

k8s-triage-robot commented Oct 16, 2024

tssurya commented Oct 21, 2024

tssurya commented Oct 21, 2024

k8s-ci-robot commented Oct 29, 2024

astoycos commented Oct 29, 2024

k8s-ci-robot commented Oct 29, 2024

astoycos commented Oct 29, 2024

astoycos commented Oct 29, 2024

k8s-ci-robot commented Oct 29, 2024

astoycos commented Oct 29, 2024

		// implementation can apply any of the matching policies to the connection, and
		// there is no way for the user to reliably determine which one it will choose.

v0.1.6 api review #235

v0.1.6 api review #235

Conversation

astoycos commented Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-triage-robot commented Oct 16, 2024

tssurya commented Oct 21, 2024

tssurya commented Oct 21, 2024

k8s-ci-robot commented Oct 29, 2024

astoycos commented Oct 29, 2024

k8s-ci-robot commented Oct 29, 2024

astoycos commented Oct 29, 2024

astoycos commented Oct 29, 2024

k8s-ci-robot commented Oct 29, 2024

astoycos commented Oct 29, 2024

astoycos commented Jun 18, 2024 •

edited

Loading

aojea Jul 18, 2024 •

edited

Loading