r/aws_lb_listener: add support for additional SSL certificates #2203
Conversation
The listener's default certificate is included in the DecribeListenerCertificates API call which does not need to be tracked as an additional certificate.
|
Documentation written. I can also confirm that the acceptance suite for this resource passes. Currently writing the updated tests for this argument now. |
oarmstrong
changed the title
|
That's just added the tests. I've also modified the corresponding data source, not sure if you'd prefer that in a separate PR though. |
oarmstrong
changed the title
radeksimko
added
the
enhancement
|
It may make more sense to rename the original |
radeksimko
added
the
size/L
|
Thanks to raise this PR to add this feature. When will it be merged? Regarding argument naming, I am fine for |
It's good to go from me. Just awaiting feedback from the maintainers. |
|
Seems we missed the release in 0.11, hope to get it merged in next release. Wait, it is not in list of next release. Who can help to add your PR in next release list? |
|
@ozbillwang Once, the PR is merged, the maintainers update the CHANGELOG |
|
Is there anyone who knows when to merge? |
|
I'm still not 100% with it and assume it will need some polish. Just want a bit of a review at this point. |
|
@radeksimko @Ninir any idea when this will be merged :) |
|
I'm able to get on any review points pretty much immediately! |
There was a problem hiding this comment.
Hi @oarmstrong
thanks for the PR and sorry for slightly delayed review.
One major question aside from my inline comments: Since the certificates have their own API and default certificate is easily identified I'm wondering if this is better modeled as a separate resource, i.e. aws_lb_listener_certificate with listener_arn being a required field?
Based on past experience we usually prefer more granular resources rather than monoliths for various reasons:
- we can parallelise management of those resources
- users can leverage
countand other interpolation goodies - It is
much more difficultimpossible to manage the same thing in multiple places - so if we decide to have a separate resource in addition to a listener field in the future, the user would need to pick one otherwise we wouldn't be able to figure out which one is the point of truth.aws_security_group.ingressandaws_security_group_ruleis a great (sad) example of this confusing state.
Let me know what you think.
The code looks otherwise good.
| ListenerArn: aws.String(larn), | ||
| }) | ||
| if awsErr, ok := err.(awserr.Error); ok { | ||
| if awsErr.Code() == "TooManyCertificates" || awsErr.Code() == "CertificateNotFound" { |
There was a problem hiding this comment.
What is the reason for retrying on TooManyCertificates here? 🤔
There was a problem hiding this comment.
Only as I essentially copied CertificateNotFound. Neither makes sense to retry on IMO.
| }) | ||
|
|
||
| if err != nil { | ||
| return errwrap.Wrapf("Error adding certificates to LB Listener: {{err}}", err) |
There was a problem hiding this comment.
errwrap is typically useful in situations where we need to keep track of the error type. In this case error will only ever be escalated to the UI where all that matters is strings ("stringifyed" error message), not the original type.
In other words fmt.Errorf() will do the job, IMO.
There was a problem hiding this comment.
Right. I'm with you. Comment noted for all instances.
| ListenerArn: aws.String(d.Id()), | ||
| }) | ||
| if err != nil { | ||
| return errwrap.Wrapf("Error retrieving ListenerCertficates: {{err}}", err) |
There was a problem hiding this comment.
errwrap is typically useful in situations where we need to keep track of the error type. In this case error will only ever be escalated to the UI where all that matters is strings ("stringifyed" error message), not the original type.
In other words fmt.Errorf() will do the job, IMO.
| ListenerArn: aws.String(d.Id()), | ||
| }) | ||
| if err != nil { | ||
| return errwrap.Wrapf("Error retrieving ListenerCertficates: {{err}}", err) |
There was a problem hiding this comment.
errwrap is typically useful in situations where we need to keep track of the error type. In this case error will only ever be escalated to the UI where all that matters is strings ("stringifyed" error message), not the original type.
In other words fmt.Errorf() will do the job, IMO.
| ListenerArn: aws.String(d.Id()), | ||
| }) | ||
| if err != nil { | ||
| return errwrap.Wrapf("Error modifying LB Listener: {{err}}", err) |
There was a problem hiding this comment.
errwrap is typically useful in situations where we need to keep track of the error type. In this case error will only ever be escalated to the UI where all that matters is strings ("stringifyed" error message), not the original type.
In other words fmt.Errorf() will do the job, IMO.
| ListenerArn: aws.String(d.Id()), | ||
| }) | ||
| if err != nil { | ||
| return errwrap.Wrapf("Error modifying LB Listener: {{err}}", err) |
There was a problem hiding this comment.
errwrap is typically useful in situations where we need to keep track of the error type. In this case error will only ever be escalated to the UI where all that matters is strings ("stringifyed" error message), not the original type.
In other words fmt.Errorf() will do the job, IMO.
radeksimko
added
the
waiting-response
|
Thanks for the review. I'll get on to it a little later. |
|
So, the reason for this resource. Basically I was attempting to get this done quickly and simply so editing an existing resource felt much easier for me. It was noted in the issue that a separate resource would be useful too and I agreed 100%. I wasn't aware of how impossible it was to have this resource as well as a separate resource (e.g. the security group case). It's probably best to do it right from the outset then and just abandon this attempt and go straight for the new resource. If that's your opinion for the "correct" implementation (and I tend to agree with that too). I can take a crack at the new resource but I may need guidance. Would you prefer to close this PR and for myself to start a new one when I have a first attempt? I'll reuse a lot of the code from this attempt though as it seems to have passed your initial review. Thanks for your time! /cc @radeksimko |
Sure, feel free to ask. Just FYI I will be mostly unavailable from the end of this week until the EOY.
Yep, the code looked good - you can reuse most of it and yes it's probably best to close this PR. If you need some guidance, take a look at any merged |
|
Cheers. I'll close this PR and open a new one with the revised method. |
|
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
locked and limited conversation to collaborators
breathingdust
removed
the
waiting-response
This PR is for #1853.
We add an additional argument (
additional_certificate_arns) to theaws_lb_listenerresource. This argument allows for adding additional non-default certificates to a listener.I'm well aware that there are currently no tests or documentation and that still needs to be done. It has been manually tested with as many cases as I could think of though. The existing test suite still runs, but I have not run the acceptance tests at all.
If at all possible, some feedback on the general style of my code would be great. Variable naming and error checking are two points that I've been particularly bad at in the past. I'm also not sure about defining https://github.com/terraform-providers/terraform-provider-aws/pull/2203/files#diff-6f5ff88b797d88c79786bcdb04a23eefR314 in line like that. Do we have an existing function somewhere that does the same job or should I move this elsewhere?
Another suggestion in the linked issue is for it to be possible to define additional certificates as a separate resource - must like security group rules. I'm not too sure if this design would allow for that?
As a final question, how would you like the commits before this can be merged in? Squished? Some kind of prefix to the messages?
Appreciate your time. Hopefully there isn't anything too major wrong with my code and I can get the documentation and tests done quickly too.