From 8ef87a5be34bd1967616e122c9ca326beae6cbcb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Andr=C3=A9?= Date: Thu, 17 Oct 2024 10:00:07 +0200 Subject: [PATCH 1/2] Validate MTU for custom network Set the minimum MTU to 1380 when deploying on IPv6. This value corresponds to the minimum MTU for IPv6, which is 1280, and the ovn-kubernetes overhead, which is 100. --- .../openstack/validation/cloudinfo.go | 23 +++-- .../openstack/validation/platform.go | 10 ++ .../openstack/validation/platform_test.go | 97 ++++++++++++++----- .../networking/v2/extensions/mtu/requests.go | 87 +++++++++++++++++ .../networking/v2/extensions/mtu/results.go | 8 ++ vendor/modules.txt | 1 + 6 files changed, 197 insertions(+), 29 deletions(-) create mode 100644 vendor/github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/mtu/requests.go create mode 100644 vendor/github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/mtu/results.go diff --git a/pkg/asset/installconfig/openstack/validation/cloudinfo.go b/pkg/asset/installconfig/openstack/validation/cloudinfo.go index 8b13f3aa9ab..a051c165b84 100644 --- a/pkg/asset/installconfig/openstack/validation/cloudinfo.go +++ b/pkg/asset/installconfig/openstack/validation/cloudinfo.go @@ -15,6 +15,7 @@ import ( tokensv3 "github.com/gophercloud/gophercloud/openstack/identity/v3/tokens" "github.com/gophercloud/gophercloud/openstack/imageservice/v2/images" "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/layer3/floatingips" + "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/mtu" networkquotasets "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/quotas" "github.com/gophercloud/gophercloud/openstack/networking/v2/networks" "github.com/gophercloud/gophercloud/openstack/networking/v2/subnets" @@ -35,11 +36,11 @@ import ( // CloudInfo caches data fetched from the user's openstack cloud type CloudInfo struct { APIFIP *floatingips.FloatingIP - ExternalNetwork *networks.Network + ExternalNetwork *Network Flavors map[string]Flavor IngressFIP *floatingips.FloatingIP ControlPlanePortSubnets []*subnets.Subnet - ControlPlanePortNetwork *networks.Network + ControlPlanePortNetwork *Network OSImage *images.Image ComputeZones []string VolumeZones []string @@ -50,6 +51,12 @@ type CloudInfo struct { clients *clients } +// Network holds a gophercloud network with additional info such as MTU. +type Network struct { + networks.Network + mtu.NetworkMTUExt +} + type clients struct { networkClient *gophercloud.ServiceClient computeClient *gophercloud.ServiceClient @@ -298,7 +305,7 @@ func (ci *CloudInfo) getFlavor(flavorName string) (Flavor, error) { }, nil } -func (ci *CloudInfo) getNetworkByName(networkName string) (*networks.Network, error) { +func (ci *CloudInfo) getNetworkByName(networkName string) (*Network, error) { if networkName == "" { return nil, nil } @@ -310,15 +317,16 @@ func (ci *CloudInfo) getNetworkByName(networkName string) (*networks.Network, er return nil, err } - network, err := networks.Get(ci.clients.networkClient, networkID).Extract() + var network Network + err = networks.Get(ci.clients.networkClient, networkID).ExtractInto(&network) if err != nil { return nil, err } - return network, nil + return &network, nil } -func (ci *CloudInfo) getNetwork(controlPlanePort *openstack.PortTarget) (*networks.Network, error) { +func (ci *CloudInfo) getNetwork(controlPlanePort *openstack.PortTarget) (*Network, error) { networkName := controlPlanePort.Network.Name networkID := controlPlanePort.Network.ID if networkName == "" && networkID == "" { @@ -336,7 +344,8 @@ func (ci *CloudInfo) getNetwork(controlPlanePort *openstack.PortTarget) (*networ return nil, err } - allNetworks, err := networks.ExtractNetworks(allPages) + var allNetworks []Network + err = networks.ExtractNetworksInto(allPages, &allNetworks) if err != nil { return nil, err } diff --git a/pkg/asset/installconfig/openstack/validation/platform.go b/pkg/asset/installconfig/openstack/validation/platform.go index 2417a82ce1d..a7373eb61b5 100644 --- a/pkg/asset/installconfig/openstack/validation/platform.go +++ b/pkg/asset/installconfig/openstack/validation/platform.go @@ -15,6 +15,13 @@ import ( "github.com/openshift/installer/pkg/types/openstack" ) +const ( + // MTU of 1280 is the minimum allowed for IPv6 + 100 for the + // OVN-Kubernetes encapsulation overhead + // https://issues.redhat.com/browse/OCPBUGS-2921 + minimumMTUv6 = 1380 +) + // ValidatePlatform checks that the specified platform is valid. func ValidatePlatform(p *openstack.Platform, n *types.Networking, ci *CloudInfo) field.ErrorList { var allErrs field.ErrorList @@ -90,6 +97,9 @@ func validateControlPlanePort(p *openstack.Platform, n *types.Networking, ci *Cl } else if ci.ControlPlanePortNetwork.ID != networkID { allErrs = append(allErrs, field.Invalid(fldPath.Child("controlPlanePort").Child("network"), networkDetail, "network must contain subnets")) } + if hasIPv6Subnet && ci.ControlPlanePortNetwork.MTU < minimumMTUv6 { + allErrs = append(allErrs, field.InternalError(fldPath.Child("controlPlanePort").Child("network"), fmt.Errorf("network should have an MTU of at least %d", minimumMTUv6))) + } } return allErrs } diff --git a/pkg/asset/installconfig/openstack/validation/platform_test.go b/pkg/asset/installconfig/openstack/validation/platform_test.go index 7fe0584e582..fe115e5c105 100644 --- a/pkg/asset/installconfig/openstack/validation/platform_test.go +++ b/pkg/asset/installconfig/openstack/validation/platform_test.go @@ -5,6 +5,7 @@ import ( "github.com/gophercloud/gophercloud/openstack/imageservice/v2/images" "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/layer3/floatingips" + "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/mtu" "github.com/gophercloud/gophercloud/openstack/networking/v2/networks" "github.com/gophercloud/gophercloud/openstack/networking/v2/subnets" "github.com/stretchr/testify/assert" @@ -54,17 +55,20 @@ func withControlPlanePortSubnets(subnetCIDR, allocationPoolStart, allocationPool {Start: allocationPoolStart, End: allocationPoolEnd}, }, } - Allsubnets := []*subnets.Subnet{&subnet} - ci.ControlPlanePortSubnets = Allsubnets + allsubnets := []*subnets.Subnet{&subnet} + ci.ControlPlanePortSubnets = allsubnets } } func validPlatformCloudInfo(options ...func(*CloudInfo)) *CloudInfo { ci := CloudInfo{ - ExternalNetwork: &networks.Network{ - ID: "71b97520-69af-4c35-8153-cdf827z96e60", - Name: validExternalNetwork, - AdminStateUp: true, - Status: "ACTIVE", + ExternalNetwork: &Network{ + networks.Network{ + ID: "71b97520-69af-4c35-8153-cdf827z96e60", + Name: validExternalNetwork, + AdminStateUp: true, + Status: "ACTIVE", + }, + mtu.NetworkMTUExt{}, }, APIFIP: &floatingips.FloatingIP{ ID: validFIP1, @@ -412,8 +416,8 @@ func TestMachineSubnet(t *testing.T) { subnet := subnets.Subnet{ ID: "00000000-5a89-4465-8d54-3517ec2bad48", } - Allsubnets := []*subnets.Subnet{&subnet} - ci.ControlPlanePortSubnets = Allsubnets + allsubnets := []*subnets.Subnet{&subnet} + ci.ControlPlanePortSubnets = allsubnets return ci }(), networking: validNetworking(), @@ -434,9 +438,11 @@ func TestMachineSubnet(t *testing.T) { NetworkID: "00000000-1a11-4465-8d54-3517ec2bad48", CIDR: "172.0.0.1/24", } - Allsubnets := []*subnets.Subnet{&subnet} - ci.ControlPlanePortSubnets = Allsubnets - ci.ControlPlanePortNetwork = &networks.Network{ID: "00000000-2a22-4465-8d54-3517ec2bad48"} + allSubnets := []*subnets.Subnet{&subnet} + ci.ControlPlanePortSubnets = allSubnets + network := Network{} + network.ID = "00000000-2a22-4465-8d54-3517ec2bad48" + ci.ControlPlanePortNetwork = &network return ci }(), networking: func() *types.Networking { @@ -462,8 +468,8 @@ func TestMachineSubnet(t *testing.T) { ID: validSubnetID, CIDR: "172.0.0.1/16", } - Allsubnets := []*subnets.Subnet{&subnet} - ci.ControlPlanePortSubnets = Allsubnets + allsubnets := []*subnets.Subnet{&subnet} + ci.ControlPlanePortSubnets = allsubnets return ci }(), networking: func() *types.Networking { @@ -505,8 +511,8 @@ func TestMachineSubnet(t *testing.T) { CIDR: "2001:db8::/64", IPVersion: 6, } - Allsubnets := []*subnets.Subnet{&subnet, &subnetv6} - ci.ControlPlanePortSubnets = Allsubnets + allsubnets := []*subnets.Subnet{&subnet, &subnetv6} + ci.ControlPlanePortSubnets = allsubnets return ci }(), networking: func() *types.Networking { @@ -535,8 +541,8 @@ func TestMachineSubnet(t *testing.T) { ID: validSubnetID, CIDR: "172.0.0.1/16", } - Allsubnets := []*subnets.Subnet{&subnet} - ci.ControlPlanePortSubnets = Allsubnets + allsubnets := []*subnets.Subnet{&subnet} + ci.ControlPlanePortSubnets = allsubnets return ci }(), networking: func() *types.Networking { @@ -576,8 +582,8 @@ func TestMachineSubnet(t *testing.T) { CIDR: "10.0.0.0/16", IPVersion: 4, } - Allsubnets := []*subnets.Subnet{&subnet, &subnetv6} - ci.ControlPlanePortSubnets = Allsubnets + allsubnets := []*subnets.Subnet{&subnet, &subnetv6} + ci.ControlPlanePortSubnets = allsubnets return ci }(), networking: func() *types.Networking { @@ -609,8 +615,8 @@ func TestMachineSubnet(t *testing.T) { CIDR: "2001:db8::/64", IPVersion: 6, } - Allsubnets := []*subnets.Subnet{&subnetv6} - ci.ControlPlanePortSubnets = Allsubnets + allsubnets := []*subnets.Subnet{&subnetv6} + ci.ControlPlanePortSubnets = allsubnets return ci }(), networking: func() *types.Networking { @@ -623,6 +629,53 @@ func TestMachineSubnet(t *testing.T) { }(), expectedErrMsg: `platform.openstack.controlPlanePort.fixedIPs: Internal error: one IPv4 subnet must be specified`, }, + { + name: "MTU too low", + platform: func() *openstack.Platform { + p := validPlatform() + fixedIPv6 := openstack.FixedIP{ + Subnet: openstack.SubnetFilter{ID: "00000000-1111-4465-8d54-3517ec2bad48"}, + } + p.ControlPlanePort = &openstack.PortTarget{ + FixedIPs: []openstack.FixedIP{fixedIPv6}, + Network: openstack.NetworkFilter{ID: "71b97520-69af-4c35-8153-cdf827z96e60"}, + } + return p + }(), + cloudInfo: func() *CloudInfo { + ci := validPlatformCloudInfo() + subnetv6 := subnets.Subnet{ + ID: "00000000-1111-4465-8d54-3517ec2bad48", + CIDR: "2001:db8::/64", + IPVersion: 6, + NetworkID: "71b97520-69af-4c35-8153-cdf827z96e60", + } + allSubnets := []*subnets.Subnet{&subnetv6} + ci.ControlPlanePortSubnets = allSubnets + + network := Network{ + networks.Network{ + ID: "71b97520-69af-4c35-8153-cdf827z96e60", + Name: "too-low", + AdminStateUp: true, + Status: "ACTIVE", + Subnets: []string{"00000000-1111-4465-8d54-3517ec2bad48"}, + }, + mtu.NetworkMTUExt{MTU: 1200}, + } + ci.ControlPlanePortNetwork = &network + return ci + }(), + networking: func() *types.Networking { + n := validNetworking() + machineNetworkEntry := &types.MachineNetworkEntry{ + CIDR: *ipnet.MustParseCIDR("2001:db8::/64"), + } + n.MachineNetwork = []types.MachineNetworkEntry{*machineNetworkEntry} + return n + }(), + expectedErrMsg: "platform.openstack.controlPlanePort.network: Internal error: network should have an MTU of at least 1380", + }, } for _, tc := range cases { diff --git a/vendor/github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/mtu/requests.go b/vendor/github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/mtu/requests.go new file mode 100644 index 00000000000..bffb7f5ef74 --- /dev/null +++ b/vendor/github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/mtu/requests.go @@ -0,0 +1,87 @@ +package mtu + +import ( + "fmt" + "net/url" + + "github.com/gophercloud/gophercloud" + "github.com/gophercloud/gophercloud/openstack/networking/v2/networks" +) + +// ListOptsExt adds an MTU option to the base ListOpts. +type ListOptsExt struct { + networks.ListOptsBuilder + + // The maximum transmission unit (MTU) value to address fragmentation. + // Minimum value is 68 for IPv4, and 1280 for IPv6. + MTU int `q:"mtu"` +} + +// ToNetworkListQuery adds the router:external option to the base network +// list options. +func (opts ListOptsExt) ToNetworkListQuery() (string, error) { + q, err := gophercloud.BuildQueryString(opts.ListOptsBuilder) + if err != nil { + return "", err + } + + params := q.Query() + if opts.MTU > 0 { + params.Add("mtu", fmt.Sprintf("%d", opts.MTU)) + } + + q = &url.URL{RawQuery: params.Encode()} + return q.String(), err +} + +// CreateOptsExt adds an MTU option to the base Network CreateOpts. +type CreateOptsExt struct { + networks.CreateOptsBuilder + + // The maximum transmission unit (MTU) value to address fragmentation. + // Minimum value is 68 for IPv4, and 1280 for IPv6. + MTU int `json:"mtu,omitempty"` +} + +// ToNetworkCreateMap adds an MTU to the base network creation options. +func (opts CreateOptsExt) ToNetworkCreateMap() (map[string]interface{}, error) { + base, err := opts.CreateOptsBuilder.ToNetworkCreateMap() + if err != nil { + return nil, err + } + + if opts.MTU == 0 { + return base, nil + } + + networkMap := base["network"].(map[string]interface{}) + networkMap["mtu"] = opts.MTU + + return base, nil +} + +// CreateOptsExt adds an MTU option to the base Network UpdateOpts. +type UpdateOptsExt struct { + networks.UpdateOptsBuilder + + // The maximum transmission unit (MTU) value to address fragmentation. + // Minimum value is 68 for IPv4, and 1280 for IPv6. + MTU int `json:"mtu,omitempty"` +} + +// ToNetworkUpdateMap adds an MTU to the base network uptade options. +func (opts UpdateOptsExt) ToNetworkUpdateMap() (map[string]interface{}, error) { + base, err := opts.UpdateOptsBuilder.ToNetworkUpdateMap() + if err != nil { + return nil, err + } + + if opts.MTU == 0 { + return base, nil + } + + networkMap := base["network"].(map[string]interface{}) + networkMap["mtu"] = opts.MTU + + return base, nil +} diff --git a/vendor/github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/mtu/results.go b/vendor/github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/mtu/results.go new file mode 100644 index 00000000000..497c9c37a04 --- /dev/null +++ b/vendor/github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/mtu/results.go @@ -0,0 +1,8 @@ +package mtu + +// NetworkMTUExt represents an extended form of a Network with additional MTU field. +type NetworkMTUExt struct { + // The maximum transmission unit (MTU) value to address fragmentation. + // Minimum value is 68 for IPv4, and 1280 for IPv6. + MTU int `json:"mtu"` +} diff --git a/vendor/modules.txt b/vendor/modules.txt index 730cbe1249f..0ba7fb252a5 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -696,6 +696,7 @@ github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/attributes github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/external github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/layer3/floatingips github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/layer3/routers +github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/mtu github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/quotas github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/security/groups github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/security/rules From 401c0d7941d9f64fa6aab0f76d80ff62afe07167 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Andr=C3=A9?= Date: Tue, 8 Oct 2024 08:51:47 +0200 Subject: [PATCH 2/2] Validate MTU when controlPlanePort uses FixedIPs The minimum MTU for the IPv6 network was previously only enforced when the controlPlanePort used the Network filter. This commit change the validation to also work when using the Subnet filter from FixedIPs. This commit does two things: - populate the ControlPlanePortNetwork struct in CloudInfo even when using the Subnet filter and not the Network one. - move the validation for MTU out of the branch for Network filter from the install-config.yaml --- pkg/asset/installconfig/openstack/validation/cloudinfo.go | 8 ++++++-- pkg/asset/installconfig/openstack/validation/platform.go | 6 +++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/pkg/asset/installconfig/openstack/validation/cloudinfo.go b/pkg/asset/installconfig/openstack/validation/cloudinfo.go index a051c165b84..64d571e0775 100644 --- a/pkg/asset/installconfig/openstack/validation/cloudinfo.go +++ b/pkg/asset/installconfig/openstack/validation/cloudinfo.go @@ -330,11 +330,15 @@ func (ci *CloudInfo) getNetwork(controlPlanePort *openstack.PortTarget) (*Networ networkName := controlPlanePort.Network.Name networkID := controlPlanePort.Network.ID if networkName == "" && networkID == "" { - return nil, nil + if len(ci.ControlPlanePortSubnets) > 0 && ci.ControlPlanePortSubnets[0].NetworkID != "" { + networkID = ci.ControlPlanePortSubnets[0].NetworkID + } else { + return nil, nil + } } opts := networks.ListOpts{} if networkID != "" { - opts.ID = controlPlanePort.Network.ID + opts.ID = networkID } if networkName != "" { opts.Name = controlPlanePort.Network.Name diff --git a/pkg/asset/installconfig/openstack/validation/platform.go b/pkg/asset/installconfig/openstack/validation/platform.go index a7373eb61b5..f6bd9a07a96 100644 --- a/pkg/asset/installconfig/openstack/validation/platform.go +++ b/pkg/asset/installconfig/openstack/validation/platform.go @@ -97,9 +97,9 @@ func validateControlPlanePort(p *openstack.Platform, n *types.Networking, ci *Cl } else if ci.ControlPlanePortNetwork.ID != networkID { allErrs = append(allErrs, field.Invalid(fldPath.Child("controlPlanePort").Child("network"), networkDetail, "network must contain subnets")) } - if hasIPv6Subnet && ci.ControlPlanePortNetwork.MTU < minimumMTUv6 { - allErrs = append(allErrs, field.InternalError(fldPath.Child("controlPlanePort").Child("network"), fmt.Errorf("network should have an MTU of at least %d", minimumMTUv6))) - } + } + if hasIPv6Subnet && ci.ControlPlanePortNetwork != nil && ci.ControlPlanePortNetwork.MTU < minimumMTUv6 { + allErrs = append(allErrs, field.InternalError(fldPath.Child("controlPlanePort").Child("network"), fmt.Errorf("network should have an MTU of at least %d", minimumMTUv6))) } return allErrs }