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

Fix nil pointer dereference in subnet validation #3150

Merged
merged 2 commits into from
Sep 11, 2023
Merged

Fix nil pointer dereference in subnet validation #3150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
478d0ef
Fix nil pointer dereference when addressPrefixes is set, but addressP…
bennerv Sep 6, 2023
27a8c91
Use a mix of addressPrefix and addressPrefixes in e2e
bennerv Sep 6, 2023
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
4 changes: 3 additions & 1 deletion pkg/deploy/assets/cluster-development-predeploy.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@
},
{
"properties": {
"addressPrefix": "[parameters('masterAddressPrefix')]",
"addressPrefixes": [
"[parameters('masterAddressPrefix')]"
],
"routeTable": {
"id": "[resourceid('Microsoft.Network/routeTables', concat(parameters('clusterName'), '-rt'))]",
"tags": null
Expand Down
4 changes: 3 additions & 1 deletion pkg/deploy/generator/resources_cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ func (g *generator) clusterMasterSubnet() *arm.Resource {
return &arm.Resource{
Resource: &mgmtnetwork.Subnet{
SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{
AddressPrefix: to.StringPtr("[parameters('masterAddressPrefix')]"),
AddressPrefixes: &[]string{
*to.StringPtr("[parameters('masterAddressPrefix')]"),
},
RouteTable: &mgmtnetwork.RouteTable{
ID: to.StringPtr("[resourceid('Microsoft.Network/routeTables', concat(parameters('clusterName'), '-rt'))]"),
},
Expand Down
46 changes: 33 additions & 13 deletions pkg/validate/dynamic/dynamic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ var (
errMsgInvalidVNetLocation = "The vnet location '%s' must match the cluster location '%s'."
)

const minimumSubnetMaskSize int = 27

type Subnet struct {
// ID is a resource id of the subnet
ID string
Expand Down Expand Up @@ -764,23 +766,41 @@ func (dv *dynamic) ValidateSubnets(ctx context.Context, oc *api.OpenShiftCluster
)
}

_, net, err := net.ParseCIDR(*ss.AddressPrefix)
if err != nil {
return err
// Handle both addressPrefix & addressPrefixes
if ss.AddressPrefix == nil {
for _, address := range *ss.AddressPrefixes {
bennerv marked this conversation as resolved.
Show resolved Hide resolved
if err = validateSubnetSize(s, address); err != nil {
return err
}
Comment on lines +772 to +774
Copy link
Member

@cblecker cblecker Sep 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If this is validating a range, do we want to error if any element of the slice doesn't validate?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Honestly I'm not sure.

I'm not even sure how openshift will handle this. You can technically specify multiple IP address ranges on a subnet now (wasn't that the point of a subnet to begin with!??).

For now, we've only seen customers with a single one defined. You have to opt into the MSFT AFEC feature flag Microsoft.Network/AllowMultipleAddressPrefixesOnSubnet in order to use it for now.

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 not even sure how openshift will handle this. You can technically specify multiple IP address ranges on a subnet now (wasn't that the point of a subnet to begin with!??).

For now, we've only seen customers with a single one defined. You have to opt into the MSFT AFEC feature flag Microsoft.Network/AllowMultipleAddressPrefixesOnSubnet in order to use it for now.

I poked around in the installer code and nothing I found indicated that the installer will be unhappy if you provide a subnet with multiple address ranges. Complicated stuff though, so it would be good to ask the installer folks to verify.

If OCP doesn't support multiple address ranges, we can error out here if the cx has put more than one and then the original question in this thread is moot.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the investigation.

I also poked around in machine-config-provider-azure and there is never an explicit subnet address assignment used during NIC provisioning, so it should be handled transparently. The only thing that is specified during creation is the subnet ID.

So I think the proper validation here would be to add up all the address spaces and make sure the total address space is >27 mask, also taking into account the number of reserved IP addresses in smaller address spaces may have an impact.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not perfect, but instead of adding up individual ranges, I'm going to validate that at least one address prefix has a /27 or greater and error out if not.

If we have a bunch of /30s it's not going to make sense as there isn't enough address space for anything there. The cases around this are going to get crazy, and at least we're returning a valid message to the customer.

}
} else {
if err = validateSubnetSize(s, *ss.AddressPrefix); err != nil {
return err
}
}
}

ones, _ := net.Mask.Size()
if ones > 27 {
return api.NewCloudError(
http.StatusBadRequest,
api.CloudErrorCodeInvalidLinkedVNet,
s.Path,
errMsgSubnetInvalidSize,
s.ID,
)
}
return nil
}

// validateSubnetSize checks if the subnet mask is >27, and returns
// an error if so as it is too small for OCP
func validateSubnetSize(s Subnet, address string) error {
_, net, err := net.ParseCIDR(address)
if err != nil {
return err
}

ones, _ := net.Mask.Size()
if ones > minimumSubnetMaskSize {
return api.NewCloudError(
http.StatusBadRequest,
api.CloudErrorCodeInvalidLinkedVNet,
s.Path,
errMsgSubnetInvalidSize,
s.ID,
)
}
return nil
}

Expand Down
29 changes: 29 additions & 0 deletions pkg/validate/dynamic/dynamic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package dynamic
import (
"context"
"errors"
"fmt"
"net/http"
"testing"
"time"
Expand Down Expand Up @@ -1727,3 +1728,31 @@ func TestCheckBYONsg(t *testing.T) {
})
}
}

func TestValidateSubnetSize(t *testing.T) {
subnetId := "id"
subnetPath := "path"
for _, tt := range []struct {
name string
address string
subnet Subnet
wantErr string
}{
{
name: "subnet size is too small",
address: "10.0.0.0/32",
subnet: Subnet{ID: subnetId, Path: subnetPath},
wantErr: fmt.Sprintf("400: InvalidLinkedVNet: %s: The provided subnet '%s' is invalid: must be /27 or larger.", subnetPath, subnetId),
},
{
name: "subnet size is gucci gang",
Copy link
Contributor

Choose a reason for hiding this comment

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

🚀

address: "10.0.0.0/27",
subnet: Subnet{ID: subnetId, Path: subnetPath},
},
} {
t.Run(tt.name, func(t *testing.T) {
err := validateSubnetSize(tt.subnet, tt.address)
utilerror.AssertErrorMessage(t, err, tt.wantErr)
})
}
}