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

provider/aws: implement CloudFront Lambda Function Associations (supersedes #10985) #11291

Merged
merged 12 commits into from
Jan 20, 2017

Conversation

catsby
Copy link
Contributor

@catsby catsby commented Jan 19, 2017

Continue the work done in #10985 with just a few tweaks to restore the TypeSet in lambda_function_association.

Should also fix #11295

@stack72
Copy link
Contributor

stack72 commented Jan 19, 2017

Solid work - this LGTM! Nice to see this coming in - it should unblock some people :)

@MrGossett
Copy link
Contributor

Thanks!

@@ -44,6 +49,35 @@ func trustedSignersConf() []interface{} {
return []interface{}{"1234567890EX", "1234567891EX"}
}

func lambdaSetHash(v interface{}) int {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is essentially where my PR was at 9c5a25d on Jan 5. I was derailed by the comment from @apparentlymart asking to remove the hash function.

Sorry this got delayed 2 weeks as a result 🤓

Copy link
Contributor Author

@catsby catsby Jan 19, 2017

Choose a reason for hiding this comment

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

Sorry this got delayed 2 weeks as a result

Nah it's not your fault.

@apparentlymart was correct in that the manually added Set function should be omitted, unless there's a really good reason.

That doesn't make it his fault either 😄

The problem here was reconstructing "sets" in code in the test instead of configuration. All the previous examples I found in the codebase were simple set's of strings which is trivial to reconstruct, but NewSet is more difficult with sets that have a more complicated schema.

Like I said you did 99% of the work here I just needed to do some battlefield surgery 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Roger. Thanks again 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MrGossett So.... funny enough, I had to reinstate that lambda set hash you were asked to remove 😆

From Martin's comment:

We only really continue to support explicit Set for compatibility with schemas that predate the automatic hashing.

This is true, but unfortunately lambda_function_association is being added as a sub param to two params that do themselves have custom hash function for the IDs.

So, I had to add lambda_function_association to the two CacheBehaviorHash (?) method(s), but to do so, I'd need to hash them... so I needed that method 😐

So anyway it's re-added

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@catsby
Copy link
Contributor Author

catsby commented Jan 19, 2017

Ran some test:

=== RUN   TestAccAWSCloudFrontDistribution_importBasic
--- PASS: TestAccAWSCloudFrontDistribution_importBasic (1587.37s)
=== RUN   TestAccAWSCloudFrontDistribution_S3Origin
--- PASS: TestAccAWSCloudFrontDistribution_S3Origin (1626.38s)
=== RUN   TestAccAWSCloudFrontDistribution_S3OriginWithTags
--- PASS: TestAccAWSCloudFrontDistribution_S3OriginWithTags (1734.81s)

@catsby
Copy link
Contributor Author

catsby commented Jan 19, 2017

Hold off on this - need to investigate something

@catsby
Copy link
Contributor Author

catsby commented Jan 19, 2017

I added documentation and the lambda_function_association objects to the cacheBehaviorHash methods to track them, and added docs in 6d4f400

@catsby
Copy link
Contributor Author

catsby commented Jan 19, 2017

Hey @stack72 when you get a chance can you look this over? Not sure about the TravisCI error 🤔

handles query strings, cookies and headers (maximum one).

* `lambda_function_association` (Optional) - A complext config that triggers a lambda function with
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo on complext

Lambda@Edge](http://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/what-is-lambda-at-edge.html)
for more information

* `event_type` (Required) - The specific event to trigger this functionon.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo on functionon

@stack72
Copy link
Contributor

stack72 commented Jan 20, 2017

@catsby the travis fail is because of this

--- FAIL: TestCloudFrontStructure_flattenDefaultCacheBehavior (0.00s)
panic: interface conversion: interface {} is []interface {}, not *schema.Set [recovered]
	panic: interface conversion: interface {} is []interface {}, not *schema.Set

@stack72
Copy link
Contributor

stack72 commented Jan 20, 2017

LGTM!
image

for more information

* `event_type` (Required) - The specific event to trigger this function.
Valid values: `viewwer-request`, `origin-request`, `viewer-response`,
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: viewwer-request

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, thanks!

@@ -294,6 +311,9 @@ func flattenCacheBehavior(cb *cloudfront.CacheBehavior) map[string]interface{} {
if len(cb.TrustedSigners.Items) > 0 {
m["trusted_signers"] = flattenTrustedSigners(cb.TrustedSigners)
}
if len(cb.LambdaFunctionAssociations.Items) > 0 {
m["lambda_function_association"] = flattenLambdaFunctionAssociations(cb.LambdaFunctionAssociations)
Copy link
Contributor

Choose a reason for hiding this comment

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

is this supposed to be plural? lambda_function_associations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, it's a key in the schema. If it were plural then the config to use it would be

lambda_funcation_associations {
  event_type = "blah"
  arn = "blah"
}
lambda_funcation_associations {
  event_type = "blah"
  arn = "blah"
}
...

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense.

@@ -337,6 +363,9 @@ func TestCloudFrontStructure_flattenCacheBehavior(t *testing.T) {
if out["target_origin_id"] != "myS3Origin" {
t.Fatalf("Expected out[target_origin_id] to be myS3Origin, got %v", out["target_origin_id"])
}
if reflect.DeepEqual(out["lambda_function_associations"], in["lambda_function_associations"]) != true {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a feeling this is passing because reflect.DeepEqual is comparing nil to nil. The singular version is used in defaultCacheBehaviorConf, so the plural lambda_function_associations doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, I'll investigate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in fd3f4bb

@catsby catsby merged commit 7c5b3a5 into master Jan 20, 2017
@catsby catsby deleted the pr-10985 branch January 20, 2017 22:34
@catsby catsby mentioned this pull request Jan 23, 2017
@ghost
Copy link

ghost commented Apr 17, 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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants