Don't require default server TLS secret #1512
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
soneillf5
approved these changes
Apr 6, 2021
soneillf5
reviewed
Apr 6, 2021
cmd/nginx-ingress/main.go
Outdated
| glog.Fatalf("A TLS cert and key for the default server is not found") | ||
| sslRejectHandshake = true | ||
| } else { | ||
| glog.Fatalf("Error reading the default server TLS cert and key from %s: %v", configs.DefaultServerSecretPath, err) |
soneillf5
suggested changes
Apr 6, 2021
cmd/nginx-ingress/main.go
Outdated
| @@ -440,9 +443,11 @@ func main() { | |||
| bytes := configs.GenerateCertAndKeyFileContent(secret) | |||
| nginxManager.CreateSecret(configs.DefaultServerSecretName, bytes, nginx.TLSSecretFileMode) | |||
| } else { | |||
| _, err = os.Stat("/etc/nginx/secrets/default") | |||
| _, err = os.Stat(configs.DefaultServerSecretPath) | |||
There was a problem hiding this comment.
Can we write a unit test for this logic?
There was a problem hiding this comment.
good idea 👍 there was a bug
soneillf5
suggested changes
Apr 7, 2021
cmd/nginx-ingress/main.go
Outdated
| return false, nil | ||
| } | ||
|
|
||
| return statErr == nil, statErr |
There was a problem hiding this comment.
Sorry for the additional comments, I don't want to be too harsh, but this doesn't look correct yet.
if os.IsNotExist() -> is true then the file doesn't exist, however you're returning (statErr == nil), shouldn't that just be return true, statErr ? In which case the code becomes:
if os.IsNotExist(statErr) {
return false, nil
}
return true, statErr
which simplifies to:
return os.IsNotExist(statErr), statErr
I don't really understand this function, it appears to be just wrapping os.IsNotExist.
In terms of idiomatic error handling in Go, I think the usual practice is something like:
if os.IsTimeout(err) {
...
}
if os.IsNotExist(err) {
...
}
...
// handle valid case
return value
If the default server TLS secret is not configured via -default-server-tls-secret cli arg or it is not present on the filesystem at /etc/nginx/secrets/default, the Ingress Controller will configure NGINX to reject TLS connections to the default server. Note: The default server is used in NGINX configuration to handle HTTP and HTTPS request to hosts that are not configured by any Ingress, VirtualServer or TransportServer resources. The default server simply returns 404 responses.
8a7ece9 to
2d9a279
Compare
lucacome
approved these changes
Apr 7, 2021
soneillf5
approved these changes
Apr 8, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
If the default server TLS secret is not configured via -default-server-tls-secret cli arg or it is not present on the filesystem at /etc/nginx/secrets/default, the Ingress Controller will configure NGINX to reject TLS connections to the default server.
Note: The default server is used in NGINX configuration to handle HTTP and HTTPS request to hosts that are not configured by any Ingress, VirtualServer or TransportServer resources. The default server simply returns 404 responses.
Also note: because currently handling missing/invalid TLS secret relies on the default server TLS secret being present on the file system, this PR #1500 (which changes the handling) needs to be merged first.