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

SameLabels support for ACNP peer Namespace selection #4537

Merged
merged 7 commits into from
Mar 19, 2024

Conversation

Dyanngg
Copy link
Contributor

@Dyanngg Dyanngg commented Jan 6, 2023

This PR adds the sameLabels field in ACNP peer's namespaces.
The usecase for this field is to allow cluster admins to create ACNPs that
isolate Namespaces based on their label values. For example, if there are
numerous Namespaces in the cluster that has label tier=production and
other Namespaces with label tier=dev, admins can create a single ACNP
that says the production Namespaces can only communicate within
themselves, and same for the dev Namespaces.

@Dyanngg Dyanngg added this to the Antrea v1.11 release milestone Jan 6, 2023
@codecov
Copy link

codecov bot commented Jan 6, 2023

Codecov Report

Merging #4537 (6f55f15) into main (f32f796) will decrease coverage by 5.06%.
The diff coverage is 6.49%.

❗ Current head 6f55f15 differs from pull request most recent head 3c2b7d5. Consider uploading reports for the commit 3c2b7d5 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4537      +/-   ##
==========================================
- Coverage   73.57%   68.51%   -5.06%     
==========================================
  Files         410      403       -7     
  Lines       61105    58816    -2289     
==========================================
- Hits        44956    40297    -4659     
- Misses      13257    15711    +2454     
+ Partials     2892     2808      -84     
Flag Coverage Δ *Carryforward flag
e2e-tests 38.36% <6.49%> (-0.36%) ⬇️
integration-tests 34.29% <ø> (+1.06%) ⬆️ Carriedforward from 4e5fba7
kind-e2e-tests 41.20% <ø> (-7.50%) ⬇️ Carriedforward from 4e5fba7
unit-tests 60.02% <ø> (-4.81%) ⬇️ Carriedforward from 4e5fba7

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
...ntroller/networkpolicy/networkpolicy_controller.go 83.92% <0.00%> (-1.24%) ⬇️
pkg/controller/networkpolicy/validate.go 52.42% <0.00%> (-27.17%) ⬇️
...g/controller/networkpolicy/clusternetworkpolicy.go 54.96% <6.99%> (-19.82%) ⬇️

... and 185 files with indirect coverage changes

@Dyanngg Dyanngg force-pushed the same-labels branch 10 times, most recently from d9adf87 to dbfd891 Compare January 10, 2023 22:06
@Dyanngg Dyanngg changed the title [WIP] SameLabels support for ACNP peer Namespace selection SameLabels support for ACNP peer Namespace selection Jan 10, 2023
@Dyanngg Dyanngg force-pushed the same-labels branch 2 times, most recently from 9eb0b92 to e18f8c6 Compare January 11, 2023 06:15
@Dyanngg
Copy link
Contributor Author

Dyanngg commented Jan 11, 2023

/test-multicluster-e2e

1 similar comment
@Dyanngg
Copy link
Contributor Author

Dyanngg commented Jan 12, 2023

/test-multicluster-e2e

Copy link
Contributor

@qiyueyao qiyueyao left a comment

Choose a reason for hiding this comment

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

I believe SameLabels need to monitor Namespace changes, in addNamespace/updateNamespace.

@Dyanngg
Copy link
Contributor Author

Dyanngg commented Jan 27, 2023

I believe SameLabels need to monitor Namespace changes, in addNamespace/updateNamespace.

Thanks for the heads up. Updated the logic in these functions to better filter the ACNPs that need to be reprocessed

@Dyanngg Dyanngg requested a review from qiyueyao January 27, 2023 19:59
@Dyanngg
Copy link
Contributor Author

Dyanngg commented Mar 2, 2023

@tnqn could you help review this? Hope to merge it by 1.11. Thanks

@Dyanngg
Copy link
Contributor Author

Dyanngg commented Mar 2, 2023

/test-all

@Dyanngg Dyanngg force-pushed the same-labels branch 2 times, most recently from a2da5ad to 70eccc8 Compare March 2, 2024 00:22
@Dyanngg
Copy link
Contributor Author

Dyanngg commented Mar 2, 2024

Any plan to add SameLabels into antrea-network-policy.md doc?

Will do in a separate PR

tnqn
tnqn previously approved these changes Mar 14, 2024
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM, two minor comments.

podIPs map[string][]string
p80, p81, p8080, p8081, p8082, p8085, p6443 int32
nodes map[string]string
selfNamespace *crdv1beta1.PeerNamespaces
Copy link
Member

Choose a reason for hiding this comment

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

This is a "invariant" variable defined for reuse, right? I think it's more clear to initialize its value here, instead of in a function. So do other cases like "p80, p81, p8080, p8081, p8082, p8085, p6443".

Comment on lines 846 to 847
_, err := k.crdClient.CrdV1alpha3().Groups(namespace).Get(context.TODO(), name, metav1.GetOptions{})
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

I had a PR to simplify such code: no need to get a resource first as create will fail with a specific AlreadyExists error anyway.

@tnqn tnqn added the action/release-note Indicates a PR that should be included in release notes. label Mar 14, 2024
Signed-off-by: Dyanngg <[email protected]>
@jsalatiel
Copy link

In which version is this expected to be released?

@tnqn tnqn added this to the Antrea v2.0 release milestone Mar 19, 2024
@tnqn
Copy link
Member

tnqn commented Mar 19, 2024

In which version is this expected to be released?

@jsalatiel It will be in the next release: v2.0

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@tnqn
Copy link
Member

tnqn commented Mar 19, 2024

/test-all

@tnqn tnqn merged commit 59f642a into antrea-io:main Mar 19, 2024
52 of 56 checks passed
@Dyanngg Dyanngg deleted the same-labels branch March 19, 2024 17:57
hongliangl added a commit to hongliangl/antrea that referenced this pull request Apr 2, 2024
In NodeNetworkPolicy e2e tests, we have test the following cases:

- Nodes to Nodes. We deploy two hostNetwork Pods on different Nodes.
- Node to remote Pods. We deploy a hostNetwork Pod on a Node and a
  non-hostNetwork Pod on another Node.

For case of Node to local Pods, we don't test it since the UDP probing
from a non-hostNetwork Pod to the hostNetwork Pod deployed on the same
Node will get a failure. The reason is that the reply packets use the
local Antrea gateway IP as source IP, instead of the local Node IP, which
is the destination IP of the request packets, resulting in the failure
of test Pods initialization.

This PR fixes the e2e test failure by reverting the test Pods
initialization modified by PR antrea-io#4537.

Signed-off-by: Hongliang Liu <[email protected]>
hongliangl added a commit to hongliangl/antrea that referenced this pull request Apr 2, 2024
In NodeNetworkPolicy e2e tests, we have the following cases:

- Nodes to Nodes. We deploy two hostNetwork Pods on different Nodes.
- Node to remote Pods. We deploy a hostNetwork Pod on a Node and a
  non-hostNetwork Pod on another Node.

For the case of Node to local Pods, we don't test it since the UDP probing
from a non-hostNetwork Pod to the hostNetwork Pod deployed on the same
Node will get a failure. The reason is that the reply packets use the
local Antrea gateway IP as source IP, instead of the local Node IP, which
is the destination IP of the request packets, resulting in the failure
of test Pods initialization.

This PR fixes the e2e test failure by reverting the test Pods
initialization modified by PR antrea-io#4537.

Signed-off-by: Hongliang Liu <[email protected]>
hongliangl added a commit to hongliangl/antrea that referenced this pull request Apr 2, 2024
In NodeNetworkPolicy e2e tests, we have the following cases:

- Node to Node. We deploy two hostNetwork Pods on different Nodes.
- Node to remote Pods. We deploy a hostNetwork Pod on a Node and a
  non-hostNetwork Pod on another Node.

For the case of Node to local Pods, we don't test it since the UDP probing
from a non-hostNetwork Pod to the hostNetwork Pod deployed on the same
Node will get a failure. The reason is that the reply packets use the
local Antrea gateway IP as source IP, instead of the local Node IP, which
is the destination IP of the request packets, resulting in the failure
of test Pods initialization.

This PR fixes the e2e test failure by reverting the test Pods
initialization modified by PR antrea-io#4537.

Signed-off-by: Hongliang Liu <[email protected]>
hongliangl added a commit to hongliangl/antrea that referenced this pull request Apr 9, 2024
In NodeNetworkPolicy e2e tests, we have the following cases:

- Node to Nodes. We deploy two hostNetwork Pods on different Nodes.
- Node to remote Pods. We deploy a hostNetwork Pod on a Node and a
  non-hostNetwork Pod on another Node.

For the case of Node to local Pods, we don't test it since the UDP probing
from a non-hostNetwork Pod to the hostNetwork Pod deployed on the same
Node will get a failure. The reason is that the reply packets use the
local Antrea gateway IP as source IP, instead of the local Node IP, which
is the destination IP of the request packets, resulting in the failure
of test Pods initialization.

This PR fixes the e2e test failure by reverting the test Pods
initialization modified by PR antrea-io#4537.

Signed-off-by: Hongliang Liu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/release-note Indicates a PR that should be included in release notes. area/network-policy Issues or PRs related to network policies.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants