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

Make subnet spec id field required for SSA to work with CC #3748

Merged
merged 5 commits into from
Sep 27, 2022
Merged

Make subnet spec id field required for SSA to work with CC #3748

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
14109e7
Enable clusterctl upgrade test
sedefsavas Sep 15, 2022
1bc6ce8
Make subnet spec id field required for SSA to work with CC
sedefsavas Sep 22, 2022
55a78fa
Add external VPC CC test
sedefsavas Sep 25, 2022
342dd42
Fixes for patches
sedefsavas Sep 25, 2022
39f088b
Remove multi-az e2e test (it is no longer supported)
sedefsavas Sep 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion api/v1beta2/network_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ func (v *VPCSpec) IsIPv6Enabled() bool {
// SubnetSpec configures an AWS Subnet.
type SubnetSpec struct {
// ID defines a unique identifier to reference this resource.
ID string `json:"id,omitempty"`
ID string `json:"id"`
Copy link
Contributor

Choose a reason for hiding this comment

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

So, what happens in case of existing clusters is that their ID will be set to a default empty string, right?
Does that affect the cluster at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This means if a subnet spec is defined, it should have the id field.
Making id a unique identifier solves SSA coauthoring issue. More info in the CAPI issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kubernetes-sigs/cluster-api#6320

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I saw this one. Just was wondering about existing configs since now it will not just omit the value but update it to an empty string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For existing configs (by this I mean existing clusters), even though initially id field was empty, after the creation, ids are filled by CAPA controllers, so no existing cluster should have their id fields empty during upgrading to v1beta2.

If you are asking about using an existing template, they will no longer work for creating new clusters.
This is a breaking change for existing templates that use the unsupported use cases and the reason why we needed a new API version is because of this. We will need to document this properly with the v1beta2 release.


// CidrBlock is the CIDR block to be used when the provider creates a managed VPC.
CidrBlock string `json:"cidrBlock,omitempty"`
Expand Down Expand Up @@ -284,6 +284,8 @@ func (s *SubnetSpec) String() string {
}

// Subnets is a slice of Subnet.
// +listType=map
// +listMapKey=id
type Subnets []SubnetSpec

// ToMap returns a map from id to subnet.
Expand Down
10 changes: 10 additions & 0 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 @@ -446,8 +446,13 @@ spec:
description: Tags is a collection of tags describing the
resource.
type: object
required:
- id
type: object
type: array
x-kubernetes-list-map-keys:
- id
x-kubernetes-list-type: map
vpc:
description: VPC configuration.
properties:
Expand Down Expand Up @@ -1719,8 +1724,13 @@ spec:
description: Tags is a collection of tags describing the
resource.
type: object
required:
- id
type: object
type: array
x-kubernetes-list-map-keys:
- id
x-kubernetes-list-type: map
vpc:
description: VPC configuration.
properties:
Expand Down
5 changes: 5 additions & 0 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 @@ -1153,8 +1153,13 @@ spec:
description: Tags is a collection of tags describing the
resource.
type: object
required:
- id
type: object
type: array
x-kubernetes-list-map-keys:
- id
x-kubernetes-list-type: map
vpc:
description: VPC configuration.
properties:
Expand Down
5 changes: 5 additions & 0 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 @@ -757,8 +757,13 @@ spec:
description: Tags is a collection of tags describing
the resource.
type: object
required:
- id
type: object
type: array
x-kubernetes-list-map-keys:
- id
x-kubernetes-list-type: map
vpc:
description: VPC configuration.
properties:
Expand Down
6 changes: 3 additions & 3 deletions test/e2e/data/e2e_conf.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ providers:
- name: aws
type: InfrastructureProvider
versions:
- name: v1.5.0 # latest published release in the v1alpha4 series; this is used for v1beta1 --> v1beta2 clusterctl upgrades test only.
- name: v1.5.0 # latest published release in the v1beta1 series; this is used for v1beta1 --> v1beta2 clusterctl upgrades test only.
value: "https://github.com/kubernetes-sigs/cluster-api-provider-aws/releases/download/v1.5.0/infrastructure-components.yaml"
type: "url"
contract: v1beta1
Expand All @@ -94,7 +94,7 @@ providers:
- name: v1.6.99
# Use manifest from source files
value: ../../../config/default
contract: v1beta1
# Do not add contract field for v1beta1 --> v1beta2 clusterctl upgrades test to work.
files:
- sourcePath: "./infrastructure-aws/generated/cluster-template-efs-support.yaml"
- sourcePath: "./infrastructure-aws/generated/cluster-template-external-csi.yaml"
Expand All @@ -105,7 +105,6 @@ providers:
- sourcePath: "./infrastructure-aws/generated/cluster-template-limit-az.yaml"
- sourcePath: "./infrastructure-aws/generated/cluster-template-machine-pool.yaml"
- sourcePath: "./infrastructure-aws/generated/cluster-template-md-remediation.yaml"
- sourcePath: "./infrastructure-aws/generated/cluster-template-multi-az.yaml"
- sourcePath: "./infrastructure-aws/generated/cluster-template-nested-multitenancy.yaml"
- sourcePath: "./infrastructure-aws/generated/cluster-template-remote-management-cluster.yaml"
- sourcePath: "./infrastructure-aws/generated/cluster-template-simple-multitenancy.yaml"
Expand All @@ -122,6 +121,7 @@ providers:
- sourcePath: "./infrastructure-aws/generated/cluster-template-nested-multitenancy-clusterclass.yaml"
- sourcePath: "./infrastructure-aws/kustomize_sources/nested-multitenancy-clusterclass/clusterclass-multi-tenancy.yaml"
- sourcePath: "./infrastructure-aws/generated/cluster-template-self-hosted-clusterclass.yaml"
- sourcePath: "./infrastructure-aws/generated/cluster-template-external-vpc-clusterclass.yaml"
- sourcePath: "./shared/v1beta2_provider/metadata.yaml"
- sourcePath: "./infrastructure-aws/generated/cluster-template-ignition.yaml"
replacements:
Expand Down
28 changes: 28 additions & 0 deletions ...a/infrastructure-aws/kustomize_sources/external-vpc-clusterclass/byo-infra-variables.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
- op: add
Copy link
Contributor

@Skarlso Skarlso Sep 27, 2022 •
edited

Choose a reason for hiding this comment

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

So... (and this is probably my lack of knowledge in this area, but please bear with me) The way these different from currently configurable networking, like just plain adding your VPC ID into the cluster config, is that these values will now be managed fields and used in SSA. So the user doesn't have to change things in a config file but rather apply new patch values? Did I get that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this particular scenario tests BYO VPC and subnets which was not working before SSA and id field being compulsory.

path: /spec/topology/variables/-
value:
name: byoInfra
value: "true"
- op: add
path: /spec/topology/variables/-
value:
name: vpcID
value: "${BYO_VPC_ID}"
- op: add
path: /spec/topology/variables/-
value:
name: publicSubnetID
value: "${BYO_PUBLIC_SUBNET_ID}"
- op: add
path: /spec/topology/variables/-
value:
name: privateSubnetID
value: "${BYO_PRIVATE_SUBNET_ID}"
- op: add
path: /spec/topology/variables/-
value:
name: fdForBYOSubnets
value: "us-west-2a"
- op: replace
path: /spec/topology/workers/machineDeployments/0/failureDomain
value: "us-west-2a"
16 changes: 16 additions & 0 deletions ...2e/data/infrastructure-aws/kustomize_sources/external-vpc-clusterclass/kustomization.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
bases:
- ../topology/cluster-template.yaml

patches:
- path: ./byo-infra-variables.yaml
target:
group: cluster.x-k8s.io
version: v1beta1
kind: Cluster
- path: ./limited-az-variable.yaml
target:
group: cluster.x-k8s.io
version: v1beta1
kind: Cluster


5 changes: 5 additions & 0 deletions ...a/infrastructure-aws/kustomize_sources/external-vpc-clusterclass/limited-az-variable.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
- op: add
path: /spec/topology/variables/-
value:
name: vpcAZUsageLimit
value: "1"
4 changes: 0 additions & 4 deletions test/e2e/data/infrastructure-aws/kustomize_sources/multi-az/kustomization.yaml
View file
Open in desktop

This file was deleted.

18 changes: 0 additions & 18 deletions test/e2e/data/infrastructure-aws/kustomize_sources/multi-az/patches/multi-az.yaml
View file
Open in desktop

This file was deleted.

77 changes: 77 additions & 0 deletions test/e2e/data/infrastructure-aws/kustomize_sources/topology/clusterclass-ci-default.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,36 @@ spec:
type: string
default: ""
example: "1"
- name: vpcID
required: false
schema:
openAPIV3Schema:
type: string
default: ""
- name: publicSubnetID
required: false
schema:
openAPIV3Schema:
type: string
default: ""
- name: privateSubnetID
required: false
schema:
openAPIV3Schema:
type: string
default: ""
- name: fdForBYOSubnets
required: false
schema:
openAPIV3Schema:
type: string
default: ""
- name: byoInfra
required: false
schema:
openAPIV3Schema:
type: string
default: "false"
- name: selfHosted
required: false
schema:
Expand Down Expand Up @@ -214,6 +244,53 @@ spec:
path: "/spec/template/spec/network/vpc/availabilityZoneUsageLimit"
valueFrom:
template: "{{ .vpcAZUsageLimit }}"
- name: byoInfra
enabledIf: '{{ eq .byoInfra "true" }}'
definitions:
- selector:
apiVersion: infrastructure.cluster.x-k8s.io/v1beta2
kind: AWSClusterTemplate
matchResources:
infrastructureCluster: true
jsonPatches:
- op: add
path: "/spec/template/spec/network/vpc/id"
valueFrom:
variable: vpcID
- op: add
path: /spec/template/spec/network/subnets
valueFrom:
template: |
- id: "{{ .publicSubnetID }}"
- id: "{{ .privateSubnetID }}"
- name: awsMachineTemplateControlPlaneForBYO
enabledIf: '{{ eq .byoInfra "true" }}'
definitions:
- selector:
apiVersion: infrastructure.cluster.x-k8s.io/v1beta2
kind: AWSMachineTemplate
matchResources:
controlPlane: true
jsonPatches:
- op: add
path: "/spec/template/spec/failureDomain"
valueFrom:
variable: fdForBYOSubnets
- name: awsMachineTemplateWorkerForBYO
enabledIf: '{{ eq .byoInfra "true" }}'
definitions:
- selector:
apiVersion: infrastructure.cluster.x-k8s.io/v1beta2
kind: AWSMachineTemplate
matchResources:
machineDeploymentClass:
names:
- default-worker
jsonPatches:
- op: add
path: "/spec/template/spec/failureDomain"
valueFrom:
variable: fdForBYOSubnets
---
apiVersion: infrastructure.cluster.x-k8s.io/v1beta2
kind: AWSClusterTemplate
Expand Down
1 change: 0 additions & 1 deletion test/e2e/shared/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ const (
AwsNodeMachineType = "AWS_NODE_MACHINE_TYPE"
AwsAvailabilityZone1 = "AWS_AVAILABILITY_ZONE_1"
AwsAvailabilityZone2 = "AWS_AVAILABILITY_ZONE_2"
MultiAzFlavor = "multi-az"
LimitAzFlavor = "limit-az"
SpotInstancesFlavor = "spot-instances"
SSMFlavor = "ssm"
Expand Down
29 changes: 0 additions & 29 deletions test/e2e/suites/unmanaged/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,35 +388,6 @@ func getEvents(namespace string) *corev1.EventList {
return eventsList
}

func getSubnetID(filterKey, filterValue, clusterName string) *string {
var subnetOutput *ec2.DescribeSubnetsOutput
var err error

ec2Client := ec2.New(e2eCtx.AWSSession)
subnetInput := &ec2.DescribeSubnetsInput{
Filters: []*ec2.Filter{
{
Name: aws.String(filterKey),
Values: []*string{
aws.String(filterValue),
},
},
{
Name: aws.String("tag-key"),
Values: aws.StringSlice([]string{"sigs.k8s.io/cluster-api-provider-aws/cluster/" + clusterName}),
},
},
}

Eventually(func() int {
subnetOutput, err = ec2Client.DescribeSubnets(subnetInput)
Expect(err).NotTo(HaveOccurred())
return len(subnetOutput.Subnets)
}, e2eCtx.E2EConfig.GetIntervals("", "wait-infra-subnets")...).Should(Equal(1))

return subnetOutput.Subnets[0].SubnetId
}

func getVolumeIds(info statefulSetInfo, k8sclient crclient.Client) []*string {
ginkgo.By("Retrieving IDs of dynamically provisioned volumes.")
statefulset := &appsv1.StatefulSet{}
Expand Down
3 changes: 1 addition & 2 deletions test/e2e/suites/unmanaged/unmanaged_CAPI_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,7 @@ var _ = ginkgo.Context("[unmanaged] [Cluster API Framework]", func() {
})
})

// TODO: clusterctl init uses v1beta2, this needs to be fixed.
ginkgo.PDescribe("Clusterctl Upgrade Spec [from latest v1beta1 release to v1beta2]", func() {
ginkgo.Describe("Clusterctl Upgrade Spec [from latest v1beta1 release to v1beta2]", func() {
ginkgo.BeforeEach(func() {
// As the resources cannot be defined by the It() clause in CAPI tests, using the largest values required for all It() tests in this CAPI test.
requiredResources = &shared.TestResource{EC2Normal: 5 * e2eCtx.Settings.InstanceVCPU, IGW: 2, NGW: 2, VPC: 2, ClassicLB: 2, EIP: 2}
Expand Down