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

fix: route issues on Swiftv2 Windows #3205

Open
wants to merge 41 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
6b60041
long-term solution for route issues windows
paulyufan2 Nov 26, 2024
909e842
fix linter issues
paulyufan2 Nov 26, 2024
e2ec36a
fix the ut
paulyufan2 Nov 26, 2024
4f283bd
fix an issue
paulyufan2 Nov 26, 2024
6632c76
hardcode gw to try
paulyufan2 Nov 26, 2024
7d3b8c1
bypass ut
paulyufan2 Nov 26, 2024
12facb8
modify gw
paulyufan2 Dec 6, 2024
298d5fc
hard route to test
paulyufan2 Dec 6, 2024
535bcb6
bypass UTs to validate
paulyufan2 Dec 6, 2024
ea4f9a1
hardcode nexthop
paulyufan2 Dec 9, 2024
49dc1dc
change hardcoded nexthop
paulyufan2 Dec 9, 2024
eec67b8
add default route
paulyufan2 Dec 9, 2024
6cbd918
add default route
paulyufan2 Dec 9, 2024
7a1cf77
get rid of hardcode ip
paulyufan2 Dec 12, 2024
f931460
fix bugs and uts
paulyufan2 Dec 12, 2024
fe6a5e9
change a func name
paulyufan2 Dec 12, 2024
9437123
fix linter issues
paulyufan2 Dec 12, 2024
e37e7b5
fix linter issues
paulyufan2 Dec 12, 2024
38b82a6
fix addRoutes
paulyufan2 Dec 12, 2024
6e6e683
add log for cidrs
paulyufan2 Jan 15, 2025
c1c6da3
add log for cidrs
paulyufan2 Jan 15, 2025
d609b14
test without podcidrs
paulyufan2 Jan 15, 2025
34c6061
remove logs
paulyufan2 Jan 15, 2025
4f8ef4f
revert changes back
paulyufan2 Jan 15, 2025
e3982fb
Merge branch 'master' into defaultroutelongtermsolution
paulyufan2 Jan 21, 2025
b32f48f
separate pod cidr add
paulyufan2 Jan 21, 2025
fb7b987
remove gw
paulyufan2 Jan 21, 2025
45cf623
add empty gw ip
paulyufan2 Jan 21, 2025
7ddf286
fix ut
paulyufan2 Jan 21, 2025
d1c2ae3
allow gw as empty string
paulyufan2 Jan 21, 2025
500698f
add unchanged gw
paulyufan2 Jan 22, 2025
11604e6
add unchanged gw
paulyufan2 Jan 22, 2025
a4d484f
add unchanged gw
paulyufan2 Jan 22, 2025
f15292a
do not pass gwip
paulyufan2 Jan 22, 2025
35a33ae
fix a bug
paulyufan2 Jan 22, 2025
8e94fc6
just for testing
paulyufan2 Jan 22, 2025
91847f5
bug fix
paulyufan2 Jan 22, 2025
a3053a8
use default gw ip
paulyufan2 Jan 22, 2025
6389118
use default gw ip
paulyufan2 Jan 22, 2025
0c67be2
remove extra line
paulyufan2 Jan 22, 2025
e6f4d86
fix windows ut
paulyufan2 Jan 22, 2025
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
46 changes: 46 additions & 0 deletions cns/middlewares/k8sSwiftV2.go
Original file line number Diff line number Diff line change
Expand Up @@ -249,3 +249,49 @@ func (k *K8sSWIFTv2Middleware) getIPConfig(ctx context.Context, podInfo cns.PodI
func (k *K8sSWIFTv2Middleware) Type() cns.SWIFTV2Mode {
return cns.K8sSWIFTV2
}

// CNS gets node, pod and service CIDRs from configuration env and parse them to get the v4 and v6 IPs
func (k *K8sSWIFTv2Middleware) GetCidrs() (v4IPs, v6IPs []string, err error) {
v4IPs = []string{}
v6IPs = []string{}
Comment on lines +277 to +278
Copy link
Member

Choose a reason for hiding this comment

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

Don't use named returns please, they have surprising semantics. For example, these two lines are unnecessary since they are auto-initialized.


// Get and parse infraVNETCIDRs from env
infraVNETCIDRs, err := configuration.InfraVNETCIDRs()
if err != nil {
return nil, nil, errors.Wrapf(err, "failed to get infraVNETCIDRs from env")
}
infraVNETCIDRsv4, infraVNETCIDRsv6, err := utils.ParseCIDRs(infraVNETCIDRs)
if err != nil {
return nil, nil, errors.Wrapf(err, "failed to parse infraVNETCIDRs")
}

// Get and parse podCIDRs from env
paulyufan2 marked this conversation as resolved.
Show resolved Hide resolved
podCIDRs, err := configuration.PodCIDRs()
if err != nil {
return nil, nil, errors.Wrapf(err, "failed to get podCIDRs from env")
}
podCIDRsV4, podCIDRv6, err := utils.ParseCIDRs(podCIDRs)
if err != nil {
return nil, nil, errors.Wrapf(err, "failed to parse podCIDRs")
}

// Get and parse serviceCIDRs from env
serviceCIDRs, err := configuration.ServiceCIDRs()
if err != nil {
return nil, nil, errors.Wrapf(err, "failed to get serviceCIDRs from env")
}
serviceCIDRsV4, serviceCIDRsV6, err := utils.ParseCIDRs(serviceCIDRs)
if err != nil {
return nil, nil, errors.Wrapf(err, "failed to parse serviceCIDRs")
}

v4IPs = append(v4IPs, infraVNETCIDRsv4...)
v4IPs = append(v4IPs, podCIDRsV4...)
v4IPs = append(v4IPs, serviceCIDRsV4...)

v6IPs = append(v6IPs, infraVNETCIDRsv6...)
v6IPs = append(v6IPs, podCIDRv6...)
v6IPs = append(v6IPs, serviceCIDRsV6...)

return v4IPs, v6IPs, nil
}
82 changes: 32 additions & 50 deletions cns/middlewares/k8sSwiftV2_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,7 @@ import (
"net/netip"

"github.com/Azure/azure-container-networking/cns"
"github.com/Azure/azure-container-networking/cns/configuration"
"github.com/Azure/azure-container-networking/cns/logger"
"github.com/Azure/azure-container-networking/cns/middlewares/utils"
"github.com/Azure/azure-container-networking/crd/multitenancy/api/v1alpha1"
"github.com/pkg/errors"
)
Expand All @@ -30,50 +28,12 @@ func (k *K8sSWIFTv2Middleware) setRoutes(podIPInfo *cns.PodIpInfo) error {
routes = append(routes, virtualGWRoute, route)

case cns.InfraNIC:
// Get and parse infraVNETCIDRs from env
infraVNETCIDRs, err := configuration.InfraVNETCIDRs()
// Linux uses 169.254.1.1 as the default ipv4 gateway and fe80::1234:5678:9abc as the default ipv6 gateway
infraRoutes, err := k.setInfraRoutes(podIPInfo)
if err != nil {
return errors.Wrapf(err, "failed to get infraVNETCIDRs from env")
}
infraVNETCIDRsv4, infraVNETCIDRsv6, err := utils.ParseCIDRs(infraVNETCIDRs)
if err != nil {
return errors.Wrapf(err, "failed to parse infraVNETCIDRs")
}

// Get and parse podCIDRs from env
podCIDRs, err := configuration.PodCIDRs()
if err != nil {
return errors.Wrapf(err, "failed to get podCIDRs from env")
}
podCIDRsV4, podCIDRv6, err := utils.ParseCIDRs(podCIDRs)
if err != nil {
return errors.Wrapf(err, "failed to parse podCIDRs")
}

// Get and parse serviceCIDRs from env
serviceCIDRs, err := configuration.ServiceCIDRs()
if err != nil {
return errors.Wrapf(err, "failed to get serviceCIDRs from env")
}
serviceCIDRsV4, serviceCIDRsV6, err := utils.ParseCIDRs(serviceCIDRs)
if err != nil {
return errors.Wrapf(err, "failed to parse serviceCIDRs")
}

ip, err := netip.ParseAddr(podIPInfo.PodIPConfig.IPAddress)
if err != nil {
return errors.Wrapf(err, "failed to parse podIPConfig IP address %s", podIPInfo.PodIPConfig.IPAddress)
}

if ip.Is4() {
routes = append(routes, addRoutes(podCIDRsV4, overlayGatewayv4)...)
routes = append(routes, addRoutes(serviceCIDRsV4, overlayGatewayv4)...)
routes = append(routes, addRoutes(infraVNETCIDRsv4, overlayGatewayv4)...)
} else {
routes = append(routes, addRoutes(podCIDRv6, overlayGatewayV6)...)
routes = append(routes, addRoutes(serviceCIDRsV6, overlayGatewayV6)...)
routes = append(routes, addRoutes(infraVNETCIDRsv6, overlayGatewayV6)...)
return errors.Wrap(err, "failed to set routes for infraNIC interface")
}
routes = infraRoutes
podIPInfo.SkipDefaultRoutes = true

case cns.NodeNetworkInterfaceBackendNIC: //nolint:exhaustive // ignore exhaustive types check
Expand All @@ -86,7 +46,14 @@ func (k *K8sSWIFTv2Middleware) setRoutes(podIPInfo *cns.PodIpInfo) error {
return nil
}

func addRoutes(cidrs []string, gatewayIP string) []cns.Route {
// assignSubnetPrefixLengthFields is a no-op for linux swiftv2 as the default prefix-length is sufficient
func (k *K8sSWIFTv2Middleware) assignSubnetPrefixLengthFields(_ *cns.PodIpInfo, _ v1alpha1.InterfaceInfo, _ string) error {
return nil
}

paulyufan2 marked this conversation as resolved.
Show resolved Hide resolved
func (k *K8sSWIFTv2Middleware) addDefaultRoute(*cns.PodIpInfo, string) {}

func (k *K8sSWIFTv2Middleware) addRoutes(cidrs []string, gatewayIP string) []cns.Route {
routes := make([]cns.Route, len(cidrs))
for i, cidr := range cidrs {
routes[i] = cns.Route{
Expand All @@ -97,9 +64,24 @@ func addRoutes(cidrs []string, gatewayIP string) []cns.Route {
return routes
}

// assignSubnetPrefixLengthFields is a no-op for linux swiftv2 as the default prefix-length is sufficient
func (k *K8sSWIFTv2Middleware) assignSubnetPrefixLengthFields(_ *cns.PodIpInfo, _ v1alpha1.InterfaceInfo, _ string) error {
return nil
}
func (k *K8sSWIFTv2Middleware) setInfraRoutes(podIPInfo *cns.PodIpInfo) ([]cns.Route, error) {
Copy link
Contributor

@QxBytes QxBytes Jan 16, 2025

Choose a reason for hiding this comment

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

Can we change this to getInfraRoutes since it seems like we are only returning the routes rather than setting anything? Also maybe add a comment to any new functions?

var routes []cns.Route

func (k *K8sSWIFTv2Middleware) addDefaultRoute(*cns.PodIpInfo, string) {}
ip, err := netip.ParseAddr(podIPInfo.PodIPConfig.IPAddress)
if err != nil {
return nil, errors.Wrapf(err, "failed to parse podIPConfig IP address %s", podIPInfo.PodIPConfig.IPAddress)
}

v4IPs, v6IPs, err := k.GetCidrs()
if err != nil {
return nil, errors.Wrap(err, "failed to get CIDRs")
}

if ip.Is4() {
routes = append(routes, k.addRoutes(v4IPs, overlayGatewayv4)...)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm comparing the getInfraRoutes code in the _linux.go file and the getInfraRoutes code in the _windows.go file and noticed that overlayGatewayv4 / v6 is added in linux-- could you provide background on this (does it make an appearance in windows, and what is it used for?). Also the casing is inconsistent for v4 and V6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I think I added comments somewhere;
To sum up:
Linux: always use overlayGatewayv4 as the default gateway IP (168.x.x.x) for IPv4 and overlayGatewayv6 for IPv6; note that ipv6 is not supported for both Linux and Windows right now but codes already implemented ipv6 gateway and I will not touch it.

Windows: always use 0.0.0.0 as the default gateway IP for containerd to program the routing table and ipv6 is not supported.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for clarifying, could we also add that note (regarding the gateway ip) above this function if it is still present after the refactor?

} else {
routes = append(routes, k.addRoutes(v6IPs, overlayGatewayV6)...)
}

return routes, nil
}
8 changes: 5 additions & 3 deletions cns/middlewares/k8sSwiftV2_linux_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package middlewares
import (
"context"
"fmt"
"reflect"
"testing"

"github.com/Azure/azure-container-networking/cns"
Expand Down Expand Up @@ -342,10 +343,10 @@ func TestSetRoutesSuccess(t *testing.T) {
} else {
assert.Equal(t, ipInfo.SkipDefaultRoutes, false)
}

}

for i := range podIPInfo {
assert.DeepEqual(t, podIPInfo[i].Routes, desiredPodIPInfo[i].Routes)
reflect.DeepEqual(podIPInfo[i].Routes, desiredPodIPInfo[i].Routes)
Copy link
Member

Choose a reason for hiding this comment

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

Preference towards github.com/google/go-cmp/cmp.Equal over reflect.DeepEqual (it's a drop-in replacement)

}
}

Expand Down Expand Up @@ -378,9 +379,10 @@ func TestSetRoutesFailure(t *testing.T) {
}

func TestAddRoutes(t *testing.T) {
middleware := K8sSWIFTv2Middleware{Cli: mock.NewClient()}
cidrs := []string{"10.0.0.0/24", "20.0.0.0/24"}
gatewayIP := "192.168.1.1"
routes := addRoutes(cidrs, gatewayIP)
routes := middleware.addRoutes(cidrs, gatewayIP)
expectedRoutes := []cns.Route{
{
IPAddress: "10.0.0.0/24",
Expand Down
63 changes: 60 additions & 3 deletions cns/middlewares/k8sSwiftV2_windows.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package middlewares

import (
"net"
"net/netip"

"github.com/Azure/azure-container-networking/cns"
"github.com/Azure/azure-container-networking/cns/middlewares/utils"
"github.com/Azure/azure-container-networking/crd/multitenancy/api/v1alpha1"
Expand All @@ -20,6 +23,13 @@ func (k *K8sSWIFTv2Middleware) setRoutes(podIPInfo *cns.PodIpInfo) error {
}
podIPInfo.Routes = append(podIPInfo.Routes, route)

// set routes(pod/node/service cidrs) for infraNIC interface
// Swiftv2 Windows does not support IPv6
infraRoutes, err := k.setInfraRoutes(podIPInfo)
if err != nil {
return errors.Wrap(err, "failed to set routes for infraNIC interface")
}
podIPInfo.Routes = append(podIPInfo.Routes, infraRoutes...)
podIPInfo.SkipDefaultRoutes = true
}
return nil
Expand Down Expand Up @@ -50,11 +60,58 @@ func (k *K8sSWIFTv2Middleware) assignSubnetPrefixLengthFields(podIPInfo *cns.Pod
return nil
}

// add default route with gateway IP to podIPInfo
func (k *K8sSWIFTv2Middleware) addDefaultRoute(podIPInfo *cns.PodIpInfo, gwIP string) {
// add default route with gateway IP to podIPInfo for delegated interface
func (k *K8sSWIFTv2Middleware) addDefaultRoute(podIPInfo *cns.PodIpInfo, gatewayIP string) {
paulyufan2 marked this conversation as resolved.
Show resolved Hide resolved
route := cns.Route{
IPAddress: "0.0.0.0/0",
GatewayIPAddress: gwIP,
GatewayIPAddress: gatewayIP,
}
podIPInfo.Routes = append(podIPInfo.Routes, route)
}

// always pick up .1 as the default ipv4 gateway for each IP address
func (k *K8sSWIFTv2Middleware) getIPv4Gateway(cidr string) (string, error) {
ip, _, err := net.ParseCIDR(cidr)
if err != nil {
return "", errors.Wrap(err, "failed to parse cidr")
}
ip = ip.To4()
ip[3] = 1

return ip.String(), nil
}

// Windows uses .1 as the gateway IP for each CIDR
paulyufan2 marked this conversation as resolved.
Show resolved Hide resolved
func (k *K8sSWIFTv2Middleware) addRoutes(cidrs []string) []cns.Route {
routes := make([]cns.Route, len(cidrs))
for i, cidr := range cidrs {
gatewayIP, _ := k.getIPv4Gateway(cidr)
routes[i] = cns.Route{
IPAddress: cidr,
GatewayIPAddress: gatewayIP,
}
}
return routes
}

func (k *K8sSWIFTv2Middleware) setInfraRoutes(podIPInfo *cns.PodIpInfo) ([]cns.Route, error) {
var routes []cns.Route

ip, err := netip.ParseAddr(podIPInfo.PodIPConfig.IPAddress)
if err != nil {
return nil, errors.Wrapf(err, "failed to parse podIPConfig IP address %s", podIPInfo.PodIPConfig.IPAddress)
}

v4IPs, v6IPs, err := k.GetCidrs()
if err != nil {
return nil, errors.Wrap(err, "failed to get CIDRs")
}

if ip.Is4() {
routes = append(routes, k.addRoutes(v4IPs)...)
} else {
routes = append(routes, k.addRoutes(v6IPs)...)
}

return routes, nil
}
37 changes: 36 additions & 1 deletion cns/middlewares/k8sSwiftV2_windows_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,22 @@ import (
"testing"

"github.com/Azure/azure-container-networking/cns"
"github.com/Azure/azure-container-networking/cns/configuration"
"github.com/Azure/azure-container-networking/cns/middlewares/mock"
"github.com/Azure/azure-container-networking/crd/multitenancy/api/v1alpha1"
"gotest.tools/v3/assert"
)

func TestSetRoutesSuccess(t *testing.T) {
middleware := K8sSWIFTv2Middleware{Cli: mock.NewClient()}
t.Setenv(configuration.EnvPodCIDRs, "10.0.1.10/24")
t.Setenv(configuration.EnvServiceCIDRs, "10.0.0.0/16")
t.Setenv(configuration.EnvInfraVNETCIDRs, "10.240.0.10/16")

podIPInfo := []cns.PodIpInfo{
{
PodIPConfig: cns.IPSubnet{
IPAddress: "10.0.1.10",
IPAddress: "10.0.1.100",
PrefixLength: 32,
},
NICType: cns.InfraNIC,
Expand All @@ -30,6 +34,34 @@ func TestSetRoutesSuccess(t *testing.T) {
MacAddress: "12:34:56:78:9a:bc",
},
}
desiredPodIPInfo := []cns.PodIpInfo{
{
PodIPConfig: cns.IPSubnet{
IPAddress: "10.0.1.100",
PrefixLength: 32,
},
NICType: cns.InfraNIC,
Routes: []cns.Route{
{
IPAddress: "10.0.1.10/24",
GatewayIPAddress: "10.0.1.1",
},
{
IPAddress: "10.0.0.0/16",
GatewayIPAddress: "10.0.0.1",
},
{
IPAddress: "10.240.0.10/16",
GatewayIPAddress: "10.240.0.1",
},
{
IPAddress: "0.0.0.0/0",
GatewayIPAddress: "0.0.0.0",
},
},
},
}

for i := range podIPInfo {
ipInfo := &podIPInfo[i]
err := middleware.setRoutes(ipInfo)
Expand All @@ -40,6 +72,9 @@ func TestSetRoutesSuccess(t *testing.T) {
assert.Equal(t, ipInfo.SkipDefaultRoutes, false)
}
}

// check if the routes are set as expected
reflect.DeepEqual(podIPInfo[0].Routes, desiredPodIPInfo[0].Routes)
}

func TestAssignSubnetPrefixSuccess(t *testing.T) {
Expand Down
Loading