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

docs: Add Port Range Information #33389

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

Conversation

nathanjsweet
Copy link
Member

@nathanjsweet nathanjsweet commented Jun 25, 2024

- SCTP and Port Ranges are now supported, both are removed from the unsupported features table.

  • DNS rules and L7 rules do not support port ranges yet. Notes are added to call attention to this.
  • Add a Port Range example.

Edit:

  • SCTP support will be addressed in a different PR.

@nathanjsweet nathanjsweet added the release-note/misc This PR makes changes that have no direct user impact. label Jun 25, 2024
@nathanjsweet nathanjsweet requested review from a team as code owners June 25, 2024 16:16
@nathanjsweet nathanjsweet added the release-blocker/1.16 This issue will prevent the release of the next version of Cilium. label Jun 25, 2024
@nathanjsweet nathanjsweet force-pushed the pr/nathanjsweet/port-range-documentation-updates branch from 08ac957 to f183229 Compare June 25, 2024 16:18
Documentation/network/kubernetes/policy.rst Show resolved Hide resolved
Documentation/security/policy/language.rst Outdated Show resolved Hide resolved
examples/policies/l4/l4_port_range.yaml Show resolved Hide resolved
Documentation/security/policy/language.rst Outdated Show resolved Hide resolved
Documentation/security/policy/language.rst Outdated Show resolved Hide resolved
Documentation/security/policy/language.rst Show resolved Hide resolved
@nathanjsweet nathanjsweet force-pushed the pr/nathanjsweet/port-range-documentation-updates branch from f183229 to 6df75d8 Compare June 26, 2024 17:02
@nathanjsweet nathanjsweet force-pushed the pr/nathanjsweet/port-range-documentation-updates branch from 6df75d8 to a46a3ad Compare June 27, 2024 20:35
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

I think we should consolidate the restrictions around port ranges into the section where port ranges are introduced and described, and also avoid using notes so much (see this guidance in the docs).

Documentation/security/policy/language.rst Outdated Show resolved Hide resolved
Documentation/security/policy/language.rst Show resolved Hide resolved
Documentation/security/policy/language.rst Outdated Show resolved Hide resolved
    - Port Ranges are now supported so it is removed from
    the unsupported features table.
    - DNS rules and L7 rules do not support port ranges yet.
    Notes are added to call attention to this.
    - Add a Port Range example.

Signed-off-by: Nate Sweet <[email protected]>
@nathanjsweet nathanjsweet force-pushed the pr/nathanjsweet/port-range-documentation-updates branch from a46a3ad to 8d73d50 Compare June 28, 2024 16:06
// parsed as a single uint16. In the future, this field may support
// ranges in the form "1024-2048"
// Port can be an L4 port number, or a name in the form of "http"
// or "http-8080". EndPort is ignored if Port is a named port.
Copy link
Contributor

@tommyp1ckles tommyp1ckles Jun 28, 2024

Choose a reason for hiding this comment

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

Non blocking question: Do we really want to continuing maintaining a mirror of this k8s API struct in docs, feels like it will be easy for this to get out of date easily.

Copy link
Member

Choose a reason for hiding this comment

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

I guess it's a balance. We don't change the API super often. Those structs are ultimately the source of truth for how the API behaves though. Perhaps in an ideal world we'd have docs tooling to automatically embed the latest API struct definitions in here rather than manually updating them.

Other than that, we can either copy the text over into the docs or we can copy the text plus definitions over, either way we have the question of how to make sure the docs describe the semantics of the API. I don't know if it makes a significant difference between the two.

@joestringer
Copy link
Member

/test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-blocker/1.16 This issue will prevent the release of the next version of Cilium. release-note/misc This PR makes changes that have no direct user impact.
Projects
Status: Active
Development

Successfully merging this pull request may close these issues.

None yet

3 participants