Skip to content

fix(rest-api): enforce the IPv4 contract for ETV Subnets #5529

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
chet wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(rest-api): enforce the IPv4 contract for ETV Subnets #5529

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
25 changes: 15 additions & 10 deletions rest-api/api/pkg/api/handler/subnet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ const DefaultReservedIPCount = 2

// ~~~~~ Create Handler ~~~~~ //

// CreateSubnetHandler is the API Handler for creating new Subnet
// CreateSubnetHandler creates IPv4 Subnets for Ethernet virtualizer VPCs.
// FNN VPCs use the separate VPC Prefix resource.
type CreateSubnetHandler struct {
dbSession *cdb.Session
tc temporalClient.Client
Expand All @@ -65,8 +66,8 @@ func NewCreateSubnetHandler(dbSession *cdb.Session, tc temporalClient.Client, sc
}

// Handle godoc
// @Summary Create a Subnet
// @Description Create a Subnet
// @Summary Create Subnet
// @Description Creates an IPv4 Subnet in an Ethernet virtualizer VPC. FNN VPCs use VPC Prefixes.
// @Tags Subnet
// @Accept json
// @Produce json
Expand Down Expand Up @@ -134,9 +135,9 @@ func (csh CreateSubnetHandler) Handle(c echo.Context) error {
return cutil.NewAPIErrorResponse(c, http.StatusBadRequest, "Tenant for VPC in request does not match tenant in org", nil)
}

// Verify if vpc is ethernet virtualized
// A nil `NetworkVirtualizationType` represents legacy Ethernet virtualizer VPCs.
if vpc.NetworkVirtualizationType != nil && *vpc.NetworkVirtualizationType != cdbm.VpcEthernetVirtualizer {
return cutil.NewAPIErrorResponse(c, http.StatusBadRequest, fmt.Sprintf("VPC: %v specified in request must have Ethernet network virtualization type in order to create Subnets", vpc.ID), nil)
return cutil.NewAPIErrorResponse(c, http.StatusBadRequest, fmt.Sprintf("VPC: %v must be an Ethernet virtualizer VPC for REST Subnet creation", vpc.ID), nil)
}

// Verify if vpc is ready
Expand All @@ -161,21 +162,25 @@ func (csh CreateSubnetHandler) Handle(c echo.Context) error {
return cutil.NewAPIErrorResponse(c, http.StatusBadRequest, "The Site where the Subnet is being created must be in Registered state in order to proceed", nil)
}

// Validate IPBlocks in request
// NOTE: model validation ensures non-nil IPv4BlockID
// Validate the Ready, tenant-allocated IPv4 IP block for this Subnet.
// Model validation ensures IPv4BlockID is non-nil.
ipBlockFilter := cdbm.IPBlockFilterInput{}
ipBlockFilter.TenantAllocated(tenant.ID)
ipBlockFilter.Statuses = []string{cdbm.IPBlockStatusReady}
ipv4Block, err := common.GetIPBlockFromIDString(ctx, nil, *apiRequest.IPv4BlockID, ipBlockFilter, csh.dbSession)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
if err != nil {
logger.Warn().Err(err).Msg("error getting IPv4 IPBlock in request")
return cutil.NewAPIErrorResponse(c, http.StatusBadRequest, "Error retrieving ipv4 IPBlock from request", nil)
return cutil.NewAPIErrorResponse(c, http.StatusBadRequest, "Could not find a Ready, tenant-allocated IPv4 IP block specified by ipv4BlockId", nil)
}
if ipv4Block.ProtocolVersion != cdbm.IPBlockProtocolVersionV4 {
logger.Warn().Msg("ipv4BlockId must reference an IPv4 IP block for Subnet creation")
return cutil.NewAPIErrorResponse(c, http.StatusBadRequest, "ipv4BlockId must reference an IPv4 IP block for Subnet creation", nil)
}
if vpc.SiteID != ipv4Block.SiteID {
logger.Warn().Msg("IPv4 Block specified in request and VPC do not belong to the same Site")
return cutil.NewAPIErrorResponse(c, http.StatusBadRequest, "IPv4 Block specified in request and VPC do not belong to the same Site", nil)
}
// NOTE: validation ensures that IPv6BlockID will be nil, ie, it is not supported yet
// when IPv6 is supported, further validations must ensure that the RoutingType of v4 and v6 must match
// A REST Subnet uses one IPv4 block, so its routing type comes from that block.
routingType := ipv4Block.RoutingType

// Check for name uniqueness for the tenant, ie, Tenant cannot have another Subnet with same name at the Site
Expand Down
67 changes: 57 additions & 10 deletions rest-api/api/pkg/api/handler/subnet_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ func TestSubnetHandler_Create(t *testing.T) {
vpc6 := testSubnetBuildVpc(t, dbSession, ip, tenant2, site, tnOrg1, "testVPC", cutil.GetPtr(cdbm.VpcEthernetVirtualizer), cdbm.VpcStatusReady, cutil.GetPtr(uuid.New()))
vpc7 := testSubnetBuildVpc(t, dbSession, ip, tenant2, site4, tnOrg2, "testVPC", cutil.GetPtr(cdbm.VpcEthernetVirtualizer), cdbm.VpcStatusReady, cutil.GetPtr(uuid.New()))
vpc8 := testSubnetBuildVpc(t, dbSession, ip, tenant1, site, tnOrg1, "testVPC", cutil.GetPtr(cdbm.VpcFNN), cdbm.VpcStatusReady, cutil.GetPtr(uuid.New()))
vpcLegacy := testSubnetBuildVpc(t, dbSession, ip, tenant1, site, tnOrg1, "legacyVPC", nil, cdbm.VpcStatusReady, cutil.GetPtr(uuid.New()))

cfg := common.GetTestConfig()
tempClient := &tmocks.Client{}
Expand Down Expand Up @@ -186,6 +187,14 @@ func TestSubnetHandler_Create(t *testing.T) {
parentPref1, err := ipam.CreateIpamEntryForIPBlock(ctx, ipamStorage, ipb1.Prefix, ipb1.PrefixLength, ipb1.RoutingType, ipb1.InfrastructureProviderID.String(), ipb1.SiteID.String())
assert.Nil(t, err)
assert.NotNil(t, parentPref1)
ipbV6 := testIPBlockBuildIPBlock(t, dbSession, "testipbv6", site, ip, &tenant1.ID, cdbm.IPBlockRoutingTypeDatacenterOnly, "2001:db8::", 64, cdbm.IPBlockProtocolVersionV6, false, cdbm.IPBlockStatusReady, ipu)
// Do not create an IPAM parent for this Pending block. Its test must fail
// during source-block validation.
ipbPending := testIPBlockBuildIPBlock(t, dbSession, "testipbpending", site, ip, &tenant1.ID, cdbm.IPBlockRoutingTypeDatacenterOnly, "192.172.0.0", 16, cdbm.IPBlockProtocolVersionV4, false, cdbm.IPBlockStatusPending, ipu)
ipbLegacy := testIPBlockBuildIPBlock(t, dbSession, "testipblegacy", site, ip, &tenant1.ID, cdbm.IPBlockRoutingTypeDatacenterOnly, "192.173.0.0", 16, cdbm.IPBlockProtocolVersionV4, false, cdbm.IPBlockStatusReady, ipu)
parentPrefLegacy, err := ipam.CreateIpamEntryForIPBlock(ctx, ipamStorage, ipbLegacy.Prefix, ipbLegacy.PrefixLength, ipbLegacy.RoutingType, ipbLegacy.InfrastructureProviderID.String(), ipbLegacy.SiteID.String())
assert.Nil(t, err)
assert.NotNil(t, parentPrefLegacy)

ipb2 := testIPBlockBuildIPBlock(t, dbSession, "testipb", site2, ip2, &tenant2.ID, cdbm.IPBlockRoutingTypeDatacenterOnly, "192.168.0.0", 16, cdbm.IPBlockProtocolVersionV4, false, cdbm.IPBlockStatusReady, ipu)
parentPref2, err := ipam.CreateIpamEntryForIPBlock(ctx, ipamStorage, ipb2.Prefix, ipb2.PrefixLength, ipb2.RoutingType, ipb2.InfrastructureProviderID.String(), ipb2.SiteID.String())
Expand Down Expand Up @@ -215,15 +224,19 @@ func TestSubnetHandler_Create(t *testing.T) {
prefixLen := 24
okBody, err := json.Marshal(model.APISubnetCreateRequest{Name: "ok1", Description: cutil.GetPtr(""), VpcID: vpc1.ID.String(), IPv4BlockID: cutil.GetPtr(ipb1.ID.String()), PrefixLength: prefixLen})
assert.Nil(t, err)
okBodyLegacyVpc, err := json.Marshal(model.APISubnetCreateRequest{Name: "legacy-vpc", Description: cutil.GetPtr(""), VpcID: vpcLegacy.ID.String(), IPv4BlockID: cutil.GetPtr(ipbLegacy.ID.String()), PrefixLength: prefixLen})
assert.Nil(t, err)
errBodyPendingIPv4Block, err := json.Marshal(model.APISubnetCreateRequest{Name: "pending-ip-block", Description: cutil.GetPtr(""), VpcID: vpc1.ID.String(), IPv4BlockID: cutil.GetPtr(ipbPending.ID.String()), PrefixLength: prefixLen})
assert.Nil(t, err)

prefixLen = 16
okBodyFG, err := json.Marshal(model.APISubnetCreateRequest{Name: "okFG", Description: cutil.GetPtr(""), VpcID: vpc1.ID.String(), IPv4BlockID: cutil.GetPtr(ipbFG.ID.String()), PrefixLength: prefixLen})
assert.Nil(t, err)
prefixLen = 30
okBodySlash30, err := json.Marshal(model.APISubnetCreateRequest{Name: "ok31", Description: cutil.GetPtr(""), VpcID: vpc1.ID.String(), IPv4BlockID: cutil.GetPtr(ipb1.ID.String()), PrefixLength: prefixLen})
okBodySlash30, err := json.Marshal(model.APISubnetCreateRequest{Name: "ok30", Description: cutil.GetPtr(""), VpcID: vpc1.ID.String(), IPv4BlockID: cutil.GetPtr(ipb1.ID.String()), PrefixLength: prefixLen})
assert.Nil(t, err)
prefixLen = 31
errBodySlash31, err := json.Marshal(model.APISubnetCreateRequest{Name: "err32", Description: cutil.GetPtr(""), VpcID: vpc1.ID.String(), IPv4BlockID: cutil.GetPtr(ipb1.ID.String()), PrefixLength: prefixLen})
errBodySlash31, err := json.Marshal(model.APISubnetCreateRequest{Name: "err31", Description: cutil.GetPtr(""), VpcID: vpc1.ID.String(), IPv4BlockID: cutil.GetPtr(ipb1.ID.String()), PrefixLength: prefixLen})
assert.Nil(t, err)
prefixLen = 24
okBodyNameClash, err := json.Marshal(model.APISubnetCreateRequest{Name: "ok1", Description: cutil.GetPtr(""), VpcID: vpc2.ID.String(), IPv4BlockID: cutil.GetPtr(ipb3.ID.String()), PrefixLength: prefixLen})
Expand All @@ -241,11 +254,13 @@ func TestSubnetHandler_Create(t *testing.T) {
assert.Nil(t, err)
errBodyBadIPv4BlockID, err := json.Marshal(model.APISubnetCreateRequest{Name: "ok1", Description: cutil.GetPtr(""), VpcID: vpc1.ID.String(), IPv4BlockID: cutil.GetPtr(uuid.New().String()), PrefixLength: prefixLen})
assert.Nil(t, err)
errBodyIPv6AsIPv4Block, err := json.Marshal(model.APISubnetCreateRequest{Name: "ipv6-as-ipv4", Description: cutil.GetPtr(""), VpcID: vpc1.ID.String(), IPv4BlockID: cutil.GetPtr(ipbV6.ID.String()), PrefixLength: prefixLen})
assert.Nil(t, err)
errBodyTenantSitePrefixID, err := json.Marshal(model.APISubnetCreateRequest{Name: "private-prefix", Description: cutil.GetPtr(""), VpcID: vpc1.ID.String(), IPv4BlockID: cutil.GetPtr(tenantSitePrefix.ID.String()), PrefixLength: prefixLen})
assert.Nil(t, err)
errBodyNoIPv4, err := json.Marshal(model.APISubnetCreateRequest{Name: "ok1", Description: cutil.GetPtr(""), VpcID: vpc1.ID.String(), PrefixLength: prefixLen})
assert.Nil(t, err)
errBodyBadIPv6BlockID, err := json.Marshal(model.APISubnetCreateRequest{Name: "ok1", Description: cutil.GetPtr(""), VpcID: vpc1.ID.String(), IPv6BlockID: cutil.GetPtr(uuid.New().String()), PrefixLength: prefixLen})
errBodyBadIPv6BlockID, err := json.Marshal(model.APISubnetCreateRequest{Name: "ok1", Description: cutil.GetPtr(""), VpcID: vpc1.ID.String(), IPv4BlockID: cutil.GetPtr(ipb1.ID.String()), IPv6BlockID: cutil.GetPtr(uuid.New().String()), PrefixLength: prefixLen})
assert.Nil(t, err)

errBodyBadIPv4BlockIDTenantMismatch, err := json.Marshal(model.APISubnetCreateRequest{Name: "ok1", Description: cutil.GetPtr(""), VpcID: vpc1.ID.String(), IPv4BlockID: cutil.GetPtr(ipb2.ID.String()), PrefixLength: prefixLen})
Expand Down Expand Up @@ -344,23 +359,23 @@ func TestSubnetHandler_Create(t *testing.T) {
expectedStatus: http.StatusBadRequest,
},
{
name: "error when vpc in request is not ethernet virtualization",
name: "REST Subnet creation rejects an FNN VPC",
reqOrgName: tnOrg1,
reqBody: string(errBodyBadVpctype),
user: tnu,
expectedErr: true,
expectedStatus: http.StatusBadRequest,
},
{
name: "error when ipv6block is present in request",
name: "REST Subnet creation rejects ipv6BlockId",
reqOrgName: tnOrg1,
reqBody: string(errBodyBadIPv6BlockID),
user: tnu,
expectedErr: true,
expectedStatus: http.StatusBadRequest,
},
{
name: "error when ipv4block is not present in request",
name: "REST Subnet creation requires ipv4BlockId",
reqOrgName: tnOrg1,
reqBody: string(errBodyNoIPv4),
user: tnu,
Expand All @@ -375,14 +390,32 @@ func TestSubnetHandler_Create(t *testing.T) {
expectedErr: true,
expectedStatus: http.StatusBadRequest,
},
{
name: "Pending tenant IPv4 block is rejected before IPAM allocation",
reqOrgName: tnOrg1,
reqBody: string(errBodyPendingIPv4Block),
user: tnu,
expectedErr: true,
expectedStatus: http.StatusBadRequest,
expectedErrMsg: "Could not find a Ready, tenant-allocated IPv4 IP block specified by ipv4BlockId",
},
{
name: "ipv4BlockId rejects an IPv6 IP block",
reqOrgName: tnOrg1,
reqBody: string(errBodyIPv6AsIPv4Block),
user: tnu,
expectedErr: true,
expectedStatus: http.StatusBadRequest,
expectedErrMsg: "ipv4BlockId must reference an IPv4 IP block for Subnet creation",
},
{
name: "Tenant SitePrefix cannot be the source IP Block for a Subnet",
reqOrgName: tnOrg1,
reqBody: string(errBodyTenantSitePrefixID),
user: tnu,
expectedErr: true,
expectedStatus: http.StatusBadRequest,
expectedErrMsg: "Error retrieving ipv4 IPBlock from request",
expectedErrMsg: "Could not find a Ready, tenant-allocated IPv4 IP block specified by ipv4BlockId",
},
{
name: "error when ipv4 block in request is not derived for tenant",
Expand Down Expand Up @@ -419,6 +452,16 @@ func TestSubnetHandler_Create(t *testing.T) {
expectedGateway: "192.168.0.1",
expectedPrefix: "192.168.0.0",
},
{
name: "Ready legacy VPC without networkVirtualizationType remains compatible",
reqOrgName: tnOrg1,
reqBody: string(okBodyLegacyVpc),
user: tnu,
expectedErr: false,
expectedStatus: http.StatusCreated,
expectedGateway: "192.173.0.1",
expectedPrefix: "192.173.0.0",
},
{
name: "success case with Full Grant",
reqOrgName: tnOrg1,
Expand All @@ -431,7 +474,7 @@ func TestSubnetHandler_Create(t *testing.T) {
verifyChildSpanner: true,
},
{
name: "success case with /31",
name: "success case with /30",
reqOrgName: tnOrg1,
reqBody: string(okBodySlash30),
user: tnu,
Expand All @@ -441,7 +484,7 @@ func TestSubnetHandler_Create(t *testing.T) {
expectedPrefix: "192.168.1.0",
},
{
name: "error case with /32",
name: "error case with /31",
reqOrgName: tnOrg1,
reqBody: string(errBodySlash31),
user: tnu,
Expand Down Expand Up @@ -555,7 +598,11 @@ func TestSubnetHandler_Create(t *testing.T) {
}
} else {
if tc.expectedErrMsg != "" {
assert.Contains(t, rec.Body.String(), tc.expectedErrMsg)
var apiErr struct {
Message string `json:"message"`
}
require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &apiErr))
assert.Equal(t, tc.expectedErrMsg, apiErr.Message)
}
}

Expand Down
19 changes: 10 additions & 9 deletions rest-api/api/pkg/api/model/subnet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,30 +16,31 @@ import (
)

const (
// SubnetBlockSizeMin is the minimum value of the SubnetSize field
// SubnetBlockSizeMin is the minimum IPv4 prefix length accepted by REST Subnet creation.
SubnetBlockSizeMin = 8
// SubnetBlockSizeMax is the maximum value of the SubnetSize field
// SubnetBlockSizeMax is the maximum IPv4 prefix length accepted by REST Subnet creation.
SubnetBlockSizeMax = 30

validationErrorIPv4BlockIDRequired = "IPv4BlockID is required in request"
validationErrorIPv6SubnetNotSupported = "IPv6 Subnet creation is not supported at this time"
validationErrorIPv4BlockIDRequired = "REST Subnet creation requires ipv4BlockId"
validationErrorIPv6SubnetNotSupported = "ipv6BlockId is not supported for Subnets in Ethernet virtualizer VPCs; FNN VPCs use VPC Prefixes"
validationErrorSubnetBlockSizeMin = "prefixLength must be at least 8"
validationErrorSubnetBlockSizeMax = "prefixLength must be at most 30"
)

// APISubnetCreateRequest is the data structure to capture user request to create a new Subnet
// APISubnetCreateRequest captures a request to create an IPv4 Subnet in an Ethernet virtualizer VPC.
// FNN VPCs use the separate VPC Prefix resource.
type APISubnetCreateRequest struct {
// Name is the name of the Subnet
Name string `json:"name"`
// Description is the description of the Subnet
Description *string `json:"description"`
// VpcID is the ID of the vpc containing the Subnet
VpcID string `json:"vpcId"`
// IPv4BlockID is the derived IPv4BlockId for the tenant from an allocation
// IPv4BlockID identifies a Ready, derived IPv4 block for the tenant from an allocation.
IPv4BlockID *string `json:"ipv4BlockId"`
// IPv6BlockID is the derived IPv6BlockId for the tenant from an allocation
// IPv6BlockID is retained so legacy ipv6BlockId input can be rejected explicitly.
IPv6BlockID *string `json:"ipv6BlockId"`
// PrefixLength is the length of the prefix
// PrefixLength is the length of the IPv4 prefix.
PrefixLength int `json:"prefixLength"`
}

Expand All @@ -60,7 +61,7 @@ func (scr *APISubnetCreateRequest) Validate() error {
validation.Required.Error(validationErrorIPv4BlockIDRequired),
validation.When(scr.IPv4BlockID != nil, validationis.UUID.Error(validationErrorInvalidUUID))),
validation.Field(&scr.IPv6BlockID,
// IPv6 is not supported yet
// Subnets in Ethernet virtualizer VPCs support IPv4 only.
validation.Nil.Error(validationErrorIPv6SubnetNotSupported)),
validation.Field(&scr.PrefixLength,
validation.Required.Error(validationErrorValueRequired),
Expand Down
24 changes: 12 additions & 12 deletions rest-api/api/pkg/api/model/subnet_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,10 @@ func TestAPISubnetCreateRequest_Validate(t *testing.T) {
prefix32 := 32
prefix31 := 31
tests := []struct {
desc string
obj APISubnetCreateRequest
expectErr bool
desc string
obj APISubnetCreateRequest
expectErr bool
expectedErrMsg string
}{
{
desc: "error when Name is not provided",
Expand Down Expand Up @@ -53,12 +54,13 @@ func TestAPISubnetCreateRequest_Validate(t *testing.T) {
expectErr: true,
},
{
desc: "error when IPv6Block is specified",
obj: APISubnetCreateRequest{Name: "ab", Description: cutil.GetPtr("abc"), VpcID: uuid.New().String(), IPv6BlockID: cutil.GetPtr(uuid.New().String()), PrefixLength: prefix24},
expectErr: true,
desc: "REST Subnet creation rejects ipv6BlockId",
obj: APISubnetCreateRequest{Name: "ab", Description: cutil.GetPtr("abc"), VpcID: uuid.New().String(), IPv4BlockID: cutil.GetPtr(uuid.New().String()), IPv6BlockID: cutil.GetPtr(uuid.New().String()), PrefixLength: prefix24},
expectErr: true,
expectedErrMsg: "ipv6BlockId: " + validationErrorIPv6SubnetNotSupported + ".",
},
{
desc: "error when neither IPv6Block nor IPv6Block are specified",
desc: "REST Subnet creation requires ipv4BlockId",
obj: APISubnetCreateRequest{Name: "ab", Description: cutil.GetPtr("abc"), VpcID: uuid.New().String(), PrefixLength: prefix24},
expectErr: true,
},
Expand All @@ -77,11 +79,6 @@ func TestAPISubnetCreateRequest_Validate(t *testing.T) {
obj: APISubnetCreateRequest{Name: "ab", Description: cutil.GetPtr("abc"), VpcID: uuid.New().String(), IPv4BlockID: cutil.GetPtr(uuid.New().String()), PrefixLength: prefix24},
expectErr: false,
},
{
desc: "ok when only IPv4BlockID is specified",
obj: APISubnetCreateRequest{Name: "ab", Description: cutil.GetPtr("abc"), VpcID: uuid.New().String(), IPv4BlockID: cutil.GetPtr(uuid.New().String()), PrefixLength: prefix24},
expectErr: false,
},
{
desc: "error when /32 subnet is created",
obj: APISubnetCreateRequest{Name: "ab", Description: cutil.GetPtr("abc"), VpcID: uuid.New().String(), IPv4BlockID: cutil.GetPtr(uuid.New().String()), PrefixLength: prefix32},
Expand All @@ -98,6 +95,9 @@ func TestAPISubnetCreateRequest_Validate(t *testing.T) {
err := tc.obj.Validate()
assert.Equal(t, tc.expectErr, err != nil)
if err != nil {
if tc.expectedErrMsg != "" {
assert.Equal(t, tc.expectedErrMsg, err.Error())
}
fmt.Println(err.Error())
}
})
Expand Down
Loading
Loading