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.

Sign up for GitHub

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

Jump to bottom

Add authentication options to aws_lb_listener and aws_lb_listener_rule #6094

Merged
merged 1 commit into from
Oct 9, 2018
Merged

Add authentication options to aws_lb_listener and aws_lb_listener_rule #6094

merged 1 commit into from
Oct 9, 2018

Conversation

mikael-lindstrom
Copy link
Contributor

Fixes #4707 and #4711

This is based on #4722 and #5336 which is refactored since the changes for fixed_reponse and redirect was added.

Changes proposed in this pull request:

  • Add support for Cognito and OIDC authentication to aws_lb_listener.
  • Add support for Cognito and OIDC authentication to aws_lb_listener_rule.

Documentation updated and acceptance tests added.

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSLBListener_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSLBListener_ -timeout 120m
=== RUN   TestAccAWSLBListener_basic
--- PASS: TestAccAWSLBListener_basic (257.07s)
=== RUN   TestAccAWSLBListener_https
--- PASS: TestAccAWSLBListener_https (245.05s)
=== RUN   TestAccAWSLBListener_redirect
--- PASS: TestAccAWSLBListener_redirect (288.57s)
=== RUN   TestAccAWSLBListener_fixedResponse
--- PASS: TestAccAWSLBListener_fixedResponse (283.59s)
=== RUN   TestAccAWSLBListener_cognito
--- PASS: TestAccAWSLBListener_cognito (238.11s)
=== RUN   TestAccAWSLBListener_oidc
--- PASS: TestAccAWSLBListener_oidc (256.54s)
PASS

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSLBListenerRule_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSLBListenerRule_ -timeout 120m
=== RUN   TestAccAWSLBListenerRule_basic
--- PASS: TestAccAWSLBListenerRule_basic (295.23s)
=== RUN   TestAccAWSLBListenerRule_redirect
--- PASS: TestAccAWSLBListenerRule_redirect (230.97s)
=== RUN   TestAccAWSLBListenerRule_fixedResponse
--- PASS: TestAccAWSLBListenerRule_fixedResponse (237.44s)
=== RUN   TestAccAWSLBListenerRule_updateRulePriority
--- PASS: TestAccAWSLBListenerRule_updateRulePriority (286.20s)
=== RUN   TestAccAWSLBListenerRule_changeListenerRuleArnForcesNew
--- PASS: TestAccAWSLBListenerRule_changeListenerRuleArnForcesNew (297.66s)
=== RUN   TestAccAWSLBListenerRule_multipleConditionThrowsError
--- PASS: TestAccAWSLBListenerRule_multipleConditionThrowsError (2.03s)
=== RUN   TestAccAWSLBListenerRule_priority
--- PASS: TestAccAWSLBListenerRule_priority (555.70s)
=== RUN   TestAccAWSLBListenerRule_cognito
--- PASS: TestAccAWSLBListenerRule_cognito (232.10s)
=== RUN   TestAccAWSLBListenerRule_oidc
--- PASS: TestAccAWSLBListenerRule_oidc (244.30s)
PASS

@ghost ghost added size/XXL Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Oct 8, 2018
@mikael-lindstrom mikael-lindstrom mentioned this pull request Oct 8, 2018
Closed
@bflad bflad added the service/elbv2 Issues and PRs that pertain to the elbv2 service. label Oct 8, 2018
@bflad bflad self-assigned this Oct 8, 2018
bflad
bflad approved these changes Oct 9, 2018
View reviewed changes
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.

I left some reservations below, but none of them are blockers for merge. Overall this functionality is a huge win to get in! Thanks, @mikael-lindstrom and @atsushi-ishibashi! 🚀

--- PASS: TestAccAWSLBListenerRule_multipleConditionThrowsError (1.03s)
--- PASS: TestAccAWSLBListener_https (173.29s)
--- PASS: TestAccAWSLBListenerRule_oidc (185.61s)
--- PASS: TestAccAWSLBListener_basic (189.27s)
--- PASS: TestAccAWSLBListenerRule_redirect (193.78s)
--- PASS: TestAccAWSLBListenerRule_fixedResponse (194.34s)
--- PASS: TestAccAWSLBListenerRule_basic (201.07s)
--- PASS: TestAccAWSLBListenerRule_updateRulePriority (206.48s)
--- PASS: TestAccAWSLBListenerRule_changeListenerRuleArnForcesNew (206.70s)
--- PASS: TestAccAWSLBListenerRule_cognito (233.09s)
--- PASS: TestAccAWSLBListenerRule_priority (250.97s)
--- PASS: TestAccAWSLBListener_redirect (178.58s)
--- PASS: TestAccAWSLBListener_cognito (200.54s)
--- PASS: TestAccAWSLBListener_oidc (200.81s)
--- PASS: TestAccAWSLBListener_fixedResponse (208.96s)

aws/resource_aws_lb_listener.go
"on_unauthenticated_request": {
Type: schema.TypeString,
Optional: true,
Computed: 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 some concerns that we will see some bug reports about detecting drift for a few of these attributes marked as Computed: true, but I think getting the initial functionality merged is more important.

aws/resource_aws_lb_listener.go
action.Order = aws.Int64(int64(order.(int)))
}
if len(defaultActions) != 1 && action.Order == nil {
return errors.New("when using more then one action, you need to specify 'order' for each action")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think at some point in the future we may be able to base Order on the ordering within the Terraform configuration, but that is certainly not an enhancement blocking this from merge. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds like a great enhancement, might look into this later 👍

aws/structure.go
@@ -1907,6 +1908,20 @@ func sortListBasedonTFFile(in []string, d *schema.ResourceData, listName string)
return in, fmt.Errorf("Could not find list: %s", listName)
}

// This function sorts LB Actions to look like whats found in the tf file
func sortActionsBasedonTypeinTFFile(actionName string, actions []*elbv2.Action, d *schema.ResourceData) []*elbv2.Action {
Copy link
Contributor

Choose a reason for hiding this comment

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

This sorting logic feels a little awkward, but if it does the job for now, I'm okay with it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'm not 100% happy about it but it does the job according to the testing I did. Hopefully we can refactor this part at a later stage to be a bit cleaner 👍

@bflad bflad added this to the v1.40.0 milestone Oct 9, 2018
@bflad bflad merged commit 38f7331 into hashicorp:master Oct 9, 2018
@bflad bflad mentioned this pull request Oct 9, 2018
Closed
2 tasks
bflad added a commit that referenced this pull request Oct 9, 2018
@bflad
Update CHANGELOG for #6094
2ae7c6d
@fotos fotos mentioned this pull request Oct 9, 2018
Merged
@bflad
Copy link
Contributor

bflad commented Oct 10, 2018

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

@devonbleak devonbleak mentioned this pull request Oct 10, 2018
Closed
@ghost
Copy link

ghost commented Apr 2, 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 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bflad bflad bflad approved these changes

Assignees

@bflad bflad

Labels
documentation Introduces or discusses updates to documentation. service/elbv2 Issues and PRs that pertain to the elbv2 service. size/XXL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Milestone
v1.40.0
Development

Successfully merging this pull request may close these issues.

Support authenticate-oidc and authenticate-cognito for aws_lb_listener
2 participants
@mikael-lindstrom @bflad