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

🐛 Restore subnet management functionality #4474

Merged
merged 3 commits into from
Oct 8, 2023
Merged

🐛 Restore subnet management functionality #4474

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
391452a
Revert "Make subnet spec id field required for SSA to work with CC"
vincepri Aug 31, 2023
63f22ec
Revert "Remove multi-az e2e test (it is no longer supported)"
vincepri Aug 31, 2023
703ec18
Fix SSA support by adding Subnet.ResourceID field
vincepri Sep 1, 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
36 changes: 36 additions & 0 deletions api/v1beta1/awscluster_conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package v1beta1

import (
apiconversion "k8s.io/apimachinery/pkg/conversion"
infrav2 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
utilconversion "sigs.k8s.io/cluster-api/util/conversion"
"sigs.k8s.io/controller-runtime/pkg/conversion"
Expand Down Expand Up @@ -73,6 +74,37 @@ func (src *AWSCluster) ConvertTo(dstRaw conversion.Hub) error {

dst.Spec.NetworkSpec.AdditionalControlPlaneIngressRules = restored.Spec.NetworkSpec.AdditionalControlPlaneIngressRules

if restored.Spec.NetworkSpec.VPC.IPAMPool != nil {
if dst.Spec.NetworkSpec.VPC.IPAMPool == nil {
dst.Spec.NetworkSpec.VPC.IPAMPool = &infrav2.IPAMPool{}
}

restoreIPAMPool(restored.Spec.NetworkSpec.VPC.IPAMPool, dst.Spec.NetworkSpec.VPC.IPAMPool)
}

if restored.Spec.NetworkSpec.VPC.IsIPv6Enabled() && restored.Spec.NetworkSpec.VPC.IPv6.IPAMPool != nil {
if dst.Spec.NetworkSpec.VPC.IPv6.IPAMPool == nil {
dst.Spec.NetworkSpec.VPC.IPv6.IPAMPool = &infrav2.IPAMPool{}
}

restoreIPAMPool(restored.Spec.NetworkSpec.VPC.IPv6.IPAMPool, dst.Spec.NetworkSpec.VPC.IPv6.IPAMPool)
}

dst.Spec.NetworkSpec.AdditionalControlPlaneIngressRules = restored.Spec.NetworkSpec.AdditionalControlPlaneIngressRules
vincepri marked this conversation as resolved.
Show resolved Hide resolved

// Restore SubnetSpec.ResourceID field, if any.
for _, subnet := range restored.Spec.NetworkSpec.Subnets {
if len(subnet.ResourceID) == 0 {
vincepri marked this conversation as resolved.
Show resolved Hide resolved
continue
}
for i, dstSubnet := range dst.Spec.NetworkSpec.Subnets {
if dstSubnet.ID == subnet.ID {
dstSubnet.ResourceID = subnet.ResourceID
dstSubnet.DeepCopyInto(&dst.Spec.NetworkSpec.Subnets[i])
}
}
}

return nil
}

Expand Down Expand Up @@ -133,3 +165,7 @@ func (r *AWSClusterList) ConvertFrom(srcRaw conversion.Hub) error {

return Convert_v1beta2_AWSClusterList_To_v1beta1_AWSClusterList(src, r, nil)
}

func Convert_v1beta2_SubnetSpec_To_v1beta1_SubnetSpec(in *infrav2.SubnetSpec, out *SubnetSpec, s apiconversion.Scope) error {
return autoConvert_v1beta2_SubnetSpec_To_v1beta1_SubnetSpec(in, out, s)
}
40 changes: 28 additions & 12 deletions api/v1beta1/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

34 changes: 28 additions & 6 deletions api/v1beta2/network_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,8 +348,20 @@ func (v *VPCSpec) IsIPv6Enabled() bool {
// SubnetSpec configures an AWS Subnet.
type SubnetSpec struct {
// ID defines a unique identifier to reference this resource.
// If you're bringing your subnet, set the AWS subnet-id here, it must start with `subnet-`.
vincepri marked this conversation as resolved.
Show resolved Hide resolved
//
// When the VPC is managed by CAPA, and you'd like the provider to create a subnet for you,
// the id can be set to any placeholder value that does not start with `subnet-`;
// upon creation, the subnet AWS identifier will be populated in the `ResourceID` field and
// the `id` field is going to be used as the subnet name. If you specify a tag
// called `Name`, it takes precedence.
ID string `json:"id"`

// ResourceID is the subnet identifier from AWS, READ ONLY.
// This field is populated when the provider manages the subnet.
Comment on lines +360 to +361
Copy link

Choose a reason for hiding this comment

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

How is the read only nature of this field enforced?

Copy link
Member

Choose a reason for hiding this comment

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

Making it immutable once its non-empty would be a first step. Everything else probably gets hacky pretty quickly

Copy link

Choose a reason for hiding this comment

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

Should we add CEL then to make it immutable?

Suggested change
// ResourceID is the subnet identifier from AWS, READ ONLY.
// This field is populated when the provider manages the subnet.
// ResourceID is the subnet identifier from AWS, READ ONLY.
// This field is populated when the provider manages the subnet.
// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="resourceID is immutable"

Copy link
Member

Choose a reason for hiding this comment

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

Do we want to handle this on this PR or as a follow up?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a strong opinion to enforce this today, the reconciler will overwrite it anyways

// +optional
ResourceID string `json:"resourceID,omitempty"`

// CidrBlock is the CIDR block to be used when the provider creates a managed VPC.
CidrBlock string `json:"cidrBlock,omitempty"`

Expand Down Expand Up @@ -384,9 +396,18 @@ type SubnetSpec struct {
Tags Tags `json:"tags,omitempty"`
}

// GetResourceID returns the identifier for this subnet,
// if the subnet was not created or reconciled, it returns the subnet ID.
func (s *SubnetSpec) GetResourceID() string {
if s.ResourceID != "" {
return s.ResourceID
}
return s.ID
}

// String returns a string representation of the subnet.
func (s *SubnetSpec) String() string {
return fmt.Sprintf("id=%s/az=%s/public=%v", s.ID, s.AvailabilityZone, s.IsPublic)
return fmt.Sprintf("id=%s/az=%s/public=%v", s.GetResourceID(), s.AvailabilityZone, s.IsPublic)
}

// Subnets is a slice of Subnet.
Expand All @@ -399,7 +420,7 @@ func (s Subnets) ToMap() map[string]*SubnetSpec {
res := make(map[string]*SubnetSpec)
for i := range s {
x := s[i]
res[x.ID] = &x
res[x.GetResourceID()] = &x
}
return res
}
Expand All @@ -408,19 +429,18 @@ func (s Subnets) ToMap() map[string]*SubnetSpec {
func (s Subnets) IDs() []string {
res := []string{}
for _, subnet := range s {
res = append(res, subnet.ID)
res = append(res, subnet.GetResourceID())
}
return res
}

// FindByID returns a single subnet matching the given id or nil.
func (s Subnets) FindByID(id string) *SubnetSpec {
for _, x := range s {
if x.ID == id {
if x.GetResourceID() == id {
return &x
}
}

return nil
}

Expand All @@ -429,7 +449,9 @@ func (s Subnets) FindByID(id string) *SubnetSpec {
// or if they are in the same vpc and the cidr block is the same.
func (s Subnets) FindEqual(spec *SubnetSpec) *SubnetSpec {
for _, x := range s {
if (spec.ID != "" && x.ID == spec.ID) || (spec.CidrBlock == x.CidrBlock) || (spec.IPv6CidrBlock != "" && spec.IPv6CidrBlock == x.IPv6CidrBlock) {
if (spec.GetResourceID() != "" && x.GetResourceID() == spec.GetResourceID()) ||
(spec.CidrBlock == x.CidrBlock) ||
(spec.IPv6CidrBlock != "" && spec.IPv6CidrBlock == x.IPv6CidrBlock) {
return &x
}
}
Expand Down
32 changes: 28 additions & 4 deletions config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,8 +481,15 @@ spec:
the provider creates a managed VPC.
type: string
id:
description: ID defines a unique identifier to reference
this resource.
description: "ID defines a unique identifier to reference
this resource. If you're bringing your subnet, set the
AWS subnet-id here, it must start with `subnet-`. \n When
the VPC is managed by CAPA, and you'd like the provider
to create a subnet for you, the id can be set to any placeholder
value that does not start with `subnet-`; upon creation,
the subnet AWS identifier will be populated in the `ResourceID`
field and the `id` field is going to be used as the subnet
name. If you specify a tag called `Name`, it takes precedence."
type: string
ipv6CidrBlock:
description: IPv6CidrBlock is the IPv6 CIDR block to be
Expand Down Expand Up @@ -510,6 +517,11 @@ spec:
to determine routes for private subnets in the same AZ
as the public subnet.
type: string
resourceID:
description: ResourceID is the subnet identifier from AWS,
READ ONLY. This field is populated when the provider manages
the subnet.
type: string
routeTableId:
description: RouteTableID is the routing table id associated
with the subnet.
Expand Down Expand Up @@ -2052,8 +2064,15 @@ spec:
the provider creates a managed VPC.
type: string
id:
description: ID defines a unique identifier to reference
this resource.
description: "ID defines a unique identifier to reference
this resource. If you're bringing your subnet, set the
AWS subnet-id here, it must start with `subnet-`. \n When
the VPC is managed by CAPA, and you'd like the provider
to create a subnet for you, the id can be set to any placeholder
value that does not start with `subnet-`; upon creation,
the subnet AWS identifier will be populated in the `ResourceID`
field and the `id` field is going to be used as the subnet
name. If you specify a tag called `Name`, it takes precedence."
type: string
ipv6CidrBlock:
description: IPv6CidrBlock is the IPv6 CIDR block to be
Expand Down Expand Up @@ -2081,6 +2100,11 @@ spec:
to determine routes for private subnets in the same AZ
as the public subnet.
type: string
resourceID:
description: ResourceID is the subnet identifier from AWS,
READ ONLY. This field is populated when the provider manages
the subnet.
type: string
routeTableId:
description: RouteTableID is the routing table id associated
with the subnet.
Expand Down
16 changes: 14 additions & 2 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1284,8 +1284,15 @@ spec:
the provider creates a managed VPC.
type: string
id:
description: ID defines a unique identifier to reference
this resource.
description: "ID defines a unique identifier to reference
this resource. If you're bringing your subnet, set the
AWS subnet-id here, it must start with `subnet-`. \n When
the VPC is managed by CAPA, and you'd like the provider
to create a subnet for you, the id can be set to any placeholder
value that does not start with `subnet-`; upon creation,
the subnet AWS identifier will be populated in the `ResourceID`
field and the `id` field is going to be used as the subnet
name. If you specify a tag called `Name`, it takes precedence."
type: string
ipv6CidrBlock:
description: IPv6CidrBlock is the IPv6 CIDR block to be
Expand Down Expand Up @@ -1313,6 +1320,11 @@ spec:
to determine routes for private subnets in the same AZ
as the public subnet.
type: string
resourceID:
description: ResourceID is the subnet identifier from AWS,
READ ONLY. This field is populated when the provider manages
the subnet.
type: string
routeTableId:
description: RouteTableID is the routing table id associated
with the subnet.
Expand Down
18 changes: 16 additions & 2 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -888,8 +888,17 @@ spec:
when the provider creates a managed VPC.
type: string
id:
description: ID defines a unique identifier to reference
this resource.
description: "ID defines a unique identifier to
reference this resource. If you're bringing your
subnet, set the AWS subnet-id here, it must start
with `subnet-`. \n When the VPC is managed by
CAPA, and you'd like the provider to create a
subnet for you, the id can be set to any placeholder
value that does not start with `subnet-`; upon
creation, the subnet AWS identifier will be populated
in the `ResourceID` field and the `id` field is
going to be used as the subnet name. If you specify
a tag called `Name`, it takes precedence."
type: string
ipv6CidrBlock:
description: IPv6CidrBlock is the IPv6 CIDR block
Expand Down Expand Up @@ -920,6 +929,11 @@ spec:
routes for private subnets in the same AZ as the
public subnet.
type: string
resourceID:
description: ResourceID is the subnet identifier
from AWS, READ ONLY. This field is populated when
the provider manages the subnet.
type: string
routeTableId:
description: RouteTableID is the routing table id
associated with the subnet.
Expand Down
Loading