Support CodeBuild within VPC #2547

adventureisyou · 2017-12-05T15:01:05Z

First attempt at a go contribution, happy to implement any suggestions.

Update aws_codebuild_project.
Acceptance test.
Documentation.

There is an issue with the acceptance test (hence WIP). Creation of the CodeBuild project requires the DescribeSecurityGroups permission. This is added to the IAM role, but there is a race condition between the policy actually being applied and the creation of the CodeBuild project, meaning that it can fail. I've added a depends_on = ["aws_iam_policy_attachment.codebuild_policy_attachment"] clause to the aws_codebuild_policy resource, but this doesn't help. I guess it is the natural lag in applying IAM policies. Any ideas? Edit: this is fixed now.

…IAM race condition

bflad · 2017-12-05T16:11:46Z

Ah yes, the evils of IAM eventual consistency! 🙁 In many downstream resources we opt to retry creation actions if the AWS error is specific enough to indicate its some form of permissions error code/message.

I haven't looked at the codebuild resource, but here's an example from codedeploy which might help: https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_codedeploy_deployment_group.go#L391-L423

adventureisyou · 2017-12-05T17:09:34Z

Aha! Thanks for the quick reply. Guess I should have looked harder. The CodeBuild code has the same retry logic, but the error message slightly doesn't match.

if isAWSErr(err, "InvalidInputException", "CodeBuild is not authorized to perform") {
	return resource.RetryableError(err)
}

My error is: InvalidInputException: Not authorized to perform DescribeSecurityGroups

Should I change the existing line to this? (I think it might end up retrying lots if the end user's IAM credentials are not authorized to perform actions, rather than just CodeBuild)

if isAWSErr(err, "InvalidInputException", "ot authorized to perform") {
	return resource.RetryableError(err)
}

The safe way is to just add another error message capture, I guess. Preferences?

…h more IAM eventual consistency. Remove unnecessary depends_on clause from CodeBuild basic acceptance test.

adventureisyou · 2017-12-05T17:19:53Z

Went ahead with the safer route after giving it some more thought. This should be good to go now.

kilsbo · 2018-01-19T16:13:27Z

Please merge! 💯

bflad

Hey @DrSolaris, sorry this has taken so long to get a review. Overall this is actually looking pretty good! Unfortunately due to the acceptance test panic I received, we'll need rework some of this, but should not be too bad. If you do not have time to implement these changes please let us know and we can fix this up with your commits in tact. Let us know if you have any questions!

bflad · 2018-01-27T08:06:07Z

aws/resource_aws_codebuild_project.go

@@ -324,6 +358,29 @@ func expandProjectEnvironment(d *schema.ResourceData) *codebuild.ProjectEnvironm
 	return projectEnv
 }

+func expandVpcConfig(d *schema.ResourceData) *codebuild.VpcConfig {


We really try to limit the usage of *schema.ResourceData in downstream functions unless its necessary in multiple pieces of logic. It helps make the code more testable and reusable (if necessary in the future). Could you update this function's signature to expandCodeBuildVpcConfig(m map[string]interface{}) *codebuild.VpcConfig? Thanks!

bflad · 2018-01-27T08:06:24Z

aws/resource_aws_codebuild_project_test.go

@@ -401,6 +442,18 @@ resource "aws_codebuild_project" "foo" {
  tags {
    "Environment" = "Test"
  }
+
+  vpc_config {


Since this new attribute is optional and has some complexity, we should probably split it out into its own acceptance test. We need to verify the resource creation (what's done here already), the update of vpc_config attributes (e.g. add/remove subnet), and the removal of the vpc_config altogether from a configuration (if removing VPC configuration is supported). It would also be good to check that the new attributes are at least making it correctly into the state. e.g.

resource.TestCheckResourceSet("aws_codebuild_project.foo", "vpc_config.0.vpc_id"), resource.TestCheckResource("aws_codebuild_project.foo", "vpc_config.0.subnets.#", "1"), resource.TestCheckResource("aws_codebuild_project.foo", "vpc_config.0.security_group_ids.#", "1"),

bflad · 2018-01-27T08:14:31Z

aws/resource_aws_codebuild_project.go

@@ -324,6 +358,29 @@ func expandProjectEnvironment(d *schema.ResourceData) *codebuild.ProjectEnvironm
 	return projectEnv
 }

+func expandVpcConfig(d *schema.ResourceData) *codebuild.VpcConfig {
+	configs := d.Get("vpc_config").(*schema.Set).List()
+	data := configs[0].(map[string]interface{})


This line is generating a panic for me running the acceptance test:

=== RUN TestAccAWSCodeBuildProject_basic panic: runtime error: index out of range goroutine 2579 [running]: github.com/terraform-providers/terraform-provider-aws/aws.expandVpcConfig(0xc42084b110, 0x3715e47) /Users/bflad/go/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_codebuild_project.go:363 +0x790

It will go away with the function signature refactor and TypeList changes mentioned above.

bflad · 2018-01-27T08:15:49Z

aws/resource_aws_codebuild_project.go

@@ -179,6 +179,31 @@ func resourceAwsCodeBuildProject() *schema.Resource {
 				ValidateFunc: validateAwsCodeBuildTimeout,
 			},
 			"tags": tagsSchema(),
+			"vpc_config": {
+				Type:     schema.TypeSet,


I would switch this to Type: schema.TypeList,. There are subtleties between the two, but the gist being that list in this context is much easier to work with when there's only 1 item.

bflad · 2018-01-27T08:17:22Z

aws/resource_aws_codebuild_project.go

@@ -213,6 +238,11 @@ func resourceAwsCodeBuildProjectCreate(d *schema.ResourceData, meta interface{})
 		params.TimeoutInMinutes = aws.Int64(int64(v.(int)))
 	}

+	if _, ok := d.GetOk("vpc_config"); ok {


I would change this to (note about function signature update below in another comment):

if v, ok := d.GetOk("vpc_config"); ok { params.VpcConfig = expandCodeBuildVpcConfig(v.([]interface{})[0].(map[string]interface{})) }

bflad · 2018-01-27T08:19:42Z

aws/resource_aws_codebuild_project.go

@@ -390,6 +447,10 @@ func resourceAwsCodeBuildProjectRead(d *schema.ResourceData, meta interface{}) e
 		return err
 	}

+	if err := d.Set("vpc_config", schema.NewSet(resourceAwsCodeBuildVpcConfigHash, flattenAwsCodebuildVpcConfig(project.VpcConfig))); err != nil {


When this is TypeList this can become:

if err := d.Set("vpc_config", flattenAwsCodebuildVpcConfig(project.VpcConfig)); err != nil {

bflad · 2018-01-27T08:21:29Z

aws/resource_aws_codebuild_project.go

@@ -567,6 +653,25 @@ func resourceAwsCodeBuildProjectArtifactsHash(v interface{}) int {
 	return hashcode.String(buf.String())
 }

+func resourceAwsCodeBuildVpcConfigHash(v interface{}) int {


This is extraneous with switch to TypeList 👍

CodeBuild in VPC continuing Issue #2547

bflad · 2018-02-22T22:49:01Z

This support has been merged in with your commits squashed via #3324 and will be released in v1.10.0 of the AWS provider, likely tomorrow. Thanks for you work here!

bflad · 2018-02-27T12:46:05Z

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

ghost · 2020-04-07T17:54:15Z

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!

Ryan Moore added 3 commits December 5, 2017 14:48

Add VPC config block to aws_codebuild_project resource

56a5b3d

Documentation for VPC block within aws_codebuild_project resource

4581628

Update basic codebuild test to support VPC block. Currently contains …

cd23499

…IAM race condition

Add further error catching to CodeBuild resource creation to deal wit…

0bef8ec

…h more IAM eventual consistency. Remove unnecessary depends_on clause from CodeBuild basic acceptance test.

adventureisyou changed the title ~~[WIP] Support CodeBuild within VPC~~ Support CodeBuild within VPC Dec 5, 2017

Formatting fix for codepipline environment variables documentation

f417d49

radeksimko added the enhancement Requests to existing resources that expand the functionality or scope. label Dec 12, 2017

bflad added the service/codebuild Issues and PRs that pertain to the codebuild service. label Jan 27, 2018

bflad suggested changes Jan 27, 2018

View reviewed changes

moofish32 mentioned this pull request Feb 11, 2018

CodeBuild in VPC continuing Issue #2547 #3324

Merged

bflad added a commit that referenced this pull request Feb 22, 2018

Merge pull request #3324 from moofish32/f-codebuild-in-vpc-2495

173342c

CodeBuild in VPC continuing Issue #2547

bflad closed this Feb 22, 2018

bflad added this to the v1.10.0 milestone Feb 22, 2018

bflad mentioned this pull request Feb 22, 2018

Add support for CodeBuild with VPC #2495

Closed

bflad added a commit that referenced this pull request Feb 22, 2018

Update CHANGELOG for #2547 and #3324

a842695

ghost locked and limited conversation to collaborators Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support CodeBuild within VPC #2547

Support CodeBuild within VPC #2547

adventureisyou commented Dec 5, 2017 •

edited

Loading

bflad commented Dec 5, 2017

adventureisyou commented Dec 5, 2017

adventureisyou commented Dec 5, 2017

kilsbo commented Jan 19, 2018

bflad left a comment

bflad Jan 27, 2018

bflad Jan 27, 2018

bflad Jan 27, 2018

bflad Jan 27, 2018

bflad Jan 27, 2018

bflad Jan 27, 2018

bflad Jan 27, 2018

bflad commented Feb 22, 2018

bflad commented Feb 27, 2018

ghost commented Apr 7, 2020

Support CodeBuild within VPC #2547

Support CodeBuild within VPC #2547

Conversation

adventureisyou commented Dec 5, 2017 • edited Loading

bflad commented Dec 5, 2017

adventureisyou commented Dec 5, 2017

adventureisyou commented Dec 5, 2017

kilsbo commented Jan 19, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad Jan 27, 2018

Choose a reason for hiding this comment

bflad Jan 27, 2018

Choose a reason for hiding this comment

bflad Jan 27, 2018

Choose a reason for hiding this comment

bflad Jan 27, 2018

Choose a reason for hiding this comment

bflad Jan 27, 2018

Choose a reason for hiding this comment

bflad Jan 27, 2018

Choose a reason for hiding this comment

bflad Jan 27, 2018

Choose a reason for hiding this comment

bflad commented Feb 22, 2018

bflad commented Feb 27, 2018

ghost commented Apr 7, 2020

adventureisyou commented Dec 5, 2017 •

edited

Loading