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

fix(ec2): ARM-backed bastion hosts try to run x86-based Amazon Linux AMI #12280

Merged
merged 16 commits into from
Jan 27, 2021
Merged

fix(ec2): ARM-backed bastion hosts try to run x86-based Amazon Linux AMI #12280

merged 16 commits into from
Jan 27, 2021

Conversation

Jacob-Doetsch
Copy link
Contributor

@Jacob-Doetsch Jacob-Doetsch commented Dec 30, 2020 •
edited
Loading

Fixes #12279

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Dec 30, 2020 •
edited
Loading

@github-actions github-actions bot added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Dec 30, 2020
@Jacob-Doetsch Jacob-Doetsch mentioned this pull request Dec 30, 2020
Closed
@Jacob-Doetsch Jacob-Doetsch force-pushed the jacob-doetsch/bastionhostlinux-arm-support branch from 944f3f7 to 56d87a3 Compare December 30, 2020 07:31
Jacob-Doetsch
Jacob-Doetsch commented Dec 30, 2020
View reviewed changes
packages/@aws-cdk/aws-ec2/lib/instance-types.ts Outdated
/**
* The instance's CPU architecture
*/
public readonly architecture: InstanceArchitecture;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't sure if it would have been better to forgo a new property and instead just make the resolveArchitecture method public. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the current public name. The best of both worlds would be to make resolveArchitecture a getter :)

public get architecture(): InstanceArchitecture {
  // ...body of resolveArchitecture here...
}

@Jacob-Doetsch Jacob-Doetsch force-pushed the jacob-doetsch/bastionhostlinux-arm-support branch 2 times, most recently from 0a5ea83 to f42eae9 Compare January 3, 2021 23:53
@Jacob-Doetsch
fix(ec2): fix misconfiguration of arm-backed bastion host instances (a…
1e16596 
…ws#12279)

Adds support for ARM-based instance types used with BastionHostLinux: identifies ARM-based instance types and uses the architecutre-appropriate Amazon Linux 2 AMI and SSM Agent package.

Fixes aws#12279
@Jacob-Doetsch Jacob-Doetsch force-pushed the jacob-doetsch/bastionhostlinux-arm-support branch from f42eae9 to 1e16596 Compare January 4, 2021 06:12
rix0rrr
rix0rrr previously requested changes Jan 6, 2021
View reviewed changes
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

I like what you're doing. Some small comments.

Something that occurred to me: don't recent Amazon Linux AMIs come with the SSM Agent preinstalled? Why do we even need to yum install it at all?

packages/@aws-cdk/aws-ec2/lib/bastion-host.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ec2/lib/bastion-host.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ec2/lib/instance-types.ts Outdated
/**
* The instance's CPU architecture
*/
public readonly architecture: InstanceArchitecture;
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the current public name. The best of both worlds would be to make resolveArchitecture a getter :)

public get architecture(): InstanceArchitecture {
  // ...body of resolveArchitecture here...
}

packages/@aws-cdk/aws-ec2/lib/instance-types.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed rix0rrr’s stale review January 7, 2021 08:20

Pull request has been modified.

@Jacob-Doetsch Jacob-Doetsch force-pushed the jacob-doetsch/bastionhostlinux-arm-support branch from afae5fb to f725a33 Compare January 7, 2021 08:23
@Jacob-Doetsch
Copy link
Contributor Author

@rix0rrr You're right, there is no reason to manually install the SSM Agent in this case since it is preinstalled on Amazon Linux 2. I have removed the UserData stuff.

@Jacob-Doetsch Jacob-Doetsch force-pushed the jacob-doetsch/bastionhostlinux-arm-support branch from a4aab0f to 5bcc8d6 Compare January 8, 2021 06:35
rix0rrr
rix0rrr previously requested changes Jan 11, 2021
View reviewed changes
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

Almost there!

packages/@aws-cdk/aws-ec2/lib/instance-types.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed rix0rrr’s stale review January 11, 2021 17:09

Pull request has been modified.

@Jacob-Doetsch
Copy link
Contributor Author

@rix0rrr Let me know if I have addressed all your concerns. Thanks!

@rix0rrr rix0rrr changed the title fix(ec2): fix misconfiguration of arm-backed bastion host instances fix(ec2): ARM-backed bastion hosts try to run x86-based Amazon Linux AMI Jan 27, 2021
@mergify
Copy link
Contributor

mergify bot commented Jan 27, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: c433674
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Jan 27, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 1a73d76 into aws:master Jan 27, 2021
@keithduncan keithduncan mentioned this pull request Dec 3, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hoegertn hoegertn hoegertn left review comments

@rix0rrr rix0rrr rix0rrr approved these changes

Assignees

@rix0rrr rix0rrr

Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

(ec2): using ARM-based instances in BastionHostLinux results in an invalid instance configuration
4 participants
@Jacob-Doetsch @aws-cdk-automation @rix0rrr @hoegertn