-
Notifications
You must be signed in to change notification settings - Fork 1.5k
SPLAT-2172: AWS dedicate host support #10079
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
5e41e6e
e530a67
6f20dfe
dbd42b6
e6282c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,6 +44,10 @@ func PreTerraform(ctx context.Context, clusterID string, installConfig *installc | |
| return err | ||
| } | ||
|
|
||
| if err := tagSharedDedicatedHosts(ctx, clusterID, installConfig); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := tagSharedIAMRoles(ctx, clusterID, installConfig); err != nil { | ||
| return err | ||
| } | ||
|
|
@@ -244,6 +248,50 @@ func tagSharedIAMProfiles(ctx context.Context, clusterID string, installConfig * | |
| return nil | ||
| } | ||
|
|
||
| // tagSharedDedicatedHosts tags users BYO dedicated hosts so they are not destroyed by the Installer. | ||
| func tagSharedDedicatedHosts(ctx context.Context, clusterID string, installConfig *installconfig.InstallConfig) error { | ||
| var dhNames []string | ||
|
|
||
| // DH is only allowed to be created in "worker" pool at this time. No need to check default platform. | ||
| for _, compute := range installConfig.Config.Compute { | ||
| if compute.Name == "worker" { | ||
| mpool := awstypes.MachinePool{} | ||
| mpool.Set(installConfig.Config.AWS.DefaultMachinePlatform) | ||
| mpool.Set(compute.Platform.AWS) | ||
| if mpool.HostPlacement != nil && mpool.HostPlacement.DedicatedHost != nil { | ||
| for _, name := range mpool.HostPlacement.DedicatedHost { | ||
| dhNames = append(dhNames, name.ID) | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+255
to
+266
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tag shared dedicated hosts on every supported compute pool. This collector still hard-codes Suggested fix- // DH is only allowed to be created in "worker" pool at this time. No need to check default platform.
for _, compute := range installConfig.Config.Compute {
- if compute.Name == "worker" {
- mpool := awstypes.MachinePool{}
- mpool.Set(installConfig.Config.AWS.DefaultMachinePlatform)
- mpool.Set(compute.Platform.AWS)
- if mpool.HostPlacement != nil && mpool.HostPlacement.DedicatedHost != nil {
- for _, name := range mpool.HostPlacement.DedicatedHost {
- dhNames = append(dhNames, name.ID)
- }
- }
- }
+ if compute.Name == types.MachinePoolEdgeRoleName {
+ continue
+ }
+ mpool := awstypes.MachinePool{}
+ mpool.Set(installConfig.Config.AWS.DefaultMachinePlatform)
+ mpool.Set(compute.Platform.AWS)
+ if mpool.HostPlacement != nil {
+ for _, host := range mpool.HostPlacement.DedicatedHost {
+ dhNames = append(dhNames, host.ID)
+ }
+ }
}🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| if len(dhNames) == 0 { | ||
| return nil | ||
vr4manta marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| logrus.Debugf("Tagging shared dedicated hosts: %v", dhNames) | ||
|
|
||
| tagKey, tagValue := sharedTag(clusterID) | ||
|
|
||
| ec2Client, err := awsconfig.NewEC2Client(ctx, awsconfig.EndpointOptions{ | ||
| Region: installConfig.Config.Platform.AWS.Region, | ||
| Endpoints: installConfig.Config.Platform.AWS.ServiceEndpoints, | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create EC2 client: %w", err) | ||
| } | ||
|
|
||
| if _, err = ec2Client.CreateTags(ctx, &ec2.CreateTagsInput{ | ||
| Resources: dhNames, | ||
| Tags: []ec2types.Tag{{Key: &tagKey, Value: &tagValue}}, | ||
| }); err != nil { | ||
| return errors.Wrap(err, "could not add tags to dedicated host") | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func sharedTag(clusterID string) (string, string) { | ||
| return fmt.Sprintf("kubernetes.io/cluster/%s", clusterID), "shared" | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| package aws | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
|
|
||
| "github.com/aws/aws-sdk-go-v2/aws" | ||
| "github.com/aws/aws-sdk-go-v2/service/ec2" | ||
| "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| // Host holds metadata for a dedicated host. | ||
| type Host struct { | ||
| // ID the ID of the host. | ||
| ID string | ||
| // Zone the zone the host belongs to. | ||
| Zone string | ||
| // Tags is the map of the Host's tags. | ||
| Tags Tags | ||
| } | ||
|
|
||
| // dedicatedHosts retrieves a list of dedicated hosts and returns them in a map keyed by the host ID. | ||
| func dedicatedHosts(ctx context.Context, client *ec2.Client, hosts []string) (map[string]Host, error) { | ||
| hostsByID := map[string]Host{} | ||
|
|
||
| input := &ec2.DescribeHostsInput{} | ||
| if len(hosts) > 0 { | ||
| input.HostIds = hosts | ||
| } | ||
|
|
||
| paginator := ec2.NewDescribeHostsPaginator(client, input) | ||
| for paginator.HasMorePages() { | ||
| page, err := paginator.NextPage(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("fetching dedicated hosts: %w", err) | ||
| } | ||
|
|
||
| for _, host := range page.Hosts { | ||
| id := aws.ToString(host.HostId) | ||
| if id == "" { | ||
| // Skip entries lacking an ID (should not happen) | ||
| continue | ||
| } | ||
|
|
||
| logrus.Debugf("Found dedicated host: %s", id) | ||
| hostsByID[id] = Host{ | ||
| ID: id, | ||
| Zone: aws.ToString(host.AvailabilityZone), | ||
| Tags: FromAWSTags(host.Tags), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return hostsByID, nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🌐 Web query:
In Kubernetes CRDx-kubernetes-validations, doeshas(self.someList)evaluate to true when the list field is present but empty ([]), and isminItems: 1the recommended schema-level way to require at least one list element?💡 Result:
Yes. In CRD CEL,
has(self.someList)is a presence test (for JSON-like objects: “is this field/key defined?”), not a “non-empty” test—so ifsomeListis present in the object as[],has(self.someList)evaluates to true. This aligns with CEL’s intent of unifying presence checks across protobuf and JSON-like types. [1][2]has(self.someList) && self.someList.size() > 0.Yes. To require at least one list element at the schema level,
minItems: 1is the standard / recommended OpenAPI-schema validation for arrays (preferred over CEL when an OpenAPI validation exists). [3][4]Sources: Kubernetes CEL docs [2], cel-go README (has macro semantics) [1], Kubernetes CRD validation best practices (prefer OpenAPI validations) [3], controller-tools / OpenAPI marker for
MinItems=1[4].Enforce non-empty
dedicatedHostarray whenaffinity: DedicatedHost.The
has(self.dedicatedHost)check only validates field presence, so a config withaffinity: DedicatedHostanddedicatedHost: []passes validation despite being unusable. AddminItems: 1to the array definition to reject empty lists at the schema boundary.Suggested fix
dedicatedHost: + minItems: 1 description: |-Apply the same change at lines 1812–1861, 3362–3411, and 5112–5161.
📝 Committable suggestion
🤖 Prompt for AI Agents