Skip to content

MGMT-22553: Add missing fallbacks for network configs in VIP validation #8704

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

Open
LiorSoffer wants to merge 1 commit into
base: master
Choose a base branch
from
Open

MGMT-22553: Add missing fallbacks for network configs in VIP validation #8704

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
45 changes: 45 additions & 0 deletions internal/cluster/validations/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1061,6 +1061,51 @@ var _ = Describe("Parse functions", func() {
})
})

var _ = Describe("ValidateClusterUpdateVIPAddresses - partial network updates", func() {
var cluster common.Cluster

BeforeEach(func() {
cluster = common.Cluster{
Cluster: models.Cluster{
OpenshiftVersion: "4.14.0",
CPUArchitecture: "x86_64",
MachineNetworks: []*models.MachineNetwork{
{Cidr: "192.168.1.0/24"},
{Cidr: "2001:db8::/64"},
},
ServiceNetworks: []*models.ServiceNetwork{
{Cidr: "172.30.0.0/16"},
{Cidr: "fd02::/112"},
},
ClusterNetworks: []*models.ClusterNetwork{
{Cidr: "10.128.0.0/14", HostPrefix: 23},
{Cidr: "fd01::/48", HostPrefix: 64},
},
APIVips: []*models.APIVip{
{IP: "192.168.1.100"},
{IP: "2001:db8::100"},
},
IngressVips: []*models.IngressVip{
{IP: "192.168.1.101"},
{IP: "2001:db8::101"},
},
},
}
})

It("preserves MachineNetworks when only updating ClusterNetworks in dual-stack cluster", func() {
params := &models.V2ClusterUpdateParams{
ClusterNetworks: []*models.ClusterNetwork{
{Cidr: "10.128.0.0/15", HostPrefix: 22},
{Cidr: "fd01::/48", HostPrefix: 66},
},
}

err := ValidateClusterUpdateVIPAddresses(true, &cluster, params, nil)
Expect(err).ShouldNot(HaveOccurred())
})
})

func TestCluster(t *testing.T) {
RegisterFailHandler(Fail)
RunSpecs(t, "cluster validations tests")
Expand Down
42 changes: 34 additions & 8 deletions internal/cluster/validations/validations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,8 @@ func ValidateClusterCreateIPAddresses(ipV6Supported bool, clusterId strfmt.UUID,
targetConfiguration.LoadBalancer = params.LoadBalancer
targetConfiguration.OpenshiftVersion = swag.StringValue(params.OpenshiftVersion)
targetConfiguration.PrimaryIPStack = primaryIPStack
return validateVIPAddresses(ipV6Supported, targetConfiguration)
// For create, if MachineNetworks is provided, we should validate VIPs against them
return validateVIPAddresses(ipV6Supported, targetConfiguration, params.MachineNetworks != nil)
}

func validateVIPsWithUMA(cluster *common.Cluster, params *models.V2ClusterUpdateParams, vipDhcpAllocation bool) error {
Expand Down Expand Up @@ -310,9 +311,26 @@ func ValidateClusterUpdateVIPAddresses(ipV6Supported bool, cluster *common.Clust
targetConfiguration.APIVips = apiVips
targetConfiguration.IngressVips = ingressVips
targetConfiguration.UserManagedNetworking = params.UserManagedNetworking
targetConfiguration.ClusterNetworks = params.ClusterNetworks
targetConfiguration.ServiceNetworks = params.ServiceNetworks
targetConfiguration.MachineNetworks = params.MachineNetworks
targetConfiguration.ClusterNetworks = cluster.ClusterNetworks
targetConfiguration.ServiceNetworks = cluster.ServiceNetworks
targetConfiguration.MachineNetworks = cluster.MachineNetworks

if len(params.APIVips) > 0 {
targetConfiguration.APIVips = params.APIVips
}
if len(params.IngressVips) > 0 {
targetConfiguration.IngressVips = params.IngressVips
}
if len(params.ClusterNetworks) > 0 {
targetConfiguration.ClusterNetworks = params.ClusterNetworks
}
if len(params.ServiceNetworks) > 0 {
targetConfiguration.ServiceNetworks = params.ServiceNetworks
}
if len(params.MachineNetworks) > 0 {
targetConfiguration.MachineNetworks = params.MachineNetworks
}

targetConfiguration.LoadBalancer = cluster.LoadBalancer

// Copy fields that should be preserved from the existing cluster
Expand All @@ -328,7 +346,11 @@ func ValidateClusterUpdateVIPAddresses(ipV6Supported bool, cluster *common.Clust
}

targetConfiguration.PrimaryIPStack = primaryIPStack
return validateVIPAddresses(ipV6Supported, targetConfiguration)
// machineNetworksInParams indicates whether MachineNetworks was explicitly provided.
// VIP-in-network validation should only run when MachineNetworks is in params because
// in saas mode, MachineNetworks may be auto-calculated from VIPs later and the actual vip-in-network
// validation happens after auto calculation in updateNonDhcpNetworkParams
return validateVIPAddresses(ipV6Supported, targetConfiguration, params.MachineNetworks != nil)
}

func VerifyParsableVIPs(apiVips []*models.APIVip, ingressVips []*models.IngressVip) error {
Expand Down Expand Up @@ -485,7 +507,7 @@ func validateVIPAddressFamily(ipV6Supported bool, targetConfiguration common.Clu
return allAddresses, nil
}

func validateVIPAddresses(ipV6Supported bool, targetConfiguration common.Cluster) error {
func validateVIPAddresses(ipV6Supported bool, targetConfiguration common.Cluster, machineNetworksInParams bool) error {
var allAddresses []*string
var multiErr error
var err error
Expand Down Expand Up @@ -538,7 +560,10 @@ func validateVIPAddresses(ipV6Supported bool, targetConfiguration common.Cluster
return common.NewApiError(http.StatusBadRequest, err)
}
} else if !network.IsLoadBalancerUserManaged(&targetConfiguration) {
if len(targetConfiguration.MachineNetworks) > 0 {
// Only validate vip in machine networks when MachineNetworks was explicitly provided in params.
// If not provided, auto-calculation may set MachineNetworks later (saas mode) or the
// actual validation happens in updateNonDhcpNetworkParams after auto-calculation.
if len(targetConfiguration.MachineNetworks) > 0 && machineNetworksInParams {
for i := range targetConfiguration.APIVips { // len of APIVips and IngressVips should be the same. asserted above.
err = network.VerifyVipsForClusterManagedLoadBalancer(
nil,
Expand All @@ -554,7 +579,8 @@ func validateVIPAddresses(ipV6Supported bool, targetConfiguration common.Cluster
if multiErr != nil && !strings.Contains(multiErr.Error(), "0 errors occurred") {
return multiErr
}
} else if reqDualStack {
// Machine networks are not auto-calculated for dual-stack clusters, so they must be provided explicitly
} else if reqDualStack && len(targetConfiguration.MachineNetworks) == 0 {
return errors.New("Dual-stack cluster cannot be created with empty Machine Networks")
}
} else {
Expand Down