Internal Antrea NetworkPolicy supports references to ExternalEntity. #774

suwang48404 · 2020-06-01T22:27:46Z

This change allows controller to compute for new AntreaNetworkPolicy
(#716), and incorporates ExternalEnitity
information in the internal NetworkPolicy between controller and agents.

antrea-bot · 2020-06-01T22:27:59Z

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

/test-e2e: to trigger e2e tests.
/skip-e2e: to skip e2e tests.
/test-conformance: to trigger conformance tests.
/skip-conformance: to skip conformance tests.
/test-networkpolicy: to trigger networkpolicy tests.
/skip-networkpolicy: to skip networkpolicy tests.
/test-windows-conformance: to trigger windows conformance tests.
/skip-windows-conformance: to skip windows conformance tests.
/test-all: to trigger all tests.
/skip-all: to skip all tests.

These commands can only be run by members of the vmware-tanzu organization.

pkg/apis/networking/types.go

jianjuns · 2020-06-01T23:00:03Z

pkg/apis/networking/types.go

+}
+
+// GroupMemberExternalEntity represents an ExternalEntity related member to be populated in Groups.
+type GroupMemberExternalEntity struct {


@tnqn: again should we use an unified struct for both Pod and ExternalEntity?

If they can share almost same fields and we can come up with a good name for the struct, I think we can have a unified struct.

Again first do you think shared structs can enable more code sharing?

For GroupMemberExternalEntity and GroupMemberPod, one difference is that GroupMemberExternalEntity could include multiple endpoints, but GroupMemberPod includes only a single IP. Would you change Pod to use multiple endpoints as well?

I am ok with code sharing, but I think implementation wise it may be too wide spread a change. Do u think it is reasonable that we define a new common structure; and this new common structure shall only be used for ExternalEntity passing for now.

In subsequent releases, we can fully migrate to this new common structure?

we can define something like

type NetworkEntityReference struct {
name string
namespace string
kind string
}
type Endpoint struct {
IP IPAddress
Ports []NamedPort
}
type NetworkEntity struct {
Reference NetworkEntityReference
Endpoints []Endpoint
}
this can eventually be used to carry Pod info as well. But to limit the impact, we will only used it externalEntity, will that work for u?

I think the internal ExternalEntity defined here is consistent with external facing one at Add API types for Namespaced Antrea NetworkPolicy #716. I assume it is agreed upon. The reason that Abishek defined as such I assume it is because it is more flexible allowing different ports being exported on different IP?

whole we definitely can do as suggested to use the common GroupMemberSource . May I suggest we keep the existing PodGroupMember, so that we don't need to change a lot of agent/controller code?

I apologize that I didn't notice this detail when reviewing Add API types for Namespaced Antrea NetworkPolicy #716, but I think it's unnecessary to make named port various for a single Entity. Originally we got NamedPort from all ports of all container of a Pod, and it's guaranteed to not conflict. Besides, named port is not used to describe which ports are exported but used to resolve port names for Policies. It doesn't require an ExternalEntity must have a named port when we have a Policy selecting it and allowing a port number, e.g. 80. A named port is only required when the Policy allows a port name, e.g. api.

If we agree on the common struct, I agree that you could probably just use the common struct for ExternalEntity and keep GroupMemberPod as is, we can then have another PR to migrate GroupMemberPod to the common struct.

Regarding to 1), Initially I have the similar feeling, but I kind of grow to the current ExternalEndPoint as I believe @abhiraut must have thought it through. It allows us describes a use case where VM has multiple IPs, and some services(ports) exposed via one IP, others with another IP. Unlikely yes; possible, why not?

IMO in this case the object would be a bit complicated to use. Say a VM have multiple IPs and user wants to define a named port "etcd" to "2379", the named port needs to be repeated in every Endpoint, otherwise agent won't resolve "etcd" to "2379" for those Endpoints that don't have this named port, while it would be redundant for most cases that a name maps to a port for all IPs. I think it's not very worth to ask redundant input for for most cases to support some unlikely needed cases.
I don't mean to make the change in this PR, but want to better understand the motivation of NamedPorts per IP as the API and code could be simpler without it. Any idea? @jianjuns @Dyanngg

@quan, What u said is true.

I don't know the reason @abhiraut have, but I kind of like it because it offers flexibility. In most use cases, it is no issue because Pod/VM generally has single IP. In case there are multiple IPs on VM, for security reasons, it is common practice that services generally open only on primary IP.

Incidentally, since u r online, can u please approve this. I lost the approval while re push to deal with CI stability issue

suwang48404 · 2020-06-02T04:20:34Z

Hi @jianjuns @tnqn @antoninbas,

I wonder if u guys could give a quick response. The reason is that the implementations on controller, agent and antrea plus are pending because of this change.

And I apologize that we have not pushed this for review earlier.

suwang48404 · 2020-06-02T14:01:08Z

/test-all

suwang48404 · 2020-06-02T14:02:09Z

/test-all

suwang48404 · 2020-06-02T17:12:28Z

@jianjuns @tnqn is the change in its current form reasonable? if so, we will be able to start implementation on agent/controller based on this change.

thx

suwang48404 · 2020-06-03T16:39:05Z

@tnqn @jianjuns
per ur comment, defined an common struct GroupMember for both Pod and ExternalEntity. The intentions is

first stage, implementing ExternalEntity processing logic in controller/agent using this new common struct GroupMember; while Pod logic continue to use existing GroupMemberPod.
second stage, migrate Pod logic on controller/agent to this GroupMember struct.

I hope this works out for everyone.

Dyanngg · 2020-06-03T18:10:38Z

pkg/apis/networking/types.go

