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

Improve documentation for Antrea-native policy #3512

Merged
merged 1 commit into from
Jun 10, 2022

Conversation

Dyanngg
Copy link
Contributor

@Dyanngg Dyanngg commented Mar 23, 2022

Changes include:

  • Adding more examples and usage of the namespaces match: self field.
  • Refining the strict-ns-isolation sample yaml so that it does not select core-dns Pods in appliedTo.
  • Reorganized sections for better readability.
  • Adding note about indeterministic behavior for conflicting rules.
  • Addressing typos and grammar issues.

Signed-off-by: Yang Ding dingyang@vmware.com

@Dyanngg Dyanngg added the kind/documentation Categorizes issue or PR as related to a documentation. label Mar 23, 2022
@Dyanngg
Copy link
Contributor Author

Dyanngg commented Mar 23, 2022

/skip-all

@codecov-commenter
Copy link

codecov-commenter commented Mar 24, 2022

Codecov Report

Merging #3512 (52871b1) into main (9677ccd) will decrease coverage by 8.19%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3512      +/-   ##
==========================================
- Coverage   64.55%   56.35%   -8.20%     
==========================================
  Files         278      392     +114     
  Lines       39513    55091   +15578     
==========================================
+ Hits        25507    31046    +5539     
- Misses      12022    21639    +9617     
- Partials     1984     2406     +422     
Flag Coverage Δ
integration-tests 38.02% <ø> (?)
kind-e2e-tests 50.73% <ø> (-1.45%) ⬇️
unit-tests 43.84% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...g/agent/apiserver/handlers/featuregates/handler.go 4.54% <0.00%> (-77.28%) ⬇️
...kg/apiserver/registry/system/supportbundle/rest.go 22.17% <0.00%> (-50.44%) ⬇️
pkg/support/dump.go 7.90% <0.00%> (-49.16%) ⬇️
pkg/support/dump_others.go 0.00% <0.00%> (-44.74%) ⬇️
...gregator/apiserver/handlers/flowrecords/handler.go 1.44% <0.00%> (-37.69%) ⬇️
...egator/apiserver/handlers/recordmetrics/handler.go 4.00% <0.00%> (-36.00%) ⬇️
...g/agent/apiserver/handlers/addressgroup/handler.go 5.00% <0.00%> (-35.00%) ⬇️
...agent/apiserver/handlers/appliedtogroup/handler.go 5.00% <0.00%> (-35.00%) ⬇️
pkg/apiserver/handlers/loglevel/handler.go 6.25% <0.00%> (-31.25%) ⬇️
pkg/ovs/ovsctl/ofctl.go 19.17% <0.00%> (-16.44%) ⬇️
... and 136 more

docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Show resolved Hide resolved
@Dyanngg
Copy link
Contributor Author

Dyanngg commented Mar 25, 2022

/skip-all

@Dyanngg Dyanngg force-pushed the enhance-np-doc branch 2 times, most recently from bb33f0d to 7b9a7f2 Compare March 29, 2022 21:21
@Dyanngg Dyanngg requested a review from qiyueyao March 29, 2022 21:21
@Dyanngg
Copy link
Contributor Author

Dyanngg commented Mar 29, 2022

/skip-all @antoninbas @tnqn could you take another look to see if there are additional comments? Thanks

@Dyanngg
Copy link
Contributor Author

Dyanngg commented Mar 29, 2022

/skip-all

@Dyanngg
Copy link
Contributor Author

Dyanngg commented Apr 28, 2022

@antoninbas @tnqn @jianjuns Could you please give this a review? Want to merge this eow. Thanks!

docs/antrea-network-policy.md Outdated Show resolved Hide resolved
Comment on lines 1331 to 1366
and up to 50,000 unique priorities at rule level, across all Tiers except for
the "baseline" Tier. For any two Antrea-native policy rules, their rule level
priorities are only considered equal if their policy objects share the same Tier
and have the same policy priority, plus the rules themselves are of the same rule
priority (rule priority is the sequence number of the rule within the policy's
ingress or egress section).
- For the "baseline" Tier, the max supported unique priorities (at rule level) is 150.
- If there are multiple Antrea-native policy rules created at the same rule-level
priority (same policy Tier, policy priority and rule priority), who happen to select
overlapping traffic patterns but have conflicting rule actions (e.g.`Allow`v.s.`Deny`),
the behavior of such traffic will be undeterministic. For example, consider two
AntreaNetworkPolicies created in the same Namespace and Tier with the same policy
priority. The first policy applies to all `app=web` Pods in the Namespace and has only
one ingress rule to `Deny` all traffic from `role=dev` Pods. The other policy also
applies to all `app=web` Pods in the Namespace and has only one ingress rule, which is
to `Allow` all traffic from `app=client` Pods. Those two ingress rules might not always
conflict, but in case a Pod with both `app=client` and `role=dev` labels initiate
traffic towards the web Pods in the Namespace, both rules will be matched at the same
priority with conflicting actions. It will be the policy writer's responsibility to
identify such ambiguities in rule definitions and avoid potential undeterministic
rule enforcement results. In general, it is recommended that rules of conflicting
actions are always created at different rule-level priorities.
Copy link
Contributor

Choose a reason for hiding this comment

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

honestly, I feel like we should just discourage users from defining policies at the same priority if they apply to the same Pods. Having the notion of rule-level priority across multiple policies is a bit confusing as it is not based on a user-provided priority value, but on the rule position in the list of rules.
what do you think @jianjuns @tnqn ?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can first say the behaviors are not deterministic / predictable if two policies with the same priority apply to the same Pod, if there are both allow and deny rules. Then we can explain further the details like rule priority (assuming most readers will not read it)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hoping that with this paragraph it discourages people from defining policies at the same priority if they apply to the same Pods. From Antrea perspective it is hard to enforce this though.

@Dyanngg
Copy link
Contributor Author

Dyanngg commented Apr 29, 2022

/skip-all

@Dyanngg
Copy link
Contributor Author

Dyanngg commented Apr 29, 2022

/skip-all

@Dyanngg
Copy link
Contributor Author

Dyanngg commented May 2, 2022

/skip-all

@Dyanngg Dyanngg requested a review from antoninbas May 11, 2022 22:27
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
Comment on lines 1331 to 1366
rule enforcement results. In general, it is recommended that rules of conflicting
actions are always created at different rule-level priorities.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is not strong enough. IMO, it should be:

In general, it is recommended not to create rules with conflicting actions in policy resources with the same priority, if these rules may apply to the same traffic, as the behavior could be undeterministic.

And actually I think this should be the first sentence of that bullet list item (and in bold), and you can get into the details after if needed, as Jianjun suggested. The whole concept of rule level priority for rules which are defined across policies with the same priority, is an implementation detail as far as I am concerned. I don't think it is really needs to be a part of the API contract. Inserting a new rule in one of the policies could change rule enforcement order, in ways which are surprising to users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

A few minor comments.

docs/antrea-network-policy.md Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
@Dyanngg
Copy link
Contributor Author

Dyanngg commented Jun 8, 2022

/skip-all

Copy link
Contributor

@jianjuns jianjuns 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 you missed the last review comments from Antonin. @Dyanngg

docs/antrea-network-policy.md Outdated Show resolved Hide resolved
Signed-off-by: Yang Ding <dingyang@vmware.com>
@Dyanngg
Copy link
Contributor Author

Dyanngg commented Jun 10, 2022

Addressed all comments so far /skip-all

@jianjuns jianjuns merged commit 8eedd9f into antrea-io:main Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/documentation Categorizes issue or PR as related to a documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants