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.

Sign up for GitHub

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

Jump to bottom

Add scope field for toServices in Antrea-native policies #4397

Merged
merged 1 commit into from
Dec 20, 2022
Merged

Add scope field for toServices in Antrea-native policies #4397

merged 1 commit into from
Dec 20, 2022

Conversation

Dyanngg
Copy link
Contributor

@Dyanngg Dyanngg commented Nov 17, 2022 •
edited
Loading

Adds a scope field for toServices feature in Antrea-native policy
egress rules. When scope is set to ClusterSet, users can simply
provide the name and Namespace of the exported Service, and
Antrea will match rule with egress traffic intended for backends of
the exported Service across all clusters in the ClusterSet.
It is equivalent to putting the imported Service name (antrea-mc-
[svcName]) in toServices field without setting scope.

Signed-off-by: Dyanngg [email protected]

@Dyanngg Dyanngg added the area/multi-cluster Issues or PRs related to multi cluster. label Nov 17, 2022
@Dyanngg Dyanngg added this to the Antrea v1.10 release milestone Nov 17, 2022
@jianjuns jianjuns requested a review from luolanzone November 17, 2022 00:40
@codecov
Copy link

codecov bot commented Nov 17, 2022 •
edited
Loading

Codecov Report

Merging #4397 (cee3655) into main (656a1f9) will increase coverage by 0.24%.
The diff coverage is 89.28%.

❗ Current head cee3655 differs from pull request most recent head 0588522. Consider uploading reports for the commit 0588522 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4397      +/-   ##
==========================================
+ Coverage   69.78%   70.02%   +0.24%     
==========================================
  Files         379      391      +12     
  Lines       55438    55532      +94     
==========================================
+ Hits        38688    38887     +199     
+ Misses      14081    13981     -100     
+ Partials     2669     2664       -5
Flag Coverage Δ *Carryforward flag
e2e-tests 61.47% <100.00%> (?)
integration-tests 34.55% <72.72%> (-0.04%) ⬇️ Carriedforward from 0588522
kind-e2e-tests 46.69% <12.50%> (ø) Carriedforward from 0588522
unit-tests 58.14% <89.28%> (+0.04%) ⬆️ Carriedforward from 0588522

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

Impacted Files Coverage Δ
pkg/controller/networkpolicy/crd_utils.go 84.43% <76.92%> (-9.15%) ⬇️
...icluster/controllers/multicluster/common/helper.go 92.98% <100.00%> (+0.25%) ⬆️
...uster/commonarea/acnp_resourceimport_controller.go 69.74% <100.00%> (ø)
...lticluster/commonarea/resourceimport_controller.go 80.21% <100.00%> (ø)
...ntroller/networkpolicy/networkpolicy_controller.go 84.57% <100.00%> (ø)
pkg/controller/networkpolicy/validate.go 52.44% <100.00%> (ø)
...trollers/multicluster/resourceexport_controller.go 75.40% <0.00%> (-3.48%) ⬇️
pkg/ovs/openflow/ofctrl_bridge.go 76.98% <0.00%> (-0.57%) ⬇️
...ter/apis/multicluster/v1alpha1/clusterset_types.go 100.00% <0.00%> (ø)
... and 34 more

@luolanzone
Copy link
Contributor

@Dyanngg Could you re-base and resolve conflicts? thanks.

@Dyanngg Dyanngg force-pushed the to-mc-services branch 2 times, most recently from 36a37b9 to 4425556 Compare November 21, 2022 23:23
luolanzone
luolanzone reviewed Nov 23, 2022
View reviewed changes
pkg/apis/crd/v1alpha1/types.go Outdated Show resolved Hide resolved
pkg/controller/networkpolicy/antreanetworkpolicy.go Outdated Show resolved Hide resolved
pkg/controller/networkpolicy/antreanetworkpolicy.go Outdated Show resolved Hide resolved
pkg/controller/networkpolicy/crd_utils.go Outdated Show resolved Hide resolved
@Dyanngg Dyanngg changed the title Add toMultiClusterServices field for Antrea-native policies Add toMulticlusterServices field for Antrea-native policies Dec 1, 2022
luolanzone
luolanzone reviewed Dec 5, 2022
View reviewed changes
build/charts/antrea/crds/clusternetworkpolicy.yaml Outdated Show resolved Hide resolved
pkg/controller/networkpolicy/validate.go Outdated Show resolved Hide resolved
pkg/controller/networkpolicy/validate.go Outdated Show resolved Hide resolved
@luolanzone
Copy link
Contributor

@Dyanngg could you address the remain comments? I think overall is good, we may merge this after you address the comments. thanks.

tnqn
tnqn reviewed Dec 14, 2022
View reviewed changes
build/charts/antrea/crds/clusternetworkpolicy.yaml Outdated Show resolved Hide resolved
pkg/apis/crd/v1alpha1/types.go Outdated Show resolved Hide resolved
@Dyanngg Dyanngg force-pushed the to-mc-services branch 2 times, most recently from a31fc9f to b089a0c Compare December 14, 2022 23:19
@luolanzone
Copy link
Contributor

@Dyanngg there is an out-of-date manifest, please check and fix it. https://github.com/antrea-io/antrea/actions/runs/3699555406/jobs/6267162564

@tnqn
Copy link
Member

tnqn commented Dec 15, 2022

LGTM, one minor comment

@jianjuns
Copy link
Contributor

@tnqn : is it an incompatible API change? Or, we do not consider client lib changes in versioning?

@tnqn
Copy link
Member

tnqn commented Dec 15, 2022

@tnqn : is it an incompatible API change? Or, we do not consider client lib changes in versioning?

This only adds an optional field and is compatible. If you mean the type change NamespacedName -> PeerService, I think it's fine. If the lib consumer doesn't upgrade client lib, using NamespacedName would still work, it just won't be able to use multicluster service. If they upgrade client lib, they should change the type to PeerService, which kind of change happens in many libraries.

@Dyanngg Dyanngg force-pushed the to-mc-services branch 2 times, most recently from 9e25a17 to ec6b127 Compare December 15, 2022 18:06
@Dyanngg
Copy link
Contributor Author

Dyanngg commented Dec 15, 2022

/test-all /test-multicluster-e2e

@tnqn
Copy link
Member

tnqn commented Dec 19, 2022

Needs to resolve the conflicts and update the PR title and description as they are stale. Could you squash the commits as well? Otherwise the automatic generated commit would include some historical information about toMulticlusterServices.

@Dyanngg
Add ClusterSet scope for toServices field of Antrea-native policies
0588522 
Signed-off-by: Dyanngg <[email protected]>

Change toMulticlusterService into scope field in toServices

Signed-off-by: Dyanngg <[email protected]>

Update mc manifests

Signed-off-by: graysonwu <[email protected]>
@Dyanngg Dyanngg changed the title Add toMulticlusterServices field for Antrea-native policies Add scope field for toServices in Antrea-native policies Dec 19, 2022
@Dyanngg
Copy link
Contributor Author

Dyanngg commented Dec 19, 2022

/test-all /test-multicluster-e2e

@Dyanngg Dyanngg requested a review from tnqn December 19, 2022 22:03
tnqn
tnqn approved these changes Dec 20, 2022
View reviewed changes
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 tnqn merged commit 76ee297 into antrea-io:main Dec 20, 2022
GraysonWu pushed a commit to GraysonWu/antrea that referenced this pull request Jan 27, 2023
@Dyanngg @GraysonWu
Add ClusterSet scope for toServices field of Antrea-native policies (a…
754fb93 
…ntrea-io#4397)

Adds a scope field for toServices feature in Antrea-native policy
egress rules. When scope is set to ClusterSet, users can simply
provide the name and Namespace of the exported Service, and
Antrea will match rule with egress traffic intended for backends of
the exported Service across all clusters in the ClusterSet.
It is equivalent to putting the imported Service name (antrea-mc-
[svcName]) in toServices field without setting scope.

Signed-off-by: Dyanngg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jianjuns jianjuns jianjuns left review comments

@luolanzone luolanzone luolanzone left review comments

@tnqn tnqn tnqn approved these changes

@GraysonWu GraysonWu Awaiting requested review from GraysonWu

Assignees
No one assigned
Labels
area/multi-cluster Issues or PRs related to multi cluster.
Projects
None yet
Milestone
Antrea v1.10 release
Development

Successfully merging this pull request may close these issues.
4 participants
@Dyanngg @luolanzone @tnqn @jianjuns