resource/lb_listener_rule: make priority optional #3219

loivis · 2018-01-31T22:33:47Z

Implement #1574 .

We've been using a script to calculate available priority when registering a target group to listener. Initially it was just a workaround. But as we all know, workaround usually lies there forever as solution.

The idea in #1574 to make priority optional sounds great to me and drives to the implementation.

loivis · 2018-01-31T22:43:51Z

⎇  make testacc TEST=./aws TESTARGS='-run=TestAccAWSLBListenerRule_autoPriority'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSLBListenerRule_autoPriority -timeout 120m
=== RUN   TestAccAWSLBListenerRule_autoPriority
--- PASS: TestAccAWSLBListenerRule_autoPriority (466.43s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	466.477s

loivis · 2018-01-31T23:02:41Z

Making it optional doesn’t work well if registering multiple target groups to the same listener at the same time. In this case, it’s hard to guarantee unique value for each target group. Maybe setting a dependency chain across target groups, like a->b->c->d?

tomelliff · 2018-02-01T08:36:14Z

Does it not make more sense to expose the highest listener priority on the ALB data source instead? That way you could easily get the highest and then increment from there so if you have multiple target groups to register you just keep increasing the increment on each target group?

loivis · 2018-02-01T09:12:19Z

@tomelliff Indeed and that's exactly the workaround that we're using currently, except that we don't fall into the scenario to register multiple target groups at the same time. I just imaged the scenario someone might use.

I created this PR simply based on the votes from #1574, 7 for optional priority and 3 for expose priority

Personally, making priority optional fits our needs perfectly because in our pipeline we register only one target group for each service.

Personally, I would also like to implement the other approach to expose next available priority from data source lb_listener, as you mentioned, as what our workaround is doing now. Actually I was half on the way but chose to finish this PR first. With both implemented, users will have more freedom to choose either that fits their needs. @bflad What do you think from higher level? I'm not sure if we prefer more flexibility with both or just choose one way to go?

tomelliff · 2018-02-01T09:28:32Z

This works for me so I'm happy with this approach but I wonder if it's not confusing for those who do need to register multiple listeners at the same time. Although I guess you'll get an error if you do attempt to register multiple listeners this way and then hopefully people will realise.

loivis · 2018-02-01T09:33:07Z

Maybe a few more words to document will help those who read document:smile:

bflad · 2018-02-14T19:14:17Z

Seems like if we wanted to handle parallelism here, we could make priority optional by:

If priority configured: setting the priority and failing normally if unable to create rule
If priority is not configured: grabbing highest priority as a starting point and continually retrying priority + 1 on API "duplicate rule priority" error up to the max priority if there is one

loivis · 2018-02-14T20:00:44Z

Thanks for pointing out the direction, @bflad . I'll take a look existing code with retryable error. I saw such codes a few times but never thought to use it before.

…iority

loivis · 2018-02-18T16:33:38Z

Added retry logic as suggusted, together with acceptance test to register 100 rules to the same listener.

bflad

@loivis can you see my comments below and let me know if you have any questions? Thanks!

bflad · 2018-02-19T18:33:32Z

aws/resource_aws_lb_listener_rule.go

+		err := resource.Retry(5*time.Minute, func() *resource.RetryError {
+			var err error
+			lastPriority, _, err := getListenerRulePriority(elbconn, listenerArn)
+			log.Println(lastPriority)


This should be log.Printf("[DEBUG] Found listener highest priority: %d", lastPriority) or be removed

forgot to remove:sweat_smile:

bflad · 2018-02-19T18:35:21Z

aws/resource_aws_lb_listener_rule.go

+			params.Priority = aws.Int64(lastPriority + 1)
+			resp, err = elbconn.CreateRule(params)
+			if err != nil {
+				if ec2err, ok := err.(awserr.Error); ok && ec2err.Code() == "PriorityInUse" {


Nitpick: This can be simplified and use the SDK constant with isAWSErr(err, elbv2.ErrCodePriorityInUseException, "")

nice...good to know

bflad · 2018-02-19T18:42:19Z

aws/resource_aws_lb_listener_rule.go

@@ -312,3 +342,45 @@ func isRuleNotFound(err error) bool {
 	elberr, ok := err.(awserr.Error)
 	return ok && elberr.Code() == "RuleNotFound"
 }
+
+func getListenerRulePriority(conn *elbv2.ELBV2, arn string) (last, next int64, err error) {


We should probably update the signature here to make it clear what we're trying to retrieve and remove the next output since we do not need it with the current implementation, e.g.

func highestListenerRulePriority(conn *elbv2.ELBV2, arn string) (priority int64, err error) {

Yes, of course. It was the initial implementation but later synced with #3374

bflad · 2018-02-19T18:52:40Z

website/docs/r/lb_listener_rule.html.markdown

@@ -61,7 +61,7 @@ resource "aws_lb_listener_rule" "host_based_routing" {
 The following arguments are supported:

 * `listener_arn` - (Required, Forces New Resource) The ARN of the listener to which to attach the rule.
-* `priority` - (Required) The priority for the rule between `1` and `50000`. A listener can't have multiple rules with the same priority.
+* `priority` - (Optional) The priority for the rule between `1` and `50000`. Leaving it unset will automatically set the rule with the lighest available priority. A listener can't have multiple rules with the same priority.


Typo 🕶 : lighest available priority should likely be next available priority after currently existing rules

bflad · 2018-02-19T18:58:15Z

aws/resource_aws_lb_listener_rule_test.go

@@ -191,6 +191,56 @@ func TestAccAWSLBListenerRule_multipleConditionThrowsError(t *testing.T) {
 	})
 }

+func TestAccAWSLBListenerRule_autoPriority(t *testing.T) {


Two things:

This test name is currently a little misleading because its testing both automatic and static priorities. I would simply rename it to _priority

This test should check for expected errors too

Trying to set a rule with automatic priority 50001 (we should expect the invalid parameter exception, to ease this, we can have an existing highest priority rule with 50000)

Trying to set a second rule with the same priority as another (we should expect the priority in use exception)

I didn't mean to test static priority here since it's already tested. I was testing to switch between explicit priority and auto priority without any problem. I agree to simplify the name to _priority then include more error checks here.

bflad · 2018-02-19T19:01:56Z

aws/resource_aws_lb_listener_rule_test.go

+  }
+}
+
+resource "aws_lb_listener_rule" "third" {


We test a static rule priority with the _autoPriorityStatic configuration below. We should either just use this rule here and remove the static configuration below or remove this rule here.

It was the leftover when I tried to use the gap priority in the beginning. I think it's still useful here as test case of existing priorities with gap in between. And as explained above, static priority here is not intended to test static priority but to test switching priority argument.

Valid point 👍

bflad · 2018-02-19T19:03:04Z

aws/resource_aws_lb_listener_rule_test.go

+func testAccAWSLBListenerRuleConfig_autoPriority100Rules(lbName, targetGroupName string) string {
+	return testAccAWSLBListenerRuleConfig_autoPriorityStatic(lbName, targetGroupName) + fmt.Sprintf(`
+resource "aws_lb_listener_rule" "limit" {
+  count = 97


Can we make this match the test name or vice versa please?

Sorry I'm not sure if I get your point. Did you mean either count = 100 or testAccAWSLBListenerRuleConfig_autoPriority97Rules? If that's the case, maybe change to testAccAWSLBListenerRuleConfig_autoPriorityRuleNumberLimit? The intension was to fill the listener with 100 rules limit per load balancer and there was already 3 existing rules.

Okay I wasn't immediately aware of the rules per listener limit.

If we're just trying to test concurrency of auto-assigning priorities (I think more important here), let's just do 10, which should always pass.

If we're trying to test hitting the max rules per listener limit (I do not think its important to test one below the limit), that should be a separate test of 101 rules and ExpectError. AWS could change that limit and it makes it hard to understand why we're testing 3 + 97 rules here without making the test name more explicit like you suggested with testAccAWSLBListenerRuleConfig_autoPriorityRuleNumberLimit.

I'll do both then, testing for 10 and 101.

loivis · 2018-02-19T22:09:36Z

⎇  make testacc TEST=./aws TESTARGS='-run=TestAccAWSLBListenerRule_priority'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSLBListenerRule_priority -timeout 120m
=== RUN   TestAccAWSLBListenerRule_priority
--- PASS: TestAccAWSLBListenerRule_priority (482.18s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	482.236s

loivis · 2018-02-23T12:17:18Z

@bflad anything else to change before approve and merge?

bflad

Thanks so much for this contribution and working with me, @loivis! Let's get this in! 🚀

=== RUN   TestAccAWSLBListenerRule_multipleConditionThrowsError
--- PASS: TestAccAWSLBListenerRule_multipleConditionThrowsError (1.08s)
=== RUN   TestAccAWSLBListenerRule_basic
--- PASS: TestAccAWSLBListenerRule_basic (201.94s)
=== RUN   TestAccAWSLBListenerRule_changeListenerRuleArnForcesNew
--- PASS: TestAccAWSLBListenerRule_changeListenerRuleArnForcesNew (207.16s)
=== RUN   TestAccAWSLBListenerRule_updateRulePriority
--- PASS: TestAccAWSLBListenerRule_updateRulePriority (213.72s)
=== RUN   TestAccAWSLBListenerRule_priority
--- PASS: TestAccAWSLBListenerRule_priority (397.28s)

bflad · 2018-02-27T12:58:12Z

This has been released in version 1.10.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-07T17:46:35Z

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/M Managed by automation to categorize the size of a PR. size/L Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Jan 31, 2018

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 Feb 1, 2018

tomelliff mentioned this pull request Feb 1, 2018

Find LB listener by the LB ARN #2110

Closed

loivis mentioned this pull request Feb 14, 2018

data-source/lb_listener: export rule priorities for lb listener #3374

Closed

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

xiaowei.wang added 5 commits February 18, 2018 10:07

resource/lb_listener_rule: make priority optional

9f44764

resource/lb_listener_rule: add acceptance tests for optional priority

8eb4773

resource/lb_listener_rule: update document for optional priority

626a8dc

resource/lb_listener_rule: merge if conditions when fetching priority

1a0ad7d

resource/lb_listener_rule: sync function with pr hashicorp#3374

520f40f

loivis force-pushed the 1574-lb-listener-rule-priority-optional branch from bed33c9 to 520f40f Compare February 18, 2018 09:08

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

resource/lb_listener_rule: try to handle parallelism with optional pr…

edcfea4

…iority

bflad reviewed Feb 19, 2018

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Feb 19, 2018

resource/lb_listener_rule: code change as per review

4a0b27d

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

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Feb 24, 2018

bflad approved these changes Feb 24, 2018

View reviewed changes

bflad added this to the v1.10.0 milestone Feb 24, 2018

bflad merged commit 1ba6ec4 into hashicorp:master Feb 24, 2018

bflad added a commit that referenced this pull request Feb 24, 2018

Update CHANGELOG for #3219

77bf3e6

loivis deleted the 1574-lb-listener-rule-priority-optional branch March 1, 2018 08:48

bflad mentioned this pull request Jun 12, 2018

ALB Listener Priority #4813

Closed

pecigonzalo mentioned this pull request Jun 26, 2018

r/aws_lb_listener_rule: Support 0 as priority, to explicitly set to "dynamic" #4985

Closed

ghost locked and limited conversation to collaborators Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/lb_listener_rule: make priority optional #3219

resource/lb_listener_rule: make priority optional #3219

loivis commented Jan 31, 2018 •

edited

Loading

loivis commented Jan 31, 2018

loivis commented Jan 31, 2018

tomelliff commented Feb 1, 2018

loivis commented Feb 1, 2018

tomelliff commented Feb 1, 2018

loivis commented Feb 1, 2018

bflad commented Feb 14, 2018

loivis commented Feb 14, 2018

loivis commented Feb 18, 2018

bflad left a comment

bflad Feb 19, 2018

loivis Feb 19, 2018

bflad Feb 19, 2018

loivis Feb 19, 2018

bflad Feb 19, 2018

loivis Feb 19, 2018

bflad Feb 19, 2018

bflad Feb 19, 2018

loivis Feb 19, 2018

bflad Feb 19, 2018

loivis Feb 19, 2018

bflad Feb 19, 2018

bflad Feb 19, 2018

loivis Feb 19, 2018

bflad Feb 19, 2018

loivis Feb 19, 2018

loivis commented Feb 19, 2018

loivis commented Feb 23, 2018

bflad left a comment

bflad commented Feb 27, 2018

ghost commented Apr 7, 2020

resource/lb_listener_rule: make priority optional #3219

resource/lb_listener_rule: make priority optional #3219

Conversation

loivis commented Jan 31, 2018 • edited Loading

loivis commented Jan 31, 2018

loivis commented Jan 31, 2018

tomelliff commented Feb 1, 2018

loivis commented Feb 1, 2018

tomelliff commented Feb 1, 2018

loivis commented Feb 1, 2018

bflad commented Feb 14, 2018

loivis commented Feb 14, 2018

loivis commented Feb 18, 2018

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

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

loivis commented Feb 19, 2018

loivis commented Feb 23, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad commented Feb 27, 2018

ghost commented Apr 7, 2020

loivis commented Jan 31, 2018 •

edited

Loading