Fix that Antrea Agent may crash when dualstack (#4754)

When a Cluster is dualstack, it assumes that every Node is configured with both IPv4 and IPv6 addresses for Antrea Agent. However, if a Node is only configured with either IPv4 or IPv6 address, Antrea Agent will get crashed. Signed-off-by: Hongliang Liu <lhongliang@vmware.com>
antrea-io · Mar 27, 2023 · 3739774 · 3739774
1 parent 0037700
commit 3739774
Show file tree

Hide file tree

Showing 3 changed files with 162 additions and 18 deletions.
diff --git a/pkg/agent/agent.go b/pkg/agent/agent.go
@@ -355,8 +355,15 @@ func (i *Initializer) Initialize() error {
 		return err
 	}
 
-	i.networkConfig.IPv4Enabled = config.IsIPv4Enabled(i.nodeConfig, i.networkConfig.TrafficEncapMode)
-	i.networkConfig.IPv6Enabled = config.IsIPv6Enabled(i.nodeConfig, i.networkConfig.TrafficEncapMode)
+	var err error
+	i.networkConfig.IPv4Enabled, err = config.IsIPv4Enabled(i.nodeConfig, i.networkConfig.TrafficEncapMode)
+	if err != nil {
+		return err
+	}
+	i.networkConfig.IPv6Enabled, err = config.IsIPv6Enabled(i.nodeConfig, i.networkConfig.TrafficEncapMode)
+	if err != nil {
+		return err
+	}
 
 	if err := i.prepareHostNetwork(); err != nil {
 		return err
@@ -460,14 +467,15 @@ func persistRoundNum(num uint64, bridgeClient ovsconfig.OVSBridgeClient, interva
 
 // initOpenFlowPipeline sets up necessary Openflow entries, including pipeline, classifiers, conn_track, and gateway flows
 // Every time the agent is (re)started, we go through the following sequence:
-//   1. agent determines the new round number (this is done by incrementing the round number
-//   persisted in OVSDB, or if it's not available by picking round 1).
-//   2. any existing flow for which the round number matches the round number obtained from step 1
-//   is deleted.
-//   3. all required flows are installed, using the round number obtained from step 1.
-//   4. after convergence, all existing flows for which the round number matches the previous round
-//   number (i.e. the round number which was persisted in OVSDB, if any) are deleted.
-//   5. the new round number obtained from step 1 is persisted to OVSDB.
+//  1. agent determines the new round number (this is done by incrementing the round number
+//     persisted in OVSDB, or if it's not available by picking round 1).
+//  2. any existing flow for which the round number matches the round number obtained from step 1
+//     is deleted.
+//  3. all required flows are installed, using the round number obtained from step 1.
+//  4. after convergence, all existing flows for which the round number matches the previous round
+//     number (i.e. the round number which was persisted in OVSDB, if any) are deleted.
+//  5. the new round number obtained from step 1 is persisted to OVSDB.
+//
 // The rationale for not persisting the new round number until after all previous flows have been
 // deleted is to avoid a situation in which some stale flows are never deleted because of successive
 // agent restarts (with the agent crashing before step 4 can be completed). With the sequence

diff --git a/pkg/agent/config/node_config.go b/pkg/agent/config/node_config.go
@@ -179,16 +179,40 @@ type NetworkConfig struct {
 	IPv6Enabled           bool
 }
 
-// IsIPv4Enabled returns true if the cluster network supports IPv4.
-func IsIPv4Enabled(nodeConfig *NodeConfig, trafficEncapMode TrafficEncapModeType) bool {
-	return nodeConfig.PodIPv4CIDR != nil ||
-		(trafficEncapMode.IsNetworkPolicyOnly() && nodeConfig.NodeIPv4Addr != nil)
+// IsIPv4Enabled returns true if the cluster network supports IPv4. Legal cases are:
+// - NetworkPolicyOnly, NodeIPv4Addr != nil, IPv4 is enabled
+// - NetworkPolicyOnly, NodeIPv4Addr == nil, IPv4 is disabled
+// - Non-NetworkPolicyOnly, PodIPv4CIDR != nil, NodeIPv4Addr != nil, IPv4 is enabled
+// - Non-NetworkPolicyOnly, PodIPv4CIDR == nil, IPv4 is disabled
+func IsIPv4Enabled(nodeConfig *NodeConfig, trafficEncapMode TrafficEncapModeType) (bool, error) {
+	if trafficEncapMode.IsNetworkPolicyOnly() {
+		return nodeConfig.NodeIPv4Addr != nil, nil
+	}
+	if nodeConfig.PodIPv4CIDR != nil {
+		if nodeConfig.NodeIPv4Addr != nil {
+			return true, nil
+		}
+		return false, fmt.Errorf("K8s Node should have an IPv4 address if IPv4 Pod CIDR is defined")
+	}
+	return false, nil
 }
 
-// IsIPv6Enabled returns true if the cluster network supports IPv6.
-func IsIPv6Enabled(nodeConfig *NodeConfig, trafficEncapMode TrafficEncapModeType) bool {
-	return nodeConfig.PodIPv6CIDR != nil ||
-		(trafficEncapMode.IsNetworkPolicyOnly() && nodeConfig.NodeIPv6Addr != nil)
+// IsIPv6Enabled returns true if the cluster network supports IPv6. Legal cases are:
+// - NetworkPolicyOnly, NodeIPv6Addr != nil, IPv6 is enabled
+// - NetworkPolicyOnly, NodeIPv6Addr == nil, IPv6 is disabled
+// - Non-NetworkPolicyOnly, PodIPv6CIDR != nil, NodeIPv6Addr != nil, IPv6 is enabled
+// - Non-NetworkPolicyOnly, PodIPv6CIDR == nil, IPv6 is disabled
+func IsIPv6Enabled(nodeConfig *NodeConfig, trafficEncapMode TrafficEncapModeType) (bool, error) {
+	if trafficEncapMode.IsNetworkPolicyOnly() {
+		return nodeConfig.NodeIPv6Addr != nil, nil
+	}
+	if nodeConfig.PodIPv6CIDR != nil {
+		if nodeConfig.NodeIPv6Addr != nil {
+			return true, nil
+		}
+		return false, fmt.Errorf("K8s Node should have an IPv6 address if IPv6 Pod CIDR is defined")
+	}
+	return false, nil
 }
 
 // NeedsTunnelToPeer returns true if Pod traffic to peer Node needs to be encapsulated by OVS tunneling.

diff --git a/pkg/agent/config/node_config_test.go b/pkg/agent/config/node_config_test.go
@@ -163,3 +163,115 @@ func TestNetworkConfig_NeedsDirectRoutingToPeer(t *testing.T) {
 		})
 	}
 }
+
+func TestIsIPv4Enabled(t *testing.T) {
+	_, podIPv4CIDR, _ := net.ParseCIDR("10.10.0.0/24")
+	_, nodeIPv4Addr, _ := net.ParseCIDR("192.168.77.100/24")
+	tests := []struct {
+		name                string
+		nodeConfig          *NodeConfig
+		trafficEncapMode    TrafficEncapModeType
+		expectedErrString   string
+		expectedWithError   bool
+		expectedIPv4Enabled bool
+	}{
+		{
+			name: "Non-NetworkPolicyOnly, with IPv4PodCIDR, without NodeIPv4Addr",
+			nodeConfig: &NodeConfig{
+				PodIPv4CIDR: podIPv4CIDR,
+			},
+			expectedWithError:   true,
+			expectedErrString:   "K8s Node should have an IPv4 address if IPv4 Pod CIDR is defined",
+			expectedIPv4Enabled: false,
+		},
+		{
+			name: "Non-NetworkPolicyOnly, with IPv4PodCIDR, with NodeIPv4Addr",
+			nodeConfig: &NodeConfig{
+				PodIPv4CIDR:  podIPv4CIDR,
+				NodeIPv4Addr: nodeIPv4Addr,
+			},
+			expectedIPv4Enabled: true,
+		},
+		{
+			name:                "NetworkPolicyOnly, without NodeIPv4Addr",
+			nodeConfig:          &NodeConfig{},
+			trafficEncapMode:    TrafficEncapModeNetworkPolicyOnly,
+			expectedIPv4Enabled: false,
+		},
+		{
+			name: "NetworkPolicyOnly, with NodeIPv4Addr",
+			nodeConfig: &NodeConfig{
+				NodeIPv4Addr: nodeIPv4Addr,
+			},
+			trafficEncapMode:    TrafficEncapModeNetworkPolicyOnly,
+			expectedIPv4Enabled: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ipv4Enabled, err := IsIPv4Enabled(tt.nodeConfig, tt.trafficEncapMode)
+			if tt.expectedWithError {
+				assert.ErrorContains(t, err, tt.expectedErrString)
+			} else {
+				assert.NoError(t, err)
+			}
+			assert.Equal(t, tt.expectedIPv4Enabled, ipv4Enabled)
+		})
+	}
+}
+
+func TestIsIPv6Enabled(t *testing.T) {
+	_, podIPv6CIDR, _ := net.ParseCIDR("10:10::/64")
+	_, nodeIPv6Addr, _ := net.ParseCIDR("192:168:77::100/80")
+	tests := []struct {
+		name                string
+		nodeConfig          *NodeConfig
+		trafficEncapMode    TrafficEncapModeType
+		expectedWithError   bool
+		expectedErrString   string
+		expectedIPv6Enabled bool
+	}{
+		{
+			name: "Non-NetworkPolicyOnly, with IPv6PodCIDR, without NodeIPv6Addr",
+			nodeConfig: &NodeConfig{
+				PodIPv6CIDR: podIPv6CIDR,
+			},
+			expectedWithError:   true,
+			expectedErrString:   "K8s Node should have an IPv6 address if IPv6 Pod CIDR is defined",
+			expectedIPv6Enabled: false,
+		},
+		{
+			name: "Non-NetworkPolicyOnly, with IPv6PodCIDR, with NodeIPv6Addr",
+			nodeConfig: &NodeConfig{
+				PodIPv6CIDR:  podIPv6CIDR,
+				NodeIPv6Addr: nodeIPv6Addr,
+			},
+			expectedIPv6Enabled: true,
+		},
+		{
+			name:                "NetworkPolicyOnly, without NodeIPv6Addr",
+			nodeConfig:          &NodeConfig{},
+			trafficEncapMode:    TrafficEncapModeNetworkPolicyOnly,
+			expectedIPv6Enabled: false,
+		},
+		{
+			name: "NetworkPolicyOnly, with NodeIPv6Addr",
+			nodeConfig: &NodeConfig{
+				NodeIPv6Addr: nodeIPv6Addr,
+			},
+			trafficEncapMode:    TrafficEncapModeNetworkPolicyOnly,
+			expectedIPv6Enabled: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ipv6Enabled, err := IsIPv6Enabled(tt.nodeConfig, tt.trafficEncapMode)
+			if tt.expectedWithError {
+				assert.ErrorContains(t, err, tt.expectedErrString)
+			} else {
+				assert.NoError(t, err)
+			}
+			assert.Equal(t, tt.expectedIPv6Enabled, ipv6Enabled)
+		})
+	}
+}