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.

Sign up for GitHub

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

Jump to bottom

OCPBUGS-29975: Allow multiple machine networks #6071

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

OCPBUGS-29975: Allow multiple machine networks #6071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
715cc3e
Fix typo in function name
zaneb Mar 8, 2024
2503b6b
Refactor GetMachineNetworkForUserManagedNetworking()
zaneb Mar 8, 2024
c409fe6
Use plural in function name
zaneb Mar 8, 2024
7f35e9f
Remove redundant code
zaneb Feb 28, 2024
1232119
Use fmt.Errorf() instead of deprecated errors package
zaneb Mar 8, 2024
51e76b1
MGMT-7587: Check all networks for CIDR overlap
zaneb Feb 29, 2024
7d1ac38
Validate VIPs against all MachineNetworks
zaneb Feb 29, 2024
c595634
Allow host to be in any MachineNetwork
zaneb Mar 11, 2024
41e0cbd
Allow multiple MachineNetworks in dual stack
zaneb Feb 28, 2024
bae9303
Revert "Forbid multiple machine networks in single-stack clusters"
zaneb Feb 27, 2024
f8bcf77
Find BootMACAddress from any machine network
zaneb Mar 11, 2024
ff9be84
Update comment on L2 connectivity checks and MachineNetworks
zaneb Mar 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 8 additions & 36 deletions internal/bminventory/inventory.go
View file
Open in desktop
Expand Up @@ -2145,48 +2145,36 @@ func (b *bareMetalInventory) updateNonDhcpNetworkParams(cluster *common.Cluster,
return common.NewApiError(http.StatusBadRequest, err)
}
if interactivity == Interactive && (params.ClusterUpdateParams.APIVips != nil || params.ClusterUpdateParams.IngressVips != nil) {
var primaryMachineNetworkCidr string
var secondaryMachineNetworkCidr string

matchRequired := network.GetApiVipById(&targetConfiguration, 0) != "" || network.GetIngressVipById(&targetConfiguration, 0) != ""

// We want to calculate Machine Network based on the API/Ingress VIPs only in case of the
// single-stack cluster. Auto calculation is not supported for dual-stack in which we
// require that user explicitly provides all the Machine Networks.
if reqDualStack {
if params.ClusterUpdateParams.MachineNetworks != nil {
primaryMachineNetworkCidr = string(params.ClusterUpdateParams.MachineNetworks[0].Cidr)
} else {
primaryMachineNetworkCidr = network.GetPrimaryMachineCidrForUserManagedNetwork(cluster, log)
}

err = network.VerifyMachineNetworksDualStack(targetConfiguration.MachineNetworks, reqDualStack)
if err != nil {
log.WithError(err).Warnf("Verify dual-stack machine networks")
return common.NewApiError(http.StatusBadRequest, err)
}
secondaryMachineNetworkCidr, err = network.GetSecondaryMachineCidr(&targetConfiguration)
if err != nil {
return common.NewApiError(http.StatusBadRequest, err)
}

if err = network.VerifyVips(cluster.Hosts, primaryMachineNetworkCidr, network.GetApiVipById(&targetConfiguration, 0), network.GetIngressVipById(&targetConfiguration, 0), log); err != nil {
if err = network.VerifyVips(cluster.Hosts, targetConfiguration.MachineNetworks, network.GetApiVipById(&targetConfiguration, 0), network.GetIngressVipById(&targetConfiguration, 0), log); err != nil {
log.WithError(err).Warnf("Verify VIPs")
return common.NewApiError(http.StatusBadRequest, err)
}

if len(targetConfiguration.IngressVips) == 2 && len(targetConfiguration.APIVips) == 2 { // in case there's a second set of VIPs
if err = network.VerifyVips(cluster.Hosts, secondaryMachineNetworkCidr, network.GetApiVipById(&targetConfiguration, 1), network.GetIngressVipById(&targetConfiguration, 1), log); err != nil {
if err = network.VerifyVips(cluster.Hosts, targetConfiguration.MachineNetworks, network.GetApiVipById(&targetConfiguration, 1), network.GetIngressVipById(&targetConfiguration, 1), log); err != nil {
log.WithError(err).Warnf("Verify VIPs")
return common.NewApiError(http.StatusBadRequest, err)
}
}

} else {
primaryMachineNetworkCidr, err = network.CalculateMachineNetworkCIDR(network.GetApiVipById(&targetConfiguration, 0), network.GetIngressVipById(&targetConfiguration, 0), cluster.Hosts, matchRequired)
primaryMachineNetworkCidr, err := network.CalculateMachineNetworkCIDR(network.GetApiVipById(&targetConfiguration, 0), network.GetIngressVipById(&targetConfiguration, 0), cluster.Hosts, matchRequired)
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be wrong according the concept presented by this PR, because we may have to machine-network - one per vip and one of both vips.

if err != nil {
return common.NewApiError(http.StatusBadRequest, errors.Wrap(err, "Calculate machine network CIDR"))
}
calculatedMachineNetworks := []*models.MachineNetwork{}
if primaryMachineNetworkCidr != "" {
// We set the machine networks in the ClusterUpdateParams, so they will be viewed as part of the request
// to update the cluster
Expand All @@ -2195,8 +2183,9 @@ func (b *bareMetalInventory) updateNonDhcpNetworkParams(cluster *common.Cluster,
// returned with an error. Therefore, params.ClusterUpdateParams.MachineNetworks is empty here before
// the assignment below.
params.ClusterUpdateParams.MachineNetworks = []*models.MachineNetwork{{Cidr: models.Subnet(primaryMachineNetworkCidr)}}
calculatedMachineNetworks = params.ClusterUpdateParams.MachineNetworks
}
if err = network.VerifyVips(cluster.Hosts, primaryMachineNetworkCidr, network.GetApiVipById(&targetConfiguration, 0), network.GetIngressVipById(&targetConfiguration, 0), log); err != nil {
if err = network.VerifyVips(cluster.Hosts, calculatedMachineNetworks, network.GetApiVipById(&targetConfiguration, 0), network.GetIngressVipById(&targetConfiguration, 0), log); err != nil {
log.WithError(err).Warnf("Verify VIPs")
return common.NewApiError(http.StatusBadRequest, err)
}
Expand Down Expand Up @@ -2450,25 +2439,8 @@ func (b *bareMetalInventory) updateNetworks(db *gorm.DB, params installer.V2Upda

if params.ClusterUpdateParams.ClusterNetworks != nil || params.ClusterUpdateParams.ServiceNetworks != nil ||
params.ClusterUpdateParams.MachineNetworks != nil {
// TODO MGMT-7587: Support any number of subnets
// Assumes that the number of cluster networks equal to the number of service networks
for index := range cluster.ClusterNetworks {
machineNetworkCidr := ""
if len(cluster.MachineNetworks) > index {
machineNetworkCidr = string(cluster.MachineNetworks[index].Cidr)
}

serviceNetworkCidr := ""
if len(cluster.ServiceNetworks) > index {
serviceNetworkCidr = string(cluster.ServiceNetworks[index].Cidr)
}

if err = network.VerifyClusterCIDRsNotOverlap(machineNetworkCidr,
string(cluster.ClusterNetworks[index].Cidr),
serviceNetworkCidr,
userManagedNetworking); err != nil {
return common.NewApiError(http.StatusBadRequest, err)
}
if err := network.VerifyNoNetworkCidrOverlaps(cluster.ClusterNetworks, cluster.MachineNetworks, cluster.ServiceNetworks); err != nil {
return common.NewApiError(http.StatusBadRequest, err)
}
}
if updated {
Expand Down
153 changes: 53 additions & 100 deletions internal/bminventory/inventory_test.go
View file
Open in desktop
Expand Up @@ -4197,7 +4197,7 @@ var _ = Describe("cluster", func() {
},
})
verifyApiErrorString(reply, http.StatusBadRequest,
"ingress-vip <1.2.3.20> does not belong to machine-network-cidr <10.11.0.0/16>")
"ingress-vip <1.2.3.20> does not belong to any Machine Network")
})
It("Same api and ingress", func() {
apiVip := "10.11.12.15"
Expand Down Expand Up @@ -4319,33 +4319,6 @@ var _ = Describe("cluster", func() {
actual := reply.(*installer.V2UpdateClusterCreated)
Expect(len(actual.Payload.MachineNetworks)).To(Equal(2))
})
It("Wrong order of machine network CIDRs in non dhcp for dual-stack", func() {
mockClusterUpdatability(1)
mockSuccess(1)

apiVip := "10.11.12.15"
ingressVip := "10.11.12.16"
reply := bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
APIVips: []*models.APIVip{{IP: models.IP(apiVip)}},
IngressVips: []*models.IngressVip{{IP: models.IP(ingressVip)}},
ClusterNetworks: []*models.ClusterNetwork{{Cidr: "10.128.0.0/14", HostPrefix: 23}, {Cidr: "fd01::/48", HostPrefix: 64}},
MachineNetworks: []*models.MachineNetwork{{Cidr: "10.11.0.0/16"}, {Cidr: "fd2e:6f44:5dd8:c956::/120"}},
},
})
Expect(reply).To(BeAssignableToTypeOf(installer.NewV2UpdateClusterCreated()))

reply = bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
APIVips: []*models.APIVip{{IP: models.IP(apiVip)}},
IngressVips: []*models.IngressVip{{IP: models.IP(ingressVip)}},
MachineNetworks: []*models.MachineNetwork{{Cidr: "fd2e:6f44:5dd8:c956::/120"}, {Cidr: "10.12.0.0/16"}},
},
})
verifyApiErrorString(reply, http.StatusBadRequest, "First machine network has to be IPv4 subnet")
})
It("API VIP in wrong subnet for dual-stack", func() {
apiVip := "10.11.12.15"
ingressVip := "10.11.12.16"
Expand All @@ -4358,7 +4331,7 @@ var _ = Describe("cluster", func() {
MachineNetworks: []*models.MachineNetwork{{Cidr: "10.12.0.0/16"}, {Cidr: "fd2e:6f44:5dd8:c956::/120"}},
},
})
verifyApiErrorString(reply, http.StatusBadRequest, "api-vip <10.11.12.15> does not belong to machine-network-cidr <10.12.0.0/16>")
verifyApiErrorString(reply, http.StatusBadRequest, "api-vip <10.11.12.15> does not belong to any Machine Network")
})
})

Expand Down Expand Up @@ -4826,9 +4799,9 @@ var _ = Describe("cluster", func() {

Context("Networks", func() {
var (
clusterNetworks = common.TestIPv4Networking.ClusterNetworks
serviceNetworks = common.TestIPv4Networking.ServiceNetworks
machineNetworks = common.TestIPv4Networking.MachineNetworks
clusterNetworks = []*models.ClusterNetwork{{Cidr: "1.1.0.0/24", HostPrefix: 24}, {Cidr: "2.2.0.0/24", HostPrefix: 24}}
serviceNetworks = []*models.ServiceNetwork{{Cidr: "3.3.0.0/24"}, {Cidr: "4.4.0.0/24"}}
machineNetworks = []*models.MachineNetwork{{Cidr: "5.5.0.0/24"}, {Cidr: "6.6.0.0/24"}}
)

setNetworksClusterID := func(clusterID strfmt.UUID,
Expand Down Expand Up @@ -5017,22 +4990,16 @@ var _ = Describe("cluster", func() {
verifyApiErrorString(reply, http.StatusBadRequest, "Machine network CIDR '': Failed to parse CIDR '': invalid CIDR address: ")
})

It("Multiple machine networks illegal for single-stack", func() {
reply := bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
MachineNetworks: []*models.MachineNetwork{{Cidr: "15.15.0.0/21"}, {Cidr: "16.16.0.0/21"}},
},
})
verifyApiErrorString(reply, http.StatusBadRequest, "Single-stack cluster cannot contain multiple Machine Networks")
})
It("Override networks - additional subnet", func() {
clusterNetworks = []*models.ClusterNetwork{{Cidr: "11.11.0.0/21", HostPrefix: 24}, {Cidr: "12.12.0.0/21", HostPrefix: 24}}
serviceNetworks = []*models.ServiceNetwork{{Cidr: "13.13.0.0/21"}, {Cidr: "14.14.0.0/21"}}
machineNetworks = []*models.MachineNetwork{{Cidr: "15.15.0.0/21"}, {Cidr: "16.16.0.0/21"}}

It("Override networks - additional cluster subnet", func() {
mockSuccess(1)
reply := bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
ClusterNetworks: []*models.ClusterNetwork{{Cidr: "11.11.0.0/21", HostPrefix: 24}, {Cidr: "12.12.0.0/21", HostPrefix: 24}},
ClusterNetworks: clusterNetworks,
ServiceNetworks: serviceNetworks,
MachineNetworks: machineNetworks,
VipDhcpAllocation: swag.Bool(true),
Expand All @@ -5041,19 +5008,20 @@ var _ = Describe("cluster", func() {
Expect(reply).To(BeAssignableToTypeOf(installer.NewV2UpdateClusterCreated()))
actual := reply.(*installer.V2UpdateClusterCreated)

validateNetworkConfiguration(actual.Payload,
&[]*models.ClusterNetwork{{Cidr: "11.11.0.0/21", HostPrefix: 24}, {Cidr: "12.12.0.0/21", HostPrefix: 24}},
&serviceNetworks,
&machineNetworks)
validateNetworkConfiguration(actual.Payload, &clusterNetworks, &serviceNetworks, &machineNetworks)
validateHostsRequestedHostname(actual.Payload)
})

It("Override networks - 2 additional cluster subnets", func() {
It("Override networks - 2 additional subnets", func() {
clusterNetworks = []*models.ClusterNetwork{{Cidr: "10.10.0.0/21", HostPrefix: 24}, {Cidr: "11.11.0.0/21", HostPrefix: 24}, {Cidr: "12.12.0.0/21", HostPrefix: 24}}
serviceNetworks = []*models.ServiceNetwork{{Cidr: "13.13.0.0/21"}, {Cidr: "14.14.0.0/21"}}
machineNetworks = []*models.MachineNetwork{{Cidr: "15.15.0.0/21"}, {Cidr: "16.16.0.0/21"}}

mockSuccess(1)
reply := bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
ClusterNetworks: []*models.ClusterNetwork{{Cidr: "10.10.0.0/21", HostPrefix: 24}, {Cidr: "11.11.0.0/21", HostPrefix: 24}, {Cidr: "12.12.0.0/21", HostPrefix: 24}},
ClusterNetworks: clusterNetworks,
ServiceNetworks: serviceNetworks,
MachineNetworks: machineNetworks,
VipDhcpAllocation: swag.Bool(true),
Expand All @@ -5062,10 +5030,7 @@ var _ = Describe("cluster", func() {
Expect(reply).To(BeAssignableToTypeOf(installer.NewV2UpdateClusterCreated()))
actual := reply.(*installer.V2UpdateClusterCreated)

validateNetworkConfiguration(actual.Payload,
&[]*models.ClusterNetwork{{Cidr: "10.10.0.0/21", HostPrefix: 24}, {Cidr: "11.11.0.0/21", HostPrefix: 24}, {Cidr: "12.12.0.0/21", HostPrefix: 24}},
&serviceNetworks,
&machineNetworks)
validateNetworkConfiguration(actual.Payload, &clusterNetworks, &serviceNetworks, &machineNetworks)
validateHostsRequestedHostname(actual.Payload)
})

Expand Down Expand Up @@ -14471,7 +14436,7 @@ var _ = Describe("TestRegisterCluster", func() {
VipDhcpAllocation: swag.Bool(false),
},
})
verifyApiErrorString(reply, http.StatusBadRequest, "api-vip <10.11.12.15> does not belong to machine-network-cidr <1.2.3.0/24>")
verifyApiErrorString(reply, http.StatusBadRequest, "api-vip <10.11.12.15> does not belong to any Machine Network")
})
It("Ingress VIP not in Machine Network", func() {
apiVip := "1.2.3.5"
Expand All @@ -14487,7 +14452,7 @@ var _ = Describe("TestRegisterCluster", func() {
VipDhcpAllocation: swag.Bool(false),
},
})
verifyApiErrorString(reply, http.StatusBadRequest, "ingress-vip <10.11.12.16> does not belong to machine-network-cidr <1.2.3.0/24>")
verifyApiErrorString(reply, http.StatusBadRequest, "ingress-vip <10.11.12.16> does not belong to any Machine Network")
})
It("API VIP and Ingress VIP with empty Machine Networks", func() {
apiVip := "10.11.12.15"
Expand All @@ -14504,40 +14469,6 @@ var _ = Describe("TestRegisterCluster", func() {
})
verifyApiErrorString(reply, http.StatusBadRequest, "Dual-stack cluster cannot be created with empty Machine Networks")
})

It("API VIP from IPv6 Machine Network", func() {
apiVip := "1001:db8::64"
ingressVip := "1.2.3.6"

reply := bm.V2RegisterCluster(ctx, installer.V2RegisterClusterParams{
NewClusterParams: &models.ClusterCreateParams{
APIVips: []*models.APIVip{{IP: models.IP(apiVip)}},
IngressVips: []*models.IngressVip{{IP: models.IP(ingressVip)}},
ClusterNetworks: common.TestDualStackNetworking.ClusterNetworks,
MachineNetworks: common.TestDualStackNetworking.MachineNetworks,
ServiceNetworks: common.TestDualStackNetworking.ServiceNetworks,
VipDhcpAllocation: swag.Bool(false),
},
})
verifyApiErrorString(reply, http.StatusBadRequest, "api-vip <1001:db8::64> does not belong to machine-network-cidr <1.2.3.0/24>")
})

It("Ingress VIP from IPv6 Machine Network", func() {
apiVip := "1.2.3.5"
ingressVip := "1001:db8::65"

reply := bm.V2RegisterCluster(ctx, installer.V2RegisterClusterParams{
NewClusterParams: &models.ClusterCreateParams{
APIVips: []*models.APIVip{{IP: models.IP(apiVip)}},
IngressVips: []*models.IngressVip{{IP: models.IP(ingressVip)}},
ClusterNetworks: common.TestDualStackNetworking.ClusterNetworks,
MachineNetworks: common.TestDualStackNetworking.MachineNetworks,
ServiceNetworks: common.TestDualStackNetworking.ServiceNetworks,
VipDhcpAllocation: swag.Bool(false),
},
})
verifyApiErrorString(reply, http.StatusBadRequest, "ingress-vip <1001:db8::65> does not belong to machine-network-cidr <1.2.3.0/24>")
})
})

Context("V2 Update cluster", func() {
Expand Down Expand Up @@ -14981,9 +14912,9 @@ var _ = Describe("TestRegisterCluster", func() {

Context("Networking", func() {
var (
clusterNetworks = common.TestIPv4Networking.ClusterNetworks
serviceNetworks = common.TestIPv4Networking.ServiceNetworks
machineNetworks = common.TestIPv4Networking.MachineNetworks
clusterNetworks = []*models.ClusterNetwork{{Cidr: "1.1.1.0/24", HostPrefix: 24}, {Cidr: "2.2.2.0/24", HostPrefix: 24}}
serviceNetworks = []*models.ServiceNetwork{{Cidr: "3.3.3.0/24"}, {Cidr: "4.4.4.0/24"}}
machineNetworks = []*models.MachineNetwork{{Cidr: "5.5.5.0/24"}, {Cidr: "6.6.6.0/24"}, {Cidr: "7.7.7.0/24"}}
)

registerCluster := func() *models.Cluster {
Expand Down Expand Up @@ -17322,9 +17253,36 @@ var _ = Describe("Dual-stack cluster", func() {
reply := bm.V2RegisterCluster(ctx, params)
verifyApiErrorString(reply, http.StatusBadRequest, errStr)
})
It("v6-first in machine networks rejected", func() {
errStr := "First machine network has to be IPv4 subnet"
params.NewClusterParams.MachineNetworks = TestDualStackNetworkingWrongOrder.MachineNetworks
})

Context("Cluster with two networks of same stack", func() {
It("only v4 in cluster networks rejected", func() {
errStr := "Second cluster network has to be IPv6 subnet"
params.NewClusterParams.ClusterNetworks = []*models.ClusterNetwork{
{Cidr: "1.3.0.0/16", HostPrefix: 16},
{Cidr: "1.4.0.0/16", HostPrefix: 16},
}
params.NewClusterParams.ServiceNetworks = common.TestDualStackNetworking.ServiceNetworks
reply := bm.V2RegisterCluster(ctx, params)
verifyApiErrorString(reply, http.StatusBadRequest, errStr)
})
It("only v4 in service networks rejected", func() {
errStr := "Second service network has to be IPv6 subnet"
params.NewClusterParams.ClusterNetworks = common.TestDualStackNetworking.ClusterNetworks
params.NewClusterParams.ServiceNetworks = []*models.ServiceNetwork{
{Cidr: "1.2.5.0/24"},
{Cidr: "1.2.6.0/24"},
}
reply := bm.V2RegisterCluster(ctx, params)
verifyApiErrorString(reply, http.StatusBadRequest, errStr)
})
It("only v4 in machine networks rejected", func() {
errStr := "One machine network has to be IPv6 subnet"
params.NewClusterParams.ClusterNetworks = common.TestDualStackNetworking.ClusterNetworks
params.NewClusterParams.MachineNetworks = []*models.MachineNetwork{
{Cidr: "1.2.3.0/24"},
{Cidr: "1.2.4.0/24"},
}
reply := bm.V2RegisterCluster(ctx, params)
verifyApiErrorString(reply, http.StatusBadRequest, errStr)
})
Expand Down Expand Up @@ -17388,11 +17346,6 @@ var _ = Describe("Dual-stack cluster", func() {
reply := bm.V2UpdateCluster(ctx, params)
verifyApiErrorString(reply, http.StatusBadRequest, "First service network has to be IPv4 subnet")
})
It("v6-first in machine networks rejected", func() {
params.ClusterUpdateParams.MachineNetworks = TestDualStackNetworkingWrongOrder.MachineNetworks
reply := bm.V2UpdateCluster(ctx, params)
verifyApiErrorString(reply, http.StatusBadRequest, "First machine network has to be IPv4 subnet")
})
})

Context("Cluster with single network when two required", func() {
Expand Down