provider/google: Improved SSL certs handling on target_https_proxy #14264
Conversation
…ion from Google Compute API URL.
Now partial URLs of SSL certs also have first-class support in a same way as full URLs.
builtin/providers/google/provider.go
Outdated
| url string | ||
| } | ||
|
|
||
| func parseUrl(url string) (resourceInfo, error) { |
There was a problem hiding this comment.
This function is kinda over-spec only for this purpose, but hope it will be useful on later usage.
There was a problem hiding this comment.
So after discussing with @danawillow, I think we'd rather a more specific, less-encompassing function that just takes a URL (full or partial) for specifically the certs, and always returns the full URL. Something like what we do for disk images, though hopefully more minimal. The reason we think it'd be better to do a more specific version is because our resources tend to have shorthand inputs (like family/image or just image for images) to make the UX more intuitive and easier to work with. I also worry that the regular expression is descriptive, not prescribed, and so it may not be followed for future resources, which would lead to some maintenance headaches. :( So for the time being, I think specific solutions are probably better than generic ones. But thank you for the hard work on this! It definitely lays the groundwork if we want to move in this direction in the future.
|
|
||
| resource.Test(t, resource.TestCase{ | ||
| PreCheck: func() { testAccPreCheck(t) }, | ||
| Providers: testAccProviders, | ||
| CheckDestroy: testAccCheckComputeTargetHttpsProxyDestroy, | ||
| Steps: []resource.TestStep{ | ||
| resource.TestStep{ | ||
| Config: testAccComputeTargetHttpsProxy_basic1, | ||
| Config: testAccComputeTargetHttpsProxy_basic(resourceName, 0, 0), |
There was a problem hiding this comment.
I totally rewrote this TestAccComputeTargetHttpsProxy_update since the old one was insufficient: it used to create a set of resources, then destroy them and recreate a new set of resources -- this is not a test of update!
Sorry for the ugly diff.
There was a problem hiding this comment.
Overall, I think a simpler approach is warranted here. Also, rather than storing whatever the user enters in state and then translating it on every call, I think it may be a better option to use the StateFunc property on the Elem schema.Schema inside the ssl_certificates field. If you need guidance on doing that, or would rather that I go ahead and do it, I'm happy to take over or offer guidance. :)
Thanks so much for submitting this PR, and all the work that went into it. I think, at the moment, a slightly tweaked design would just be more appropriate.
builtin/providers/google/provider.go
Outdated
| url string | ||
| } | ||
|
|
||
| func parseUrl(url string) (resourceInfo, error) { |
There was a problem hiding this comment.
So after discussing with @danawillow, I think we'd rather a more specific, less-encompassing function that just takes a URL (full or partial) for specifically the certs, and always returns the full URL. Something like what we do for disk images, though hopefully more minimal. The reason we think it'd be better to do a more specific version is because our resources tend to have shorthand inputs (like family/image or just image for images) to make the UX more intuitive and easier to work with. I also worry that the regular expression is descriptive, not prescribed, and so it may not be followed for future resources, which would lead to some maintenance headaches. :( So for the time being, I think specific solutions are probably better than generic ones. But thank you for the hard work on this! It definitely lays the groundwork if we want to move in this direction in the future.
|
@paddycarver Sorry for the late reply, I was busy last week...
|
tmshn
force-pushed
the
google_https_proxy_better_certs_handling
branch
from
c24ec09 to
69b3cc7
Compare
|
@paddycarver ping |
|
Hey @tmshn, my apologies for dropping the ball on this. It looks like hashicorp/terraform-provider-google#210 fixed this, so it should work now. If you continue to have problems, please let us know. And please do accept my sincerest apologies for this going unanswered; we're currently working to make sure this doesn't happen in the future. Thank you for your work and patience! |
|
@paddycarver I understand the situation, no problem! And happy to hear the issue is fixed, thanks! 👍 |
|
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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
Problem
As reported in #9672, passing a partial URL of SSL cert (e.g.:
projects/foo/global/sslCertificates/bar) totarget_https_proxywill not be idempotent; the passed URL will not be recorded in state file and Terraform will try to add the same certs on next apply even though it's actually already set in the real resource.Solution
This PR fixes this problem by converting partial URL to full URL every time before SSL certs comparison and API call.