Addresses #1314 [nginx-ingress-controller ssl nginx reload abort] #1315
Conversation
| @@ -45,17 +45,28 @@ type SSLCert struct { | |||
|
|
|||
| // AddOrUpdateCertAndKey creates a .pem file wth the cert and the key with the specified name | |||
| func (nginx *Manager) AddOrUpdateCertAndKey(name string, cert string, key string) (SSLCert, error) { | |||
| tempPemFileName := name + ".pem" | |||
| pemFileName := config.SSLDirectory + "/" + name + ".pem" | |||
There was a problem hiding this comment.
maybe this could be pemFileName := fmt.Sprintf("%v/%v", config.SSLDirectory, tempPemFileName)
There was a problem hiding this comment.
Sure, I used the same format as the existing pemFileName to keep it consistent in method. Should I update that as well? Note that tempPemFileName is meant to be just the filename, its actual Name() will be determined by the os default tmp location (/tmp/filename.pem.something) after returning from ioutil.TempFile(). It then gets moved to the location pemFileName. I did not change how that one string is generated.
Actually, that would probably make more sense in a separate linting/codestyle change PR? It doesn't fix the bug, just changes the previous style used in the code? I'm good with either, let me know.
There was a problem hiding this comment.
@fcvarela yes, I'm trying to avoid string concatenations and use fmt.Sprintf
|
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
|
I've added the suggested changes and |
|
CLAs look good, thanks! |
|
Should I squash all three commits? |
|
@fcvarela yes please |
|
Squashed! |
|
@bprashanth please review |
| return SSLCert{}, fmt.Errorf("Couldn't close temp pem file %v: %v", temporaryPemFile.Name(), err) | ||
| } | ||
|
|
||
| err = os.Rename(temporaryPemFile.Name(), pemFileName) |
There was a problem hiding this comment.
it would be even better if we validated the key/cert pair before moving it (though I think we already do so higher up the stack)
|
LGTM |
Addresses kubernetes-retired#1314 [nginx-ingress-controller ssl nginx reload abort]
Replaces async file writes with atomic renaming of temp files.