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

New resource: aws_lightsail_instance_public_ports #8611

Merged
merged 10 commits into from
Mar 25, 2021

Conversation

mavericknsk
Copy link

@mavericknsk mavericknsk commented May 13, 2019

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Fixes #700
Relates #14905

Release note for CHANGELOG:

aws/resource_aws_lightsail_public_ports

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'

...

@ghost ghost added size/M Managed by automation to categorize the size of a PR. provider Pertains to the provider itself, rather than any interaction with AWS. service/lightsail Issues and PRs that pertain to the lightsail service. labels May 13, 2019
@mavericknsk mavericknsk force-pushed the f-aws_lightsail_open_ports branch 2 times, most recently from 0e801fb to 87c6240 Compare May 13, 2019 03:18
@aeschright aeschright requested a review from a team June 26, 2019 16:53
@aeschright aeschright added new-resource Introduces a new resource. enhancement Requests to existing resources that expand the functionality or scope. labels Aug 19, 2019
@a14m
Copy link

a14m commented Sep 9, 2019

any updates on this?

@snoopdouglas
Copy link

Updates please, having all instances stuck only allowing ports 22+80 without external intervention is a royal pain.

@eskimo220
Copy link

I think this is very useful. I hope to be noticed by more people

@FreedomBen
Copy link
Contributor

Will this be merged soon? I'm currently bound to lightsail, and manually configuring the ports has led to many bugs and broken things. Would really like to have this merged. I'm a little disappointed that it's been open for so long.

@joelradon
Copy link

Please merge :)

This is a must have to use lightsail with terraform.

@missinglink
Copy link

I just lost 2 hours debugging SSL errors only to find out that lightsail has a hidden firewall tab in the UI 😡

Screenshot 2020-02-11 at 16 09 15

@missinglink
Copy link

missinglink commented Feb 11, 2020

Also worth noting that right now, any time an OS blueprint instance is recreated it will revert to the default ports of 22 + 80, so it's not really possible to have SSL termination.

@missinglink
Copy link

Hi @aeschright, this issue has 41x thumbs-ups and the code looks ready to go?
Could you please let us know what more is required before this can be merged?

@joelradon
Copy link

Also worth noting that right now, any time the instance is recreated it will revert to the default ports of 22 + 80, so it's not really possible to have SSL termination.

Yes we use terraform because of it's automation capabilities. You cannot perform complete automation on lightsail instances unless you are ONLY using port 80. Since even basic websites use 443, we cannot automate certificate portion of our scripts. We must first go into GUI and open up 443.

It sure doesn't feel like the terraform way ;)

@missinglink
Copy link

missinglink commented Feb 11, 2020

Agreed, I don't think anyone is against merging this, it's just a question of what's left to do.

FYI there is an additional issue to consider when using letsencrypt.. if your application is requesting a cert and storing it on disk, the cert file will be lost during a reboot.

This can be resolved by creating a small block storage volume and keeping the cert files there across reboots, unfortunately this is also not currently supported by terraform-provider-aws.

@joelradon
Copy link

Agreed, I don't think anyone is against merging this, it's just a question of what's left to do.

FYI there is an additional issue to consider when using letsencrypt.. if your application is requesting a cert and storing it on disk, the cert file will be lost during a reboot.

This can be resolved by creating a small block storage volume and keeping the cert files there across reboots, unfortunately this is also not currently supported by terraform-provider-aws.

Good point on the storage. That could also be handling by backing up the certs to S3 via schedule of weekly, and performing S3 sync on boot.

I do like the way you are handling it better.

@missinglink
Copy link

For some unknown reason the Default public network ports open for specific instance images section of the documentation says that 443 is open by default for all application stacks but not for base operating systems. 🤷‍♂

@snoopdouglas
Copy link

@missinglink Of course, because who'd want to do any kind of SSL termination without running WordPress?

@missinglink
Copy link

Anyhoo, regardless of the obstacles, I'm available to help out, I have a desire and motivation to improve the Lightsail support within Terraform.

@missinglink
Copy link

I was able to get this to compile under go version go1.13.5 darwin/amd64 with the following changes:

# pull in latest changes from 'terraform-providers/terraform-provider-aws'
git rebase origin/master
diff --git a/aws/resource_aws_lightsail_public_ports.go b/aws/resource_aws_lightsail_public_ports.go
index 752eff78e..7960dad6b 100644
--- a/aws/resource_aws_lightsail_public_ports.go
+++ b/aws/resource_aws_lightsail_public_ports.go
@@ -6,8 +6,8 @@ import (
        "github.com/aws/aws-sdk-go/aws"
        "github.com/aws/aws-sdk-go/aws/awserr"
        "github.com/aws/aws-sdk-go/service/lightsail"
-       "github.com/hashicorp/terraform/helper/schema"
-       "github.com/hashicorp/terraform/helper/validation"
+       "github.com/hashicorp/terraform-plugin-sdk/helper/schema"
+       "github.com/hashicorp/terraform-plugin-sdk/helper/validation"
 )

@missinglink
Copy link

worked perfectly with the following config:

resource "aws_lightsail_public_ports" "lightsail_public_ports" {
  instance_name     = "lightsail_instance_name"
  port_infos {
    protocol    = "tcp"
    from_port   = 22
    to_port     = 22
  }
  port_infos {
    protocol    = "tcp"
    from_port   = 80
    to_port     = 80
  }
  port_infos {
    protocol    = "tcp"
    from_port   = 443
    to_port     = 443
  }
}