@@ -26,6 +26,8 @@ type AppliedToGroup struct {
 	metav1.ObjectMeta
 	// Pods is a list of Pods selected by this group.
 	Pods []GroupMemberPod
+	// GroupMembers is a list of resources selected by this group.
+	GroupMembers []GroupMember


So if I understand this correctly the externalEntities should be computed and stored in GroupMember, while current code usage of GroupMemberPod will be migrated to GroupMember later?

yes, that is the intention

jianjuns

Thanks for the update! LGTM.

suwang48404 · 2020-06-03T18:41:10Z

/test-all

suwang48404 · 2020-06-03T19:01:49Z

/test-all

suwang48404 · 2020-06-03T19:26:18Z

/test-e2e

suwang48404 · 2020-06-03T19:26:32Z

/test-networkpolicy

suwang48404 · 2020-06-03T20:00:07Z

/test-conformance

suwang48404 · 2020-06-03T20:01:13Z

/test-e2e

suwang48404 · 2020-06-03T20:57:11Z

/test-all

suwang48404 · 2020-06-03T23:02:35Z

/test-e2e

This change allows controller to compute for new AntreaNetworkPolicy (antrea-io#716), and incorporates ExternalEnitity information in the internal NetworkPolicy between controller and agents.

suwang48404 · 2020-06-03T23:05:57Z

/test-all

lzhecheng · 2020-06-04T01:24:45Z

/test-e2e
/test-networkpolicy

suwang48404 · 2020-06-04T01:32:36Z

/test-conformance

…ntrea-io#774) This change allows controller to compute for new AntreaNetworkPolicy (antrea-io#716), and incorporates ExternalEnitity information in the internal NetworkPolicy between controller and agents.

vmwclabot added the cla-not-required label Jun 1, 2020

suwang48404 requested review from jianjuns, tnqn, antoninbas and Dyanngg June 1, 2020 22:28

jianjuns reviewed Jun 1, 2020

View reviewed changes

Dyanngg mentioned this pull request Jun 2, 2020

Add support for Namespace scoped NetworkPolicy CRD #775

Merged

suwang48404 force-pushed the crd branch from 80a8d73 to af5274f Compare June 3, 2020 16:35

Dyanngg reviewed Jun 3, 2020

View reviewed changes

jianjuns previously approved these changes Jun 3, 2020

View reviewed changes

Internal Antrea NetworkPolicy supports references to ExternalEntity.

8b44dcc

This change allows controller to compute for new AntreaNetworkPolicy (antrea-io#716), and incorporates ExternalEnitity information in the internal NetworkPolicy between controller and agents.

suwang48404 dismissed jianjuns’s stale review via 8b44dcc June 3, 2020 23:05

suwang48404 force-pushed the crd branch from af5274f to 8b44dcc Compare June 3, 2020 23:05

tnqn approved these changes Jun 4, 2020

View reviewed changes

suwang48404 merged commit 8c71d26 into antrea-io:master Jun 4, 2020

tnqn mentioned this pull request Sep 9, 2020

Controller support for ExternalEntity in Antrea NetworkPolicy #1084

Merged

suwang48404 deleted the crd branch October 30, 2020 19:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Internal Antrea NetworkPolicy supports references to ExternalEntity. #774

Internal Antrea NetworkPolicy supports references to ExternalEntity. #774

suwang48404 commented Jun 1, 2020

antrea-bot commented Jun 1, 2020

jianjuns Jun 1, 2020

tnqn Jun 2, 2020

jianjuns Jun 2, 2020

suwang48404 Jun 2, 2020

suwang48404 Jun 2, 2020 •

edited

Loading

suwang48404 Jun 3, 2020

tnqn Jun 3, 2020

suwang48404 Jun 3, 2020

tnqn Jun 4, 2020

suwang48404 Jun 4, 2020

suwang48404 commented Jun 2, 2020 •

edited

Loading

suwang48404 commented Jun 2, 2020

suwang48404 commented Jun 2, 2020

suwang48404 commented Jun 2, 2020

suwang48404 commented Jun 3, 2020

Dyanngg Jun 3, 2020

suwang48404 Jun 3, 2020

jianjuns left a comment

suwang48404 commented Jun 3, 2020

suwang48404 commented Jun 3, 2020

suwang48404 commented Jun 3, 2020

suwang48404 commented Jun 3, 2020

suwang48404 commented Jun 3, 2020

suwang48404 commented Jun 3, 2020

suwang48404 commented Jun 3, 2020

suwang48404 commented Jun 3, 2020

suwang48404 commented Jun 3, 2020

lzhecheng commented Jun 4, 2020

suwang48404 commented Jun 4, 2020

Internal Antrea NetworkPolicy supports references to ExternalEntity. #774

Internal Antrea NetworkPolicy supports references to ExternalEntity. #774

Conversation

suwang48404 commented Jun 1, 2020

antrea-bot commented Jun 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suwang48404 Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suwang48404 commented Jun 2, 2020 • edited Loading

suwang48404 commented Jun 2, 2020

suwang48404 commented Jun 2, 2020

suwang48404 commented Jun 2, 2020

suwang48404 commented Jun 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns left a comment

Choose a reason for hiding this comment

suwang48404 commented Jun 3, 2020

suwang48404 commented Jun 3, 2020

suwang48404 commented Jun 3, 2020

suwang48404 commented Jun 3, 2020

suwang48404 commented Jun 3, 2020

suwang48404 commented Jun 3, 2020

suwang48404 commented Jun 3, 2020

suwang48404 commented Jun 3, 2020

suwang48404 commented Jun 3, 2020

lzhecheng commented Jun 4, 2020

suwang48404 commented Jun 4, 2020

suwang48404 Jun 2, 2020 •

edited

Loading

suwang48404 commented Jun 2, 2020 •

edited

Loading