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

Disallow empty port list for ANP peers. #250

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

npinaeva
Copy link
Member

@npinaeva npinaeva commented Oct 9, 2024

Based on discussion #247
Zero-length port list is different from the nil port list, and it means do not match any port, which makes that peer not matching any traffic.
This is confusing and is more of an accident than an intention.
Closes #247

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 9, 2024
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 9, 2024
Copy link

netlify bot commented Oct 9, 2024

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

Name Link
🔨 Latest commit a14d924
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-network-policy-api/deploys/6707e03819885d00083a48b4
😎 Deploy Preview https://deploy-preview-250--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.

@aojea
Copy link
Contributor

aojea commented Oct 9, 2024

@danwinship don't you bump the versions for these changes? v1alpha2?

@danwinship
Copy link
Contributor

/approve

@danwinship don't you bump the versions for these changes? v1alpha2?

pfft. YOLO!

Yeah, it's technically an API break, but it's alpha. I guess people who are shipping this alpha API to customers (meaningful glance) will have to decide whether or not to ship the break.

(leaving un-lgtm'ed to give others a chance to comment)

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, npinaeva

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 Oct 9, 2024
@npinaeva
Copy link
Member Author

/retest

@npinaeva
Copy link
Member Author

had to rebase to fix CI

@aojea
Copy link
Contributor

aojea commented Oct 10, 2024

I see, ok, let me know when you have planned a release to update the network policies,

@aojea
Copy link
Contributor

aojea commented Oct 10, 2024

we may want to do a release before kubecon, and then freeze there, otherwise we loose credibility too

@aojea
Copy link
Contributor

aojea commented Oct 10, 2024

we can break between alpha minors

@danwinship
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 15, 2024
@k8s-ci-robot k8s-ci-robot merged commit dfafbc5 into kubernetes-sigs:main Oct 15, 2024
10 checks passed
@npinaeva npinaeva deleted the set-min-ports branch October 15, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] AdminNetworkPolicyIngressRule.Ports has two nil values?
4 participants