-
Notifications
You must be signed in to change notification settings - Fork 366
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.
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
Reject the request to a Service without an Endpoint #4656
Reject the request to a Service without an Endpoint #4656
Conversation
Codecov Report
@@ 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
*This pull request uses carry forward flags. Click here to find out more.
|
bf1a471
to
9a6448a
Compare
/test-all |
Maybe we should add E2E tests for this. |
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
9a6448a
to
451ab76
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nits.
500934c
to
be3fba1
Compare
pkg/agent/openflow/service.go
Outdated
@@ -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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use priorityLow
not priorityHigh
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do other packets match the flow? Other packets does not contain the regmark
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't it use priorityNormal if there is no special reason we want it to be prioritized higher or lower?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
func (p *proxier) HandlePacketIn(pktIn *ofctrl.PacketIn) error { | ||
if pktIn == nil { | ||
return errors.New("empty packetin for Antrea Proxy") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is it added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reminder
4020f00
to
fe2a8a3
Compare
fe2a8a3
to
d0e1848
Compare
@@ -90,20 +85,9 @@ func (s *IGMPSnooper) HandlePacketIn(pktIn *ofctrl.PacketIn) error { | |||
return nil | |||
} | |||
|
|||
func getInfoInReg(regMatch *ofctrl.MatchField, rng *openflow15.NXRange) (uint32, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is planned to be provided in package pkg/agent/openflow, maybe also update the function calls under path pkg/agent/controller/networkpolicy ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but I think we could do this in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am OK to use a new patch for it.
pkg/agent/openflow/service.go
Outdated
@@ -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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/service.go
Outdated
@@ -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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't it use priorityNormal if there is no special reason we want it to be prioritized higher or lower?
func (p *proxier) HandlePacketIn(pktIn *ofctrl.PacketIn) error { | ||
if pktIn == nil { | ||
return errors.New("empty packetin for Antrea Proxy") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is it added?
d0e1848
to
ecdc028
Compare
f3225bc
to
0b5e28f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, two nits
0b5e28f
to
74d2cc4
Compare
pkg/agent/proxy/proxier.go
Outdated
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 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
L1032-L1046 is for https://github.com/antrea-io/antrea/pull/4656/files#r1142851197 @tnqn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I proposed this solution (#4726) to optimize the packetIn mechnism, do you think we can schedule it in the next release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does NXAST_CONTROLLER2 require more recent OVS than the documented one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it is provided since OVS v2.6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then it looks great to me.
74d2cc4
to
4fa4822
Compare
c108992
to
d859684
Compare
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>
d859684
to
44e572e
Compare
/test-all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
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 placein pkg/agent/openflow/field.go.
Signed-off-by: Hongliang Liu lhongliang@vmware.com