Skip to content
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

Reorganize the code for configuring Windows networking into dedicated package #5159

Merged
merged 3 commits into from
May 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions hack/update-codegen-dockerized.sh
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ MOCKGEN_TARGETS=(
"pkg/agent/util/iptables Interface testing mock_iptables_linux.go" # Must specify linux.go suffix, otherwise compilation would fail on windows platform as source file has linux build tag.
"pkg/agent/util/netlink Interface testing mock_netlink_linux.go"
"pkg/agent/wireguard Interface testing mock_wireguard.go"
"pkg/agent/util/winnet Interface testing mock_net_windows.go"
"pkg/antctl AntctlClient ."
"pkg/controller/networkpolicy EndpointQuerier,PolicyRuleQuerier testing"
"pkg/controller/querier ControllerQuerier testing"
Expand Down
37 changes: 24 additions & 13 deletions pkg/agent/agent_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,17 @@ import (
"antrea.io/antrea/pkg/agent/interfacestore"
"antrea.io/antrea/pkg/agent/util"
antreasyscall "antrea.io/antrea/pkg/agent/util/syscall"
"antrea.io/antrea/pkg/agent/util/winnet"
"antrea.io/antrea/pkg/apis/crd/v1alpha1"
"antrea.io/antrea/pkg/ovs/ovsconfig"
"antrea.io/antrea/pkg/ovs/ovsctl"
utilip "antrea.io/antrea/pkg/util/ip"
)

var (
winnetUtil winnet.Interface = &winnet.Handle{}
// setInterfaceMTU is meant to be overridden for testing
setInterfaceMTU = util.SetInterfaceMTU
setInterfaceMTU = winnetUtil.SetNetAdapterMTU

// setInterfaceARPAnnounce is meant to be overridden for testing.
setInterfaceARPAnnounce = func(ifaceName string, value int) error { return nil }
Expand All @@ -57,11 +59,11 @@ func (i *Initializer) prepareHNSNetworkAndOVSExtension() error {
hnsNetwork, err := hcsshim.GetHNSNetworkByName(util.LocalHNSNetwork)
if err == nil {
// Enable OVS Extension on the HNS Network.
if err = util.EnableHNSNetworkExtension(hnsNetwork.Id, util.OVSExtensionID); err != nil {
if err = util.EnableHNSNetworkExtension(hnsNetwork.Id, winnet.OVSExtensionID); err != nil {
return err
}
// Enable RSC for existing vSwitch.
if err = util.EnableRSCOnVSwitch(util.LocalHNSNetwork); err != nil {
if err = winnetUtil.EnableRSCOnVSwitch(util.LocalHNSNetwork); err != nil {
return err
}
// Save the uplink adapter name to check if the OVS uplink port has been created in prepareOVSBridge stage.
Expand All @@ -87,7 +89,7 @@ func (i *Initializer) prepareHNSNetworkAndOVSExtension() error {
// adapter was not found" if no adapter is provided and no physical adapter is available on the host.
// If the discovered adapter is virtual, it likely means the physical adapter is already attached to another
// HNSNetwork. For example, docker may create HNSNetworks which attach to the physical adapter.
isVirtual, err := util.IsVirtualAdapter(adapter.Name)
isVirtual, err := winnetUtil.IsVirtualNetAdapter(adapter.Name)
if err != nil {
return err
}
Expand All @@ -107,7 +109,7 @@ func (i *Initializer) prepareHNSNetworkAndOVSExtension() error {
klog.InfoS("No default gateway found on interface", "interface", adapter.Name)
}
i.nodeConfig.UplinkNetConfig.Gateway = defaultGW
dnsServers, err := util.GetDNServersByInterfaceIndex(adapter.Index)
dnsServers, err := winnetUtil.GetDNServersByNetAdapterIndex(adapter.Index)
if err != nil {
return err
}
Expand All @@ -128,12 +130,12 @@ func (i *Initializer) prepareHNSNetworkAndOVSExtension() error {
func (i *Initializer) prepareVMNetworkAndOVSExtension() error {
klog.V(2).Info("Setting up VM network")
// Check whether VM Switch is created
exists, err := util.VMSwitchExists()
exists, err := winnetUtil.VMSwitchExists(util.LocalVMSwitch)
if err != nil {
return err
}
if exists {
vmSwitchIFName, err := util.GetVMSwitchInterfaceName()
vmSwitchIFName, err := winnetUtil.GetVMSwitchNetAdapterName(util.LocalVMSwitch)
if err != nil {
return err
}
Expand Down Expand Up @@ -168,20 +170,29 @@ func (i *Initializer) prepareVMNetworkAndOVSExtension() error {
}()

klog.V(2).InfoS("Creating VM switch", "uplinkIFName", uplinkIFName)
if err = util.CreateVMSwitch(uplinkIFName); err != nil {
if err = winnetUtil.AddVMSwitch(uplinkIFName, util.LocalVMSwitch); err != nil {
return fmt.Errorf("failed to create VM switch for interface %s: %v", uplinkIFName, err)
}
enabled, err := winnetUtil.IsVMSwitchOVSExtensionEnabled(util.LocalVMSwitch)
if err != nil {
return err
}
if !enabled {
if err = winnetUtil.EnableVMSwitchOVSExtension(util.LocalVMSwitch); err != nil {
return err
}
}

defer func() {
if !success {
if err = util.RemoveVMSwitch(); err != nil {
if err = winnetUtil.RemoveVMSwitch(util.LocalVMSwitch); err != nil {
klog.ErrorS(err, "Failed to remove VMSwitch")
}
}
}()

uplinkMACStr := strings.Replace(uplinkIface.HardwareAddr.String(), ":", "", -1)
if err = util.RenameVMNetworkAdapter(util.LocalVMSwitch, uplinkMACStr, hostIFName, true); err != nil {
if err = winnetUtil.RenameVMNetworkAdapter(util.LocalVMSwitch, uplinkMACStr, hostIFName, true); err != nil {
return fmt.Errorf("failed to rename VMNetworkAdapter as %s: %v", hostIFName, err)
}

Expand Down Expand Up @@ -298,7 +309,7 @@ func (i *Initializer) prepareOVSBridgeOnHNSNetwork() error {
// connection are routed to the selected backend Pod via the bridge interface; if we do not enable IP forwarding on
// the bridge interface, the packet will be discarded on the bridge interface as the destination of the packet
// is not the Node.
if err = util.EnableIPForwarding(brName); err != nil {
if err = winnetUtil.EnableIPForwarding(brName); err != nil {
return err
}
// Set the uplink with "no-flood" config, so that the IP of local Pods and "antrea-gw0" will not be leaked to the
Expand Down Expand Up @@ -395,11 +406,11 @@ func (i *Initializer) saveHostRoutes() error {
// IPv6 is not supported on Windows currently. Please refer to https://github.com/antrea-io/antrea/issues/5162
// for more information.
family := antreasyscall.AF_INET
filter := &util.Route{
filter := &winnet.Route{
LinkIndex: i.nodeConfig.UplinkNetConfig.Index,
GatewayAddress: net.ParseIP(i.nodeConfig.UplinkNetConfig.Gateway),
}
routes, err := util.RouteListFiltered(family, filter, util.RT_FILTER_IF|util.RT_FILTER_GW)
routes, err := winnetUtil.RouteListFiltered(family, filter, winnet.RT_FILTER_IF|winnet.RT_FILTER_GW)
if err != nil {
return err
}
Expand Down
10 changes: 6 additions & 4 deletions pkg/agent/cniserver/interface_configuration_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"k8s.io/klog/v2"

"antrea.io/antrea/pkg/agent/util"
"antrea.io/antrea/pkg/agent/util/winnet"
cnipb "antrea.io/antrea/pkg/apis/cni/v1beta1"
"antrea.io/antrea/pkg/ovs/ovsconfig"
)
Expand All @@ -45,7 +46,6 @@ const (
var (
getHnsNetworkByNameFunc = hcsshim.GetHNSNetworkByName
listHnsEndpointFunc = hcsshim.HNSListEndpointRequest
setInterfaceMTUFunc = util.SetInterfaceMTU
hostInterfaceExistsFunc = util.HostInterfaceExists
getNetInterfaceAddrsFunc = getNetInterfaceAddrs
createHnsEndpointFunc = createHnsEndpoint
Expand All @@ -60,6 +60,7 @@ var (
type ifConfigurator struct {
hnsNetwork *hcsshim.HNSNetwork
epCache *sync.Map
winnet winnet.Interface
}

// disableTXChecksumOffload is ignored on Windows.
Expand All @@ -80,6 +81,7 @@ func newInterfaceConfigurator(ovsDatapathType ovsconfig.OVSDatapathType, isOvsHa
return &ifConfigurator{
hnsNetwork: hnsNetwork,
epCache: epCache,
winnet: &winnet.Handle{},
}, nil
}

Expand Down Expand Up @@ -177,8 +179,8 @@ func (ic *ifConfigurator) configureContainerLink(
// CmdAdd request is returned; 2) for Docker runtime, the interface is created after hcsshim.HotAttachEndpoint,
// and the hcsshim call is not synchronized from the observation.
return ic.addPostInterfaceCreateHook(infraContainerID, epName, containerAccess, func() error {
ifaceName := util.VirtualAdapterName(epName)
if err := setInterfaceMTUFunc(ifaceName, mtu); err != nil {
ifaceName := winnet.VirtualAdapterName(epName)
antoninbas marked this conversation as resolved.
Show resolved Hide resolved
if err := ic.winnet.SetNetAdapterMTU(ifaceName, mtu); err != nil {
return fmt.Errorf("failed to configure MTU on container interface '%s': %v", ifaceName, err)
}
return nil
Expand Down Expand Up @@ -342,7 +344,7 @@ func (ic *ifConfigurator) checkContainerInterface(
containerIface.Sandbox, sandboxID)
}
hnsEP := strings.Split(containerIface.Name, "_")[0]
containerIfaceName := util.VirtualAdapterName(hnsEP)
containerIfaceName := winnet.VirtualAdapterName(hnsEP)
intf, err := getNetInterfaceByNameFunc(containerIfaceName)
if err != nil {
klog.Errorf("Failed to get container %s interface: %v", containerID, err)
Expand Down
51 changes: 23 additions & 28 deletions pkg/agent/cniserver/server_windows_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import (
routetest "antrea.io/antrea/pkg/agent/route/testing"
agenttypes "antrea.io/antrea/pkg/agent/types"
"antrea.io/antrea/pkg/agent/util"
winnettest "antrea.io/antrea/pkg/agent/util/winnet/testing"
cnipb "antrea.io/antrea/pkg/apis/cni/v1beta1"
ovsconfigtest "antrea.io/antrea/pkg/ovs/ovsconfig/testing"
"antrea.io/antrea/pkg/util/channel"
Expand All @@ -49,6 +50,8 @@ import (
var (
containerMACStr = "23:34:56:23:22:45"
dnsSearches = []string{"a.b.c.d"}

mockWinnet *winnettest.MockInterface
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unit test mocks should never be global variables IMO
they are very much specific to individual tests

Copy link
Contributor Author

@hongliangl hongliangl May 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, but other mock tests (like mockRoute, mockOFClient) are also used like this in the unit tests of this package, and they are initialized in function newMockCNIServer for every individual test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. This should probably be addressed in a separate PR.

)

func TestUpdateResultDNSConfig(t *testing.T) {
Expand Down Expand Up @@ -258,6 +261,7 @@ func newMockCNIServer(t *testing.T, controller *gomock.Controller, podUpdateNoti
mockOVSBridgeClient = ovsconfigtest.NewMockOVSBridgeClient(controller)
mockOFClient = openflowtest.NewMockClient(controller)
mockRoute = routetest.NewMockInterface(controller)
mockWinnet = winnettest.NewMockInterface(controller)
ifaceStore = interfacestore.NewInterfaceStore()
cniServer := newCNIServer(t)
cniServer.routeClient = mockRoute
Expand All @@ -266,6 +270,7 @@ func newMockCNIServer(t *testing.T, controller *gomock.Controller, podUpdateNoti
gateway := &config.GatewayConfig{Name: "", IPv4: gwIPv4, MAC: gwMAC}
cniServer.nodeConfig = &config.NodeConfig{Name: "node1", PodIPv4CIDR: nodePodCIDRv4, GatewayConfig: gateway}
cniServer.podConfigurator, _ = newPodConfigurator(mockOVSBridgeClient, mockOFClient, mockRoute, ifaceStore, gwMAC, "system", false, false, podUpdateNotifier)
cniServer.podConfigurator.ifConfigurator.(*ifConfigurator).winnet = mockWinnet
return cniServer
}

Expand All @@ -289,18 +294,13 @@ func prepareSetup(t *testing.T, ipamType string, name string, containerID, infra
}

func TestCmdAdd(t *testing.T) {
controller := gomock.NewController(t)
ipamType := "windows-test"
ipamMock := ipamtest.NewMockIPAMDriver(controller)
ipam.ResetIPAMDriver(ipamType, ipamMock)
oriIPAMResult := &ipam.IPAMResult{Result: *ipamResult}
ctx := context.TODO()

containerdInfraContainer := generateUUID()

defer mockHostInterfaceExists()()
defer mockGetHnsNetworkByName()()
defer mockSetInterfaceMTU(nil)()

for _, tc := range []struct {
name string
Expand Down Expand Up @@ -360,6 +360,11 @@ func TestCmdAdd(t *testing.T) {
},
} {
t.Run(tc.name, func(t *testing.T) {
controller := gomock.NewController(t)
ipamType := "windows-test"
ipamMock := ipamtest.NewMockIPAMDriver(controller)
ipam.ResetIPAMDriver(ipamType, ipamMock)

isDocker := isDockerContainer(tc.netns)
testUtil := newHnsTestUtil(generateUUID(), tc.existingHnsEndpoints, isDocker, tc.isAttached, tc.hnsEndpointCreateErr, tc.endpointAttachErr)
testUtil.setFunctions()
Expand All @@ -379,6 +384,9 @@ func TestCmdAdd(t *testing.T) {
if tc.ipamDel {
ipamMock.EXPECT().Del(gomock.Any(), gomock.Any(), gomock.Any()).Return(true, nil).Times(1)
}
if tc.endpointAttachErr == nil {
mockWinnet.EXPECT().SetNetAdapterMTU(gomock.Any(), gomock.Any()).Times(1)
}
ovsPortID := generateUUID()
if tc.connectOVS {
mockOVSBridgeClient.EXPECT().CreatePort(ovsPortName, ovsPortName, gomock.Any()).Return(ovsPortID, nil).Times(1)
Expand Down Expand Up @@ -431,18 +439,13 @@ func TestCmdAdd(t *testing.T) {
}

func TestCmdDel(t *testing.T) {
controller := gomock.NewController(t)
ipamType := "windows-test"
ipamMock := ipamtest.NewMockIPAMDriver(controller)
ipam.ResetIPAMDriver(ipamType, ipamMock)
ctx := context.TODO()

containerID := "261a1970-5b6c-11ed-8caf-000c294e5d03"
containerMAC, _ := net.ParseMAC("11:22:33:44:33:22")

defer mockHostInterfaceExists()()
defer mockGetHnsNetworkByName()()
defer mockSetInterfaceMTU(nil)()

for _, tc := range []struct {
name string
Expand Down Expand Up @@ -476,6 +479,11 @@ func TestCmdDel(t *testing.T) {
},
} {
t.Run(tc.name, func(t *testing.T) {
controller := gomock.NewController(t)
ipamType := "windows-test"
ipamMock := ipamtest.NewMockIPAMDriver(controller)
ipam.ResetIPAMDriver(ipamType, ipamMock)

isDocker := isDockerContainer(tc.netns)
requestMsg, ovsPortName := prepareSetup(t, ipamType, testPodNameA, containerID, containerID, tc.netns, nil)
hnsEndpoint := getHnsEndpoint(generateUUID(), ovsPortName)
Expand Down Expand Up @@ -530,10 +538,6 @@ func TestCmdDel(t *testing.T) {
}

func TestCmdCheck(t *testing.T) {
controller := gomock.NewController(t)
ipamType := "windows-test"
ipamMock := ipamtest.NewMockIPAMDriver(controller)
ipam.ResetIPAMDriver(ipamType, ipamMock)
ctx := context.TODO()

containerNetns := generateUUID()
Expand All @@ -544,7 +548,6 @@ func TestCmdCheck(t *testing.T) {

defer mockHostInterfaceExists()()
defer mockGetHnsNetworkByName()()
defer mockSetInterfaceMTU(nil)()
defer mockListHnsEndpoint(nil, nil)()
defer mockGetNetInterfaceAddrs(containerIPNet, nil)()
defer mockGetHnsEndpointByName(generateUUID(), mac)()
Expand Down Expand Up @@ -661,6 +664,11 @@ func TestCmdCheck(t *testing.T) {
},
} {
t.Run(tc.name, func(t *testing.T) {
controller := gomock.NewController(t)
ipamType := "windows-test"
ipamMock := ipamtest.NewMockIPAMDriver(controller)
ipam.ResetIPAMDriver(ipamType, ipamMock)

defer mockGetNetInterfaceByName(tc.netInterface)()
cniserver := newMockCNIServer(t, controller, channel.NewSubscribableChannel("podUpdate", 100))
requestMsg, _ := prepareSetup(t, ipamType, tc.podName, tc.containerID, tc.containerID, tc.netns, tc.prevResult)
Expand Down Expand Up @@ -864,16 +872,3 @@ func mockListHnsEndpoint(endpoints []hcsshim.HNSEndpoint, listError error) func(
listHnsEndpointFunc = originalListHnsEndpoint
}
}

func mockSetInterfaceMTU(setMTUError error) func() {
originalSetInterfaceMTU := setInterfaceMTUFunc
setInterfaceMTUFunc = func(ifaceName string, mtu int) error {
if setMTUError == nil {
hostIfaces.Store(ifaceName, true)
}
return setMTUError
}
return func() {
setInterfaceMTUFunc = originalSetInterfaceMTU
}
}
5 changes: 4 additions & 1 deletion pkg/agent/externalnode/external_node_controller_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,12 @@ import (

"antrea.io/antrea/pkg/agent/config"
"antrea.io/antrea/pkg/agent/util"
"antrea.io/antrea/pkg/agent/util/winnet"
"antrea.io/antrea/pkg/signals"
)

var winnetUtil winnet.Interface = &winnet.Handle{}

// moveIFConfigurations returns nil for single interface case, as it relies
// on Windows New-VMSwitch command to create a host network adapter and copy
// the uplink adapter configurations to host adapter.
Expand All @@ -45,7 +48,7 @@ func (c *ExternalNodeController) removeExternalNodeConfig() error {
klog.ErrorS(ovsErr, "Failed to delete OVS bridge")
}

if err := util.RemoveVMSwitch(); err != nil {
if err := winnetUtil.RemoveVMSwitch(util.LocalVMSwitch); err != nil {
return fmt.Errorf("failed to delete VM Switch, err: %v", err)
}
// Antrea Agent initializer creates a VM Switch corresponding to an
Expand Down
Loading
Loading