Skip to content
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

disable emitting nginx version #108

Merged
merged 1 commit into from
Jan 19, 2017
Merged

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 nginxinc: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