Add Egress CRD types #1433

jianjuns · 2020-10-23T22:52:35Z

Add types for egress policy CRDs, including Egress for the egress
policy definition, and a common AppliedTo struct for defining the scope
to which a policy is applied.

Issue: #667

antrea-bot · 2020-10-23T22:52:52Z

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-whole-conformance: to trigger all conformance tests on linux.
/skip-whole-conformance: to skip all conformance tests on linux.
/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-windows-networkpolicy: to trigger windows networkpolicy tests.
/skip-windows-networkpolicy: to skip windows networkpolicy tests.
/test-hw-offload: to trigger ovs hardware offload test.
/skip-hw-offload: to skip ovs hardware offload test.
/test-all: to trigger all tests (except whole conformance).
/skip-all: to skip all tests (except whole conformance).

codecov-io · 2020-10-23T23:28:11Z

Codecov Report

Merging #1433 (082bbef) into main (e93ed12) will increase coverage by 1.20%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1433      +/-   ##
==========================================
+ Coverage   61.92%   63.13%   +1.20%     
==========================================
  Files         262      262              
  Lines       19473    19473              
==========================================
+ Hits        12059    12294     +235     
+ Misses       6167     5915     -252     
- Partials     1247     1264      +17

Flag	Coverage Δ
e2e-tests	`25.40% <ø> (?)`
kind-e2e-tests	`53.34% <ø> (-0.05%)`	⬇️
unit-tests	`41.85% <ø> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/ipfix/ipfix_process.go	`81.81% <0.00%> (-18.19%)`	⬇️
pkg/agent/cniserver/pod_configuration.go	`53.90% <0.00%> (-3.52%)`	⬇️
pkg/controller/traceflow/controller.go	`69.48% <0.00%> (-2.60%)`	⬇️
pkg/agent/openflow/client.go	`64.16% <0.00%> (+0.50%)`	⬆️
pkg/agent/flowexporter/exporter/exporter.go	`70.83% <0.00%> (+0.69%)`	⬆️
...ntroller/networkpolicy/networkpolicy_controller.go	`71.10% <0.00%> (+0.97%)`	⬆️
pkg/agent/agent.go	`48.92% <0.00%> (+1.19%)`	⬆️
pkg/ovs/openflow/ofctrl_builder.go	`57.89% <0.00%> (+1.31%)`	⬆️
pkg/agent/flowexporter/connections/connections.go	`81.67% <0.00%> (+1.52%)`	⬆️
pkg/ovs/ovsconfig/ovs_client.go	`51.73% <0.00%> (+1.62%)`	⬆️
... and 13 more

abhiraut · 2020-12-16T02:25:08Z

pkg/apis/networking/v1alpha1/types.go

+// +genclient:noStatus
+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+
+type SNATPolicy struct {


is this going to be a Namespaced scope CRD?

I plan to start from cluster scoped.

jianjuns · 2021-03-03T07:09:28Z

@tnqn : please let me know what you think about my new version?

tnqn

I guess your command didn't finish successfully, there are 130 files are affected because of the year update.

tnqn · 2021-03-04T14:25:17Z

pkg/apis/core/v1alpha2/types.go

+	// workloads in To/From fields. If set with PodSelector,
+	// Pods are matched from Namespaces matched by the NamespaceSelector.
+	// +optional
+	NamespaceSelector *metav1.LabelSelector `json:"namespaceSelector,omitempty"`


Not related to this PR, I'm wondering if we could add a Namespace field to the EntitySelector which means selecting Pods in this namespace, just like how NetworkPolicyPeer in K8s NetworkPolicy works. Is this easier to use compared with "antrea.io/metadata.name" or "kubernetes.io/metadata.name" label?

I think Abishek and Yang proposed the label way, because upstream NetworkPolicy decide to go that direction. @Dyanngg: please comment.

Thanks for bringing it up. In the upstream proposal, we're replacing the NamespaceSelector field with a new Namespaces field, which look like the following

type Namespaces struct { Self bool Selector *metav1.LabelSelector Except []*metav1.LabelSelector }

Namespaces.Selector will essentially replace NamespaceSelector. And user can easily write a Namespace isolation policy by setting Namespaces.Self=true. We are still on the fence about whether the Except field is necessary so I wouldn't put that in to start with (although there are quite some usecases which can be greatly simplified by except, like deny all traffic except from kube-system).
Link to the KEP: kubernetes/enhancements#2522

tnqn · 2021-03-04T14:32:19Z

pkg/apis/egress/v1alpha1/types.go

+type EgressSpec struct {
+	// AppliedTo selects Pods to which the Egress will be applied.
+	// +optional
+	AppliedTo *antreacore.AppliedTo `json:"appliedTo,omitempty"`


should this be non pointer? It doesn't have a proper mean if it's nil, right?

In NetworkPolicy AppliedTo can be nil today, which means applied to all I think. Should we keep the behavior?

Thought over and now also feel empty AppliedTo probably makes more sense. Do you think so?

jianjuns · 2021-03-04T16:48:59Z

pkg/apis/core/v1alpha2/types.go

+type AppliedTo struct {
+	// Selectors is the set of EntitySelectors that select entities.
+	// +optional
+	Selectors []EntitySelector `json:"selectors,omitempty"`


@Dyanngg, @tnqn, @antoninbas : do you see any reason to make it a slice? Today Antrea NetworkPolicy uses a slice of NetworkPolicyPeer so can be multiple Selectors too.

I meant I am thinking about:

type AppliedTo struct { PodSelector *metav1.LabelSelector NamespaceSelector *metav1.LabelSelector Groups []string }

Changed to a single selector (pair) per inputs from Abhishek and Yang.

jianjuns · 2021-03-04T22:04:53Z

I guess your command didn't finish successfully, there are 130 files are affected because of the year update.

Yes, noticed it, but there is no error returned by "make codegen".

I found out it is due to a previous failed run, which I commited to local. Fixed it.

tnqn

@jianjuns I tested this patch with controller code and it basically works. The only thing missing is the CRD manifest, do you plan to add it in this PR? or I could do it with another PR.

tnqn · 2021-03-18T10:57:26Z

pkg/apis/core/v1alpha2/types.go

+	NamespaceSelector *metav1.LabelSelector `json:"namespaceSelector,omitempty"`
+	// Groups is the set of ClusterGroup names.
+	// +optional
+	Groups []string `json:"groups,omitempty"`


This is not consistent with Antrea NetworkPolicy where only a Group is allowed. It looks like multiple group will be supported by a nested group #1920.

Yes. I think we should support multiple groups finally, but if we want to change NetworkPolicy together, I can switch to a single group for now.
@Dyanngg : thoughts?

I think last time we discussed, the cardinality issue is not here in particular: the issue is that currently Antrea-native policies support multiple appliedTos, while @jianjuns suggests we only use a single core/v1alpha2.appliedTo for Antrea-native policies if we decide to share the appliedTo. So in upgrade case, there could be extra namespaceSelector and podSelector in the appliedTo list today that can not fit in the new spec. If we want to do some hacks (i.e. translating 1 old ACNP to multiple new ACNPs once appliedTo is unified), then I don't really have a preference here. If not, then maybe a list for Groups here would help, as we only need to put the groups mentioned in the old list of appliedTos in the slice there?

tnqn · 2021-03-18T11:04:00Z

pkg/apis/egress/doc.go

+// +k8s:deepcopy-gen=package
+// +groupName=egress.antrea.tanzu.vmware.com
+
+package egress


do you plan to change to crd group after the renaming is done?

jianjuns · 2021-03-18T22:14:02Z

@jianjuns I tested this patch with controller code and it basically works. The only thing missing is the CRD manifest, do you plan to add it in this PR? or I could do it with another PR.

If we generate YAML then the CRDs are visible to users. I think we can publish YAML after the feature is complete. What you think?

tnqn · 2021-03-25T08:01:56Z

pkg/apis/egress/v1alpha1/types.go

+
+// +genclient
+// +genclient:noStatus
+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object


// +genclient:nonNamespaced
@jianjuns the tag is needed to generate proper Lister, otherwise it will require a namespace to get an Egress.

tnqn

LGTM

tnqn · 2021-04-02T15:39:15Z

/test-all

Add types for egress policy CRDs, including Egress for the egress policy definition, and a common AppliedTo struct for defining the scope to which a policy is applied.

tnqn

LGTM

tnqn · 2021-04-03T06:30:21Z

/test-all

jianjuns · 2021-04-04T14:08:14Z

/test-e2e

jianjuns requested a review from abhiraut October 23, 2020 22:52

vmwclabot added the cla-not-required label Oct 23, 2020

jianjuns force-pushed the egress-pol branch from e1613a3 to 9c0f7ed Compare October 25, 2020 05:46

abhiraut reviewed Dec 16, 2020

View reviewed changes

Base automatically changed from master to main January 26, 2021 00:00

jianjuns force-pushed the egress-pol branch from 9c0f7ed to 59e69ce Compare March 3, 2021 07:07

jianjuns requested a review from tnqn March 3, 2021 07:09

jianjuns force-pushed the egress-pol branch from 59e69ce to c3ce05f Compare March 3, 2021 19:13

jianjuns marked this pull request as ready for review March 3, 2021 19:17

tnqn reviewed Mar 4, 2021

View reviewed changes

jianjuns commented Mar 4, 2021

View reviewed changes

jianjuns force-pushed the egress-pol branch 2 times, most recently from be92580 to 3b3dbdd Compare March 4, 2021 22:01

jianjuns force-pushed the egress-pol branch from 3b3dbdd to d0ebea3 Compare March 11, 2021 05:00

tnqn mentioned this pull request Mar 18, 2021

Egress Policy v1alpha1 implementation #1924

Closed

5 tasks

tnqn reviewed Mar 18, 2021

View reviewed changes

tnqn reviewed Mar 25, 2021

View reviewed changes

jianjuns force-pushed the egress-pol branch from d0ebea3 to 8cc9e90 Compare March 25, 2021 16:52

tnqn mentioned this pull request Apr 2, 2021

Add EgressGroup API and Controller #1965

Merged

jianjuns force-pushed the egress-pol branch from 8cc9e90 to b03188d Compare April 2, 2021 03:55

jianjuns changed the title ~~Add SNATPolicy CRD~~ Add Egress CRD types Apr 2, 2021

tnqn previously approved these changes Apr 2, 2021

View reviewed changes

Add egress policy CRD types

082bbef

Add types for egress policy CRDs, including Egress for the egress policy definition, and a common AppliedTo struct for defining the scope to which a policy is applied.

jianjuns dismissed tnqn’s stale review via 082bbef April 3, 2021 01:04

jianjuns force-pushed the egress-pol branch from b03188d to 082bbef Compare April 3, 2021 01:04

tnqn approved these changes Apr 3, 2021

View reviewed changes

jianjuns merged commit 4bc2316 into antrea-io:main Apr 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Egress CRD types #1433

Add Egress CRD types #1433

jianjuns commented Oct 23, 2020 •

edited

Loading

antrea-bot commented Oct 23, 2020

codecov-io commented Oct 23, 2020 •

edited

Loading

abhiraut Dec 16, 2020

jianjuns Dec 16, 2020

jianjuns commented Mar 3, 2021

tnqn left a comment

tnqn Mar 4, 2021

jianjuns Mar 4, 2021

Dyanngg Mar 4, 2021

tnqn Mar 4, 2021

jianjuns Mar 4, 2021 •

edited

Loading

jianjuns Mar 4, 2021

jianjuns Mar 4, 2021

jianjuns Mar 4, 2021 •

edited

Loading

jianjuns Mar 4, 2021

jianjuns commented Mar 4, 2021

tnqn left a comment

tnqn Mar 18, 2021

jianjuns Mar 18, 2021

Dyanngg Mar 18, 2021 •

edited

Loading

tnqn Mar 18, 2021

jianjuns Mar 18, 2021

jianjuns commented Mar 18, 2021

tnqn Mar 25, 2021

jianjuns Mar 25, 2021

tnqn left a comment

tnqn commented Apr 2, 2021

tnqn left a comment

tnqn commented Apr 3, 2021

jianjuns commented Apr 4, 2021

Add Egress CRD types #1433

Add Egress CRD types #1433

Conversation

jianjuns commented Oct 23, 2020 • edited Loading

antrea-bot commented Oct 23, 2020

codecov-io commented Oct 23, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns commented Mar 3, 2021

tnqn left a 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

jianjuns Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns commented Mar 4, 2021

tnqn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dyanngg Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns commented Mar 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Apr 2, 2021

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Apr 3, 2021

jianjuns commented Apr 4, 2021

jianjuns commented Oct 23, 2020 •

edited

Loading

codecov-io commented Oct 23, 2020 •

edited

Loading

jianjuns Mar 4, 2021 •

edited

Loading

jianjuns Mar 4, 2021 •

edited

Loading

Dyanngg Mar 18, 2021 •

edited

Loading