Skip to content

Commit

Permalink
Address comments
Browse files Browse the repository at this point in the history 
Signed-off-by: graysonwu <wgrayson@vmware.com>
  • Loading branch information
@GraysonWu
GraysonWu committed Mar 8, 2023
1 parent ac6c5d3 commit 2db5e01
Show file tree
Hide file tree
Showing 8 changed files with 80 additions and 66 deletions.
41 changes: 22 additions & 19 deletions pkg/agent/openflow/network_policy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ var (
MatchServiceGroupID = types.NewMatchKey(binding.ProtocolIP, types.ServiceGroupIDAddr, "reg7[0..31]")
MatchIGMPProtocol = types.NewMatchKey(binding.ProtocolIGMP, types.IGMPAddr, "igmp")
MatchLabelID = types.NewMatchKey(binding.ProtocolIP, types.LabelIDAddr, "tun_id")
MatchTCPFlag = types.NewMatchKey(binding.ProtocolTCP, types.TCPFlagAddr, "tcp_flags")
MatchTCPFlags = types.NewMatchKey(binding.ProtocolTCP, types.TCPFlagsAddr, "tcp_flags")
Unsupported = types.NewMatchKey(binding.ProtocolIP, types.UnSupported, "unknown")

// metricFlowIdentifier is used to identify metric flows in metric table.
Expand Down Expand Up @@ -713,31 +713,34 @@ func (c *client) NewDNSPacketInConjunction(id uint32) error {
Protocol: &protocolUDP,
Port: &dnsPort,
}

c.featureNetworkPolicy.conjMatchFlowLock.Lock()
defer c.featureNetworkPolicy.conjMatchFlowLock.Unlock()
ctxChanges := conj.serviceClause.addServiceFlows(c.featureNetworkPolicy, []v1beta2.Service{udpService}, &dnsPriority, true, false)
tcpService := v1beta2.Service{
Protocol: &protocolTCP,
Port: &dnsPort,
}
tcpServiceMatch := &conjunctiveMatch{
tableID: conj.serviceClause.ruleTable.GetID(),
matchPairs: []matchPair{
getServiceMatchPairs(tcpService, c.featureNetworkPolicy.ipProtocols, true)[0][0],
{
matchKey: MatchTCPFlag,
matchValue: TCPFlag{
// URG|ACK|PSH|RST|SYN|FIN|
Flag: 0b011000,
Mask: 0b011000,
preDNSTCPMatchPairs := getServiceMatchPairs(tcpService, c.featureNetworkPolicy.ipProtocols, true)
for _, preTcpMatchPair := range preDNSTCPMatchPairs {
tcpServiceMatch := &conjunctiveMatch{
tableID: conj.serviceClause.ruleTable.GetID(),
matchPairs: []matchPair{
preTcpMatchPair[0],
{
matchKey: MatchTCPFlags,
matchValue: TCPFlag{
// URG|ACK|PSH|RST|SYN|FIN|
Flag: 0b011000,
Mask: 0b011000,
},
},
},
},
priority: &dnsPriority,
priority: &dnsPriority,
}
ctxChange := conj.serviceClause.addConjunctiveMatchFlow(c.featureNetworkPolicy, tcpServiceMatch, false, false)
ctxChanges = append(ctxChanges, ctxChange)
}

c.featureNetworkPolicy.conjMatchFlowLock.Lock()
defer c.featureNetworkPolicy.conjMatchFlowLock.Unlock()
ctxChanges := conj.serviceClause.addServiceFlows(c.featureNetworkPolicy, []v1beta2.Service{udpService}, &dnsPriority, true, false)
ctxChange := conj.serviceClause.addConjunctiveMatchFlow(c.featureNetworkPolicy, tcpServiceMatch, false, false)
ctxChanges = append(ctxChanges, ctxChange)
if err := c.featureNetworkPolicy.applyConjunctiveMatchFlows(ctxChanges); err != nil {
return err
}
Expand Down
8 changes: 3 additions & 5 deletions pkg/agent/openflow/pipeline.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2014,12 +2014,10 @@ func (f *featureNetworkPolicy) addFlowMatch(fb binding.FlowBuilder, matchKey *ty
fb = fb.MatchProtocol(matchKey.GetOFProtocol())
case MatchLabelID:
fb = fb.MatchTunnelID(uint64(matchValue.(uint32)))
case MatchTCPFlag:
case MatchTCPFlags:
fb = fb.MatchProtocol(matchKey.GetOFProtocol())
if matchValue != nil {
tcpFlag := matchValue.(TCPFlag)
fb = fb.MatchTCPFlag(tcpFlag.Flag, tcpFlag.Mask)
}
tcpFlag := matchValue.(TCPFlag)
fb = fb.MatchTCPFlags(tcpFlag.Flag, tcpFlag.Mask)
}
return fb
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/agent/types/networkpolicy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ const (
ServiceGroupIDAddr
IGMPAddr
LabelIDAddr
TCPFlagAddr
TCPFlagsAddr
UnSupported
)

Expand Down
2 changes: 1 addition & 1 deletion pkg/ovs/openflow/interfaces.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ type FlowBuilder interface {
MatchConjID(value uint32) FlowBuilder
MatchDstPort(port uint16, portMask *uint16) FlowBuilder
MatchSrcPort(port uint16, portMask *uint16) FlowBuilder
MatchTCPFlag(flag, mask uint16) FlowBuilder
MatchTCPFlags(flag, mask uint16) FlowBuilder
MatchICMPType(icmpType byte) FlowBuilder
MatchICMPCode(icmpCode byte) FlowBuilder
MatchICMPv6Type(icmp6Type byte) FlowBuilder
Expand Down
2 changes: 1 addition & 1 deletion pkg/ovs/openflow/ofctrl_builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -598,7 +598,7 @@ func (b *ofFlowBuilder) MatchSrcPort(port uint16, portMask *uint16) FlowBuilder
return b
}

func (b *ofFlowBuilder) MatchTCPFlag(flag, mask uint16) FlowBuilder {
func (b *ofFlowBuilder) MatchTCPFlags(flag, mask uint16) FlowBuilder {
b.matchers = append(b.matchers, fmt.Sprintf("tcp_flags=%b/%b", uint8(flag), uint8(mask)))
b.Match.TcpFlags = &flag
b.Match.TcpFlagsMask = &mask
Expand Down
15 changes: 14 additions & 1 deletion pkg/ovs/openflow/ofctrl_packetin.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,20 @@ func GetTCPPacketFromIPMessage(ipPkt util.Message) (tcpPkt *protocol.TCP, err er
func GetTCPDataWithoutOptions(tcpPkt *protocol.TCP) (data []byte, err error) {
// TCP.HdrLen is 4-octet unit indicating the length of TCP header including options.
tcpOptionsLen := (tcpPkt.HdrLen - tcpStandardHdrLen) * 4
// Trim TCP option end mark which has 1 byte for kind, 1 byte for length and no value.
// Trim TCP option end mark. From RFC 793:
// Specific Option Definitions
// End of Option List
// +--------+
// |00000000|
// +--------+
// Kind=0
//
// This option code indicates the end of the option list. This
// might not coincide with the end of the TCP header according to
// the Data Offset field. This is used at the end of all options,
// not the end of each option, and need only be used if the end of
// the options would not otherwise coincide with the end of the TCP
// header.
if tcpPkt.Data[tcpOptionsLen] == tcpOptionEndKind {
tcpOptionsLen += 2
}
Expand Down
14 changes: 7 additions & 7 deletions pkg/ovs/openflow/testing/mock_openflow.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

62 changes: 31 additions & 31 deletions test/e2e/antreapolicy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1554,7 +1554,7 @@ func testANPGroupServiceRefDelete(t *testing.T) {
k8sUtils.Validate(allPods, reachability, []int32{80}, ProtocolTCP)
_, wrong, _ := reachability.Summary()
if wrong != 0 {
t.Errorf("failure -- %d wrong results", wrong)
t.Errorf("Failure -- %d wrong results", wrong)
reachability.PrintSummary(true, true, true)
}
// Delete services, pods should be connected.
Expand All @@ -1565,7 +1565,7 @@ func testANPGroupServiceRefDelete(t *testing.T) {
k8sUtils.Validate(allPods, reachability2, []int32{80}, ProtocolTCP)
_, wrong, _ = reachability2.Summary()
if wrong != 0 {
t.Errorf("failure -- %d wrong results", wrong)
t.Errorf("Failure -- %d wrong results", wrong)
reachability2.PrintSummary(true, true, true)
}
// Cleanup test resources.
Expand Down Expand Up @@ -2262,10 +2262,10 @@ func testRejectServiceTraffic(t *testing.T, data *TestData) {
log.Tracef("Probing: %s -> %s:%d", tc.clientPod.PodName(), tc.destAddr, tc.destPort)
connectivity, err := k8sUtils.ProbeAddr(tc.clientPod.Namespace(), "antrea-e2e", tc.clientPod.PodName(), tc.destAddr, tc.destPort, ProtocolTCP)
if err != nil {
t.Errorf("failure -- could not complete probe: %v", err)
t.Errorf("Failure -- could not complete probe: %v", err)
}
if connectivity != tc.expectedConnectivity {
t.Errorf("failure -- wrong results for probe: Source %s/%s --> Dest %s:%d connectivity: %v, expected: %v",
t.Errorf("Failure -- wrong results for probe: Source %s/%s --> Dest %s:%d connectivity: %v, expected: %v",
tc.clientPod.Namespace(), tc.clientPod.PodName(), tc.destAddr, tc.destPort, connectivity, tc.expectedConnectivity)
}
}
Expand All @@ -2287,10 +2287,10 @@ func testRejectServiceTraffic(t *testing.T, data *TestData) {
log.Tracef("Probing: %s -> %s:%d", tc.clientPod.PodName(), tc.destAddr, tc.destPort)
connectivity, err := k8sUtils.ProbeAddr(tc.clientPod.Namespace(), "antrea-e2e", tc.clientPod.PodName(), tc.destAddr, tc.destPort, ProtocolTCP)
if err != nil {
t.Errorf("failure -- could not complete probe: %v", err)
t.Errorf("Failure -- could not complete probe: %v", err)
}
if connectivity != tc.expectedConnectivity {
t.Errorf("failure -- wrong results for probe: Source %s/%s --> Dest %s:%d connectivity: %v, expected: %v",
t.Errorf("Failure -- wrong results for probe: Source %s/%s --> Dest %s:%d connectivity: %v, expected: %v",
tc.clientPod.Namespace(), tc.clientPod.PodName(), tc.destAddr, tc.destPort, connectivity, tc.expectedConnectivity)
}
}
Expand Down Expand Up @@ -2353,10 +2353,10 @@ func testRejectNoInfiniteLoop(t *testing.T, data *TestData) {
log.Tracef("Probing: %s -> %s:%d", tc.clientPod.PodName(), tc.destAddr, tc.destPort)
connectivity, err := k8sUtils.ProbeAddr(tc.clientPod.Namespace(), "antrea-e2e", tc.clientPod.PodName(), tc.destAddr, tc.destPort, ProtocolTCP)
if err != nil {
t.Errorf("failure -- could not complete probe: %v", err)
t.Errorf("Failure -- could not complete probe: %v", err)
}
if connectivity != tc.expectedConnectivity {
t.Errorf("failure -- wrong results for probe: Source %s/%s --> Dest %s:%d connectivity: %v, expected: %v",
t.Errorf("Failure -- wrong results for probe: Source %s/%s --> Dest %s:%d connectivity: %v, expected: %v",
tc.clientPod.Namespace(), tc.clientPod.PodName(), tc.destAddr, tc.destPort, connectivity, tc.expectedConnectivity)
}
}
Expand Down Expand Up @@ -2515,7 +2515,7 @@ func testANPMultipleAppliedTo(t *testing.T, data *TestData, singleRule bool) {
k8sUtils.Validate(allPods, reachability, []int32{80}, ProtocolTCP)
_, wrong, _ := reachability.Summary()
if wrong != 0 {
t.Errorf("failure -- %d wrong results", wrong)
t.Errorf("Failure -- %d wrong results", wrong)
reachability.PrintSummary(true, true, true)
}

Expand All @@ -2534,7 +2534,7 @@ func testANPMultipleAppliedTo(t *testing.T, data *TestData, singleRule bool) {
k8sUtils.Validate(allPods, reachability, []int32{80}, ProtocolTCP)
_, wrong, _ = reachability.Summary()
if wrong != 0 {
t.Errorf("failure -- %d wrong results", wrong)
t.Errorf("Failure -- %d wrong results", wrong)
reachability.PrintSummary(true, true, true)
}

Expand All @@ -2548,7 +2548,7 @@ func testANPMultipleAppliedTo(t *testing.T, data *TestData, singleRule bool) {
k8sUtils.Validate(allPods, reachability, []int32{80}, ProtocolTCP)
_, wrong, _ = reachability.Summary()
if wrong != 0 {
t.Errorf("failure -- %d wrong results", wrong)
t.Errorf("Failure -- %d wrong results", wrong)
reachability.PrintSummary(true, true, true)
}

Expand Down Expand Up @@ -3215,10 +3215,10 @@ func testFQDNPolicy(t *testing.T) {
log.Tracef("Probing: %s -> %s", tc.clientPod.PodName(), tc.destAddr)
connectivity, err := k8sUtils.ProbeAddr(tc.clientPod.Namespace(), "pod", tc.clientPod.PodName(), tc.destAddr, tc.destPort, ProtocolTCP)
if err != nil {
t.Errorf("failure -- could not complete probe: %v", err)
t.Errorf("Failure -- could not complete probe: %v", err)
}
if connectivity != tc.expectedConnectivity {
t.Errorf("failure -- wrong results for probe: Source %s/%s --> Dest %s:%d connectivity: %v, expected: %v",
t.Errorf("Failure -- wrong results for probe: Source %s/%s --> Dest %s:%d connectivity: %v, expected: %v",
tc.clientPod.Namespace(), tc.clientPod.PodName(), tc.destAddr, tc.destPort, connectivity, tc.expectedConnectivity)
}
}
Expand Down Expand Up @@ -3306,10 +3306,10 @@ func testFQDNPolicyInClusterService(t *testing.T) {
log.Tracef("Probing: %s -> %s", tc.clientPod.PodName(), tc.destAddr)
connectivity, err := k8sUtils.ProbeAddr(tc.clientPod.Namespace(), "pod", tc.clientPod.PodName(), tc.destAddr, tc.destPort, ProtocolTCP)
if err != nil {
t.Errorf("failure -- could not complete probe: %v", err)
t.Errorf("Failure -- could not complete probe: %v", err)
}
if connectivity != tc.expectedConnectivity {
t.Errorf("failure -- wrong results for probe: Source %s/%s --> Dest %s:%d connectivity: %v, expected: %v",
t.Errorf("Failure -- wrong results for probe: Source %s/%s --> Dest %s:%d connectivity: %v, expected: %v",
tc.clientPod.Namespace(), tc.clientPod.PodName(), tc.destAddr, tc.destPort, connectivity, tc.expectedConnectivity)
}
}
Expand Down Expand Up @@ -3355,10 +3355,10 @@ func testFQDNPolicyTCP(t *testing.T) {
log.Tracef("Probing: %s -> %s(%s)", tc.clientPod.PodName(), tc.destAddr, destIP)
connectivity, err := k8sUtils.ProbeAddr(tc.clientPod.Namespace(), "pod", tc.clientPod.PodName(), destIP, tc.destPort, ProtocolTCP)
if err != nil {
t.Errorf("failure -- could not complete probe: %v", err)
t.Errorf("Failure -- could not complete probe: %v", err)
}
if connectivity != tc.expectedConnectivity {
t.Errorf("failure -- wrong results for probe: Source %s/%s --> Dest %s:%d connectivity: %v, expected: %v",
t.Errorf("Failure -- wrong results for probe: Source %s/%s --> Dest %s:%d connectivity: %v, expected: %v",
tc.clientPod.Namespace(), tc.clientPod.PodName(), tc.destAddr, tc.destPort, connectivity, tc.expectedConnectivity)
}
}
Expand Down Expand Up @@ -3426,10 +3426,10 @@ func testToServices(t *testing.T) {
log.Tracef("Probing: %s -> %s", tc.clientPod.PodName(), tc.destAddr)
connectivity, err := k8sUtils.ProbeAddr(tc.clientPod.Namespace(), "pod", tc.clientPod.PodName(), tc.destAddr, tc.destPort, ProtocolTCP)
if err != nil {
t.Errorf("failure -- could not complete probe: %v", err)
t.Errorf("Failure -- could not complete probe: %v", err)
}
if connectivity != tc.expectedConnectivity {
t.Errorf("failure -- wrong results for probe: Source %s/%s --> Dest %s:%d connectivity: %v, expected: %v",
t.Errorf("Failure -- wrong results for probe: Source %s/%s --> Dest %s:%d connectivity: %v, expected: %v",
tc.clientPod.Namespace(), tc.clientPod.PodName(), tc.destAddr, tc.destPort, connectivity, tc.expectedConnectivity)
}
}
Expand Down Expand Up @@ -3513,10 +3513,10 @@ func testServiceAccountSelector(t *testing.T, data *TestData) {
log.Tracef("Probing: %s -> %s:%d", tc.clientPod.PodName(), tc.destAddr, tc.destPort)
connectivity, err := k8sUtils.ProbeAddr(tc.clientPod.Namespace(), "antrea-e2e", tc.clientPod.PodName(), tc.destAddr, tc.destPort, ProtocolTCP)
if err != nil {
t.Errorf("failure -- could not complete probe: %v", err)
t.Errorf("Failure -- could not complete probe: %v", err)
}
if connectivity != tc.expectedConnectivity {
t.Errorf("failure -- wrong results for probe: Source %s/%s --> Dest %s:%d connectivity: %v, expected: %v",
t.Errorf("Failure -- wrong results for probe: Source %s/%s --> Dest %s:%d connectivity: %v, expected: %v",
tc.clientPod.Namespace(), tc.clientPod.PodName(), tc.destAddr, tc.destPort, connectivity, tc.expectedConnectivity)
}
}
Expand Down Expand Up @@ -3575,10 +3575,10 @@ func testACNPNodeSelectorEgress(t *testing.T) {
log.Tracef("Probing: %s -> %s", tc.clientPod.PodName(), tc.destAddr)
connectivity, err := k8sUtils.ProbeAddr(tc.clientPod.Namespace(), "pod", tc.clientPod.PodName(), tc.destAddr, tc.destPort, ProtocolTCP)
if err != nil {
t.Errorf("failure -- could not complete probe: %v", err)
t.Errorf("Failure -- could not complete probe: %v", err)
}
if connectivity != tc.expectedConnectivity {
t.Errorf("failure -- wrong results for probe: Source %s/%s --> Dest %s:%d connectivity: %v, expected: %v",
t.Errorf("Failure -- wrong results for probe: Source %s/%s --> Dest %s:%d connectivity: %v, expected: %v",
tc.clientPod.Namespace(), tc.clientPod.PodName(), tc.destAddr, tc.destPort, connectivity, tc.expectedConnectivity)
}
}
Expand Down Expand Up @@ -3650,10 +3650,10 @@ func testACNPNodeSelectorIngress(t *testing.T, data *TestData) {
log.Tracef("Probing: %s -> %s", tc.clientPod.PodName(), tc.destAddr)
connectivity, err := k8sUtils.ProbeAddr(tc.clientPod.Namespace(), "antrea-e2e", tc.clientPod.PodName(), tc.destAddr, tc.destPort, ProtocolTCP)
if err != nil {
t.Errorf("failure -- could not complete probe: %v", err)
t.Errorf("Failure -- could not complete probe: %v", err)
}
if connectivity != tc.expectedConnectivity {
t.Errorf("failure -- wrong results for probe: Source %s/%s --> Dest %s:%d connectivity: %v, expected: %v",
t.Errorf("Failure -- wrong results for probe: Source %s/%s --> Dest %s:%d connectivity: %v, expected: %v",
tc.clientPod.Namespace(), tc.clientPod.PodName(), tc.destAddr, tc.destPort, connectivity, tc.expectedConnectivity)
}
}
Expand Down Expand Up @@ -3722,10 +3722,10 @@ func testACNPICMPSupport(t *testing.T, data *TestData) {
log.Tracef("Probing: %s -> %s", tc.clientPod.PodName(), tc.destAddr)
connectivity, err := k8sUtils.ProbeAddr(tc.clientPod.Namespace(), "antrea-e2e", tc.clientPod.PodName(), tc.destAddr, tc.destPort, ProtocolICMP)
if err != nil {
t.Errorf("failure -- could not complete probe: %v", err)
t.Errorf("Failure -- could not complete probe: %v", err)
}
if connectivity != tc.expectedConnectivity {
t.Errorf("failure -- wrong results for probe: Source %s/%s --> Dest %s:%d connectivity: %v, expected: %v",
t.Errorf("Failure -- wrong results for probe: Source %s/%s --> Dest %s:%d connectivity: %v, expected: %v",
tc.clientPod.Namespace(), tc.clientPod.PodName(), tc.destAddr, tc.destPort, connectivity, tc.expectedConnectivity)
}
}
Expand Down Expand Up @@ -3812,7 +3812,7 @@ sleep 3600
connectivity = DecideProbeResult(stderr, 3)
}
if connectivity != Rejected {
t.Errorf("failure -- wrong results for probe: Source 1.1.1.1 --> Dest %s:%d connectivity: %v, expected: Rej", nodeIP(idx), nodePort, connectivity)
t.Errorf("Failure -- wrong results for probe: Source 1.1.1.1 --> Dest %s:%d connectivity: %v, expected: Rej", nodeIP(idx), nodePort, connectivity)
}
}
failOnError(k8sUtils.DeleteACNP(builder.Name), t)
Expand Down Expand Up @@ -4035,7 +4035,7 @@ func executeTestsWithData(t *testing.T, testList []*TestCase, data *TestData) {

_, wrong, _ := step.Reachability.Summary()
if wrong != 0 {
t.Errorf("failure -- %d wrong results", wrong)
t.Errorf("Failure -- %d wrong results", wrong)
reachability.PrintSummary(true, true, true)
}
}
Expand All @@ -4062,10 +4062,10 @@ func doProbe(t *testing.T, data *TestData, p *CustomProbe, protocol AntreaPolicy
log.Tracef("Probing: %s -> %s", p.SourcePod.Pod.PodName(), p.DestPod.Pod.PodName())
connectivity, err := k8sUtils.Probe(p.SourcePod.Pod.Namespace(), p.SourcePod.Pod.PodName(), p.DestPod.Pod.Namespace(), p.DestPod.Pod.PodName(), p.Port, protocol)
if err != nil {
t.Errorf("failure -- could not complete probe: %v", err)
t.Errorf("Failure -- could not complete probe: %v", err)
}
if connectivity != p.ExpectConnectivity {
t.Errorf("failure -- wrong results for custom probe: Source %s/%s --> Dest %s/%s connectivity: %v, expected: %v",
t.Errorf("Failure -- wrong results for custom probe: Source %s/%s --> Dest %s/%s connectivity: %v, expected: %v",
p.SourcePod.Pod.Namespace(), p.SourcePod.Pod.PodName(), p.DestPod.Pod.Namespace(), p.DestPod.Pod.PodName(), connectivity, p.ExpectConnectivity)
}
}
Expand Down

0 comments on commit 2db5e01

Please sign in to comment.