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

AdminNetworkPolicy support in Antrea #5170

Merged
merged 2 commits into from
Jul 27, 2023
Merged

AdminNetworkPolicy support in Antrea #5170

merged 2 commits into from
Jul 27, 2023

Conversation

Dyanngg
Copy link
Contributor

@Dyanngg Dyanngg commented Jun 22, 2023 •
edited
Loading

PR for #5058

@Dyanngg Dyanngg force-pushed the admin-np-controller branch 2 times, most recently from 0d119a3 to acfd72b Compare June 27, 2023 21:56
@Dyanngg Dyanngg changed the title [WIP] AdminNetworkPolicy support in Antrea AdminNetworkPolicy support in Antrea Jun 27, 2023
@Dyanngg Dyanngg force-pushed the admin-np-controller branch 2 times, most recently from 70c6db5 to 3323939 Compare June 30, 2023 16:48
@Dyanngg Dyanngg added this to the Antrea v1.13 release milestone Jun 30, 2023
@Dyanngg Dyanngg requested a review from antoninbas June 30, 2023 16:53
antoninbas
antoninbas reviewed Jul 3, 2023
View reviewed changes
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

some initial review comments

pkg/util/k8s/client.go
@@ -42,38 +43,43 @@ const (

// CreateClients creates kube clients from the given config.
func CreateClients(config componentbaseconfig.ClientConnectionConfiguration, kubeAPIServerOverride string) (
Copy link
Contributor

Choose a reason for hiding this comment

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

not directly related to this PR, but I think we should consider cleaning this up a little bit given the number of clients we now support, maybe by taking some inspiration from the K8s clientset:

type Interface interface {
        K8s() clientset.Interface
        KubeAggregator() aggregatorclientset.Interface
        AntreaCRDs() crdclientset.Interface
        APIExtensions() apiextensionclientset.Interface
        AntreaMulticluster() mcclientset.Interface
        NetworkPolicy() policyclient.Interface
}

We could return an object implementing this interface in this function, and let consumers choose which client(s) they want to use.

cc @tnqn for his opinion

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 can follow up with a PR

Copy link
Member

Choose a reason for hiding this comment

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

The suggestion looks perfect to me.

docs/feature-gates.md Show resolved Hide resolved
cmd/antrea-controller/controller.go Show resolved Hide resolved
pkg/agent/controller/networkpolicy/reconciler.go
@@ -1285,7 +1285,7 @@ func resolveService(service *v1beta2.Service, member *v1beta2.GroupMember) *v1be
return service
}
for _, port := range member.Ports {
if port.Name == service.Port.StrVal && port.Protocol == *service.Protocol {
if port.Name == service.Port.StrVal && (service.Protocol == nil || port.Protocol == *service.Protocol) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is the new case (service.Protocol == nil) specific to this change (AdminNP / BaselineAdminNP support)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is. The discrepancy is due to the change of how a named port reference is written.
In K8s NP and Antrea-native policies, we can specify a protocol alongside a port name, like

ports:
  - protocol: TCP
    port: secured

If there is a port name and no protocol is present, by default TCP will be assumed. And of course the namedPort will only match those specified in the deployment pod templates only if port name and protocol (TCP if not provided) both matches. However, during the design of AdminNetworkPolicy the upstream community brought up how they think this was a mistake, since in pod template specs port name already map to port+protocol tuple, i.e.

   ports:
    - name: nginx-port
      containerPort: 80
      protocol: TCP

nginx-port <=> TCP port 80.
So in AdminNetworkPolicy, the named port is given a separate struct explicitly eliminating the protocol field. During processing here, we should consider it a match as long as the port name matches.
I will add more comments in the code to explain this

pkg/controller/networkpolicy/adminnetworkpolicy.go Outdated Show resolved Hide resolved
pkg/controller/networkpolicy/networkpolicy_controller_test.go Show resolved Hide resolved
pkg/controller/networkpolicy/validate.go Show resolved Hide resolved
@Dyanngg Dyanngg force-pushed the admin-np-controller branch 8 times, most recently from 1051a89 to a4cc497 Compare July 12, 2023 23:22
@Dyanngg Dyanngg force-pushed the admin-np-controller branch 9 times, most recently from 85d10c0 to 902a549 Compare July 14, 2023 23:50
@Dyanngg Dyanngg force-pushed the admin-np-controller branch 3 times, most recently from 647a842 to d56c7b0 Compare July 20, 2023 20:39
@Dyanngg Dyanngg requested review from tnqn and luolanzone July 21, 2023 03:41
tnqn
tnqn reviewed Jul 21, 2023
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, I remember you said you will update the PR for some test change?

@Dyanngg
Copy link
Contributor Author

Dyanngg commented Jul 21, 2023

LGTM, I remember you said you will update the PR for some test change?

It is already included in the latest change. Specifically, in conformance testing it is using my own network-policy-api repo, which is contains minor modifications and I will point it to the upstream one once my PR to that repo is merged

antoninbas
antoninbas reviewed Jul 21, 2023
View reviewed changes
ci/kind/test-netpol-v2-conformance-kind.sh Outdated Show resolved Hide resolved
ci/kind/test-netpol-v2-conformance-kind.sh Outdated Show resolved Hide resolved
ci/kind/test-netpol-v2-conformance-kind.sh Outdated Show resolved Hide resolved
ci/kind/test-netpol-v2-conformance-kind.sh Outdated Show resolved Hide resolved
ci/kind/test-netpol-v2-conformance-kind.sh Outdated Show resolved Hide resolved
@Dyanngg Dyanngg force-pushed the admin-np-controller branch 2 times, most recently from 2a7aeeb to 058b5e4 Compare July 21, 2023 21:17
@Dyanngg
Copy link
Contributor Author

Dyanngg commented Jul 21, 2023

Related PR in upstream network-policy-api repo: kubernetes-sigs/network-policy-api#129

@Dyanngg
Copy link
Contributor Author

Dyanngg commented Jul 21, 2023

/test-all

tnqn
tnqn previously approved these changes Jul 24, 2023
View reviewed changes
@Dyanngg
Copy link
Contributor Author

Dyanngg commented Jul 24, 2023

/test-all

@tnqn
Copy link
Member

tnqn commented Jul 25, 2023

@Dyanngg There are some conflicts after NetworkPolicy API was upgraded to v1beta1.
@antoninbas please let us know if your comments have been addressed properly.

@Dyanngg
Add controller logic for AdminNetworkPolicy and BANP
0ebf7b5 
Signed-off-by: Dyanngg <[email protected]>

Add BANP controller

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

Add feature gate

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

Add UT

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

Address comments

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

Add conformance file and fix issues

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

Add conformance as a github action

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

Address more comments

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

Use cloned repo for conformance testing

Signed-off-by: Dyanngg <[email protected]>
@Dyanngg
Copy link
Contributor Author

Dyanngg commented Jul 25, 2023

/test-all

@Dyanngg Dyanngg requested review from antoninbas and tnqn July 25, 2023 18:33
@Dyanngg
Copy link
Contributor Author

Dyanngg commented Jul 25, 2023

Kind failure is related to #5296

@Dyanngg
Copy link
Contributor Author

Dyanngg commented Jul 25, 2023

/test-conformance /test-all-features-conformance /test-e2e /test-networkpolicy

tnqn
tnqn approved these changes Jul 26, 2023
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
Copy link
Member

tnqn commented Jul 27, 2023

/test-all

@tnqn tnqn merged commit 95bd905 into antrea-io:main Jul 27, 2023
41 of 44 checks passed
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

@tnqn tnqn tnqn approved these changes

@GraysonWu GraysonWu Awaiting requested review from GraysonWu

@qiyueyao qiyueyao Awaiting requested review from qiyueyao

@luolanzone luolanzone Awaiting requested review from luolanzone

@antoninbas antoninbas Awaiting requested review from antoninbas

Assignees
No one assigned
Labels
action/release-note Indicates a PR that should be included in release notes.
Projects
None yet
Milestone
Antrea v1.13 release
Development

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