Reject the request to a Service without an Endpoint #4656

hongliangl · 2023-02-23T11:01:09Z

When requesting a Service without an Endpoint, the connection should be rejected, rather than timeout according to the expectation of Kubernetes sig-network tests.

This PR also relocates variable NestedServiceRegMark to a proper place
in pkg/agent/openflow/field.go.

Signed-off-by: Hongliang Liu lhongliang@vmware.com

codecov · 2023-02-23T11:24:28Z

Codecov Report

Merging #4656 (451ab76) into main (d2c4ef8) will decrease coverage by 1.78%.
The diff coverage is 60.55%.

❗ Current head 451ab76 differs from pull request most recent head 500934c. Consider uploading reports for the commit 500934c to get more accurate results

@@            Coverage Diff             @@
##             main    #4656      +/-   ##
==========================================
- Coverage   70.44%   68.66%   -1.78%     
==========================================
  Files         405      405              
  Lines       60616    60189     -427     
==========================================
- Hits        42698    41330    -1368     
- Misses      15031    16035    +1004     
+ Partials     2887     2824      -63

Flag	Coverage Δ		*Carryforward flag
e2e-tests	`38.32% <ø> (-0.08%)`	⬇️	Carriedforward from 4a80363
integration-tests	`34.20% <5.20%> (+0.24%)`	⬆️
kind-e2e-tests	`27.31% <48.33%> (-13.05%)`	⬇️
unit-tests	`60.46% <28.88%> (-1.70%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/agent/controller/networkpolicy/fqdn.go	`77.37% <0.00%> (+0.46%)`	⬆️
pkg/agent/controller/networkpolicy/packetin.go	`73.94% <ø> (+1.64%)`	⬆️
pkg/agent/openflow/packetout.go	`39.21% <39.21%> (ø)`
pkg/agent/proxy/proxier.go	`67.50% <64.47%> (-0.71%)`	⬇️
pkg/agent/openflow/client.go	`87.42% <65.51%> (+2.70%)`	⬆️
pkg/agent/controller/networkpolicy/reject.go	`83.40% <100.00%> (-1.85%)`	⬇️
pkg/agent/multicast/mcast_discovery.go	`76.89% <100.00%> (ø)`
pkg/agent/openflow/packetin.go	`74.19% <100.00%> (+0.42%)`	⬆️
pkg/agent/openflow/service.go	`95.41% <100.00%> (+0.62%)`	⬆️

... and 51 files with indirect coverage changes

hongliangl · 2023-03-01T08:44:43Z

/test-all
/test-windows-all

GraysonWu · 2023-03-03T00:40:29Z

Maybe we should add E2E tests for this.

GraysonWu · 2023-03-03T00:58:39Z

pkg/agent/openflow/packetin.go

@@ -52,7 +52,8 @@ const (
 	PacketInReasonNP ofpPacketInReason = 0
 	// PacketInReasonMC shares PacketInReasonNP for IGMP packet_in message. This is because OVS "controller" action
 	// only correctly supports reason 0 or 1. Change to another value after the OVS action is corrected.
-	PacketInReasonMC = PacketInReasonNP
+	PacketInReasonMC        = PacketInReasonNP
+	PacketInReasonSvcReject = PacketInReasonNP


Not necessary to be addressed in this PR. I'm just wondering if we could change to use a different field than reason to distinguish packetIn, so that packetIn won't be picked up by an unrelated handler. Or is it possible for OVS to make reason supporting more value? cc @wenyingd

Using reason field is to follow the standard. A second reason is, other fields like "userdata" requires a different action "NXAST_CONTROLLER2" other than the existing one, and the value is not taken in packet_in message but required NXT_PACKET_IN2.

I tried to use other value in "reason" field on OVS 2.17.0, the result showed that only value 0 and 1 can work, and other values may cause the packet fails to be received via the "controller".

In current implemenation, we are using NXAST_CONTROLLER for "controller" action. I am thinking maybe we could try with "NXAST_CONTROLLER2" to see if it works.

pkg/agent/openflow/client.go

wenyingd · 2023-03-15T10:20:54Z

pkg/agent/openflow/packetin.go

@@ -52,7 +52,8 @@ const (
 	PacketInReasonNP ofpPacketInReason = 0
 	// PacketInReasonMC shares PacketInReasonNP for IGMP packet_in message. This is because OVS "controller" action
 	// only correctly supports reason 0 or 1. Change to another value after the OVS action is corrected.
-	PacketInReasonMC = PacketInReasonNP
+	PacketInReasonMC        = PacketInReasonNP
+	PacketInReasonSvcReject = PacketInReasonNP


Using reason field is to follow the standard. A second reason is, other fields like "userdata" requires a different action "NXAST_CONTROLLER2" other than the existing one, and the value is not taken in packet_in message but required NXT_PACKET_IN2.

I tried to use other value in "reason" field on OVS 2.17.0, the result showed that only value 0 and 1 can work, and other values may cause the packet fails to be received via the "controller".

In current implemenation, we are using NXAST_CONTROLLER for "controller" action. I am thinking maybe we could try with "NXAST_CONTROLLER2" to see if it works.

pkg/agent/proxy/proxier.go

jianjuns

Nits.

pkg/agent/proxy/proxier.go

pkg/agent/openflow/service.go

pkg/agent/openflow/packetin.go

wenyingd · 2023-03-21T02:32:37Z

pkg/agent/openflow/service.go

@@ -124,6 +124,15 @@ func newFeatureService(
 	}
 }

+// serviceNoEndpointFlow generates the flow to match the packets to Service without Endpoint and send them to controller.
+func (f *featureService) serviceNoEndpointFlow() binding.Flow {
+	return EndpointDNATTable.ofTable.BuildFlow(priorityLow).


Why use priorityLow not priorityHigh?

No special reason, both priorityLow and priorityHigh can work since the reg mark is unique.

ServiceEPStateField = binding.NewRegField(4, 16, 18) EpToSelectRegMark = binding.NewRegMark(ServiceEPStateField, 0b001) EpSelectedRegMark = binding.NewRegMark(ServiceEPStateField, 0b010) EpToLearnRegMark = binding.NewRegMark(ServiceEPStateField, 0b011) NoEpToSelectRegMark = binding.NewRegMark(ServiceEPStateField, 0b100)

I feels "priorityHigh" is more appropriate for such case that the match condition is static and no conflict with other flow. OVS is always trying to match a packet from higher priority to lower.

OVS is always trying to match a packet from higher priority to lower.

From my understanding, for a packet should be rejected, if it is priorityHigh, it could be matched earlier without matching other flows, but in this way, other packets will be also matched with the flow for rejecting packet first. Is that right?

Why do other packets match the flow? Other packets does not contain the regmark

shouldn't it use priorityNormal if there is no special reason we want it to be prioritized higher or lower?

After discussed with @wenyingd offline, both priorityNormal and priorityHight are okay. Since this is related to Endpoint selection, priorityNormal is better like other Endpoint selection flows.

pkg/agent/proxy/proxier.go

wenyingd · 2023-03-21T03:07:18Z

pkg/agent/proxy/proxier.go

+func (p *proxier) HandlePacketIn(pktIn *ofctrl.PacketIn) error {
+	if pktIn == nil {
+		return errors.New("empty packetin for Antrea Proxy")
+	}


You should check if the packetIn message is for Service no Endpoint purpose first. Otherwise, all packet_in message with reason "PacketInReasonSvcReject|PacketInReasonNP|PacketInReasonMC" would be processed by the later logic because they three are sharing the same value.

Thanks, updated.

where is it added?

wenyingd · 2023-03-21T06:01:12Z

pkg/agent/multicast/mcast_discovery.go

@@ -90,20 +85,9 @@ func (s *IGMPSnooper) HandlePacketIn(pktIn *ofctrl.PacketIn) error {
 	return nil
 }

-func getInfoInReg(regMatch *ofctrl.MatchField, rng *openflow15.NXRange) (uint32, error) {


If it is planned to be provided in package pkg/agent/openflow, maybe also update the function calls under path pkg/agent/controller/networkpolicy ?

Agreed, but I think we could do this in another PR.

I am OK to use a new patch for it.

wenyingd · 2023-03-21T06:03:47Z

pkg/agent/openflow/service.go

@@ -124,6 +124,15 @@ func newFeatureService(
 	}
 }

+// serviceNoEndpointFlow generates the flow to match the packets to Service without Endpoint and send them to controller.
+func (f *featureService) serviceNoEndpointFlow() binding.Flow {
+	return EndpointDNATTable.ofTable.BuildFlow(priorityLow).


I feels "priorityHigh" is more appropriate for such case that the match condition is static and no conflict with other flow. OVS is always trying to match a packet from higher priority to lower.

pkg/agent/openflow/fields.go

pkg/agent/openflow/packetout.go

tnqn · 2023-03-21T07:02:14Z

pkg/agent/openflow/service.go

@@ -124,6 +124,15 @@ func newFeatureService(
 	}
 }

+// serviceNoEndpointFlow generates the flow to match the packets to Service without Endpoint and send them to controller.
+func (f *featureService) serviceNoEndpointFlow() binding.Flow {
+	return EndpointDNATTable.ofTable.BuildFlow(priorityLow).


shouldn't it use priorityNormal if there is no special reason we want it to be prioritized higher or lower?

pkg/agent/proxy/proxier.go

tnqn · 2023-03-21T07:12:48Z

pkg/agent/proxy/proxier.go

+func (p *proxier) HandlePacketIn(pktIn *ofctrl.PacketIn) error {
+	if pktIn == nil {
+		return errors.New("empty packetin for Antrea Proxy")
+	}


where is it added?

pkg/agent/proxy/proxier_test.go

wenyingd

Overall LGTM, two nits

pkg/agent/proxy/proxier.go

pkg/agent/openflow/packetin.go

hongliangl · 2023-03-21T12:28:34Z

pkg/agent/proxy/proxier.go

+	noEpToSelectRegField := openflow.NoEpToSelectRegMark.GetField()
+	noEpToSelectRegValue := openflow.NoEpToSelectRegMark.GetValue()
+	match := openflow.GetMatchFieldByRegID(matches, noEpToSelectRegField.GetRegID())
+	if match == nil {
+		return fmt.Errorf("error getting match NoEpToSelectRegMark")
+	}
+	regValue, err := openflow.GetInfoInReg(match, noEpToSelectRegField.GetRange().ToNXRange())
+	if err != nil {
+		klog.ErrorS(err, "Received error while unloading NoEpToSelectRegMark from OVS reg")
+		return err
+	}
+	// Filter out the packets that don't have reg mark NoEpToSelectRegMark.
+	if regValue&noEpToSelectRegValue != noEpToSelectRegValue {
+		return nil
+	}


L1032-L1046 is for https://github.com/antrea-io/antrea/pull/4656/files#r1142851197 @tnqn

This introduces another way to dispatch packet-in event to shared handlers, while all others use CustomReasonField. I think it's not easy to understand and maintain in the long term, and perhaps error-prone, for example, imagine what will happen when we use 0b101 as a new endpoint selection mark in the future, 0b101&0b100 == 0b100.

I proposed this solution (#4726) to optimize the packetIn mechnism, do you think we can schedule it in the next release?

Does NXAST_CONTROLLER2 require more recent OVS than the documented one?

No, it is provided since OVS v2.6

then it looks great to me.

pkg/agent/openflow/fields.go

When requesting a Service without an Endpoint, the connection should be rejected, rather than timeout according to the expectation of Kubernetes sig-network tests. This PR also relocates variable `NestedServiceRegMark` to a proper place in pkg/agent/openflow/field.go. Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

tnqn · 2023-03-22T02:43:10Z

/test-all

wenyingd

LGTM

When requesting a Service without an Endpoint, the connection should be rejected, rather than timeout according to the expectation of Kubernetes sig-network tests. This PR also relocates variable `NestedServiceRegMark` to a proper place in pkg/agent/openflow/field.go. Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

hongliangl requested review from wenyingd and GraysonWu February 23, 2023 11:01

hongliangl force-pushed the 20230221-service-reject-without-endpoints branch 5 times, most recently from bf1a471 to 9a6448a Compare March 1, 2023 06:16

vicky-liu added this to the Antrea v1.11 release milestone Mar 2, 2023

GraysonWu reviewed Mar 3, 2023

View reviewed changes

wenyingd reviewed Mar 15, 2023

View reviewed changes

hongliangl force-pushed the 20230221-service-reject-without-endpoints branch from 9a6448a to 451ab76 Compare March 16, 2023 14:46

tnqn mentioned this pull request Mar 17, 2023

Fix ClusterIP Service processing #4713

Merged

jianjuns reviewed Mar 20, 2023

View reviewed changes

pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved

pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved

pkg/agent/openflow/service.go Outdated Show resolved Hide resolved

pkg/agent/openflow/service.go Outdated Show resolved Hide resolved

hongliangl force-pushed the 20230221-service-reject-without-endpoints branch 2 times, most recently from 500934c to be3fba1 Compare March 20, 2023 23:51

hongliangl requested review from tnqn, wenyingd, GraysonWu and jianjuns March 21, 2023 00:40

wenyingd reviewed Mar 21, 2023

View reviewed changes

hongliangl force-pushed the 20230221-service-reject-without-endpoints branch from 4020f00 to fe2a8a3 Compare March 21, 2023 05:33

hongliangl requested a review from wenyingd March 21, 2023 05:41

hongliangl force-pushed the 20230221-service-reject-without-endpoints branch from fe2a8a3 to d0e1848 Compare March 21, 2023 05:43

wenyingd reviewed Mar 21, 2023

View reviewed changes

tnqn reviewed Mar 21, 2023

View reviewed changes

hongliangl force-pushed the 20230221-service-reject-without-endpoints branch from d0e1848 to ecdc028 Compare March 21, 2023 09:15

hongliangl force-pushed the 20230221-service-reject-without-endpoints branch 4 times, most recently from f3225bc to 0b5e28f Compare March 21, 2023 10:09

hongliangl requested review from tnqn and wenyingd March 21, 2023 10:11

wenyingd reviewed Mar 21, 2023

View reviewed changes

pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved

pkg/agent/openflow/packetin.go Outdated Show resolved Hide resolved

hongliangl force-pushed the 20230221-service-reject-without-endpoints branch from 0b5e28f to 74d2cc4 Compare March 21, 2023 12:27

hongliangl commented Mar 21, 2023

View reviewed changes

hongliangl force-pushed the 20230221-service-reject-without-endpoints branch from 74d2cc4 to 4fa4822 Compare March 21, 2023 14:01

tnqn reviewed Mar 21, 2023

View reviewed changes

pkg/agent/openflow/fields.go Outdated Show resolved Hide resolved

hongliangl force-pushed the 20230221-service-reject-without-endpoints branch 2 times, most recently from c108992 to d859684 Compare March 21, 2023 17:06

hongliangl force-pushed the 20230221-service-reject-without-endpoints branch from d859684 to 44e572e Compare March 21, 2023 17:08

hongliangl requested review from wenyingd and tnqn March 22, 2023 02:32

tnqn approved these changes Mar 22, 2023

View reviewed changes

wenyingd approved these changes Mar 22, 2023

View reviewed changes

tnqn merged commit 15a8286 into antrea-io:main Mar 22, 2023

hongliangl deleted the 20230221-service-reject-without-endpoints branch April 4, 2023 01:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject the request to a Service without an Endpoint #4656

Reject the request to a Service without an Endpoint #4656

hongliangl commented Feb 23, 2023 •

edited

Loading

codecov bot commented Feb 23, 2023 •

edited

Loading

hongliangl commented Mar 1, 2023

GraysonWu commented Mar 3, 2023

GraysonWu Mar 3, 2023

wenyingd Mar 15, 2023

wenyingd Mar 15, 2023

jianjuns left a comment

wenyingd Mar 21, 2023

hongliangl Mar 21, 2023

wenyingd Mar 21, 2023

hongliangl Mar 21, 2023

wenyingd Mar 21, 2023

tnqn Mar 21, 2023

hongliangl Mar 21, 2023

wenyingd Mar 21, 2023

hongliangl Mar 21, 2023

tnqn Mar 21, 2023

tnqn Mar 21, 2023

wenyingd Mar 21, 2023

hongliangl Mar 21, 2023

wenyingd Mar 21, 2023

wenyingd Mar 21, 2023

tnqn Mar 21, 2023

tnqn Mar 21, 2023

wenyingd left a comment

hongliangl Mar 21, 2023

tnqn Mar 21, 2023

wenyingd Mar 22, 2023

tnqn Mar 22, 2023

wenyingd Mar 22, 2023

tnqn Mar 22, 2023

tnqn commented Mar 22, 2023

wenyingd left a comment

Reject the request to a Service without an Endpoint #4656

Reject the request to a Service without an Endpoint #4656

Conversation

hongliangl commented Feb 23, 2023 • edited Loading

codecov bot commented Feb 23, 2023 • edited Loading

Codecov Report

hongliangl commented Mar 1, 2023

GraysonWu commented Mar 3, 2023

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenyingd 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn commented Mar 22, 2023

wenyingd left a comment

Choose a reason for hiding this comment

hongliangl commented Feb 23, 2023 •

edited

Loading

codecov bot commented Feb 23, 2023 •

edited

Loading