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

google_app_engine_domain_mapping AUTOMATIC certificate API is broken #4741

Closed
nfelt opened this issue Oct 23, 2019 · 3 comments · Fixed by GoogleCloudPlatform/magic-modules#2533
Labels

Comments

@nfelt
Copy link

nfelt commented Oct 23, 2019

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment
  • If an issue is assigned to the "modular-magician" user, it is either in the process of being autogenerated, or is planned to be autogenerated soon. If an issue is assigned to a user, that user is claiming responsibility for the issue. If an issue is assigned to "hashibot", a community member has claimed the issue already.

Terraform Version

  • Terraform 0.12 (google internal)
  • provider.google: version = "~> 2.14"
  • provider.google-beta: version = "~> 2.16"

Affected Resource(s)

  • google_app_engine_domain_mapping

Terraform Configuration Files

The example snippet suffices:

resource "google_app_engine_domain_mapping" "domain_mapping" {
  domain_name = "dm-test-.gcp.tfacc.hashicorptest.com"

  ssl_settings {
    ssl_management_type = "AUTOMATIC"
  }
}

Expected Behavior

Once this config is applied, it should continue to be valid, regardless of the status of the AUTOMATIC SSL cert renewal.

Actual Behavior

Once the SSL cert is provisioned, terraform plan starts showing a diff like this:

      ~ ssl_settings {
          - certificate_id      = "14606599" -> null
            ssl_management_type = "AUTOMATIC"
        }

Specifying the certificate_id in the config to avoid a diff in this case shouldn't be required - indeed it should arguably be an error to specify certificate_id when ssl_management_type = AUTOMATIC (and I expect if it I set the field, it would generate an API error).

The overall way that certs are exposed within the domain mapping resource doesn't really make sense to me. Logically, what makes sense is to have an optional certificate_id argument can be specified if and only if ssl_management_type = MANUAL (better UX would probably be to have an optional block like manual_ssl_settings entirely). For auto certs, it shouldn't really be necessary to think about the certificate ID at all, but if we need to expose it, we should provide computed attributes certificate_id and for pending_managed_certificate_id - they don't make sense as arguments.

@ghost ghost added the bug label Oct 23, 2019
@nfelt
Copy link
Author

nfelt commented Oct 23, 2019

FWIW there were some comments by @danawillow on the original PR about making these output-only fields:
GoogleCloudPlatform/magic-modules#2216 (comment)
GoogleCloudPlatform/magic-modules#2216 (comment)

Not sure exactly what the resolution was but it seems to have been to leave them as inputs? (I am not that familiar with Terraform, maybe there's some distinction I'm missing.)

@nfelt
Copy link
Author

nfelt commented Oct 24, 2019

FWIW, I figured out that ignore_changes provides a workaround for this:

resource "google_app_engine_domain_mapping" "domain_mapping" {
  ...
  lifecycle {
    ignore_changes = [
      ssl_settings[0].certificate_id,
    ]
  }
}

Looks like there's already a fix out for review in #2533 that basically does this automatically :) Thanks!

@ghost
Copy link

ghost commented Nov 25, 2019

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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Nov 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant