Skip to content

Adds cudn docs#85165

Merged
stevsmit merged 1 commit into
openshift:mainfrom
stevsmit:12599-osdocs-cudn-proc
Feb 17, 2025
Merged

Adds cudn docs#85165
stevsmit merged 1 commit into
openshift:mainfrom
stevsmit:12599-osdocs-cudn-proc

Conversation

@stevsmit

@stevsmit stevsmit commented Nov 19, 2024

Copy link
Copy Markdown
Member

@openshift-ci openshift-ci Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 19, 2024
Comment thread modules/nw-cudn-cr.adoc Outdated
@ocpdocs-previewbot

ocpdocs-previewbot commented Nov 19, 2024

Copy link
Copy Markdown

@tssurya

tssurya commented Nov 22, 2024

Copy link
Copy Markdown
Contributor

/assign @ormergi

@ormergi ormergi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi, thanks for adding these docs, please see my comments bellow and in-line ones 🙂

  1. Regarding best practices
    I think we should add text saying the ClusterUserDefiendNetwork CRD is targeted for cluster-admins.
    And it should not be granted for non-admin users blindly because if used wrongly, it could result in security issues (e.g.: create CR in default namespace, connect multiple namespaces networks).
    Or, in with the future Localnet support, cause disruptions or break the cluster network.

In addition, the NamespaceSelector should not point to default or openshifrt-* namespaces, similar to UDN CRD, for the same reasones.

  1. Regarding NamespaceSelector
    Note about the NamespaceSelector attribute, since its type is the standard k8s MatchLabel selector, it derive the exact behavior:
    In case matchExpression is used, given the following spec:
matchExpressions:
- key: kubernetes.io/metadata.name
  operator: In
  values: ["red", "blue"]

Result in provisioning namespaces who has kubernetes.io/metadata.name=red OR kubernetes.io/metadata.name=blue labels (i.e.: one of).

In contrast, the matchLabels acts diffrently in the sense the terms are "AND"'ed, for example:
Given the following spec:

matchLables: 
- "red":""
- "blue":""

Result in provisioning namespaces who has red="" AND blue="" labels.

Should we add some text for the above or keep it implicit as the current state?

  1. Regarding other Layer2 & Layer3 optional fields, should we add some text for it ot add reference to the table we have in UserDefienenNetwork doc?

Comment thread modules/nw-cudn-cr.adoc
Comment thread modules/nw-cudn-cr.adoc Outdated
Comment thread modules/nw-cudn-cr.adoc Outdated
Comment thread modules/nw-cudn-cr.adoc
Comment thread modules/nw-cudn-cr.adoc Outdated
Comment thread modules/nw-cudn-cr.adoc
@stevsmit stevsmit force-pushed the 12599-osdocs-cudn-proc branch from b753f00 to f02bb1a Compare December 2, 2024 18:37

@ormergi ormergi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for the changes, I likes the explanations on the namesapce selectors :)

Please see my inline comments.

Is there best-practices doc for CUDN objects?
If so, I suggest to add text for the following topics:

  • CUDN targeted for admins (as in line 14
  • namespace-selector should not specify default or openshift-* namespace.
    In addition, I think specifying empty label-selecor result in selecting all namespaces, we may need to say something about that.
    @tssurya @npinaeva WDYT about the empty selector part?

Comment thread modules/nw-cudn-cr.adoc Outdated
Comment thread modules/nw-cudn-cr.adoc Outdated
@stevsmit stevsmit force-pushed the 12599-osdocs-cudn-proc branch from f02bb1a to 3627131 Compare December 5, 2024 17:13

@ormergi ormergi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for the changes, LGTM

@stevsmit stevsmit force-pushed the 12599-osdocs-cudn-proc branch from 3627131 to c58dcfb Compare December 6, 2024 20:53
@stevsmit stevsmit added this to the Planned for 4.18 GA milestone Dec 6, 2024
Comment thread modules/nw-cudn-cr.adoc
@stevsmit stevsmit force-pushed the 12599-osdocs-cudn-proc branch from c58dcfb to b1c5526 Compare December 12, 2024 21:15
@stevsmit stevsmit force-pushed the 12599-osdocs-cudn-proc branch from b1c5526 to 38b3af2 Compare December 19, 2024 21:26
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 19, 2024
@stevsmit stevsmit force-pushed the 12599-osdocs-cudn-proc branch 2 times, most recently from 489198b to 6b938fe Compare December 20, 2024 19:05
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 20, 2024
@stevsmit

stevsmit commented Jan 7, 2025

Copy link
Copy Markdown
Member Author

/retest

@stevsmit stevsmit closed this Jan 7, 2025
@stevsmit stevsmit reopened this Jan 7, 2025
@stevsmit stevsmit closed this Jan 9, 2025
@stevsmit stevsmit reopened this Jan 9, 2025
@stevsmit stevsmit force-pushed the 12599-osdocs-cudn-proc branch 5 times, most recently from 27262e2 to 5dc6705 Compare January 9, 2025 17:32

@lpettyjo lpettyjo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Otherwise, LGTM!

Comment thread modules/nw-cudn-best-practices.adoc Outdated
Comment thread modules/nw-cudn-cr.adoc
@lpettyjo lpettyjo added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-in-progress Signifies that the peer review team is reviewing this PR labels Feb 11, 2025
@stevsmit stevsmit force-pushed the 12599-osdocs-cudn-proc branch from af5af82 to e7935cf Compare February 12, 2025 15:08

@maiqueb maiqueb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caught some stuff.

Comment thread modules/cudn-status-conditions.adoc Outdated
Comment thread modules/cudn-status-conditions.adoc Outdated
Comment thread modules/nw-cudn-cr.adoc Outdated
Comment thread modules/nw-cudn-cr.adoc Outdated
Comment thread modules/nw-cudn-cr.adoc Outdated
Comment thread modules/nw-cudn-cr.adoc
Comment thread modules/nw-cudn-cr.adoc Outdated
Comment thread modules/nw-cudn-cr.adoc Outdated
Comment thread modules/nw-cudn-cr.adoc Outdated
@stevsmit stevsmit force-pushed the 12599-osdocs-cudn-proc branch 4 times, most recently from a27555b to d880e72 Compare February 12, 2025 20:55

@maiqueb maiqueb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thank you !

Comment thread modules/nw-cudn-best-practices.adoc Outdated
Comment thread modules/nw-cudn-best-practices.adoc
@stevsmit stevsmit force-pushed the 12599-osdocs-cudn-proc branch 2 times, most recently from 9065f7b to 0f9ad3e Compare February 13, 2025 15:10

@ormergi ormergi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thank you, LGTM

@stevsmit stevsmit force-pushed the 12599-osdocs-cudn-proc branch 2 times, most recently from cc05282 to da4bcd8 Compare February 13, 2025 15:48
Comment thread modules/nw-cudn-cr.adoc Outdated
Comment thread modules/cudn-status-conditions.adoc
@stevsmit stevsmit force-pushed the 12599-osdocs-cudn-proc branch from da4bcd8 to 51a5969 Compare February 14, 2025 13:42
@openshift-ci

openshift-ci Bot commented Feb 14, 2025

Copy link
Copy Markdown

@stevsmit: all tests passed!

Full PR test history. Your PR dashboard.

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

@meinali566

Copy link
Copy Markdown

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Feb 17, 2025
@stevsmit stevsmit merged commit 8e5519f into openshift:main Feb 17, 2025
@stevsmit

Copy link
Copy Markdown
Member Author

/cherry-pick enterprise-4.18

@openshift-cherrypick-robot

Copy link
Copy Markdown

@stevsmit: new pull request created: #88688

Details

In response to this:

/cherry-pick enterprise-4.18

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.18 lgtm Indicates that a PR is ready to be merged. peer-review-done Signifies that the peer review team has reviewed this PR size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.