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

Add template options #117

Merged
merged 1 commit into from
Feb 23, 2017
Merged

Conversation

rchicoli
Copy link
Contributor

Allow using a custom snippet for advanced configurations in different contexts:

    nginx.org/location-snippets: |
        if ($ssl_client_verify = SUCCESS) {
            set $auth_basic off;
        }
        if ($ssl_client_verify != SUCCESS) {
            set $auth_basic "Restricted";
        }

Allow setting auth_basic and auth-basic-user-file values via config.

@rchicoli
Copy link
Contributor Author

I've tested manually this changes and everything seems to be working as expected. Let me know if I forgot something.

@pleshakov
Copy link
Contributor

@rchicoli
Thanks! Now it's possible to further customize NGINX configuration without modifying the templates.

However, I'm not sure if I understand why using map[int]string is required for storing snippets. Why not to use the []string type?

Also, it seems that having the auth_basic option is redundant since snippets will allow you to easily configure it.

@rchicoli
Copy link
Contributor Author

rchicoli commented Feb 22, 2017

Hi @pleshakov, sorry for the late answer, for some reasons I didn't get any notification. Oh well, actually there is no special reason of using map[int]string, before this implementation I was playing with map[string]string (key: value), then I realized that mapping everything would be easier. Perhaps it is a good point to replace this with []string.
Glad that you like the changes, this can be very handy in some cases.
Cool, I will delete the auth_basic stuffs as well and just leave location-snippets, server-snippets and http-snippets. Please let me know if I need to change these variable names too.
I will push another PR till EOD.

@pleshakov
Copy link
Contributor

@rchicoli
Awesome! The only suggestion I have is related to documentation: If you could group the snippet rows in the table in the examples/customization/README.md file, that'd be great.

@rchicoli
Copy link
Contributor Author

rchicoli commented Feb 22, 2017

I've updated the code and rebased against the master. The only difference now is that I had to add an extra parameter to the GetMapKeyAsStringSlice function, in order to avoid duplications. I hope it is already though.
Glad to contribute to this awesome project. Keep up the good work.

@rchicoli
Copy link
Contributor Author

Not forgetting to mention that I've tested all the changes with minikube and everything seems to work like a charm.

@pleshakov pleshakov merged commit 7458904 into nginxinc:master Feb 23, 2017
@pleshakov
Copy link
Contributor

@rchicoli Thanks!

@pleshakov pleshakov added this to the v0.8.0 milestone Feb 23, 2017
@alizdavoodi
Copy link

alizdavoodi commented Feb 27, 2017

Hi :)
I got error i thinks it's related to this PR.

executing "nginx.conf.tmpl" at <.HTTPSnippets>: can't evaluate field HTTPSnippets in type *nginx.NginxMainConfig

@pleshakov
Copy link
Contributor

@alirezaDavid
Hi. It looks like you're using the recent template file, but an outdated controller. Please rebuild the image https://github.com/nginxinc/kubernetes-ingress/tree/master/nginx-controller using the latest files from the repo.

@alizdavoodi
Copy link

@pleshakov
Thanks for your attention.
I clone fresh clone of repo.
And build like this make PREFIX=boo/nginx-ingress TAG=0.4
build successfully complete, but pod can't start because of mentioned error.

@pleshakov
Copy link
Contributor

@alirezaDavid that is strange. Are you sure that the boo/nginx-ingress:0.4 image gets deployed?

@rchicoli rchicoli deleted the add-template-options branch February 28, 2017 08:21
@rchicoli
Copy link
Contributor Author

rchicoli commented Feb 28, 2017

@alirezaDavid could you try with this image rchicoli/kubernetes-nginx-ingress:0.7.0.6. This was the one I was using for my tests, after the implementation. Could you also post your yaml file? So I can test it here with the same configuration.

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.

3 participants