@missinglink
Copy link

missinglink commented Feb 12, 2020

IMO the name port_infos should be in the singular form port_info as per https://www.terraform.io/docs/extend/schemas/schema-types.html#typeset or name it after the aws API and call it public_port?

@missinglink
Copy link

missinglink commented Feb 12, 2020

I tried changing the aws_lightsail_instance.user_data and doing a terraform apply:

-/+ aws_lightsail_instance.lightsail_instance_cdn (new resource required)
...
\napt-get update -y\napt-get upgrade -y\n" (forces new resource)
      username:               "ubuntu" => <computed>

  + aws_lightsail_public_ports.lightsail_public_ports
      id:                     <computed>
      instance_name:          "lightsail_instance_cdn"
      port_infos.#:           "3"
      port_infos.0.from_port: "22"
      port_infos.0.protocol:  "tcp"
      port_infos.0.to_port:   "22"
      port_infos.1.from_port: "80"
      port_infos.1.protocol:  "tcp"
      port_infos.1.to_port:   "80"
      port_infos.2.from_port: "443"
      port_infos.2.protocol:  "tcp"
      port_infos.2.to_port:   "443"

  + aws_lightsail_static_ip_attachment.lightsail_ip_assignment_cdn
      id:                     <computed>
      instance_name:          "${aws_lightsail_instance.lightsail_instance_cdn.id}"
      ip_address:             <computed>
      static_ip_name:         "lightsail_ip_cdn"


Plan: 3 to add, 0 to change, 1 to destroy.

However there seems to be a race condition where maybe the ports are assigned first?
The instance was recreated but the port mapping was still 22/80

I ran terraform apply again and it added the ports correctly on the second execution:

Terraform will perform the following actions:

  + aws_lightsail_public_ports.lightsail_public_ports
      id:                     <computed>
      instance_name:          "lightsail_instance_cdn"
      port_infos.#:           "3"
      port_infos.0.from_port: "22"
      port_infos.0.protocol:  "tcp"
      port_infos.0.to_port:   "22"
      port_infos.1.from_port: "80"
      port_infos.1.protocol:  "tcp"
      port_infos.1.to_port:   "80"
      port_infos.2.from_port: "443"
      port_infos.2.protocol:  "tcp"
      port_infos.2.to_port:   "443"


Plan: 1 to add, 0 to change, 0 to destroy.

@missinglink
Copy link

The following script can be used to compile the code for testing https://gist.github.com/missinglink/1cc17e40184ac82203d6dc26373969aa

# requires golang (Im using go version go1.13.5 darwin/amd64)
./custom_provider_aws.sh

This will generate the following file in .terraform.d

-rwxr-xr-x  1 peter  staff   198M Feb 12 16:39 ../.terraform.d/plugins/darwin_amd64/terraform-provider-aws_v9.00.0_x4

You can then reference that version from your configs:

provider "aws" {
  region = "us-east-1"
  version = "~> 9.00"
}

see: https://www.terraform.io/docs/extend/writing-custom-providers.html
see: https://www.terraform.io/docs/configuration/providers.html#third-party-plugins

@missinglink
Copy link

missinglink commented Feb 12, 2020

adding a depends_on condition to the aws_lightsail_public_ports resource seems to resolve the race condition:

depends_on        = ["aws_lightsail_instance.lightsail_instance_cdn"]

this is my first time looking at the terraform internals, anyone know of there is a way to define that dependency internally within the provider code?

@leacollaboro
Copy link

What's the latest with this? Thanks!

Base automatically changed from master to main January 23, 2021 00:55
@breathingdust breathingdust requested a review from a team as a code owner January 23, 2021 00:55
@alon-sabi-ai
Copy link

Is this going anywhere?

@YakDriver YakDriver self-assigned this Mar 24, 2021
@YakDriver
Copy link
Member

@mavericknsk Thank you for this PR! I will be looking at it in the near future. I may need to make some minor changes. Coordinate with me if you intend to push to this branch and make sure to pull any changes I've made. Please note that you will still receive all credit for the PR and your code. Thanks again for your interest!

@ghost ghost added size/XL Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. and removed size/M Managed by automation to categorize the size of a PR. labels Mar 24, 2021
@YakDriver YakDriver changed the title support lightsail open ports resource New resource: aws_lightsail_instance_public_ports Mar 24, 2021
Copy link
Member

@YakDriver YakDriver left a comment

Choose a reason for hiding this comment

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

Looks great! 🎉

Acceptance tests on commercial (us-west-2):

--- PASS: TestAccAWSLightsailInstancePublicPorts_multiple (46.45s)
--- PASS: TestAccAWSLightsailInstancePublicPorts_basic (54.95s)

Acceptance tests on GovCloud:

--- SKIP: TestAccAWSLightsailInstancePublicPorts_basic (1.29s)
--- SKIP: TestAccAWSLightsailInstancePublicPorts_multiple (1.29s)

@ghost
Copy link

ghost commented Mar 26, 2021

This has been released in version 3.34.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@ghost
Copy link

ghost commented Apr 24, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Apr 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. new-resource Introduces a new resource. provider Pertains to the provider itself, rather than any interaction with AWS. service/lightsail Issues and PRs that pertain to the lightsail service. size/XL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws_lightsail provider should support open port management