[GLBC] Better certificate handling #639
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
This change is |
|
cc @tonglil I'm betting you haven't noticed this pre-shared cert bug, but better for you to know in advance. |
nicksardo
changed the title
|
Coverage increased (+0.2%) to 46.42% when pulling 8f0f2b780ed511c624ff30fa5ecff744d8cfd2d3 on nicksardo:gce-ssl-name-fix into 4bd0f49 on kubernetes:master. |
|
Coverage increased (+0.2%) to 46.412% when pulling 8f0f2b780ed511c624ff30fa5ecff744d8cfd2d3 on nicksardo:gce-ssl-name-fix into 4bd0f49 on kubernetes:master. |
nicksardo
force-pushed
the
gce-ssl-name-fix
branch
from
8f0f2b7
to
748b5c5
Compare
|
|
||
| // Retrieve known ssl certificate | ||
| if expectedCertName != "" { | ||
| l.sslCert, _ = l.cloud.GetSslCertificate(expectedCertName) |
There was a problem hiding this comment.
document why ignoring error here..,
| // Use the named GCE cert when it is specified by the annotation. | ||
| if certName != "" { | ||
| if preSharedCertName != "" { |
There was a problem hiding this comment.
move this block to its own method func (l *L7) usePreSharedCert()
(you can most likely come up with a better name)
| @@ -384,32 +408,28 @@ func (l *L7) checkSSLCert() (err error) { | |||
| // TODO: Clean this code up into a ring buffer. | |||
| primaryCertName := l.namer.Truncate(fmt.Sprintf("%v-%v", sslCertPrefix, l.Name)) | |||
There was a problem hiding this comment.
see below, move this code to func (l) nextCertificateName()
| l.sslCert = cert | ||
| return nil | ||
| } | ||
|
|
||
| // Handle secret-created cert |
There was a problem hiding this comment.
move this block of code to own func (l) populateSSLCert()
| } else { | ||
| certName = primaryCertName | ||
| if l.sslCert == nil || ingCert != l.sslCert.Certificate { | ||
| newCertName := primaryCertName |
There was a problem hiding this comment.
Make a new method nextCertficateName(), it'll make this logic easier to follow
newCertName := l.nextCertificateName()
func (l*L7) nextCertificateName() string {
primaryCertName := ...
...
if l.sslCert != nil && ...
}
| // Perform a delete in case a certificate exists with the exact name | ||
| // This certificate should be unused since we check the target proxy's certificate prior | ||
| // to this point. Although, it's possible an actor pointed a target proxy to this certificate. | ||
| if err = l.cloud.DeleteSslCertificate(newCertName); err != nil && !utils.IsHTTPErrorCode(err, http.StatusNotFound) { |
There was a problem hiding this comment.
this is a little tangential to thie review, but a lot of the StatusNotFound if checks can be made more succinct by
func ignoreHTTPNotFound(err error) error {
if err != nil && utils.IsHTTPErrorCode(err, http.StatusNotFound) { return nil }
return err
}
then all of these sites will be
if err = ignoreHTTPNotFound(l.cloud.DeleteSslCertificate(newCertName)); err != nil {
}
| return fmt.Errorf("unable to delete ssl certificate with name %q, expected it to be unused. err: %v", newCertName, err) | ||
| } | ||
|
|
||
| glog.Infof("Creating new sslCertificates %v for %v", newCertName, l.Name) |
|
Coverage increased (+0.9%) to 47.122% when pulling d592e3a225d0c9577c320bc40f378373b183b1f8 on nicksardo:gce-ssl-name-fix into 4bd0f49 on kubernetes:master. |
|
Coverage increased (+0.9%) to 47.107% when pulling d592e3a225d0c9577c320bc40f378373b183b1f8 on nicksardo:gce-ssl-name-fix into 4bd0f49 on kubernetes:master. |
|
Coverage increased (+0.9%) to 47.107% when pulling d592e3a225d0c9577c320bc40f378373b183b1f8 on nicksardo:gce-ssl-name-fix into 4bd0f49 on kubernetes:master. |
nicksardo
force-pushed
the
gce-ssl-name-fix
branch
from
2d5dc81
to
f704d81
Compare
| } | ||
| l.fws = nil |
There was a problem hiding this comment.
Looks like this line was in the wrong block, so I moved it out.
| } | ||
| l.ip = nil |
There was a problem hiding this comment.
l.ip is a bit special since it can be a shared resource among ingresses. We want to continue other resource cleanup even if this fails.
There was a problem hiding this comment.
IMO, the controller should only delete the IP if it was reserved by the controller. Might add a name-prefix check
There was a problem hiding this comment.
Ah, l.ip is never populated with a user-specified static IP.
nicksardo
force-pushed
the
gce-ssl-name-fix
branch
from
294586f
to
3af5f5c
Compare
|
Thank you @nicksardo for notifying & fixing! |
Fixes #609
Current Behavior:
On first sync after start, the controller asserts a certificate with the primary name, and possibly orphans a certificate with the secondary name. On next certificate update, a collision occurs.
New Behavior:
Due to the orphaned certificate, some users have two certificates (one with
-1and one without). Most likely the primary-named certificate is being used, but we can't make that assumption. Therefore, if the controller doesn't know what certificate is being used (on first sync), the code will now inspect the Target HTTPS Proxy for the existing ssl certificate. It'll continue using that certificate until it finds a change in the secret. If users have two certificates, the controller will attempt deletion of any certificate using the desired name.The PR also fixes a bug when transitioning from a pre-shared certificate to a secret-based certificate. The controller deletes the pre-shared certificate when it really should ignore it.