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

Add support for Platform TLS #154

Merged
merged 8 commits into from
Apr 24, 2020
Merged

Conversation

shreddedbacon
Copy link
Contributor

Looking to add support for Platform TLS

For the test process described here, I wasn't sure how to actually do this with Platform TLS.
I don't appear have the ability to create new TLS configuration IDs to have one to use as a dummy, and I am unsure if doing anything here will have any cost implications.

supress lint, fix some names

format the fixtures body properly according to a real test request

minor fix
shreddedbacon and others added 2 commits April 16, 2020 08:04
Copy link
Member

@phamann phamann left a comment

Choose a reason for hiding this comment

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

Hi @shreddedbacon, thank you for contributing this we really appreciate it.

Apologies for the large amount of requested changes, however you'll see that they all relate to the JSONAPI response format of this API and not problems with your code and should be easy to tweak.

I've only done a partial review of the PrivateKey endpoints and have left the bulk cert methods, as most of my comments apply to both implementations. Let me know once you've had a chance to refactor or have any questions.

Thanks!

@shreddedbacon
Copy link
Contributor Author

Brilliant, thanks. I will work through this and let you know when ready.

@shreddedbacon
Copy link
Contributor Author

Hi @phamann. I've adjusted my code to use the jsonapi style format as requested.
Hopefully I've got everything correct.
Thanks!

@scheying
Copy link

Hey guys! Great to see progress on the TLS API. I have been working on the "Non-Platform" TLS part and from what I see the Private Keys API is the same for "Regular TLS" and "Platform TLS". Would it make sense to give the key-related functions neutral names like GetPrivateKey instead of GetPlatformPrivateKey and move all the key related functionality to a separate file?

If you like I could throw in my implementation of exactly that and a bit of JSONAPI paging and filtering on top. But I also don't want to get in the way of what you already did @shreddedbacon .

@shreddedbacon
Copy link
Contributor Author

Hey guys! Great to see progress on the TLS API. I have been working on the "Non-Platform" TLS part and from what I see the Private Keys API is the same for "Regular TLS" and "Platform TLS". Would it make sense to give the key-related functions neutral names like GetPrivateKey instead of GetPlatformPrivateKey and move all the key related functionality to a separate file?

If you like I could throw in my implementation of exactly that and a bit of JSONAPI paging and filtering on top. But I also don't want to get in the way of what you already did @shreddedbacon .

I didn't know they were different. In that case it makes sense to name them a neutral name, in their own file. I'm happy to adjust my PR as I'm also currently using my branch to build out an operator for our platform.

How far along is your work on "Regular TLS" @scheying ?

Copy link
Member

@phamann phamann left a comment

Choose a reason for hiding this comment

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

Thanks for the speedy changes to this, @shreddedbacon, I appreciate your patience here. It's looking much better! Most of my comments are minor comment style changes, some small copy/pasta errors and a question around using pointers for the pagination input structs.

My last major change request would be to update the tests to be more inline with the style we use in the package. Which would be: one TestClient_PlatformPrivateKey function which creates a key (defers its deletion) then uses that to perform the other operations (list, update, delete), with each operation assert the properties or length returned are valid. We also have seperate functions to test the validations. An example of this style can be found in some of the recent PRs, such as users: https://github.com/fastly/go-fastly/blob/master/fastly/user_test.go#L19-L133

Hope that helps. After this last change set, we should be good to merge!

@phamann
Copy link
Member

phamann commented Apr 23, 2020

Would it make sense to give the key-related functions neutral names like GetPrivateKey instead of GetPlatformPrivateKey and move all the key related functionality to a separate file?

@scheying This is a great question and in theory makes sense to me. I'm afraid that I'm not an internal SME (subject-matter-expert) on our TLS APIs, so will get someone on that team to have a look and share their thoughts here.

@shreddedbacon
Copy link
Contributor Author

Thanks for the speedy changes to this, @shreddedbacon, I appreciate your patience here. It's looking much better! Most of my comments are minor comment style changes, some small copy/pasta errors and a question around using pointers for the pagination input structs.

My last major change request would be to update the tests to be more inline with the style we use in the package. Which would be: one TestClient_PlatformPrivateKey function which creates a key (defers its deletion) then uses that to perform the other operations (list, update, delete), with each operation assert the properties or length returned are valid. We also have seperate functions to test the validations. An example of this style can be found in some of the recent PRs, such as users: https://github.com/fastly/go-fastly/blob/master/fastly/user_test.go#L19-L133

Hope that helps. After this last change set, we should be good to merge!

No problem at all. Thanks for your patience.
I was mainly using existing code to try and find how things were styled, but I guess it has changed over time.

Will adjust again!

…atform-tls, added additional tests to suit test styling and additional description updates
Copy link
Member

@phamann phamann left a comment

Choose a reason for hiding this comment

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

👍 LGTM

Thank you for all of your work and promptness on this PR @shreddedbacon really appreciated. I'll double check that the TLS team are happy, but should get it merged and released ASAP.

@shreddedbacon
Copy link
Contributor Author

Hi again @phamann
I've made adjustments but just want to check with you on something as I couldn't see it on any other Get requests.

For the filters to actually work, I had to add the header

Headers: map[string]string{
  "Accept": "application/vnd.api+json",
}

As described in the API documentation https://docs.fastly.com/api/platform-tls#tls_bulk_certificates_81cc5acbf847f71ecd4068ed58bfc5c5

When I remove this header, the filter no longer works. Is there something I'm missing here? I noticed a few other functions use filters and have the same Accept heading requirement in the API, but yet they haven't had to define the header for this.

@phamann
Copy link
Member

phamann commented Apr 23, 2020

For the filters to actually work, I had to add the header. When I remove this header, the filter no longer works. Is there something I'm missing here?

@shreddedbacon You're not missing anything here. This is another nuance of our JSONAPI API requests and not your implementation. For those requests you are using the client.Get() helper which is intended for plain HTTP requests and has no knowledge about JSONAPI. It would be better if we had a GetJSONAPI() helper as per the other methods PutJSONAPI etc (which I've just realised we don't have...) and that would internally call RequestJSONAPI which sets the correct Accept and Content-Type JSONAPI header values.

@shreddedbacon
Copy link
Contributor Author

Awesome. Thanks for smashing through this with me, appreciate it!

Copy link
Member

@thommahoney thommahoney left a comment

Choose a reason for hiding this comment

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

This is really great; thank you so much for your contribution to this library!

Comment on lines 197 to 198
// `tls_domains.id` filter seems to work where `tls_domain.id` does not, documentation wrong?
// https://docs.fastly.com/api/platform-tls#tls_bulk_certificates_81cc5acbf847f71ecd4068ed58bfc5c5
Copy link
Member

Choose a reason for hiding this comment

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

Both filtering parameters should be supported, with a preference for the plural. I'll follow up with the appropriate teams.

@shreddedbacon
Copy link
Contributor Author

Hi @thommahoney.
Adjustments made 🤞

@phamann
Copy link
Member

phamann commented Apr 24, 2020

Looking good! Once again thank you for your contribution 👍

I'll let you know once released.

@phamann phamann merged commit 2f76148 into fastly:master Apr 24, 2020
@phamann phamann mentioned this pull request Apr 24, 2020
@phamann
Copy link
Member

phamann commented Apr 24, 2020

This was released in v1.10.0 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants