-
Notifications
You must be signed in to change notification settings - Fork 39
CNP: update docs to use CNP instead of ANP and BANP #333
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
Conversation
✅ Deploy Preview for kubernetes-sigs-network-policy-api ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
bowei
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Don't we write a new blog post vs update the old ones?
site-src/api-overview.md
Outdated
| cluster](user-stories.md#story-5-cluster-wide-default-guardrails). | ||
|
|
||
| ### AdminNetworkPolicy Actions | ||
| ### ClusterNetworkPolicy Actions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest just leave this as ### Actions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I edited all headers to remote the CNP from it, it probably was only important when we had ANP vs BANP
|
#328 there is another blog post from @frozenprocess ? Ah I see comment on that one already: #328 (comment) |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
I am not sure what you mean, I did write a new blog post |
|
@bowei will you have some time this week to give it another look? |
Dyanngg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the work put in this API evolution @npinaeva! Antrea is interested in picking up ClusterNetworkPolicy and implement it once the API is ready
|
|
||
| - **Pass**: Skips all further ClusterNetworkPolicy rules in the | ||
| current tier for the selected traffic, and passes | ||
| evaluation to the next tier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically 'NetworkPolicy' is not the "next tier", not sure if we want to call it out here. I know that we have the diagram above which illustrates the policy evaluation model, but I don't want people to think that once we're done with "Admin" tier rules we move on to the "Baseline" tier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be explained either way (it's based on the definition of terms etc), but we should be consistent across all the docs to avoid confusion. Either the notion of tiers is just a convenient way to refer 1) to Admin -> NP -> Baseline or 2) we are more strict and say only Admin and Baseline are tiers.
I think in the code comments, it is closer to 1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, I think this part is confusing because I didn't update the tiers section properly, let me update it with the API doc
| BaselineAdminNetworkPolicies. | ||
| or Deny rules. For example, intra-namespace traffic management can be delegated to namespace | ||
| admins explicitly with the use of `Pass` rules. More specifically traffic selected by a `Pass` rule | ||
| will skip any lower precedence `Admin` tier rules and proceed to be evaluated by `NetworkPolicy` and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this feels like a bit of a run-on sentence for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like chopping it at the i.e. and just making that another sentence would suffice?
| - npinaeva | ||
| --- | ||
|
|
||
| # ClusterNetworkPolicy or what happened to the AdminNetworkPolicy and BaselineAdminNetworkPolicy? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest to make it more direct so people understand the change.
# API update for v1alpha2: `ClusterNetworkPolicy` replaces `AdminNetworkPolicy` and `BaselineAdminNetworkPolicy`
We have merged `v1alpha1.AdminNetworkPolicy` and `v1alpha1.BaselineAdminNetworkPolicy` into a single API in `v1alpha2.ClusterNetworkPolicy`.
(put the rest of your discussion below)
bowei
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some specific comments.
Note the comment on the diagram.
Generally, looks ok
|
|
||
|  | ||
|
|
||
| The model stays the same with the new API, the only difference is that evaluation order is now defined by the `priority` field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean tier instead of priority?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yes, good catch
| Other changes bring the functionality of `BaselineAdminNetworkPolicy` to parity with `AdminNetworkPolicy`. This includes: | ||
| - Allowing multiple `ClusterNetworkPolicy` resources with `tier=Baseline` by using the same `priority` field as for `tier=Admin`. | ||
| - Supporting `Pass` action in `ClusterNetworkPolicy` with `tier=Baseline` to allow skipping all further rules in the `Baseline` tier. | ||
| - Supporting `domainNames` matching for `egress` rules in `ClusterNetworkPolicy` with `tier=Baseline`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is experimental channel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean I should add a note saying that domainNames is an experimental feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should have two priority arrows and number 1 - 1000 on each. This diagram makes it seem like the priority # extends across all of the tiers, which it does not. Also, the NetworkPolicy tier does not have priority numbers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, I am thinking if just leaving Evaluated first and Evaluated last is good enough? Or maybe replace Priority with "evaluation order"?
I don't think adding detail on the actual priority field was a point of this picture
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SG
|
|
||
| - **Pass**: Skips all further ClusterNetworkPolicy rules in the | ||
| current tier for the selected traffic, and passes | ||
| evaluation to the next tier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be explained either way (it's based on the definition of terms etc), but we should be consistent across all the docs to avoid confusion. Either the notion of tiers is just a convenient way to refer 1) to Admin -> NP -> Baseline or 2) we are more strict and say only Admin and Baseline are tiers.
I think in the code comments, it is closer to 1)
| BaselineAdminNetworkPolicies. | ||
| or Deny rules. For example, intra-namespace traffic management can be delegated to namespace | ||
| admins explicitly with the use of `Pass` rules. More specifically traffic selected by a `Pass` rule | ||
| will skip any lower precedence `Admin` tier rules and proceed to be evaluated by `NetworkPolicy` and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like chopping it at the i.e. and just making that another sentence would suffice?
|
I updated install docs as discussed in the last meeting, see the last commit |
Add a blog post with short description of changes, some migration examples and future plans. Signed-off-by: Nadia Pinaeva <[email protected]>
We don't release install.yaml artifact anymore, simply use `crd/standard` files from the right tag/main branch. Add mkdocs feature that enables "copy to clipboard" button on the bash commands. Signed-off-by: Nadia Pinaeva <[email protected]>
|
/lgtm (I don't know why but I can't seem to resolve any comment threads) |
Add a blog post with short description of changes, some migration examples and future plans.
I have also added a new version of the CNP model made in drawio, and the source file should be import-able to make future changes, but lmk if you have better ideas to store images
Fixes a part of #312