Skip to content

Conversation

@dwradcliffe
Copy link
Contributor

It's generally a best practice to disable server_tokens to prevent the nginx version from being emitted.

@pleshakov
Copy link
Contributor

@dwradcliffe
I don't think it's necessary to disable server_tokens. We keep the default NGINX configuration of the Ingress controller very close to the default NGINX configuration. However, it's important to have the ability to change the defaults easily for the most common use-cases. The server_token directive is important and we should definitely add the ConfigMaps key to configure this directive.
Let me know if you're interested in implementing that.

@dwradcliffe
Copy link
Contributor Author

Yeah I can take a look at adding that.

@dwradcliffe
Copy link
Contributor Author

@pleshakov PR updated with config option for server_tokens

@dwradcliffe
Copy link
Contributor Author

I must have missed something here... getting this error:
nginx.go:213] Failed to write template template: ingress.tmpl:20:13: executing "ingress.tmpl" at <$server.DisableServe...>: DisableServerTokens is not a field of struct type nginx.Server

@pleshakov
Copy link
Contributor

@dwradcliffe
looks like the compiled version of the controller doesn't have DisableServerTokens field. Suggest to recompile it.

I tried it and it works! I only suggest to rename the key: disable-server-tokens -> server-tokens to make it more consistent with the NGINX directive.

@dwradcliffe
Copy link
Contributor Author

server-tokens with a default value of true?

@pleshakov
Copy link
Contributor

@dwradcliffe yes

@dwradcliffe
Copy link
Contributor Author

@pleshakov ok, done. Tested on my cluster and seems to be working properly.

@pleshakov
Copy link
Contributor

@dwradcliffe Looks good! could you squash your commits before I merge?

@dwradcliffe
Copy link
Contributor Author

squashed!

@pleshakov pleshakov merged commit 2c931e6 into nginx:master Jan 19, 2017
@dwradcliffe dwradcliffe deleted the patch-1 branch January 19, 2017 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants