-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Support path-only when referencing ssl certificate in compute_target_https_proxy #210
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,12 @@ import ( | |
|
||
"github.com/hashicorp/terraform/helper/schema" | ||
"google.golang.org/api/compute/v1" | ||
"regexp" | ||
) | ||
|
||
const ( | ||
sslCertificateRegex = "projects/(.+)/global/sslCertificates/(.+)$" | ||
canonicalSslCertificateTemplate = "https://www.googleapis.com/compute/v1/projects/%s/global/sslCertificates/%s" | ||
) | ||
|
||
func resourceComputeTargetHttpsProxy() *schema.Resource { | ||
|
@@ -26,7 +32,13 @@ func resourceComputeTargetHttpsProxy() *schema.Resource { | |
"ssl_certificates": &schema.Schema{ | ||
Type: schema.TypeList, | ||
Required: true, | ||
Elem: &schema.Schema{Type: schema.TypeString}, | ||
Elem: &schema.Schema{ | ||
Type: schema.TypeString, | ||
ValidateFunc: validateRegexp(sslCertificateRegex), | ||
StateFunc: toCanonicalSslCertificate, | ||
}, | ||
MinItems: 1, | ||
MaxItems: 1, | ||
}, | ||
|
||
"url_map": &schema.Schema{ | ||
|
@@ -129,51 +141,12 @@ func resourceComputeTargetHttpsProxyUpdate(d *schema.ResourceData, meta interfac | |
} | ||
|
||
if d.HasChange("ssl_certificates") { | ||
proxy, err := config.clientCompute.TargetHttpsProxies.Get( | ||
project, d.Id()).Do() | ||
|
||
_old, _new := d.GetChange("ssl_certificates") | ||
_oldCerts := _old.([]interface{}) | ||
_newCerts := _new.([]interface{}) | ||
current := proxy.SslCertificates | ||
|
||
_oldMap := make(map[string]bool) | ||
_newMap := make(map[string]bool) | ||
|
||
for _, v := range _oldCerts { | ||
_oldMap[v.(string)] = true | ||
} | ||
|
||
for _, v := range _newCerts { | ||
_newMap[v.(string)] = true | ||
} | ||
|
||
sslCertificates := make([]string, 0) | ||
// Only modify certificates in one of our old or new states | ||
for _, v := range current { | ||
_, okOld := _oldMap[v] | ||
_, okNew := _newMap[v] | ||
|
||
// we deleted the certificate | ||
if okOld && !okNew { | ||
continue | ||
} | ||
|
||
sslCertificates = append(sslCertificates, v) | ||
|
||
// Keep track of the fact that we have added this certificate | ||
if okNew { | ||
delete(_newMap, v) | ||
} | ||
} | ||
|
||
// Add fresh certificates | ||
for k, _ := range _newMap { | ||
sslCertificates = append(sslCertificates, k) | ||
} | ||
// Exactly one ssl certificate must be specified. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you confirm that the API call actually fails if more than one is specified? If it silently lets it go through, we might be breaking people with this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The google cloud documentation for this resource is misleading, it says "Currently, exactly one SSL certificate must be specified." but you can specify more than one and it works. I sent feedback to that team to fix their documentation. I updated the code, Terraform documentation and the test to confirm we can support more than one ssl certificate. |
||
certs := make([]string, 1) | ||
certs[0] = d.Get("ssl_certificates.0").(string) | ||
|
||
cert_ref := &compute.TargetHttpsProxiesSetSslCertificatesRequest{ | ||
SslCertificates: sslCertificates, | ||
SslCertificates: certs, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be simplified a bit to
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not applicable anymore. |
||
} | ||
op, err := config.clientCompute.TargetHttpsProxies.SetSslCertificates( | ||
project, d.Id(), cert_ref).Do() | ||
|
@@ -208,24 +181,7 @@ func resourceComputeTargetHttpsProxyRead(d *schema.ResourceData, meta interface{ | |
return handleNotFoundError(err, d, fmt.Sprintf("Target HTTPS proxy %q", d.Get("name").(string))) | ||
} | ||
|
||
_certs := d.Get("ssl_certificates").([]interface{}) | ||
current := proxy.SslCertificates | ||
|
||
_certMap := make(map[string]bool) | ||
_newCerts := make([]interface{}, 0) | ||
|
||
for _, v := range _certs { | ||
_certMap[v.(string)] = true | ||
} | ||
|
||
// Store intersection of server certificates and user defined certificates | ||
for _, v := range current { | ||
if _, ok := _certMap[v]; ok { | ||
_newCerts = append(_newCerts, v) | ||
} | ||
} | ||
|
||
d.Set("ssl_certificates", _newCerts) | ||
d.Set("ssl_certificates", proxy.SslCertificates) | ||
d.Set("self_link", proxy.SelfLink) | ||
d.Set("id", strconv.FormatUint(proxy.Id, 10)) | ||
|
||
|
@@ -256,3 +212,14 @@ func resourceComputeTargetHttpsProxyDelete(d *schema.ResourceData, meta interfac | |
d.SetId("") | ||
return nil | ||
} | ||
|
||
func toCanonicalSslCertificate(v interface{}) string { | ||
value := v.(string) | ||
|
||
m := regexp.MustCompile(sslCertificateRegex).FindStringSubmatch(value) | ||
if m == nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm just curious - will There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should never be nil. Removed this line. |
||
return value | ||
} | ||
|
||
return fmt.Sprintf(canonicalSslCertificateTemplate, m[1], m[2]) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit: having
Required
set implicitly setsMinItems
to1
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed