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 managed SSL cert to beta. #1420

Merged
merged 8 commits into from
Mar 1, 2019
Merged

Add managed SSL cert to beta. #1420

merged 8 commits into from
Mar 1, 2019

Conversation

nat-henderson
Copy link
Contributor

All right!

This is an interesting one. SSL certs and Managed SSL Certs are the same resource type in GCP, so this is the first time that we have had two resources that point to the same endpoints.

We think this is the right move. Here's the API object for SSL certs in v1:
image

and beta:

image

You can see that the beta object contains the v1 object as a subset - and that code which operates with the v1 object is interoperable with the beta object, even though the beta object also contains a separate system for distinguishing managed and unmanaged ssl certificates.

We decided that this is confusing, unnecessarily so, and that the right move here is to treat managed and unmanaged ssl certs as different objects which share an api endpoint. We believe this is safe, because according to the deprecation policy, as long as compute continues to support v1, the original api will be usable. This has some side effects - technically, for instance, you could import a managed certificate as a self-managed one, if you wanted to - but they are negligible.

This resource needs to be handled with extreme caution. The setup steps for this resource are complex - and may include out of band changes to DNS host servers. I have drafted a cautionary statement for the docs, but would appreciate additional help in conveying the issue.


[all]

[terraform]

Changes ancillary to adding managed SSL certs.

[terraform-beta]

Add managed SSL certs to beta.

[ansible]

[inspec]

@nat-henderson
Copy link
Contributor Author

Copy link
Member

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

It's probably worth getting @SirGitsalot's opinion on the API deviation. Assuming the test passes, LGTM

products/compute/api.yaml Show resolved Hide resolved
products/compute/terraform.yaml Show resolved Hide resolved
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
This PR seems not to have generated downstream PRs before, as of a6afe63.

Pull request statuses

No diff detected in terraform-provider-google.
No diff detected in Inspec.

New Pull Requests

I built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed.
depends: hashicorp/terraform-provider-google-beta#458
depends: modular-magician/ansible#194

@SirGitsalot
Copy link
Member

It's probably worth getting @SirGitsalot's opinion on the API deviation. Assuming the test passes, LGTM

LGTM

@SirGitsalot
Copy link
Member

Oops wrong button

@SirGitsalot SirGitsalot reopened this Feb 21, 2019
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, f5ceb05.

Pull request statuses

terraform-provider-google-beta already has an open PR.
Ansible already has an open PR.

New Pull Requests

I built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed.
depends: hashicorp/terraform-provider-google#3106
depends: modular-magician/inspec-gcp#117

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, fc6f4cf.

Pull request statuses

terraform-provider-google-beta already has an open PR.
terraform-provider-google already has an open PR.
Ansible already has an open PR.
Ansible already has an open PR.

New Pull Requests

I didn't open any new pull requests because of this PR.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 27161ad.

Pull request statuses

terraform-provider-google-beta already has an open PR.
terraform-provider-google already has an open PR.
Ansible already has an open PR.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

@nat-henderson
Copy link
Contributor Author

It looks like we're blocked on a fix to magic modules that will automatically close the no-op ansible PR. @rambleraptor has said he's on it. :)

If we're coming up on the next terraform release and this still isn't in, I'll just push the button to submit 'em. This will be in the next release.

@nat-henderson
Copy link
Contributor Author

nat-henderson commented Feb 25, 2019

Merged TF downstreams but leaving this PR open, since we're not quite there on Ansible yet.

@rambleraptor
Copy link
Contributor

rambleraptor commented Feb 25, 2019 via email

@nat-henderson
Copy link
Contributor Author

Oh, that's no problem - the issue is that the Ansible generator is still adding files to Ansible downstream even though they should be excluded.

@rambleraptor
Copy link
Contributor

rambleraptor commented Feb 25, 2019 via email

@nat-henderson
Copy link
Contributor Author

Hm, does that mean I should merge the ansible change? I can do that, if it would help - I haven't done that yet.

@romaric1
Copy link

romaric1 commented Mar 1, 2019

hi guys :) can we push this feature , i really need it , if you need help no problem ;)

@rileykarson
Copy link
Member

rileykarson commented Mar 1, 2019

If you're looking for support in Terraform, this is available in version 2.1.0 of the google-beta provider. Looks like we missed adding it to the sidebar (want make a change including it @ndmckinley?) but it's available here: https://www.terraform.io/docs/providers/google/r/compute_managed_ssl_certificate.html

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

Successfully merging this pull request may close these issues.

7 participants