Skip to content

Commit

Permalink
Bug 2007106: Extend dual-stack network subnet validations
Browse files Browse the repository at this point in the history 
This commit extends the validations performed for dual-stack OCP
clusters by making sure the following requirements are met if a cluster
is dual-stack

* exactly 2 machine networks are provided (if any)
* exactly 2 service networks are provided
* exactly 2 cluster networks are provided
* for every list containing 2 networks, the first one has to be IPv4 and
  the second one IPv6

Closes-bug: #2007106
Contributes-to: #2005440
Closes: OCPBUGSM-35447
Contributes-to: OCPBUGSM-35273
  • Loading branch information
@mkowalski
mkowalski committed Oct 4, 2021
1 parent e93b52c commit f2f96f1
Show file tree
Hide file tree
Showing 6 changed files with 267 additions and 103 deletions.
119 changes: 75 additions & 44 deletions internal/bminventory/inventory_test.go
View file
Expand Up @@ -6545,7 +6545,7 @@ var _ = Describe("[V2ClusterUpdate] cluster", func() {
MachineNetworks: []*models.MachineNetwork{{Cidr: "fd2e:6f44:5dd8:c956::/120"}, {Cidr: "10.12.0.0/16"}},
},
})
verifyApiErrorString(reply, http.StatusBadRequest, "IPv6 network provided before IPv4")
verifyApiErrorString(reply, http.StatusBadRequest, "First machine network has to be IPv4 subnet")
})
})
Context("Advanced networking validations", func() {
Expand Down Expand Up @@ -12476,19 +12476,26 @@ var _ = Describe("[V2UpdateCluster] IPv6 support disabled", func() {
var _ = Describe("Dual-stack cluster", func() {

var (
bm *bareMetalInventory
cfg Config
db *gorm.DB
ctx = context.Background()
bm *bareMetalInventory
cfg Config
db *gorm.DB
ctx = context.Background()
TestDualStackNetworkingWrongOrder = common.TestNetworking{
ClusterNetworks: append(common.TestIPv4Networking.ClusterNetworks, common.TestIPv6Networking.ClusterNetworks...),
ServiceNetworks: append(common.TestIPv4Networking.ServiceNetworks, common.TestIPv6Networking.ServiceNetworks...),
MachineNetworks: append(common.TestIPv4Networking.MachineNetworks, common.TestIPv6Networking.MachineNetworks...),
APIVip: common.TestIPv4Networking.APIVip,
IngressVip: common.TestIPv4Networking.IngressVip,
}
)

clusterNetworksWrongOrder := common.TestDualStackNetworking.ClusterNetworks
clusterNetworksWrongOrder := TestDualStackNetworkingWrongOrder.ClusterNetworks
clusterNetworksWrongOrder[0], clusterNetworksWrongOrder[1] = clusterNetworksWrongOrder[1], clusterNetworksWrongOrder[0]

serviceNetworksWrongOrder := common.TestDualStackNetworking.ServiceNetworks
serviceNetworksWrongOrder := TestDualStackNetworkingWrongOrder.ServiceNetworks
serviceNetworksWrongOrder[0], serviceNetworksWrongOrder[1] = serviceNetworksWrongOrder[1], serviceNetworksWrongOrder[0]

machineNetworksWrongOrder := common.TestDualStackNetworking.MachineNetworks
machineNetworksWrongOrder := TestDualStackNetworkingWrongOrder.MachineNetworks
machineNetworksWrongOrder[0], machineNetworksWrongOrder[1] = machineNetworksWrongOrder[1], machineNetworksWrongOrder[0]

BeforeEach(func() {
Expand All @@ -12502,7 +12509,6 @@ var _ = Describe("Dual-stack cluster", func() {
})

Context("Register cluster", func() {

var params installer.RegisterClusterParams

BeforeEach(func() {
Expand All @@ -12512,31 +12518,46 @@ var _ = Describe("Dual-stack cluster", func() {
})

Context("Cluster with wrong network order", func() {

const errorMsg = "IPv6 network provided before IPv4"

It("v6-first in cluster networks rejected", func() {
params.NewClusterParams.ClusterNetworks = clusterNetworksWrongOrder
params.NewClusterParams.ClusterNetworks = TestDualStackNetworkingWrongOrder.ClusterNetworks
reply := bm.RegisterCluster(ctx, params)
verifyApiErrorString(reply, http.StatusBadRequest, errorMsg)
verifyApiErrorString(reply, http.StatusBadRequest, "First cluster network has to be IPv4 subnet")
})

It("v6-first in service networks rejected", func() {
params.NewClusterParams.ServiceNetworks = serviceNetworksWrongOrder
params.NewClusterParams.ServiceNetworks = TestDualStackNetworkingWrongOrder.ServiceNetworks
reply := bm.RegisterCluster(ctx, params)
verifyApiErrorString(reply, http.StatusBadRequest, errorMsg)
verifyApiErrorString(reply, http.StatusBadRequest, "First service network has to be IPv4 subnet")
})

It("v6-first in machine networks rejected", func() {
params.NewClusterParams.MachineNetworks = machineNetworksWrongOrder
params.NewClusterParams.MachineNetworks = TestDualStackNetworkingWrongOrder.MachineNetworks
reply := bm.RegisterCluster(ctx, params)
verifyApiErrorString(reply, http.StatusBadRequest, errorMsg)
verifyApiErrorString(reply, http.StatusBadRequest, "First machine network has to be IPv4 subnet")
})
})

Context("Cluster with single network when two required", func() {
It("Single service network", func() {
params.NewClusterParams.ClusterNetworks = common.TestDualStackNetworking.ClusterNetworks
params.NewClusterParams.ServiceNetworks = common.TestIPv4Networking.ServiceNetworks
reply := bm.RegisterCluster(ctx, params)
verifyApiErrorString(reply, http.StatusBadRequest, "Expected 2 service networks, found 1")
})
It("Single cluster network", func() {
params.NewClusterParams.ClusterNetworks = common.TestIPv4Networking.ClusterNetworks
params.NewClusterParams.ServiceNetworks = common.TestDualStackNetworking.ServiceNetworks
reply := bm.RegisterCluster(ctx, params)
verifyApiErrorString(reply, http.StatusBadRequest, "Expected 2 cluster networks, found 1")
})
It("Single machine network", func() {
params.NewClusterParams.ServiceNetworks = common.TestDualStackNetworking.ServiceNetworks
params.NewClusterParams.MachineNetworks = common.TestIPv4Networking.MachineNetworks
reply := bm.RegisterCluster(ctx, params)
verifyApiErrorString(reply, http.StatusBadRequest, "Expected 2 machine networks, found 1")
})
})
})

Context("Update cluster", func() {

var params installer.UpdateClusterParams

BeforeEach(func() {
Expand All @@ -12547,31 +12568,46 @@ var _ = Describe("Dual-stack cluster", func() {
})

Context("Cluster with wrong network order", func() {

const errorMsg = "IPv6 network provided before IPv4"

It("v6-first in cluster networks rejected", func() {
params.ClusterUpdateParams.ClusterNetworks = clusterNetworksWrongOrder
params.ClusterUpdateParams.ClusterNetworks = TestDualStackNetworkingWrongOrder.ClusterNetworks
reply := bm.UpdateCluster(ctx, params)
verifyApiErrorString(reply, http.StatusBadRequest, errorMsg)
verifyApiErrorString(reply, http.StatusBadRequest, "First cluster network has to be IPv4 subnet")
})

It("v6-first in service networks rejected", func() {
params.ClusterUpdateParams.ServiceNetworks = serviceNetworksWrongOrder
params.ClusterUpdateParams.ServiceNetworks = TestDualStackNetworkingWrongOrder.ServiceNetworks
reply := bm.UpdateCluster(ctx, params)
verifyApiErrorString(reply, http.StatusBadRequest, errorMsg)
verifyApiErrorString(reply, http.StatusBadRequest, "First service network has to be IPv4 subnet")
})

It("v6-first in machine networks rejected", func() {
params.ClusterUpdateParams.MachineNetworks = machineNetworksWrongOrder
params.ClusterUpdateParams.MachineNetworks = TestDualStackNetworkingWrongOrder.MachineNetworks
reply := bm.UpdateCluster(ctx, params)
verifyApiErrorString(reply, http.StatusBadRequest, errorMsg)
verifyApiErrorString(reply, http.StatusBadRequest, "First machine network has to be IPv4 subnet")
})
})

Context("Cluster with single network when two required", func() {
It("Single service network", func() {
params.ClusterUpdateParams.ClusterNetworks = common.TestDualStackNetworking.ClusterNetworks
params.ClusterUpdateParams.ServiceNetworks = common.TestIPv4Networking.ServiceNetworks
reply := bm.UpdateCluster(ctx, params)
verifyApiErrorString(reply, http.StatusBadRequest, "Expected 2 service networks, found 1")
})
It("Single cluster network", func() {
params.ClusterUpdateParams.ClusterNetworks = common.TestIPv4Networking.ClusterNetworks
params.ClusterUpdateParams.ServiceNetworks = common.TestDualStackNetworking.ServiceNetworks
reply := bm.UpdateCluster(ctx, params)
verifyApiErrorString(reply, http.StatusBadRequest, "Expected 2 cluster networks, found 1")
})
It("Single machine network", func() {
params.ClusterUpdateParams.ServiceNetworks = common.TestDualStackNetworking.ServiceNetworks
params.ClusterUpdateParams.MachineNetworks = common.TestIPv4Networking.MachineNetworks
reply := bm.UpdateCluster(ctx, params)
verifyApiErrorString(reply, http.StatusBadRequest, "Expected 2 machine networks, found 1")
})
})
})

Context("[V2] Update cluster", func() {

var params installer.V2UpdateClusterParams

BeforeEach(func() {
Expand All @@ -12582,25 +12618,20 @@ var _ = Describe("Dual-stack cluster", func() {
})

Context("Cluster with wrong network order", func() {

const errorMsg = "IPv6 network provided before IPv4"

It("v6-first in cluster networks rejected", func() {
params.ClusterUpdateParams.ClusterNetworks = clusterNetworksWrongOrder
params.ClusterUpdateParams.ClusterNetworks = TestDualStackNetworkingWrongOrder.ClusterNetworks
reply := bm.V2UpdateCluster(ctx, params)
verifyApiErrorString(reply, http.StatusBadRequest, errorMsg)
verifyApiErrorString(reply, http.StatusBadRequest, "First cluster network has to be IPv4 subnet")
})

It("v6-first in service networks rejected", func() {
params.ClusterUpdateParams.ServiceNetworks = serviceNetworksWrongOrder
params.ClusterUpdateParams.ServiceNetworks = TestDualStackNetworkingWrongOrder.ServiceNetworks
reply := bm.V2UpdateCluster(ctx, params)
verifyApiErrorString(reply, http.StatusBadRequest, errorMsg)
verifyApiErrorString(reply, http.StatusBadRequest, "First service network has to be IPv4 subnet")
})

It("v6-first in machine networks rejected", func() {
params.ClusterUpdateParams.MachineNetworks = machineNetworksWrongOrder
params.ClusterUpdateParams.MachineNetworks = TestDualStackNetworkingWrongOrder.MachineNetworks
reply := bm.V2UpdateCluster(ctx, params)
verifyApiErrorString(reply, http.StatusBadRequest, errorMsg)
verifyApiErrorString(reply, http.StatusBadRequest, "First machine network has to be IPv4 subnet")
})
})
})
Expand Down
29 changes: 17 additions & 12 deletions internal/cluster/validations/validation_test.go
View file
Expand Up @@ -10,6 +10,8 @@ import (
_ "github.com/jinzhu/gorm/dialects/postgres"
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
"github.com/openshift/assisted-service/internal/network"
"github.com/openshift/assisted-service/models"
auth "github.com/openshift/assisted-service/pkg/auth"
"github.com/openshift/assisted-service/pkg/ocm"
"github.com/patrickmn/go-cache"
Expand Down Expand Up @@ -679,35 +681,38 @@ var _ = Describe("IPv6 support", func() {
}
})

var _ = Describe("Network amount and order", func() {
var _ = Describe("Machine Network amount and order", func() {
tests := []struct {
element []*string
element []*models.MachineNetwork
valid bool
}{
{
element: []*string{swag.String("1.2.5.0/24"), swag.String("1002:db8::/119")},
element: []*models.MachineNetwork{{Cidr: "1.2.5.0/24"}, {Cidr: "1002:db8::/119"}},
valid: true,
},
{
element: []*string{swag.String("1002:db8::/119"), swag.String("1.2.5.0/24")},
// Invalid because violates the "IPv4 subnet as the first one" constraint
element: []*models.MachineNetwork{{Cidr: "1002:db8::/119"}, {Cidr: "1.2.5.0/24"}},
valid: false,
},
{
element: []*string{swag.String("1.2.5.0/24"), swag.String("1002:db8::/119"), swag.String("1.2.6.0/24"), swag.String("1.2.7.0/24")},
valid: true,
// Invalid because violates the "exactly 2 networks" constraint
element: []*models.MachineNetwork{{Cidr: "1.2.5.0/24"}, {Cidr: "1002:db8::/119"}, {Cidr: "1.2.6.0/24"}, {Cidr: "1.2.7.0/24"}},
valid: false,
},
{
element: []*string{swag.String("1002:db8::/119"), swag.String("1.2.5.0/24"), swag.String("1.2.6.0/24"), swag.String("1.2.7.0/24")},
// Invalid because violates the "exactly 2 networks" constraint
element: []*models.MachineNetwork{{Cidr: "1002:db8::/119"}, {Cidr: "1.2.5.0/24"}, {Cidr: "1.2.6.0/24"}, {Cidr: "1.2.7.0/24"}},
valid: false,
},
}
for _, t := range tests {
t := t
It(fmt.Sprintf("Dual-stack network order validation. IP addresses/CIDRs: %v", t.element), func() {
for _, test := range tests {
t := test
It(fmt.Sprintf("Dual-stack machine network order validation. IP addresses/CIDRs: %v", t.element), func() {
if t.valid {
Expect(ValidateDualStackIPNetworksOrder(t.element...)).ToNot(HaveOccurred())
Expect(network.VerifyMachineNetworksDualStack(t.element, true)).ToNot(HaveOccurred())
} else {
Expect(ValidateDualStackIPNetworksOrder(t.element...)).To(HaveOccurred())
Expect(network.VerifyMachineNetworksDualStack(t.element, true)).To(HaveOccurred())
}
})
}
Expand Down
79 changes: 54 additions & 25 deletions internal/cluster/validations/validations.go
View file
Expand Up @@ -404,13 +404,63 @@ func ValidateIPAddresses(ipV6Supported bool, obj interface{}) error {
if err != nil {
return err
}
networkTypes := []string{"ClusterNetworks", "ServiceNetworks", "MachineNetworks"}
for _, net := range networkTypes {
networks := common.GetNetworkCidrAttr(obj, net)
err = ValidateDualStackIPNetworksOrder(networks...)
err = validateDualStackNetworks(obj)
if err != nil {
return err
}
return nil
}

func validateDualStackNetworks(clusterParams interface{}) error {
var machineNetworks []*models.MachineNetwork
var serviceNetworks []*models.ServiceNetwork
var clusterNetworks []*models.ClusterNetwork
var err error
var ipv4, ipv6 bool
reqDualStack := false

machineNetworks = network.DerefMachineNetworks(funk.Get(clusterParams, "MachineNetworks"))
serviceNetworks = network.DerefServiceNetworks(funk.Get(clusterParams, "ServiceNetworks"))
clusterNetworks = network.DerefClusterNetworks(funk.Get(clusterParams, "ClusterNetworks"))

ipv4, ipv6, err = network.GetAddressFamilies(machineNetworks)
if err != nil {
return err
}
reqDualStack = reqDualStack || (ipv4 && ipv6)

if !reqDualStack {
ipv4, ipv6, err = network.GetAddressFamilies(serviceNetworks)
if err != nil {
return err
}
reqDualStack = ipv4 && ipv6
}

if !reqDualStack {
ipv4, ipv6, err = network.GetAddressFamilies(clusterNetworks)
if err != nil {
return err
}
reqDualStack = ipv4 && ipv6
}

if reqDualStack {
if common.IsSliceNonEmpty(machineNetworks) {
if err := network.VerifyMachineNetworksDualStack(machineNetworks, true); err != nil {
return err
}
}
if common.IsSliceNonEmpty(serviceNetworks) {
if err := network.VerifyServiceNetworksDualStack(serviceNetworks, true); err != nil {
return err
}
}
if common.IsSliceNonEmpty(clusterNetworks) {
if err := network.VerifyClusterNetworksDualStack(clusterNetworks, true); err != nil {
return err
}
}
}
return nil
}
Expand All @@ -437,27 +487,6 @@ func ValidateIPAddressFamily(ipV6Supported bool, elements ...*string) error {
return nil
}

// ValidateDualStackIPNetworksOrder returns an error if for the dual-stack cluster the first
// network is not IPv4
func ValidateDualStackIPNetworksOrder(elements ...*string) error {
ipv4 := false
ipv6 := false
ipv6BeforeV4 := false
for _, e := range elements {
if e == nil || *e == "" {
continue
}
currRecordIPv6Stack := strings.Contains(*e, ":")
ipv4 = ipv4 || !currRecordIPv6Stack
ipv6 = ipv6 || currRecordIPv6Stack
ipv6BeforeV4 = ipv6BeforeV4 || (strings.Contains(*e, ":") && !ipv4)
}
if ipv4 && ipv6BeforeV4 {
return errors.Errorf("IPv6 network provided before IPv4")
}
return nil
}

func ValidateDiskEncryptionParams(diskEncryptionParams *models.DiskEncryption) error {
if diskEncryptionParams == nil {
return nil
Expand Down
22 changes: 0 additions & 22 deletions internal/network/cidr_validations.go
View file
Expand Up @@ -3,7 +3,6 @@ package network
import (
"net"

"github.com/openshift/assisted-service/models"
"github.com/pkg/errors"
)

Expand Down Expand Up @@ -126,24 +125,3 @@ func VerifyNetworkHostPrefix(prefix int64) error {
}
return nil
}

// Verify if the constrains for dual-stack machine networks are met:
// * there are exactly two machine networks
// * the first one is IPv4 subnet
// * the second one is IPv6 subnet
func VerifyMachineNetworksDualStack(networks []*models.MachineNetwork, isDualStack bool) error {
if !isDualStack {
return nil
}
if len(networks) != 2 {
return errors.Errorf("Expected 2 machine networks, found %d", len(networks))
}
if !IsIPV4CIDR(string(networks[0].Cidr)) {
return errors.Errorf("First machine network has to be IPv4 subnet")
}
if !IsIPv6CIDR(string(networks[1].Cidr)) {
return errors.Errorf("Second machine network has to be IPv6 subnet")
}

return nil
}

0 comments on commit f2f96f1

Please sign in to comment.