r/aws_api_gateway_stage: implement access logging #4369

appilon · 2018-04-26T17:39:23Z

implement access logging for apigw stage
add acceptance test with different accepted log formats
make note of sanitization performed on destination_arn

TF_ACC=1 go test ./aws -v -run=TestAccAWSAPIGatewayStage_accessLogSettings -timeout 120m
=== RUN   TestAccAWSAPIGatewayStage_accessLogSettings
--- PASS: TestAccAWSAPIGatewayStage_accessLogSettings (331.21s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	331.260s

bflad

Looking good @appilon! Left some comments below -- please let me know if you have any questions 😄

bflad · 2018-04-26T18:56:43Z

aws/resource_aws_api_gateway_stage.go

@@ -177,6 +202,8 @@ func resourceAwsApiGatewayStageRead(d *schema.ResourceData, meta interface{}) er
 	}
 	log.Printf("[DEBUG] Received API Gateway Stage: %s", stage)

+	d.Set("access_log_settings", flattenApiGatewayStageAccessLogSettings(stage.AccessLogSettings))


We should do error checking when setting non-scalar attributes 👍 e.g.

if err := d.Set("access_log_settings", flattenApiGatewayStageAccessLogSettings(stage.AccessLogSettings)); err != nil { return fmt.Errorf("error setting access_log_settings: %s", err) }

bflad · 2018-04-26T19:00:45Z

aws/resource_aws_api_gateway_stage_test.go

+				Config: testAccAWSAPIGatewayStageConfig_basic(rName),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSAPIGatewayStageExists("aws_api_gateway_stage.test", &conf),
+					testAccCheckAWSAPIGatewayStageAccessLogSettingsIsNil(&conf),


You can combine both this check and the below with resource.TestCheckResourceAttr("aws_api_gateway_stage.test", "access_logs_settings.#", "0")

because my flattener returns nil if the sdk struct is nil, access_logs_settings.# doesn't even exist in the state, should I just check that access_logs_settings is not set? Alternatively I could change my flattener to return an empty array. It boils down to what do we prefer for the 'zero' value on a nested block, nil or empty []interface{}?

I tend to lean towards returning empy list/set/map so it explicitly gets set as a zero value in the state. I believe the reasoning is that it helps with writing conditionals like ${length(aws_api_gateway_stage.example.access_log_settings)} (where if its not set it'll return an error about the attribute not existing instead of returning 0).

bflad · 2018-04-26T19:02:20Z

aws/resource_aws_api_gateway_stage_test.go

+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckAWSAPIGatewayStageDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{


Minor nitpick: The resource.TestStep declarations here are extraneous as its already defined by []resource.TestStep{ above

bflad · 2018-04-26T19:07:57Z

aws/resource_aws_api_gateway_stage_test.go

+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSAPIGatewayStageExists("aws_api_gateway_stage.test", &conf),
+					resource.TestCheckResourceAttr("aws_api_gateway_stage.test", "access_log_settings.#", "1"),
+					testAccCheckAWSAPIGatewayStageAccessLogSettingsDestinationArnIsValid("aws_api_gateway_stage.test"),


Something for consideration instead of writing a new function:

resource.TestMatchResourceAttr("aws_api_gateway_stage.test", "access_log_settings.0.destination_arn", regexp.MustCompile(fmt.Sprintf("^arn:[^:]+:logs:[^:]+:[^:]+:foo-bar-%s$", rName))),

your regex skills are lit 🔥

sadly that rule didn't work I was inspired by you though and tweaked it let me know if you think it's robust enough.

logGroupArnRegex := regexp.MustCompile(fmt.Sprintf("^(arn:aws:logs:)(.*)(log-group:foo-bar-%s)$", rName))

Looks like it just needed log-group: before foo-bar-%s:

resource.TestMatchResourceAttr("aws_api_gateway_stage.test", "access_log_settings.0.destination_arn", regexp.MustCompile(fmt.Sprintf("^arn:[^:]+:logs:[^:]+:[^:]+:log-group:foo-bar-%s$", rName))),

Whatever you think is best. I try to avoid wildcards.

I will say we should to try to not hardcode arn:aws at the beginning because at some point these tests will be running in US GovCloud (aws-us-gov) and maybe China partitions (aws-cn).

This does bring up a valid point that maybe we should have a simple ARN matching TestCheckFunc instead of handcrafting these.

👍 to arn matcher and to your points on harcoded arn:aws I didn't even think of that!

bflad · 2018-04-26T19:09:42Z

website/docs/r/api_gateway_stage.html.markdown

@@ -69,6 +69,7 @@ The following arguments are supported:
 * `rest_api_id` - (Required) The ID of the associated REST API
 * `stage_name` - (Required) The name of the stage
 * `deployment_id` - (Required) The ID of the deployment that the stage points to
+* `access_log_settings` - (Optional) enables access logs for the api stage


Should probably mention: ...API stage. Detailed below.

bflad · 2018-04-26T19:12:47Z

website/docs/r/api_gateway_stage.html.markdown

+#### `access_log_settings`
+
+* `destination_arn` - (Required) ARN of the log group to send the logs to.
+~> **Note:** Referencing a log group arn from a Terraform resource will normally include a `:*` suffix which is not valid for the destination arn, Terraform automatically strips this suffix when configuring this argument.


I don't think we need this bold callout that makes it sound like Terraform is doing something wrong or this is Terraform specific behavior (as the ARN value with :* coming from the CloudWatch API). I think a simple Automatically removes trailing `:*` if present. in the line above would suffice. 👍

bflad · 2018-04-26T19:15:12Z

aws/resource_aws_api_gateway_stage.go

+		return nil
+	}
+	item := make(map[string]interface{})
+	if accessLogSettings.DestinationArn != nil {


Something to consider when working in the AWS provider is that the SDK provides helper functions so you don't need these wrapping nil checks (in most cases anyways): item["destination_arn"] = aws.StringValue(accessLogSettings.DestinationArn)

appilon · 2018-04-26T21:20:47Z

aws/resource_aws_api_gateway_stage.go

@@ -276,6 +305,27 @@ func resourceAwsApiGatewayStageUpdate(d *schema.ResourceData, meta interface{})
 		newV := n.(map[string]interface{})
 		operations = append(operations, diffVariablesOps("/variables/", oldV, newV)...)
 	}
+	if d.HasChange("access_log_settings") {
+		accessLogSettings := d.Get("access_log_settings").([]interface{})


@bflad if the user deletes the nested block, is this code risky? My acceptance tests pass but I'm concerned len(accessLogSettings) might gag... (I've admittedly never used much interface{} or type casts until TF)

As far as I know helper/schema.Schema should return the appropriate Go type []interface{} here with either 0 or 1 elements: https://godoc.org/github.com/hashicorp/terraform/helper/schema#Schema

add acceptance test with different accepted log formats make note of sanitization performed on destinatation_arn

appilon · 2018-04-27T18:42:08Z

@bflad I think I've addressed all the comments!

bflad

This looks great @appilon, thanks! 🚀

2 tests passed (all tests)
=== RUN   TestAccAWSAPIGatewayStage_accessLogSettings
--- PASS: TestAccAWSAPIGatewayStage_accessLogSettings (392.52s)
=== RUN   TestAccAWSAPIGatewayStage_basic
--- PASS: TestAccAWSAPIGatewayStage_basic (519.82s)

bflad · 2018-05-02T21:50:44Z

This has been released in version 1.17.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-06T17:25:52Z

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 added size/L Managed by automation to categorize the size of a PR. labels Apr 26, 2018

appilon requested a review from a team April 26, 2018 17:45

appilon added service/apigateway Issues and PRs that pertain to the apigateway service. enhancement Requests to existing resources that expand the functionality or scope. labels Apr 26, 2018

ghost added size/L Managed by automation to categorize the size of a PR. labels Apr 26, 2018

bflad reviewed Apr 26, 2018

View reviewed changes

ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 26, 2018

appilon commented Apr 26, 2018

View reviewed changes

ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 27, 2018

implement access logging for apigw stage

9429c78

add acceptance test with different accepted log formats make note of sanitization performed on destinatation_arn

ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 27, 2018

bflad approved these changes Apr 27, 2018

View reviewed changes

bflad merged commit 2438532 into hashicorp:master Apr 27, 2018

bflad added this to the v1.17.0 milestone Apr 27, 2018

bflad added a commit that referenced this pull request Apr 27, 2018

Update CHANGELOG for #4369

4ee9475

ghost locked and limited conversation to collaborators Apr 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

r/aws_api_gateway_stage: implement access logging #4369

r/aws_api_gateway_stage: implement access logging #4369

appilon commented Apr 26, 2018 •

edited

Loading

bflad left a comment

bflad Apr 26, 2018

bflad Apr 26, 2018

appilon Apr 26, 2018 •

edited

Loading

bflad Apr 27, 2018

bflad Apr 26, 2018

bflad Apr 26, 2018

appilon Apr 26, 2018

appilon Apr 26, 2018

bflad Apr 27, 2018

bflad Apr 27, 2018

appilon Apr 27, 2018

bflad Apr 26, 2018

bflad Apr 26, 2018

bflad Apr 26, 2018

appilon Apr 26, 2018

bflad Apr 27, 2018

appilon commented Apr 27, 2018

bflad left a comment

bflad commented May 2, 2018

ghost commented Apr 6, 2020

r/aws_api_gateway_stage: implement access logging #4369

r/aws_api_gateway_stage: implement access logging #4369

Conversation

appilon commented Apr 26, 2018 • edited Loading

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appilon Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appilon commented Apr 27, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad commented May 2, 2018

ghost commented Apr 6, 2020

appilon commented Apr 26, 2018 •

edited

Loading

appilon Apr 26, 2018 •

edited

Loading