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.

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

update bastion machine type to variable in AWS HA terraform #47297

Merged
merged 5 commits into from
Oct 8, 2024

Conversation

stevenGravy
Copy link
Contributor

@stevenGravy stevenGravy commented Oct 7, 2024

Sets to use a variable instance type instead of hard-coded. This currently causes problems if you specify a x86 AMI since that's not compatible with arm types.

changelog: Allow specifying the instance type of AWS HA Terraform bastion instance

Copy link
Contributor

@hugoShaka hugoShaka left a comment

Choose a reason for hiding this comment

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

So we broke the TF code by default for every x86 user 6 moths ago, but did not catch the issue until now because we default to ARM?

The new TF variable should be added to the Makefile and the README.

@hugoShaka
Copy link
Contributor

Also, this PR is changelog worthy.

@stevenGravy stevenGravy removed the no-changelog Indicates that a PR does not require a changelog entry label Oct 7, 2024
@stevenGravy
Copy link
Contributor Author

So we broke the TF code by default for every x86 user 6 moths ago, but did not catch the issue until now because we default to ARM?

The new TF variable should be added to the Makefile and the README.

We don't put the other instance types (auth, proxy,...) though in the Makefile and README. Should they all be there then?

@stevenGravy
Copy link
Contributor Author

Also, this PR is changelog worthy.

Thanks, changelog updated.

@hugoShaka
Copy link
Contributor

We don't put the other instance types (auth, proxy,...) though in the Makefile and README. Should they all be there then?

The Makefile and README should contain every variable and mention if it's required or if we'll pick the default for the user. It's very likely that we missed adding variables there in the past. If you find missing vars, please add them to the reference 🙏

// Instance type used for bastion server
variable "bastion_instance_type" {
type = string
default = "t4g.medium"
Copy link
Contributor

Choose a reason for hiding this comment

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

In the interest of having a more "friendly" experience for new users, maybe we could default this to null, then conditionally set the instance type deployed based off of the AMI architecture from data.aws_ami.base?

Copy link
Contributor

Choose a reason for hiding this comment

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

I really like this idea, but think it's probably better implemented in a future PR as it'll require a fair bit more testing.

@stevenGravy
Copy link
Contributor Author

We don't put the other instance types (auth, proxy,...) though in the Makefile and README. Should they all be there then?

The Makefile and README should contain every variable and mention if it's required or if we'll pick the default for the user. It's very likely that we missed adding variables there in the past. If you find missing vars, please add them to the reference 🙏

thanks @hugoShaka, please see update.

Copy link
Contributor

@hugoShaka hugoShaka left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd like @webvictim 's approval before merging.

@stevenGravy stevenGravy added this pull request to the merge queue Oct 8, 2024
@hugoShaka hugoShaka removed this pull request from the merge queue due to a manual request Oct 8, 2024
@stevenGravy stevenGravy requested a review from webvictim October 8, 2024 00:44
Copy link
Contributor

@webvictim webvictim left a comment

Choose a reason for hiding this comment

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

LGTM, just grammar/formatting nits

examples/aws/terraform/ha-autoscale-cluster/Makefile Outdated Show resolved Hide resolved
examples/aws/terraform/ha-autoscale-cluster/Makefile Outdated Show resolved Hide resolved
examples/aws/terraform/ha-autoscale-cluster/Makefile Outdated Show resolved Hide resolved
examples/aws/terraform/ha-autoscale-cluster/Makefile Outdated Show resolved Hide resolved
examples/aws/terraform/ha-autoscale-cluster/Makefile Outdated Show resolved Hide resolved
examples/aws/terraform/ha-autoscale-cluster/README.md Outdated Show resolved Hide resolved
examples/aws/terraform/ha-autoscale-cluster/README.md Outdated Show resolved Hide resolved
examples/aws/terraform/ha-autoscale-cluster/README.md Outdated Show resolved Hide resolved
examples/aws/terraform/ha-autoscale-cluster/README.md Outdated Show resolved Hide resolved
// Instance type used for bastion server
variable "bastion_instance_type" {
type = string
default = "t4g.medium"
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like this idea, but think it's probably better implemented in a future PR as it'll require a fair bit more testing.

@stevenGravy stevenGravy enabled auto-merge October 8, 2024 13:59
@stevenGravy stevenGravy added this pull request to the merge queue Oct 8, 2024
Merged via the queue into master with commit 47c22ee Oct 8, 2024
40 checks passed
@stevenGravy stevenGravy deleted the stevenGravy/awshaterraform branch October 8, 2024 14:27
@public-teleport-github-review-bot

@stevenGravy See the table below for backport results.

Branch Result
branch/v14 Failed
branch/v15 Create PR
branch/v16 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants