Skip to content

Update the Network Policies Page #49390

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

network-charles
Copy link
Contributor

Description

This PR improves the understanding of the to and from selectors in a network policy. It adds an egress example when describing a namespaceSelector and podSelector. The previous explanation only included an ingress example.

Issue

Closes: #45615

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 11, 2025
@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 11, 2025
Copy link

netlify bot commented Jan 11, 2025

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 187a7df
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/67a24b07e73f7c0008e6858f
😎 Deploy Preview https://deploy-preview-49390--kubernetes-io-main-staging.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.

@network-charles
Copy link
Contributor Author

cc @natalisucks and @4rivappa

@danwinship
Copy link
Contributor

The reporter of #45615 talks about "making simpler for developers and students to quickly pick up an example and use it". I think that while they specifically reference this example, that's just because this is one of the only examples in the document. But adding another clause to this example obscures the point that the example is currently trying to make ("be careful of hyphens when using combined namespace-and-pod-selector rules").

I think what #45615 really wants is just more examples in this file in general.

@network-charles
Copy link
Contributor Author

Thanks for the feedback, @danwinship.
Have you seen this issue yet, #48772? It focuses on adding more examples to this file in general.

#45615 specifically said we should add the below.

"Behavior of to and from selectors"
also with egress -to (now it's only shown with ingress -from)

This PR adds the behavior of the "to" behavior.

Line 53 says there is a to/from entry in the example, but that's not true; only an ingress from entry was specified. This aligns with what #45615 was complaining about.

If you can take a second look again, I'll appreciate it. However, if you insist that this is still the wrong PR for this issue, I can close it.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

I am worried about people making assumptions, such as that both ingress and egress are required, or that ingress rules always use from and egress rules always use to.

What can we change to make the concept easier to learn? (We could add a tutorial, but that is a lot of work).

I know that https://kubernetes.io/docs/concepts/services-networking/network-policies/#default-deny-all-egress-traffic makes it clear why to is optional, but: we don't want people to have to read the page several times to understand it.

@network-charles
Copy link
Contributor Author

About the tutorial @sftim, if you or @danwinship can specify exactly what we should include in it, it’ll be easier to work on.

What I have noticed in this network policy page is that, it doesn’t explicitly state that “{}” acts as a wildcard to indicate no restrictions.

@network-charles network-charles force-pushed the network-charles-patch-4 branch from f423ff1 to c984636 Compare January 29, 2025 22:42
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 29, 2025
@danwinship
Copy link
Contributor

I am worried about people making assumptions, such as that ... ingress rules always use from and egress rules always use to.

That's true though. (If we had implemented ingress and egress at the same time we probably would have used something generic like peers in both cases, but ingress came first, and from made sense for the name of the array containing the peers at that time, but then when we added egress later it didn't make sense there so we had to add a second struct which was identical except for having to instead of from.)

@sftim
Copy link
Contributor

sftim commented Feb 3, 2025

About assumptions; here's a NetworkPolicy that uses neither from nor to

---
apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
  name: allow-all-ingress
spec:
  podSelector: {}
  ingress:
  - {}
  policyTypes:
  - Ingress

@sftim
Copy link
Contributor

sftim commented Feb 3, 2025

I think I explained it badly before!

@sftim
Copy link
Contributor

sftim commented Feb 3, 2025

A nice first policy to show might be one that allows all egress but restricts ingress; that's a very common story to tell.

@network-charles
Copy link
Contributor Author

A nice first policy to show might be one that allows all egress but restricts ingress; that's a very common story to tell.

That's a nice suggestion, @sftim. I believe you mean this.

apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
  name: allow-egress-restrict-ingress
  namespace: default
spec:
  podSelector: {} 
  policyTypes:
    - Egress
    - Ingress
  egress:
    - {}
  ingress: [] # Removing this line also works

I can include it at Line 267.

@sftim
Copy link
Contributor

sftim commented Feb 3, 2025

apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
  name: allow-egress-restrict-ingress
  namespace: default
spec:
  podSelector: {} 
  policyTypes:
    - Egress
    - Ingress
  egress:
    - {}
  ingress: [] # Removing this line also works

even better, IMO, is that you either:

  • drop the namespace from that example
  • use an obviously placeholder namespace name, such as example

@network-charles
Copy link
Contributor Author

I think I will drop the namespace entirely. The other examples don't have them.

@network-charles network-charles force-pushed the network-charles-patch-4 branch from c984636 to 187a7df Compare February 4, 2025 17:14
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign salaxander for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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 size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 4, 2025
@network-charles
Copy link
Contributor Author

Let me also know what you think about Lines 213 - 225 on the network policies docs page.

@network-charles
Copy link
Contributor Author

cc @divya-mohan0209

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance Network Policies docs
4 participants