Skip to content

Commit

Permalink
fix: Windows CNI Overlay Gateway Bug (#1849)
Browse files Browse the repository at this point in the history 
* fix overlay gatway bug that prevented cloud-node-manager-windows from initializing node because it couldn't reach IMDS

* add overlay invoker cns test

* use parseip instead of mask

* scope CNI overlay gateway bug fix to windows only

* update test

* adding UT to verify overlay NAT info is empty

* add podsubnet netInfo test

---------

Co-authored-by: Jaeryn <tsun.chu@microsoft.com>
(cherry picked from commit dac5b31)
  • Loading branch information
@jaer-tsun @jpayne3506
jaer-tsun authored and jpayne3506 committed Sep 11, 2023
1 parent 27376dd commit c325121
Show file tree
Hide file tree
Showing 6 changed files with 147 additions and 15 deletions.
22 changes: 12 additions & 10 deletions cni/network/invoker_cns.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,8 @@ import (
)

var (
errEmptyCNIArgs = errors.New("empty CNI cmd args not allowed")
errInvalidArgs = errors.New("invalid arg(s)")
overlayGatewayIP = "169.254.1.1"
errEmptyCNIArgs = errors.New("empty CNI cmd args not allowed")
errInvalidArgs = errors.New("invalid arg(s)")
)

type CNSIPAMInvoker struct {
Expand Down Expand Up @@ -99,19 +98,22 @@ func (invoker *CNSIPAMInvoker) Add(addConfig IPAMAddConfig) (IPAMAddResult, erro

log.Printf("[cni-invoker-cns] Received info %+v for pod %v", info, podInfo)

// set result ipconfigArgument from CNS Response Body
ip, ncipnet, err := net.ParseCIDR(info.podIPAddress + "/" + fmt.Sprint(info.ncSubnetPrefix))
if ip == nil {
return IPAMAddResult{}, errors.Wrap(err, "Unable to parse IP from response: "+info.podIPAddress+" with err %w")
}

ncgw := net.ParseIP(info.ncGatewayIPAddress)
if ncgw == nil {
if invoker.ipamMode != util.V4Overlay {
return IPAMAddResult{}, errors.Wrap(errInvalidArgs, "%w: Gateway address "+info.ncGatewayIPAddress+" from response is invalid")
}

ncgw = net.ParseIP(overlayGatewayIP)
}

// set result ipconfigArgument from CNS Response Body
ip, ncipnet, err := net.ParseCIDR(info.podIPAddress + "/" + fmt.Sprint(info.ncSubnetPrefix))
if ip == nil {
return IPAMAddResult{}, errors.Wrap(err, "Unable to parse IP from response: "+info.podIPAddress+" with err %w")
ncgw, err = getOverlayGateway(ncipnet)
if err != nil {
return IPAMAddResult{}, err
}
}

// construct ipnet for result
Expand Down
87 changes: 87 additions & 0 deletions cni/network/invoker_cns_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@ package network

import (
"errors"
"fmt"
"net"
"runtime"
"testing"

"github.com/Azure/azure-container-networking/cni"
"github.com/Azure/azure-container-networking/cni/util"
"github.com/Azure/azure-container-networking/cns"
"github.com/Azure/azure-container-networking/iptables"
"github.com/Azure/azure-container-networking/network"
Expand All @@ -25,19 +28,29 @@ func getTestIPConfigRequest() cns.IPConfigRequest {
}
}

func getTestOverlayGateway() net.IP {
if runtime.GOOS == "windows" {
return net.ParseIP("10.240.0.1")
}

return net.ParseIP("169.254.1.1")
}

func TestCNSIPAMInvoker_Add(t *testing.T) {
require := require.New(t) //nolint further usage of require without passing t
type fields struct {
podName string
podNamespace string
cnsClient cnsclient
ipamMode util.IpamMode
}
type args struct {
nwCfg *cni.NetworkConfig
args *cniSkel.CmdArgs
hostSubnetPrefix *net.IPNet
options map[string]interface{}
}

tests := []struct {
name string
fields fields
Expand Down Expand Up @@ -127,6 +140,76 @@ func TestCNSIPAMInvoker_Add(t *testing.T) {
},
wantErr: true,
},
{
name: "Test happy CNI Overlay add",
fields: fields{
podName: testPodInfo.PodName,
podNamespace: testPodInfo.PodNamespace,
ipamMode: util.V4Overlay,
cnsClient: &MockCNSClient{
require: require,
request: requestIPAddressHandler{
ipconfigArgument: cns.IPConfigRequest{
PodInterfaceID: "testcont-testifname3",
InfraContainerID: "testcontainerid3",
OrchestratorContext: marshallPodInfo(testPodInfo),
},
result: &cns.IPConfigResponse{
PodIpInfo: cns.PodIpInfo{
PodIPConfig: cns.IPSubnet{
IPAddress: "10.240.1.242",
PrefixLength: 16,
},
NetworkContainerPrimaryIPConfig: cns.IPConfiguration{
IPSubnet: cns.IPSubnet{
IPAddress: "10.240.1.0",
PrefixLength: 16,
},
DNSServers: nil,
GatewayIPAddress: "",
},
HostPrimaryIPInfo: cns.HostIPInfo{
Gateway: "10.224.0.1",
PrimaryIP: "10.224.0.5",
Subnet: "10.224.0.0/16",
},
},
Response: cns.Response{
ReturnCode: 0,
Message: "",
},
},
err: nil,
},
},
},
args: args{
nwCfg: &cni.NetworkConfig{},
args: &cniSkel.CmdArgs{
ContainerID: "testcontainerid3",
Netns: "testnetns3",
IfName: "testifname3",
},
hostSubnetPrefix: getCIDRNotationForAddress("10.224.0.0/16"),
options: map[string]interface{}{},
},
want: &cniTypesCurr.Result{
IPs: []*cniTypesCurr.IPConfig{
{
Address: *getCIDRNotationForAddress("10.240.1.242/16"),
Gateway: getTestOverlayGateway(),
},
},
Routes: []*cniTypes.Route{
{
Dst: network.Ipv4DefaultRouteDstPrefix,
GW: getTestOverlayGateway(),
},
},
},
want1: nil,
wantErr: false,
},
}
for _, tt := range tests {
tt := tt
Expand All @@ -136,13 +219,17 @@ func TestCNSIPAMInvoker_Add(t *testing.T) {
podNamespace: tt.fields.podNamespace,
cnsClient: tt.fields.cnsClient,
}
if tt.fields.ipamMode != "" {
invoker.ipamMode = tt.fields.ipamMode
}
ipamAddResult, err := invoker.Add(IPAMAddConfig{nwCfg: tt.args.nwCfg, args: tt.args.args, options: tt.args.options})
if tt.wantErr {
require.Error(err)
} else {
require.NoError(err)
}

fmt.Printf("want:%+v\nrest:%+v\n", tt.want, ipamAddResult.ipv4Result)
require.Equalf(tt.want, ipamAddResult.ipv4Result, "incorrect ipv4 response")
require.Equalf(tt.want1, ipamAddResult.ipv6Result, "incorrect ipv6 response")
})
Expand Down
2 changes: 1 addition & 1 deletion cni/network/network.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -536,7 +536,7 @@ func (plugin *NetPlugin) Add(args *cniSkel.CmdArgs) error {
logAndSendEvent(plugin, fmt.Sprintf("[cni-net] Created network %v with subnet %v.", networkID, ipamAddResult.hostSubnetPrefix.String()))
}

natInfo := getNATInfo(nwCfg.ExecutionMode, options[network.SNATIPKey], nwCfg.MultiTenancy, enableSnatForDNS)
natInfo := getNATInfo(nwCfg, options[network.SNATIPKey], enableSnatForDNS)

createEndpointInternalOpt := createEndpointInternalOpt{
nwCfg: nwCfg,
Expand Down
11 changes: 10 additions & 1 deletion cni/network/network_linux.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,17 @@ func (plugin *NetPlugin) getNetworkName(_ string, _ *IPAMAddResult, nwCfg *cni.N
return nwCfg.Name, nil
}

func getNATInfo(_ string, _ interface{}, _, _ bool) (natInfo []policy.NATInfo) {
func getNATInfo(_ *cni.NetworkConfig, _ interface{}, _ bool) (natInfo []policy.NATInfo) {
return natInfo
}

func platformInit(cniConfig *cni.NetworkConfig) {}

// isDualNicFeatureSupported returns if the dual nic feature is supported. Currently it's only supported for windows hnsv2 path
func (plugin *NetPlugin) isDualNicFeatureSupported(netNs string) bool {
return false
}

func getOverlayGateway(_ *net.IPNet) (net.IP, error) {
return net.ParseIP("169.254.1.1"), nil
}
34 changes: 34 additions & 0 deletions cni/network/network_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,16 @@ import (
"fmt"
"net"
"os"
"runtime"
"testing"

"github.com/Azure/azure-container-networking/cni"
"github.com/Azure/azure-container-networking/cni/api"
"github.com/Azure/azure-container-networking/cni/util"
"github.com/Azure/azure-container-networking/common"
acnnetwork "github.com/Azure/azure-container-networking/network"
"github.com/Azure/azure-container-networking/network/networkutils"
"github.com/Azure/azure-container-networking/network/policy"
"github.com/Azure/azure-container-networking/nns"
"github.com/Azure/azure-container-networking/telemetry"
cniSkel "github.com/containernetworking/cni/pkg/skel"
Expand Down Expand Up @@ -1006,13 +1009,17 @@ func TestGetAllEndpointState(t *testing.T) {

ep1 := getTestEndpoint("podname1", "podnamespace1", "10.0.0.1/24", "podinterfaceid1", "testcontainerid1")
ep2 := getTestEndpoint("podname2", "podnamespace2", "10.0.0.2/24", "podinterfaceid2", "testcontainerid2")
ep3 := getTestEndpoint("podname3", "podnamespace3", "10.240.1.242/16", "podinterfaceid3", "testcontainerid3")

err := plugin.nm.CreateEndpoint(nil, networkid, ep1)
require.NoError(t, err)

err = plugin.nm.CreateEndpoint(nil, networkid, ep2)
require.NoError(t, err)

err = plugin.nm.CreateEndpoint(nil, networkid, ep3)
require.NoError(t, err)

state, err := plugin.GetAllEndpointState(networkid)
require.NoError(t, err)

Expand All @@ -1032,6 +1039,13 @@ func TestGetAllEndpointState(t *testing.T) {
ContainerID: ep2.ContainerID,
IPAddresses: ep2.IPAddresses,
},
ep3.Id: {
PodEndpointId: ep3.Id,
PodName: ep3.PODName,
PodNamespace: ep3.PODNameSpace,
ContainerID: ep3.ContainerID,
IPAddresses: ep3.IPAddresses,
},
},
}

Expand Down Expand Up @@ -1067,3 +1081,23 @@ func TestGetNetworkName(t *testing.T) {
})
}
}

func TestGetOverlayNatInfo(t *testing.T) {
nwCfg := &cni.NetworkConfig{ExecutionMode: string(util.V4Swift), IPAM: cni.IPAM{Mode: string(util.V4Overlay)}}
natInfo := getNATInfo(nwCfg, nil, false)
require.Empty(t, natInfo, "overlay natInfo should be empty")
}

func TestGetPodSubnetNatInfo(t *testing.T) {
ncPrimaryIP := "10.241.0.4"
nwCfg := &cni.NetworkConfig{ExecutionMode: string(util.V4Swift)}
natInfo := getNATInfo(nwCfg, ncPrimaryIP, false)
if runtime.GOOS == "windows" {
require.Equalf(t, natInfo, []policy.NATInfo{
{VirtualIP: ncPrimaryIP, Destinations: []string{networkutils.AzureDNS}},
{Destinations: []string{networkutils.AzureIMDS}},
}, "invalid windows podsubnet natInfo")
} else {
require.Empty(t, natInfo, "linux podsubnet natInfo should be empty")
}
}
6 changes: 3 additions & 3 deletions cni/network/network_windows.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -381,15 +381,15 @@ func determineWinVer() {
}
}

func getNATInfo(executionMode string, ncPrimaryIPIface interface{}, multitenancy, enableSnatForDNS bool) (natInfo []policy.NATInfo) {
if executionMode == string(util.V4Swift) {
func getNATInfo(nwCfg *cni.NetworkConfig, ncPrimaryIPIface interface{}, enableSnatForDNS bool) (natInfo []policy.NATInfo) {
if nwCfg.ExecutionMode == string(util.V4Swift) && nwCfg.IPAM.Mode != string(util.V4Overlay) {
ncPrimaryIP := ""
if ncPrimaryIPIface != nil {
ncPrimaryIP = ncPrimaryIPIface.(string)
}

natInfo = append(natInfo, []policy.NATInfo{{VirtualIP: ncPrimaryIP, Destinations: []string{networkutils.AzureDNS}}, {Destinations: []string{networkutils.AzureIMDS}}}...)
} else if multitenancy && enableSnatForDNS {
} else if nwCfg.MultiTenancy && enableSnatForDNS {
natInfo = append(natInfo, policy.NATInfo{Destinations: []string{networkutils.AzureDNS}})
}

Expand Down

0 comments on commit c325121

Please sign in to comment.