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

Add support for specifying cache key parameters in API Gateway. #893

Merged
merged 4 commits into from
Jun 27, 2017

Conversation

betabandido
Copy link
Contributor

Pull request to fix issue #856

This pull request provides a way to specify cache key parameters, so that caching can take into account varying path parameters or query strings.

I have tested the changes, and I was able to effectively create, read or update cache key parameters. The following (adapted) example shows how to do so for path parameters:

resource "aws_api_gateway_resource" "path1" {
  rest_api_id = "${aws_api_gateway_rest_api.api.id}"
  parent_id   = "${aws_api_gateway_rest_api.api.root_resource_id}"
  path_part   = "{param1}"
}

resource "aws_api_gateway_resource" "path2" {
  rest_api_id = "${aws_api_gateway_rest_api.api.id}"
  parent_id   = "${aws_api_gateway_resource.path1.id}"
  path_part   = "{param2}"
}

resource "aws_api_gateway_method" "method" {
  rest_api_id   = "${aws_api_gateway_rest_api.api.id}"
  resource_id   = "${aws_api_gateway_resource.path2.id}"
  http_method   = "GET"
  authorization = "NONE"
  request_parameters = {
      "method.request.path.param1" = true
      "method.request.path.param2" = true
  }
}

resource "aws_api_gateway_integration" "integration" {
  rest_api_id   = "${aws_api_gateway_rest_api.api.id}"
  resource_id   = "${aws_api_gateway_resource.path2.id}"
  http_method   = "${aws_api_gateway_method.method.http_method}"
  type          = "AWS"
  uri           = "arn:aws:apigateway:${var.region}:lambda:path/2015-03-31/functions/${var.lambda_arn}/invocations"
  integration_http_method = "POST"
  request_parameters = {
      "integration.request.path.param1" = "method.request.path.param1"
      "integration.request.path.param2" = "method.request.path.param2"
  }
  cache_key_parameters = ["method.request.path.param1", "method.request.path.param2"]
}

The example shows how to set the new field (cache_key_parameters) as well as request_parameters (both in the method and integration resources).

@Ninir Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Jun 19, 2017
Copy link
Member

@radeksimko radeksimko 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 the PR, I left you 2 comments/questions there.

@@ -160,8 +172,8 @@ func resourceAwsApiGatewayIntegrationCreate(d *schema.ResourceData, meta interfa
RequestParameters: aws.StringMap(parameters),
RequestTemplates: aws.StringMap(templates),
Credentials: credentials,
CacheNamespace: nil,
CacheKeyParameters: nil,
CacheNamespace: aws.String(d.Get("resource_id").(string)),
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason the cache namespace should always be resource ID?
I had a peek 👀 into the official API docs and couldn't find any suggestions around that: https://docs.aws.amazon.com/apigateway/api-reference/link-relation/integration-put/#cacheNamespace

If there isn't a reason I think we should let the user specify their own namespace.

Also I'm not sure what kind of data it contains and how these are structured, so it's hard to tell (for me at least) whether resource_id is the best namespace key - e.g. does it automatically assign the cache distribution to a given API ID so that we don't need to put API ID to the namespace? Same questions about other fields. If it's not clear from the docs it would be worth asking the AWS support - I'm happy to ask for you.

Copy link
Member

Choose a reason for hiding this comment

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

Also I think we should leave this as nil in case the user didn't specify any caching.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I observed that when caching was manually enabled the namespace set by AWS matched the resource id. But, as you well say it might not be a requirement. I will conduct a test using a custom namespace. If it works, users should indeed be able to specify it.

In the meanwhile, if you can please ask AWS support that would be great. Hopefully we can then have a clear understanding of the usage (and the potential restrictions) of a cache namespace.

Setting the default to nil sounds good to me. I will do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just updated the pull request with your feedback. I did some testing and using a user-defined cache namespace works too. So, I added a field named cache_namespace.

@@ -89,6 +89,28 @@ func TestAccAWSAPIGatewayIntegration_basic(t *testing.T) {
resource.TestCheckResourceAttr("aws_api_gateway_integration.test", "request_templates.application/xml", "#set($inputRoot = $input.path('$'))\n{ }"),
),
},

{
Config: testAccAWSAPIGatewayIntegrationConfigCacheKeyParameters,
Copy link
Member

Choose a reason for hiding this comment

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

Instead of expanding this _basic test would you mind creating a separate one? That way we can also run tests much faster as we generally run whole tests in parallel, we cannot do that with test steps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean adding a new function named func TestAccAWSAPIGatewayIntegration_cache_key_parameters(t *testing.T) (or similar) to the same file resource_aws_api_gateway_integration_test.go?

Copy link
Member

Choose a reason for hiding this comment

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

TestAccAWSAPIGatewayIntegration_cache_key_parameters - that's pretty much what I mean. There is no need for a new file.

@radeksimko radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Jun 25, 2017
@radeksimko radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Jun 27, 2017
@radeksimko
Copy link
Member

Thank you for making those changes.

I ran the relevant acceptance tests and it looks like AWS is setting the namespace by default to the resource_id (even if not specified in the create request), which is causing spurious diff:

=== RUN   TestAccAWSAPIGatewayIntegration_basic
--- FAIL: TestAccAWSAPIGatewayIntegration_basic (51.39s)
	testing.go:428: Step 0 error: After applying this step, the plan was not empty:

		DIFF:

		UPDATE: aws_api_gateway_integration.test
		  cache_namespace: "or4rhf" => ""

The good news is that this behaviour answers my earlier question about how the data are structured in the cache. If AWS chooses to use this key by default then it should be good (unique) enough.

Do you mind setting cache_namespace as Computed, so that Terraform can accept such default namespace instead of trying to change it to empty?

It would be obviously best if we could just set a Default to resource_id, but we don't have a good mechanism in the schema to do this - there's no way to get access to values of other fields in the schema, so Computed is the only option for now.

@radeksimko radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Jun 27, 2017
@betabandido
Copy link
Contributor Author

Done :) I ran the two acceptance tests for the integration resource, and both of them passed.

@radeksimko radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Jun 27, 2017
@radeksimko radeksimko merged commit 3c202fc into hashicorp:master Jun 27, 2017
@ghost
Copy link

ghost commented Apr 12, 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 12, 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants