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, Help]r/lb_listener: Support authenticate action for lb listener #4722

Closed

Conversation

atsushi-ishibashi
Copy link
Contributor

@atsushi-ishibashi atsushi-ishibashi commented Jun 1, 2018

closed: #4707

  • testacc
  • docs

Changes proposed in this pull request:

  • aws_lb_listener supports authenticate action

@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Jun 1, 2018
@ghost ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Jun 2, 2018
@atsushi-ishibashi
Copy link
Contributor Author

atsushi-ishibashi commented Jun 2, 2018

TF_ACC=1 go test ./aws -v -run=TestAccAWSLBListener_cognito -timeout 120m
=== RUN   TestAccAWSLBListener_cognito
--- FAIL: TestAccAWSLBListener_cognito (214.14s)
	testing.go:518: Step 0 error: Error applying: 1 error(s) occurred:
		
		* aws_lb_listener.test: 1 error(s) occurred:
		
		* aws_lb_listener.test: Invalid address to set: []string{"default_action", "0", "authenticate_cognito_config"}
	testing.go:579: Error destroying resource! WARNING: Dangling resources
		may exist. The full state and error is shown below.
		
		Error: Error refreshing: 1 error(s) occurred:
		
		* aws_lb_listener.test: 1 error(s) occurred:
		
		* aws_lb_listener.test: aws_lb_listener.test: Invalid address to set: []string{"default_action", "0", "authenticate_cognito_config"}
		
		State: <nil>
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	214.173s
make: *** [testacc] Error 1

I got the above error. The below is the return of flattenElbActions(listener.DefaultActions)

[]interface {}{map[string]interface {}{"order":1, "type":"authenticate-cognito", "authenticate_cognito_config":[]map[string]interface {}{map[string]interface {}{"session_cookie_name":"AWSELBAuthSessionCookie", "session_timeout":604800, "user_pool_arn":"arn:aws:cognito-idp:us-west-2:hogehoge:userpool/us-west-2_nxECAEbsn", "user_pool_client":"3ri67ugsiniu79j3v5tk58ttlh", "user_pool_domain":"e67po", "on_unauthenticated_request":"authenticate", "scope":"openid"}}}, map[string]interface {}{"order":2, "type":"forward", "target_group_arn":"arn:aws:elasticloadbalancing:us-west-2:hogehoge:targetgroup/e67po/369f21393b0ae80b"}}

I couldn't find the reason😅 Please correct me 🙇

@bflad bflad added enhancement Requests to existing resources that expand the functionality or scope. service/elbv2 Issues and PRs that pertain to the elbv2 service. labels Jun 2, 2018
@atsushi-ishibashi atsushi-ishibashi changed the title [WIP]r/lb_listener: Support authenticate action for lb listener [WIP, Help]r/lb_listener: Support authenticate action for lb listener Jun 5, 2018
@mikael-lindstrom
Copy link
Contributor

mikael-lindstrom commented Jul 24, 2018

I tried to figure out this problem and it seems to be that flattenELbAuthenticateCognitoActionConfig returns []map[string]interface{} when it expects schema.Set. Since I didn't manage to figure out how to fix this in a nice way and since it has MaxItems: 1 it can be converted to schema.TypeList instead. I tested this and it seems to work fine but I found another issue.

It seems that although DescribeListeners returns actions in a consistent order, the order they are saved in AWS is seemingly random. Since default_action currently is schema.TypeList this sometimes causes diffs when running apply multiple times. It could probably be solved by changing default_action to schema.Set but that would mean we need to handle state migration to be backwards compatible. Instead sorting the actions from DescribeListeners based on the order saved in the state seems easier.

For oidc I found that client_secret isn't returned from the API since its a secret, so to prevent this from showing as modified every apply we need to use whatever is already stored in the state.

I have a working version that I'm using now and have pushed the code here with added a acceptance test for odic. Also moved out some functions to structure.go since they can be shared with lb_listener_rule. Let me know if this works for you.

@mikael-lindstrom
Copy link
Contributor

@atsushi-ishibashi would you mind taking a look at this and let me know what you think?

@mikael-lindstrom
Copy link
Contributor

@bflad Is there anything I can do to speed this up? If you like I can open a new PR with my changes if @atsushi-ishibashi is busy.

@bflad
Copy link
Contributor

bflad commented Oct 9, 2018

This PR has been superseded by #6094, which was just merged and will release with version 1.40.0 of the AWS provider, likely middle of this week. Thanks so much to @atsushi-ishibashi and @mikael-lindstrom for their huge efforts here!

@bflad bflad closed this Oct 9, 2018
@ghost
Copy link

ghost commented Apr 3, 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 3, 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/elbv2 Issues and PRs that pertain to the elbv2 service. size/XL Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support authenticate-oidc and authenticate-cognito for aws_lb_listener
3 participants