[WIP] Egress policy #1987

ceclinux · 2021-03-24T03:02:10Z

No description provided.

codecov-io · 2021-03-24T03:09:48Z

Codecov Report

Merging #1987 (bbf81dd) into main (9b8440d) will decrease coverage by 7.36%.
The diff coverage is 11.04%.

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

@@            Coverage Diff             @@
##             main    #1987      +/-   ##
==========================================
- Coverage   65.39%   58.03%   -7.37%     
==========================================
  Files         197      203       +6     
  Lines       17217    18069     +852     
==========================================
- Hits        11259    10486     -773     
- Misses       4785     6472    +1687     
+ Partials     1173     1111      -62

Flag	Coverage Δ
kind-e2e-tests	`43.98% <7.59%> (-12.54%)`	⬇️
unit-tests	`40.77% <6.40%> (-1.10%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/agent/controller/egress/egress_controller.go	`0.00% <0.00%> (ø)`
...gent/controller/networkpolicy/status_controller.go	`64.38% <0.00%> (-10.96%)`	⬇️
pkg/agent/openflow/pipeline.go	`66.84% <0.00%> (-5.90%)`	⬇️
pkg/agent/openflow/pipeline_other.go	`50.00% <0.00%> (-16.67%)`	⬇️
pkg/agent/util/iptables/iptables.go	`38.88% <0.00%> (-7.34%)`	⬇️
pkg/apis/controlplane/v1beta1/conversion.go	`81.00% <ø> (ø)`
pkg/controller/egress/store/egressgroup.go	`0.00% <0.00%> (ø)`
pkg/controller/types/networkpolicy.go	`100.00% <ø> (ø)`
pkg/features/antrea_features.go	`16.66% <ø> (ø)`
pkg/ovs/openflow/ofctrl_action.go	`47.63% <0.00%> (-0.21%)`	⬇️
... and 61 more

pkg/agent/controller/egress/egress_controller.go

tnqn · 2021-03-26T07:05:58Z

pkg/agent/controller/egress/egress_controller.go

+		}
+		c.ReplaceEgressGroups(policies)
+		return nil
+	}


The above function variables seem useless? It can be just inline code like:

func (c *Controller) watch() { klog.Info("Starting watch for EgressGroup") antreaClient, err := c.antreaClientProvider.GetAntreaClient() ... watcher, err := antreaClient.ControlplaneV1beta1().EgressGroups().Watch(context.TODO(), options) ... for { select { case event, ok := <-watcher.ResultChan(): if !ok { return } switch event.Type { case watch.Added: klog.V(2).Infof("Added EgressGroup (%#v)", event.Object) c.addEgressGroup(...)

The abstract class in networkpolicy is to share code for 3 watchers, which doesn't make sense here, unless you extract the abstract class from networkpolicy controller to a common package, but I would defer its refactoring before other essential changes are finished.

this is not resolved?

tnqn · 2021-03-26T07:08:13Z

pkg/agent/controller/egress/egress_controller.go

+) *Controller {
+	c := &Controller{
+		kubeClient:           kubeClient,
+		nodeIP:               nodeIP,


The SNAT IP can be any IP configured on the node, not only the Node IP in K8s Node IP.
If you are not sure how to get it, I could provide an interface like LocalIPDetector.IsLocalIP(ip string) bool for you to consume.

Thanks, I will try to implement LocalIPDetector.IsLocalIP(ip string) bool by myself first.

I have added IsLocalIP(ip string) in this PR. Please check if it is correct, thanks

tnqn · 2021-03-30T08:30:50Z

Can you squash your commits into one so that it's easy to review changes of this PR? I mean the commits except the ones merged from other PRs.

pkg/agent/controller/egress/egress_controller.go

tnqn · 2021-03-30T08:32:46Z

pkg/agent/controller/egress/egress_controller.go

+		}
+		c.ReplaceEgressGroups(policies)
+		return nil
+	}


this is not resolved?

pkg/agent/controller/egress/egress_controller.go

tnqn · 2021-03-30T08:37:03Z

pkg/agent/controller/egress/egress_controller.go

+func (c *Controller) AddEgressGroup(group *v1beta1.EgressGroup) error {
+	c.setByGroupLock.Lock()
+	defer c.setByGroupLock.Unlock()
+	klog.Infof("%#v", group)


Can you polish logging in this file? Currently they will be unreadable and unnecessarily verbose

pkg/agent/controller/egress/egress_controller.go

tnqn · 2021-03-30T09:13:25Z

pkg/agent/controller/egress/egress_controller.go

+
+	}
+	if isLocalIP(newEgressIP) {
+		id, errAllocate := c.IPAllocator.allocateForIP(newEgressIP)


The usage of the IPAllocatorLock is not right. What if the oldEgressIP is not local IP? is allocateForIP thread-safe? Typically lock should be inside the called function

fixed the logic. should checkif isLocalIP(newEgressIP) { and if isLocalIP(oldEgressIP) { as well

tnqn · 2021-03-30T09:14:59Z

pkg/agent/controller/egress/egress_controller.go

+		klog.Infof("%#v\n", c.IPAllocator)
+
+		if errAllocate != nil {
+			c.IPAllocator.allocateForIP(oldEgressIP)


Is this kind of retry? Doesn't seem elegant to me. And many error handling is missing here as well as other places in this file.

tnqn · 2021-03-30T09:15:56Z

pkg/agent/controller/egress/egress_controller.go

+
+	klog.Infof("%#v", gm.Member)
+	klog.Infof("%#v", interfaces)
+	klog.Infof("%t", isInterfaceOnPod(interfaces))


tnqn · 2021-03-30T09:23:27Z

pkg/agent/controller/egress/egress_controller.go

+		Member: group,
+	}
+	klog.Infof("%#v", groupMember)
+	c.queue.Add(groupMember)


This is not the correct usage pattern of workqueue. This is nothing different from just using a channel. Keys won't be merged as they key may be different even the IP and the pod's namespace and name is same. And it will have race condition if one pod is processed by two workers in parallel.
It should be the "Egress" name being the item enqueued to the workqueue in my mind. All egress and egressGroup handlers just enqueue affected egresses and syncHandler called by worker is the only one that will do the ID allocation and calls the underlying interfaces based on the eventual state at the moment the function is called.

pkg/agent/controller/egress/ip_allocator.go

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.

…s and iptables changes

ceclinux · 2021-03-31T11:14:24Z

Can you squash your commits into one so that it's easy to review changes of this PR? I mean the commits except the ones merged from other PRs.

squashed. I think there are files I have handled incorrectly in the git history, but it looks better now and I will organize my commits again after other works are merged.

tnqn · 2021-04-07T13:37:58Z

superseded by #2026, closing it

vmwclabot added the cla-not-required label Mar 24, 2021

tnqn reviewed Mar 24, 2021

View reviewed changes

pkg/agent/controller/egress/egress_controller.go Outdated Show resolved Hide resolved

pkg/agent/controller/egress/egress_controller.go Outdated Show resolved Hide resolved

ceclinux force-pushed the egress-controlplane branch 3 times, most recently from 23393d3 to ba64e3c Compare March 25, 2021 12:06

tnqn mentioned this pull request Mar 25, 2021

Egress Policy v1alpha1 implementation #1924

Closed

5 tasks

tnqn reviewed Mar 26, 2021

View reviewed changes

tnqn reviewed Mar 30, 2021

View reviewed changes

antoninbas added this to the Antrea v1.0 release milestone Mar 30, 2021

ceclinux force-pushed the egress-controlplane branch 4 times, most recently from 418051a to 669395e Compare March 31, 2021 08:43

jianjuns and others added 5 commits March 31, 2021 16:52

Add egress policy CRD types

5fe1674

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.

Add EgressGroup API and Controller

503a723

Add iptables interface for implementing Egress

a8f6e28

Add egress policy CRD types

1cd9745

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.

Add EgressGroup API and Controller

d5d48a0

ceclinux force-pushed the egress-controlplane branch 2 times, most recently from 47d3c96 to 4c0c149 Compare March 31, 2021 10:23

Antrea agent watches Egress and EgressGroup API updates, and apply ov…

45617b7

…s and iptables changes

ceclinux force-pushed the egress-controlplane branch from 4c0c149 to 45617b7 Compare March 31, 2021 11:09

jianjuns self-requested a review April 1, 2021 18:32

tnqn closed this Apr 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Egress policy #1987

[WIP] Egress policy #1987

ceclinux commented Mar 24, 2021

codecov-io commented Mar 24, 2021 •

edited

tnqn Mar 26, 2021

tnqn Mar 30, 2021

tnqn Mar 26, 2021

ceclinux Mar 26, 2021

ceclinux Mar 29, 2021

tnqn commented Mar 30, 2021

tnqn Mar 30, 2021

tnqn Mar 30, 2021

tnqn Mar 30, 2021

ceclinux Mar 30, 2021

tnqn Mar 30, 2021

tnqn Mar 30, 2021

tnqn Mar 30, 2021

ceclinux commented Mar 31, 2021 •

edited

tnqn commented Apr 7, 2021

[WIP] Egress policy #1987

[WIP] Egress policy #1987

Conversation

ceclinux commented Mar 24, 2021

codecov-io commented Mar 24, 2021 • edited

Codecov Report

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

tnqn commented Mar 30, 2021

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

Choose a reason for hiding this comment

ceclinux commented Mar 31, 2021 • edited

tnqn commented Apr 7, 2021

codecov-io commented Mar 24, 2021 •

edited

ceclinux commented Mar 31, 2021 •

edited