Skip to content

Simplify buildLaunchTemplateTask() part one #11452

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

Merged
merged 10 commits into from
May 11, 2021
Merged

Simplify buildLaunchTemplateTask() part one #11452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4657cb9
Refactor LaunchTemplate.AssociatePublicIP
johngmyers May 11, 2021
0557414
Refactor LaunchTemplate.BlockDeviceMappings
johngmyers May 11, 2021
33590eb
Refactor LaunchTemplate.CPUCredits
johngmyers May 11, 2021
98502cd
Refactor LaunchTemplate.HTTPPutResponseHopLimit
johngmyers May 11, 2021
07aa346
Refactor LaunchTemplate.HTTPTokens
johngmyers May 11, 2021
bfd8034
Refactor LaunchTemplate.IAMInstanceProfile
johngmyers May 11, 2021
d0793bd
Refactor LaunchTemplate.ImageID
johngmyers May 11, 2021
a1db8f1
Refactor LaunchTemplate.InstanceInterruptionBehavior
johngmyers May 11, 2021
d2adf49
Refactor LaunchTemplate.InstanceMonitoring
johngmyers May 11, 2021
a4898c9
Refactor LaunchTemplate.InstanceType
johngmyers May 11, 2021
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
199 changes: 94 additions & 105 deletions pkg/model/awsmodel/autoscalinggroup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,33 +135,107 @@ func (b *AutoscalingGroupModelBuilder) buildLaunchTemplateTask(c *fi.ModelBuilde
return nil, err
}

// @step: add the iam instance profile
link, err := b.LinkToIAMInstanceProfile(ig)
if err != nil {
return nil, fmt.Errorf("unable to find IAM profile link for instance group %q: %w", ig.ObjectMeta.Name, err)
}

tags, err := b.CloudTagsForInstanceGroup(ig)
if err != nil {
return nil, fmt.Errorf("error building cloud tags: %v", err)
}

lt := &awstasks.LaunchTemplate{
Name: fi.String(name),
Lifecycle: b.Lifecycle,
AssociatePublicIP: lc.AssociatePublicIP,
BlockDeviceMappings: lc.BlockDeviceMappings,
IAMInstanceProfile: lc.IAMInstanceProfile,
ImageID: lc.ImageID,
InstanceMonitoring: lc.InstanceMonitoring,
InstanceType: lc.InstanceType,
RootVolumeOptimization: lc.RootVolumeOptimization,
RootVolumeSize: lc.RootVolumeSize,
RootVolumeIops: lc.RootVolumeIops,
RootVolumeType: lc.RootVolumeType,
RootVolumeEncryption: lc.RootVolumeEncryption,
SSHKey: lc.SSHKey,
SecurityGroups: lc.SecurityGroups,
Tags: tags,
Tenancy: lc.Tenancy,
UserData: lc.UserData,
HTTPTokens: lc.HTTPTokens,
HTTPPutResponseHopLimit: lc.HTTPPutResponseHopLimit,
Name: fi.String(name),
Lifecycle: b.Lifecycle,
CPUCredits: fi.String(fi.StringValue(ig.Spec.CPUCredits)),
Copy link
Member

Choose a reason for hiding this comment

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

Could we keep this similar to others, without String(StringValue())?

Suggested change
CPUCredits: fi.String(fi.StringValue(ig.Spec.CPUCredits)),
CPUCredits: fi.String(""),

Copy link
Member Author

@johngmyers johngmyers May 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The previous code turns nil into *"". String(StringValue()) is the simplest way to do that. The suggested replacement ignores the value of ig.Spec.CPUCredits.

Copy link
Member

Choose a reason for hiding this comment

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

Meant to add below something like, which may be a little easier to read:

if ig.Spec.CPUCredits != nil {
 	lt.CPUCredits = ig.Spec.CPUCredits
}

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, it's just a nit, if you like it more this way, I am ok with that too.

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 find splitting the logic into two places and coding it with a verbose if statement to be harder to read, but then I find a lot of idiomatic Go to be annoyingly verbose.

HTTPPutResponseHopLimit: fi.Int64(1),
HTTPTokens: fi.String(ec2.LaunchTemplateHttpTokensStateOptional),
IAMInstanceProfile: link,
ImageID: fi.String(ig.Spec.Image),
InstanceInterruptionBehavior: ig.Spec.InstanceInterruptionBehavior,
InstanceMonitoring: ig.Spec.DetailedInstanceMonitoring,
InstanceType: fi.String(strings.Split(ig.Spec.MachineType, ",")[0]),
RootVolumeOptimization: lc.RootVolumeOptimization,
RootVolumeSize: lc.RootVolumeSize,
RootVolumeIops: lc.RootVolumeIops,
RootVolumeType: lc.RootVolumeType,
RootVolumeEncryption: lc.RootVolumeEncryption,
SSHKey: lc.SSHKey,
SecurityGroups: lc.SecurityGroups,
Tags: tags,
Tenancy: lc.Tenancy,
UserData: lc.UserData,
}

{
// @step: check the subnets are ok and pull together an array for us
subnets, err := b.GatherSubnets(ig)
if err != nil {
return nil, err
}

// @step: check if we can add an public ip to this subnet
switch subnets[0].Type {
case kops.SubnetTypePublic, kops.SubnetTypeUtility:
lt.AssociatePublicIP = fi.Bool(true)
if ig.Spec.AssociatePublicIP != nil {
lt.AssociatePublicIP = ig.Spec.AssociatePublicIP
}
case kops.SubnetTypePrivate:
lt.AssociatePublicIP = fi.Bool(false)
}
}

// @step: add any additional block devices
for i := range ig.Spec.Volumes {
x := &ig.Spec.Volumes[i]
if x.Type == "" {
x.Type = DefaultVolumeType
}
if x.Type == ec2.VolumeTypeIo1 || x.Type == ec2.VolumeTypeIo2 {
if x.Iops == nil {
x.Iops = fi.Int64(DefaultVolumeIonIops)
}
} else if x.Type == ec2.VolumeTypeGp3 {
if x.Iops == nil {
x.Iops = fi.Int64(DefaultVolumeGp3Iops)
}
if x.Throughput == nil {
x.Throughput = fi.Int64(DefaultVolumeGp3Throughput)
}
} else {
x.Iops = nil
}
deleteOnTermination := DefaultVolumeDeleteOnTermination
if x.DeleteOnTermination != nil {
deleteOnTermination = fi.BoolValue(x.DeleteOnTermination)
}
encryption := DefaultVolumeEncryption
if x.Encrypted != nil {
encryption = fi.BoolValue(x.Encrypted)
}
lt.BlockDeviceMappings = append(lt.BlockDeviceMappings, &awstasks.BlockDeviceMapping{
DeviceName: fi.String(x.Device),
EbsDeleteOnTermination: fi.Bool(deleteOnTermination),
EbsEncrypted: fi.Bool(encryption),
EbsKmsKey: x.Key,
EbsVolumeIops: x.Iops,
EbsVolumeSize: fi.Int64(x.Size),
EbsVolumeThroughput: x.Throughput,
EbsVolumeType: fi.String(x.Type),
})
}

if ig.Spec.InstanceMetadata != nil && ig.Spec.InstanceMetadata.HTTPPutResponseHopLimit != nil {
lt.HTTPPutResponseHopLimit = ig.Spec.InstanceMetadata.HTTPPutResponseHopLimit
}

if ig.Spec.InstanceMetadata != nil && ig.Spec.InstanceMetadata.HTTPTokens != nil {
lt.HTTPTokens = ig.Spec.InstanceMetadata.HTTPTokens
}

// When using a MixedInstances ASG, AWS requires the SpotPrice be defined on the ASG
// rather than the LaunchTemplate or else it returns this error:
// You cannot use a launch template that is set to request Spot Instances (InstanceMarketOptions)
Expand All @@ -174,9 +248,6 @@ func (b *AutoscalingGroupModelBuilder) buildLaunchTemplateTask(c *fi.ModelBuilde
if ig.Spec.SpotDurationInMinutes != nil {
lt.SpotDurationInMinutes = ig.Spec.SpotDurationInMinutes
}
if ig.Spec.InstanceInterruptionBehavior != nil {
lt.InstanceInterruptionBehavior = ig.Spec.InstanceInterruptionBehavior
}
if fi.BoolValue(ig.Spec.RootVolumeEncryption) && ig.Spec.RootVolumeEncryptionKey != nil {
lt.RootVolumeKmsKey = ig.Spec.RootVolumeEncryptionKey
} else {
Expand All @@ -198,12 +269,6 @@ func (b *AutoscalingGroupModelBuilder) buildLaunchTemplateTask(c *fi.ModelBuilde
}
}

if ig.Spec.CPUCredits != nil {
lt.CPUCredits = ig.Spec.CPUCredits
} else {
lt.CPUCredits = fi.String("")
}

return lt, nil
}

Expand Down Expand Up @@ -239,35 +304,16 @@ func (b *AutoscalingGroupModelBuilder) buildLaunchTemplateHelper(c *fi.ModelBuil
}
}

// @step: add the iam instance profile
link, err := b.LinkToIAMInstanceProfile(ig)
if err != nil {
return nil, fmt.Errorf("unable to find IAM profile link for instance group %q: %w", ig.ObjectMeta.Name, err)
}

t := &awstasks.LaunchTemplate{
Name: fi.String(name),
Lifecycle: b.Lifecycle,
IAMInstanceProfile: link,
ImageID: fi.String(ig.Spec.Image),
InstanceMonitoring: ig.Spec.DetailedInstanceMonitoring,
InstanceType: fi.String(strings.Split(ig.Spec.MachineType, ",")[0]),
RootVolumeOptimization: ig.Spec.RootVolumeOptimization,
RootVolumeSize: fi.Int64(int64(volumeSize)),
RootVolumeType: fi.String(volumeType),
RootVolumeEncryption: fi.Bool(rootVolumeEncryption),
SecurityGroups: []*awstasks.SecurityGroup{sgLink},
}

t.HTTPTokens = fi.String(ec2.LaunchTemplateHttpTokensStateOptional)
if ig.Spec.InstanceMetadata != nil && ig.Spec.InstanceMetadata.HTTPTokens != nil {
t.HTTPTokens = ig.Spec.InstanceMetadata.HTTPTokens
}
t.HTTPPutResponseHopLimit = fi.Int64(1)
if ig.Spec.InstanceMetadata != nil && ig.Spec.InstanceMetadata.HTTPPutResponseHopLimit != nil {
t.HTTPPutResponseHopLimit = ig.Spec.InstanceMetadata.HTTPPutResponseHopLimit
}

if ig.HasAPIServer() &&
b.APILoadBalancerClass() == kops.LoadBalancerClassNetwork {
for _, id := range b.Cluster.Spec.API.LoadBalancer.AdditionalSecurityGroups {
Expand Down Expand Up @@ -310,46 +356,6 @@ func (b *AutoscalingGroupModelBuilder) buildLaunchTemplateHelper(c *fi.ModelBuil
t.SecurityGroups = append(t.SecurityGroups, sgTask)
}

// @step: add any additional block devices to the launch configuration
for i := range ig.Spec.Volumes {
x := &ig.Spec.Volumes[i]
if x.Type == "" {
x.Type = DefaultVolumeType
}
if x.Type == ec2.VolumeTypeIo1 || x.Type == ec2.VolumeTypeIo2 {
if x.Iops == nil {
x.Iops = fi.Int64(DefaultVolumeIonIops)
}
} else if x.Type == ec2.VolumeTypeGp3 {
if x.Iops == nil {
x.Iops = fi.Int64(DefaultVolumeGp3Iops)
}
if x.Throughput == nil {
x.Throughput = fi.Int64(DefaultVolumeGp3Throughput)
}
} else {
x.Iops = nil
}
deleteOnTermination := DefaultVolumeDeleteOnTermination
if x.DeleteOnTermination != nil {
deleteOnTermination = fi.BoolValue(x.DeleteOnTermination)
}
encryption := DefaultVolumeEncryption
if x.Encrypted != nil {
encryption = fi.BoolValue(x.Encrypted)
}
t.BlockDeviceMappings = append(t.BlockDeviceMappings, &awstasks.BlockDeviceMapping{
DeviceName: fi.String(x.Device),
EbsDeleteOnTermination: fi.Bool(deleteOnTermination),
EbsEncrypted: fi.Bool(encryption),
EbsKmsKey: x.Key,
EbsVolumeIops: x.Iops,
EbsVolumeSize: fi.Int64(x.Size),
EbsVolumeThroughput: x.Throughput,
EbsVolumeType: fi.String(x.Type),
})
}

if b.AWSModelContext.UseSSHKey() {
if t.SSHKey, err = b.LinkToSSHKey(); err != nil {
return nil, err
Expand All @@ -361,23 +367,6 @@ func (b *AutoscalingGroupModelBuilder) buildLaunchTemplateHelper(c *fi.ModelBuil
return nil, err
}

// @step: check the subnets are ok and pull together an array for us
subnets, err := b.GatherSubnets(ig)
if err != nil {
return nil, err
}

// @step: check if we can add an public ip to this subnet
switch subnets[0].Type {
case kops.SubnetTypePublic, kops.SubnetTypeUtility:
t.AssociatePublicIP = fi.Bool(true)
if ig.Spec.AssociatePublicIP != nil {
t.AssociatePublicIP = ig.Spec.AssociatePublicIP
}
case kops.SubnetTypePrivate:
t.AssociatePublicIP = fi.Bool(false)
}

return t, nil
}

Expand Down
2 changes: 1 addition & 1 deletion upup/pkg/fi/cloudup/awstasks/launchtemplate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ type LaunchTemplate struct {
// Lifecycle is the resource lifecycle
Lifecycle *fi.Lifecycle

// AssociatePublicIP indicates if a public ip address is assigned to instabces
// AssociatePublicIP indicates if a public ip address is assigned to instances
AssociatePublicIP *bool
// BlockDeviceMappings is a block device mappings
BlockDeviceMappings []*BlockDeviceMapping
Expand Down