Updates needed to support Terraform TLS resources #259
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Custom TLS certificate without names default to the certificates common name or first SAN entry
This was causing problems when trying to use the page[number] parameter so I brought it more in line with the other methods.
Activations without the configuration specified default to the default configuration
* Add ListTLSSubscriptions function * Add CreateTLSSubscriptions function * Add GetTLSSubscriptions function * Add DeleteTLSSubscriptions function * Add integration test with all endpoints
I had a lot of trouble deserialising the API response into appropriate structs. Initially I could only get it to work by deserialising into a map[string]interface{} but this didn't give the rich typing I wanted to return to the user. After a long time digging through the JSONAPI implementation and an associated PR to allow nested structs (google/jsonapi#99), I found that the go-fastly dependency was on an older version of the library before this PR came in. Updating the version, and playing around some more with the slice of structs instead of slice of pointers, finally got this to work.
Furthermore I found that the Header in ListTLSSubscriptions needed to include the "Accept" header for "application/vnd.api+json" so I added it back in. I previously removed it as it didn't seem to do anything with the Filter and Pagination parameters, but it seems to be required for Include to work.
The certificate can't really be queried in the same way as the custom TLS certificate, but the ID will be present in the TLS activation automatically created for the TLS subscription, so can help filter for this activation.
bengesoff
force-pushed
the
tls-fixes-for-tf
branch
from
9ccb395
to
2732f6b
Compare
Allows deleting a subscription with active domains; a potentially dangerous operation if the subscription handles production traffic. By using a flag, the user opts in to this risky behaviour and takes responsibility for knowing what they are doing.
Integralist
reviewed
Feb 10, 2021
There was a problem hiding this comment.
Thanks @bengesoff for your work on this, it's looking great!
Just a couple of minor comments for your consideration.
| ID string `jsonapi:"primary,tls_certificate"` | ||
| } | ||
|
|
||
| type TLSAuthorizations struct { |
There was a problem hiding this comment.
Am I right in thinking this isn't documented yet (along with TLSChallenge)?
There was a problem hiding this comment.
I think so, although I can add some comments regardless if you think that'd be helpful?
There was a problem hiding this comment.
I've added a comment to this and TLSChallenge -- no official docs to go off of but hopefully it gives an idea what it does 👍🏼
Integralist
approved these changes
Feb 15, 2021
There was a problem hiding this comment.
Thanks @bengesoff this LGTM, with just one minor request to add a missing piece of documentation for TLSAuthorizations (I replied to your comment from the last review).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There were a mix of tweaks needed while developing the TLS Terraform resources. These range from changes to existing input structs and methods, to some new types and functions for the previously unimplemented functionality.
PATCHendpoint (coming separately)allow_untrusted_rootoption to Bulk Certificate creation to allow self-signed CAs to be usedjsonapidependency. I think the previous version was 3 years old and there had been some important changes to allow nested objects in the attributes. This was needed to support parsing Authorization Challenges in the TLS Subscriptions response