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

Specify to use security group id in aws instance. #5680

Merged
merged 2 commits into from
Sep 4, 2018

Conversation

uLan08
Copy link
Contributor

@uLan08 uLan08 commented Aug 26, 2018

I am a total beginner in AWS and Terraform and so as I was playing around I encountered an error when supplying the security_groups field under aws_instance. I initially used ["${aws_security_group.terraform.name}"] and this is the error

* aws_instance.example: Error launching source instance: InvalidGroup.NotFound: The security group 'terraform_example' does not exist in VPC 'vpc-id'

Changing it to ["${aws_security_group.terraform.id}"] fixed it.

The changes in this PR will specify in the docs to use the security group ID.

$ terraform -v
Terraform v0.11.7
+ provider.aws v1.31.0

@ghost ghost added the documentation Introduces or discusses updates to documentation. label Aug 26, 2018
@bflad
Copy link
Contributor

bflad commented Aug 27, 2018

@uLan08 did you intend to change the documentation for the aws_instance resource page instead of the aws_instance data source page? The first is intended for managing an EC2 instance and the second is intended for just reading information about an EC2 instance.

In the existing security_groups attribute documentation for the resource we currently note:

security_groups - (Optional, EC2-Classic and default VPC only) A list of security group names to associate with.

As far as I know, names are required with EC2-Classic, but it might be good to note IDs can be specified with the default VPC case if that is valid. Can you please update the other page with that instead? https://github.com/terraform-providers/terraform-provider-aws/blob/master/website/docs/r/instance.html.markdown

PS: I would also double check after creating your EC2 instance that running terraform plan does not want to instead have the security group IDs in vpc_security_group_ids. The EC2 API is pretty strange with its use of the two similar attributes.

@bflad bflad added service/ec2 Issues and PRs that pertain to the ec2 service. waiting-response Maintainers are waiting on response from community or contributor. labels Aug 27, 2018
@uLan08
Copy link
Contributor Author

uLan08 commented Aug 28, 2018

You're totally right. The aws_instance data source page was the one I was reading the whole time but it should have been the aws_instance resource page. I guess what brought me there is that when I initially searched for "instance" the one under data sources came up first.

Anyway, should I undo my previous commit? Or should I just create another PR?

Also I don't really understand what you mean in your PS. Do you mean the vpc_security_group_ids field only accepts IDs?

@bflad
Copy link
Contributor

bflad commented Aug 28, 2018

You're totally right. The aws_instance data source page was the one I was reading the whole time but it should have been the aws_instance resource page. I guess what brought me there is that when I initially searched for "instance" the one under data sources came up first.

We have been debating splitting out the documentation into separate "data source" and "resource" sections. I'll note this ticket to help prioritize that effort. 👍

Anyway, should I undo my previous commit? Or should I just create another PR?

That's up to you. 😄 If you want to make the appropriate change and git rebase on top of this commit, go for it and just let us know when the pull request is updated. Otherwise feel free to close this pull request and create another.

Also I don't really understand what you mean in your PS. Do you mean the vpc_security_group_ids field only accepts IDs?

Sorry, I meant that you should just double check that terraform plan does not want to change to your aws_instance resource after running terraform apply.

@ghost ghost added the documentation Introduces or discusses updates to documentation. label Aug 29, 2018
@ghost ghost added the documentation Introduces or discusses updates to documentation. label Aug 29, 2018
@uLan08
Copy link
Contributor Author

uLan08 commented Aug 29, 2018

Yeah, splitting them out would probably be better. Also maybe consider to make the tree collapsable, it's hard to see other options when one has to scroll down past the other ones.

I have updated the PR btw. Please tell me if you have anything else to add.

Sorry, I meant that you should just double check that terraform plan does not want to change to your aws_instance resource after running terraform apply.

So you're saying that running terraform plan should show potential errors when running terraform_apply? Or do you just mean that as an advice I should just always do terraform_plan first before terraform_apply?

@@ -77,7 +77,7 @@ instances. See [Shutdown Behavior](https://docs.aws.amazon.com/AWSEC2/latest/Use

* `get_password_data` - (Optional) If true, wait for password data to become available and retrieve it. Useful for getting the administrator password for instances running Microsoft Windows. The password data is exported to the `password_data` attribute. See [GetPasswordData](https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_GetPasswordData.html) for more information.
* `monitoring` - (Optional) If true, the launched EC2 instance will have detailed monitoring enabled. (Available since v0.6.0)
* `security_groups` - (Optional, EC2-Classic and default VPC only) A list of security group names to associate with.
* `security_groups` - (Optional, EC2-Classic and default VPC only) A list of security group names or IDs to associate with.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should specifically call out when to use each 👍 A list of security group names (EC2-Classic) or IDs (default VPC) to associate with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a quick question, what does default VPC mean? Are these the VPC's that are already created?

Copy link
Contributor

Choose a reason for hiding this comment

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

Information about default VPCs can be found at: https://docs.aws.amazon.com/vpc/latest/userguide/default-vpc.html

If you created your AWS account after 2013-12-04, it supports only EC2-VPC. In this case, you have a default VPC in each AWS Region. A default VPC is ready for you to use so that you don't have to create and configure your own VPC.

In general, yes, they will already exist in newer accounts, but they can also be deleted (usually for security reasons).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. It's just that I actually created another VPC so I am confused if that still classifies as a "default VPC".

Copy link
Contributor

Choose a reason for hiding this comment

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

Its only possible to have one default VPC per region. If you have created another VPC its likely that it is not the default VPC for the region. You can verify this in the AWS web console by going to Services > VPC > Your VPCs and looking at the Default VPC column.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you run terraform plan again, does it report any changes that need to happen? If not, that's actually new behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm actually yes it does. It says that it needs to recreate my aws instance.

-/+ aws_instance.example (new resource required)
      id:                           "i-0344ac8a8f27efcc2" => <computed> (forces new resource)

I actually have a repo for this if you want to take a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the looks of it, what I am doing is wrong and maybe I should have just used the default VPC. I would appreciate it if you could explain why though. Nevertheless, I updated the docs as per your request.

Copy link
Contributor

Choose a reason for hiding this comment

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

From the looks of it, what I am doing is wrong and maybe I should have just used the default VPC. I would appreciate it if you could explain why though.

The AWS provider is not intended to be opinionated about how you structure your AWS infrastructure, we just follow the API implementation provided by AWS. The code maintainers here will not tell you whether using a default or custom VPC is a better solution for whatever problem you are trying to solve as issues in this GitHub repository are intended only for bugs or feature requests with Terraform's implementation with AWS. For general questions about using AWS, I would reach out to the other Terraform community forums or the AWS forums.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright Thanks!

@bflad bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 4, 2018
@bflad bflad added this to the v1.35.0 milestone Sep 4, 2018
@bflad bflad merged commit 070fbd2 into hashicorp:master Sep 4, 2018
@ghost
Copy link

ghost commented Apr 3, 2020

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 and limited conversation to collaborators Apr 3, 2020
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. service/ec2 Issues and PRs that pertain to the ec2 service.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants