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 creating external resources associated with self-managed certificates #118

Merged

Conversation

impl
Copy link
Contributor

@impl impl commented Apr 2, 2022

Description

This change adds attributes to auth0_custom_domain and auth0_custom_domain_verification to make it possible to seamlessly manage all the additional infrastructure associated with self-managed certificates. I also added a complete example of setting up a custom domain using self-managed certs with GCP.

Dependencies:

For testing purposes I updated the go.mod for this branch to point to our go-auth0 fork. When the dependency lands, I'll update this PR to remove the temporary replace directive. (Please don't merge until then! :-) Done!

Checklist

Note: Checklist required to be completed before a PR is considered to be reviewable.

Auth0 Code of Conduct

Auth0 General Contribution Guidelines

Changes include test coverage?

  • Yes
  • Not needed

Does the description provide the correct amount of context?

  • Yes, the description provides enough context for the reviewer to understand what these changes accomplish

Have you updated the documentation?

  • Yes, I've updated the appropriate docs
  • Not needed

Is this code ready for production?

  • Yes, all code changes are intentional and no debugging calls are left over

@impl impl requested a review from a team as a code owner April 2, 2022 05:00
@sergiught sergiught self-assigned this Apr 4, 2022
@impl impl force-pushed the custom-domain-self-managed-certificates branch from 0811a1b to c3898c9 Compare April 8, 2022 06:52
Copy link
Contributor

@sergiught sergiught left a comment

Choose a reason for hiding this comment

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

Solid contribution @impl as always. There's just one thing I'd like us to add.

Considering the custom domain verification always relies on a 3rd party (in your examples you introduced the google provider) we introduced a wiremock server to be able to run the tests for domain verification. You can spin it up from the root of the project using docker compose or running the make dev-up command.

The response/request mappings are found here: https://github.com/auth0/terraform-provider-auth0/blob/main/dockerfiles/wiremock/__files/custom_domain_with_pending_verification.json.

Would you be able to add an assertion for the 2 properties we introduced on the domain verification tests and update the request/response payloads as well to be able to parse them? 🙏🏻

@impl
Copy link
Contributor Author

impl commented Apr 8, 2022

Would you be able to add an assertion for the 2 properties we introduced on the domain verification tests and update the request/response payloads as well to be able to parse them? 🙏🏻

Aha, wonderful! I was wondering how the heck those tests worked. :-) I'll update them and push up a new commit. Thanks for the thorough review!

@sergiught
Copy link
Contributor

@impl Let me know if you need any help with that, as we haven't really documented well that part in our contributing section. I'll update that next week for other folks as well to give better clarity. In the future we're aiming at giving the possibility for forks to run all the tests without requiring a real test tenant (and they'll run automatically on fork PRs), but having all requests / responses mocked. The actual Auth0 Management API should get hit only once we merge to master just to rule out any potential miss-alignments between the mocks. All of this will require some work from our end tho so I don't have an ETA when it will be available in full. For now we just use mocks for the domain verification.

@impl
Copy link
Contributor Author

impl commented Apr 10, 2022

@sergiughf, that sounds like a great plan to me. I've been able to run relevant tests against a tenant I created for acceptance testing, but of course it would also be convenient to not have to do that in some cases. I went ahead and added a test for self-managed certificates and updated some of the Auth0-managed certificates test as well. To make the WireMock files a little less confusing I ended up doing some minor refactoring, which is entirely separate in one commit in case there's anything specific there you'd like changed. Hopefully it will be enough of a bridge until you can get it set up for everything. Let me know!

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@impl impl force-pushed the custom-domain-self-managed-certificates branch from 8fc44bd to 8e18a96 Compare April 11, 2022 19:49
@impl impl force-pushed the custom-domain-self-managed-certificates branch from 8e18a96 to 5f471a6 Compare April 11, 2022 20:34
@impl impl force-pushed the custom-domain-self-managed-certificates branch from 63661af to 8c3db6c Compare April 12, 2022 16:46
Copy link
Contributor

@sergiught sergiught left a comment

Choose a reason for hiding this comment

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

@impl I can't thank you enough for this contribution and also the others. The improvements brought are highly appreciated. As always a pleasure to review and collaborate together. 🌟

@impl
Copy link
Contributor Author

impl commented Apr 12, 2022

Of course, and thank you for the review/improvements and for being so responsive to these PRs! Our Terraform config looks better by the day ;).

@sergiught sergiught merged commit f24d568 into auth0:main Apr 12, 2022
@impl impl deleted the custom-domain-self-managed-certificates branch April 12, 2022 17:03
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.

2 participants