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

[WIP] Add policy attribute to aws_api_gateway_rest_api #4211

Merged
merged 10 commits into from
Apr 20, 2018

Conversation

robwittman
Copy link

Fixes #4171.
Adds a policy attribute to the aws_api_gateway_rest_api resource.

The policy attribute is currently being applied, however on every subsequent terraform plan, its requesting to update the attribute in place. I assume this is due to how I am setting the policy in resourceAwsApiGatewayRestApiRead causing one version to be escaped, and one not.

Is there a standard for escaping -> unescaping JSON policies correctly? Sorry if it's obvious, I'm new to Go :)

Failing Test:

--- FAIL: TestAccAWSAPIGatewayRestApi_policy (9.53s)
	testing.go:518: Step 0 error: Check failed: Check 1/1 error: aws_api_gateway_rest_api.test: Attribute 'policy' expected "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Principal\":{\"AWS\":\"*\"},\"Action\":\"execute-api:Invoke\",\"Resource\":\"*\"}]}", got "{\\\"Version\\\":\\\"2012-10-17\\\",\\\"Statement\\\":[{\\\"Effect\\\":\\\"Allow\\\",\\\"Principal\\\":{\\\"AWS\\\":\\\"*\\\"},\\\"Action\\\":\\\"execute-api:Invoke\\\",\\\"Resource\\\":\\\"*\\\"}]}"

terraform plan ran after apply.

rwittman$ $GOPATH/bin/terraform plan
Refreshing Terraform state in-memory prior to plan...
The refreshed state will be used to calculate this plan, but will not be
persisted to local or remote state storage.

aws_api_gateway_rest_api.test_gateway: Refreshing state... (ID: hj4o1slwk2)

------------------------------------------------------------------------

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  ~ aws_api_gateway_rest_api.test_gateway
      policy: "{\\\"Version\\\":\\\"2012-10-17\\\",\\\"Statement\\\":[{\\\"Effect\\\":\\\"Allow\\\",\\\"Principal\\\":{\\\"AWS\\\":\\\"arn:aws:iam::717754130149:user\\/dev-encryption-service-emr-user\\\"},\\\"Action\\\":\\\"execute-api:Invoke\\\",\\\"Resource\\\":\\\"arn:aws:execute-api:us-west-2:717754130149:wwo2hqsydh\\/*\\/POST\\/v1\\/encryption-service\\/password\\/change\\\"}]}" => "{\n  \"Version\": \"2012-10-17\",\n  \"Statement\": [\n    {\n      \"Effect\": \"Allow\",\n      \"Principal\": {\n        \"AWS\": \"arn:aws:iam::717754130149:user/dev-encryption-service-emr-user\"\n      },\n      \"Action\": \"execute-api:Invoke\",\n      \"Resource\": \"arn:aws:execute-api:us-west-2:717754130149:wwo2hqsydh/*/POST/v1/encryption-service/password/change\"\n    }\n  ]\n}\n"


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

@ghost ghost added the size/M Managed by automation to categorize the size of a PR. label Apr 15, 2018
@bflad bflad added enhancement Requests to existing resources that expand the functionality or scope. service/apigateway Issues and PRs that pertain to the apigateway service. labels Apr 17, 2018
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution, @robwittman! Please see my initial comments below. The testing behavior is interesting, I'd see if its related to the wrapping fmt.Sprintf() -- we are able to test this successfully in TestAccAWSIAMUserPolicy_basic for example.

@@ -72,9 +79,15 @@ func resourceAwsApiGatewayRestApiCreate(d *schema.ResourceData, meta interface{}
description = aws.String(d.Get("description").(string))
}

var policy *string
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified with (after params declaration):

if v, ok := d.GetOk("policy"); ok && v.(string) != "" {
  params.Policy = aws.String(v.(string))
}

@@ -127,6 +127,24 @@ func TestAccAWSAPIGatewayRestApi_basic(t *testing.T) {
})
}

func TestAccAWSAPIGatewayRestApi_policy(t *testing.T) {
expectedPolicyText := fmt.Sprintf(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"AWS":"*"},"Action":"execute-api:Invoke","Resource":"*"}]}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work without the wrapping fmt.Sprintf()?

Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("aws_api_gateway_rest_api.test", "policy", expectedPolicyText),
),
},
Copy link
Contributor

@bflad bflad Apr 17, 2018

Choose a reason for hiding this comment

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

We should add test steps that attempt to:

  • update the policy
  • remove the policy

Copy link
Author

Choose a reason for hiding this comment

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

Test steps for update and removal have been added

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Apr 17, 2018
@ghost ghost added size/L Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Apr 17, 2018
Copy link
Author

@robwittman robwittman left a comment

Choose a reason for hiding this comment

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

Thanks @bflad. I removed the Sprintf() and was still getting the encoding issues. I pulled in net/url and used QueryUnescape when reading the resource, similar to resource_aws_iam_user_policy, and still no luck.

If I log the unescaped policy during resourceAwsApiGatewayRestApiRead(), i get

2018/04/17 09:08:28 [DEBUG] Decoded Policy: {\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Principal\":{\"AWS\":\"*\"},\"Action\":\"execute-api:Invoke\",\"Resource\":\"*\"}]}

which I believe is to be expected. However, the tests still fail, and terraform plan keeps trying to update the API Gateway on AWS.

I also added test steps for explicitly testing update / removal, but don't think they're running since the 1st step is failing.

Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("aws_api_gateway_rest_api.test", "policy", expectedPolicyText),
),
},
Copy link
Author

Choose a reason for hiding this comment

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

Test steps for update and removal have been added

@bflad
Copy link
Contributor

bflad commented Apr 17, 2018

2018/04/17 17:25:05 [DEBUG] Creating API Gateway
2018/04/17 17:25:05 [DEBUG] [aws-sdk-go] DEBUG: Request apigateway/CreateRestApi Details:
---[ REQUEST POST-SIGN ]-----------------------------
POST /restapis HTTP/1.1
Host: apigateway.us-west-2.amazonaws.com
User-Agent: aws-sdk-go/1.13.28 (go1.9.4; linux; amd64) APN/1.0 HashiCorp/1.0 Terraform/0.11.7
Content-Length: 340
Accept: application/json
Authorization: AWS4-HMAC-SHA256 Credential=AKIAIXP7QHO656Q4VL7Q/20180417/us-west-2/apigateway/aws4_request, SignedHeaders=accept;content-length;host;x-amz-date, Signature=20fa93182d1582f6307b208a6292fd2a28998a6986d2fb3b2509fc59f5f41100
X-Amz-Date: 20180417T172505Z
Accept-Encoding: gzip

{"minimumCompressionSize":0,"name":"bar","policy":"{\n    \"Version\": \"2012-10-17\",\n    \"Statement\": [\n        {\n            \"Effect\": \"Allow\",\n            \"Principal\": {\n                \"AWS\": \"*\"\n            },\n            \"Action\": \"execute-api:Invoke\",\n            \"Resource\": \"*\"\n        }\n    ]\n}\n"}
-----------------------------------------------------
2018/04/17 17:25:05 [DEBUG] [aws-sdk-go] DEBUG: Response apigateway/CreateRestApi Details:
---[ RESPONSE ]--------------------------------------
HTTP/2.0 201 Created
Content-Length: 375
Content-Type: application/json
Date: Tue, 17 Apr 2018 17:25:05 GMT
X-Amzn-Requestid: 45057bd1-4264-11e8-9c50-2b55ed529d4e


-----------------------------------------------------
2018/04/17 17:25:05 [DEBUG] [aws-sdk-go] {"apiKeySource":"HEADER","createdDate":1523985905,"endpointConfiguration":{"ipv6":false,"types":["EDGE"]},"id":"uhiblicswd","minimumCompressionSize":0,"name":"bar","policy":"{\\\"Version\\\":\\\"2012-10-17\\\",\\\"Statement\\\":[{\\\"Effect\\\":\\\"Allow\\\",\\\"Principal\\\":{\\\"AWS\\\":\\\"*\\\"},\\\"Action\\\":\\\"execute-api:Invoke\\\",\\\"Resource\\\":\\\"*\\\"}]}"}

This is likely happening because the API Gateway service uses JSON for its own API, which requires the extra escaping in its own response. It might make sense, odd as it is, to double unescape the policy that comes back from API Gateway and comment in the code why this is happening.

@robwittman
Copy link
Author

Here's what Im using to try and double decode, but the tests are still failing for the same reason.

if api.Policy != nil {
		policy, err := url.QueryUnescape(*api.Policy)
		log.Printf("[DEBUG] Policy: %s", policy)
		if err != nil {
			return err
		}
		decodedPolicy, err := url.QueryUnescape(policy)
		log.Printf("[DEBUG] Decoded Policy: %s", decodedPolicy)
		if err != nil {
			return err
		}
		if err := d.Set("policy", decodedPolicy); err != nil {
			return err
		}
	}

If I debug the value in d.Set() during testing, the policy is showing 2018/04/19 14:25:22 [DEBUG] Value: {\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Principal\":{\"AWS\":\"*\"},\"Action\":\"execute-api:Invoke\",\"Resource\":\"*\"}]} which looks like the correct value.

Logging the outputs of d.Set(), field_writer_map.go's setPrimitive(), and d.GetOk() all return what looks to be an unescaped string, as the test is expecting, but its still getting escaped somewhere?

@bflad bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Apr 20, 2018
@bflad bflad added this to the v1.16.0 milestone Apr 20, 2018
@bflad
Copy link
Contributor

bflad commented Apr 20, 2018

Hi @robwittman! Thanks for really diving into this. I found a working configuration:

	// The API returns policy as an escaped JSON string
	// {\\\"Version\\\":\\\"2012-10-17\\\",...}
	policy, err := strconv.Unquote(`"` + aws.StringValue(api.Policy) + `"`)
	if err != nil {
		return fmt.Errorf("error unescaping policy: %s", err)
	}
	d.Set("policy", policy)

This passes all acceptance testing 👍 Merging with your commits plus this one on top.

make testacc TEST=./aws TESTARGS='-run=TestAccAWSAPIGatewayRestApi'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSAPIGatewayRestApi -timeout 120m
=== RUN   TestAccAWSAPIGatewayRestApi_basic
--- PASS: TestAccAWSAPIGatewayRestApi_basic (25.68s)
=== RUN   TestAccAWSAPIGatewayRestApi_policy
--- PASS: TestAccAWSAPIGatewayRestApi_policy (32.48s)
=== RUN   TestAccAWSAPIGatewayRestApi_openapi
--- PASS: TestAccAWSAPIGatewayRestApi_openapi (32.38s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	90.587s

@bflad bflad merged commit c953fde into hashicorp:master Apr 20, 2018
bflad added a commit that referenced this pull request Apr 20, 2018
@robwittman
Copy link
Author

Thanks @bflad , appreciate the help. Looking forward to release!

@bflad
Copy link
Contributor

bflad commented Apr 25, 2018

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

@bazron
Copy link

bazron commented Apr 26, 2018

Not this is the place to leave this, but using this policy:
policy = <<EOF
{
"Version": "2012-10-17",
"Statement": [
{
"Effect": "Allow",
"Principal": "",
"Action": "execute-api:Invoke",
"Resource": [
"execute-api:/
/POST/notifications"
],
"Condition": {
"IpAddress": {
"aws:SourceIp": [
"xxx", "yyy", "zzz"
]
}
}
}
]
}
EOF
}

I get this error: "error unescaping policy: invalid syntax"
It looks to like the correct syntax. What's incorrect?

@danieladams456
Copy link
Contributor

Same issue, was there a reason it was done differently than the other IAM policies?

resource "aws_api_gateway_rest_api" "api" {
  name        = "${var.api_gateway_name}"
  description = "${var.api_gateway_description}"

  policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "Principal": "*",
      "Action": "execute-api:Invoke",
      "Resource": "*",
      "Condition": {
        "IpAddress": {
          "aws:SourceIp": "xxx.xxx.xxx.xxx/xx"
        }
      }
    }
  ]
}
EOF
}

@bazron
Copy link

bazron commented Apr 30, 2018

Hey @bflad ,
Just wondering if you guys are going to look at this issue.

@bflad
Copy link
Contributor

bflad commented Apr 30, 2018

Please open a new issue filling in the template (if one has not already been opened) so we have all the relevant details to troubleshoot, specifically the debug logging.

@ghost
Copy link

ghost commented Apr 6, 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 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/apigateway Issues and PRs that pertain to the apigateway service. size/L Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Support resource policy when creating AWS api gateway
4 participants