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

[ingress/controllers/nginx] Add whitelistSourceRange of global config to location block in template #312

Conversation

bviolier
Copy link

@bviolier bviolier commented Feb 21, 2017

The current nginx.tmpl only takes into account ingress.kubernetes.io/whitelist-source-range set on an Ingress but doesn't seem to take into account whitelist-source-range set in the Configmap of Nginx.

According to the docs at https://github.com/kubernetes/ingress/blob/master/controllers/nginx/configuration.md it should take that into account though:

To configure this setting globally for all Ingress rules, the whitelist-source-range value may be set in the NGINX ConfigMap.
Note: Adding an annotation to an Ingress rule overrides any global restriction.

This pull request add's the whitelist-source-range information to nginx.tmpl, only if for the location no specific one is set.

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Feb 21, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@bviolier
Copy link
Author

I signed it!

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 21, 2017
@aledbf aledbf self-assigned this Feb 21, 2017
@aledbf aledbf added the nginx label Feb 21, 2017
@aledbf
Copy link
Member

aledbf commented Feb 21, 2017

Note: Adding an annotation to an Ingress rule overrides any global restriction

The idea is to omit the value in the configmap if you use the annotation.

@bviolier
Copy link
Author

bviolier commented Feb 21, 2017

@aledbf That's what my PR should do. If an annotation is set on the ingress, it doesn't set the global one for that location block.

To be clear: the global whitelist wasn't taken into account, ever.

@aledbf
Copy link
Member

aledbf commented Feb 21, 2017

@bviolier please check the code here. This is already done internally.

@bviolier
Copy link
Author

@aledbf That's only parsing of the annotation on the Ingress, where happens the parsing/processing of the global whitelist-source-range set in the Nginx controller Configmap? I couldn't find that.

@aledbf
Copy link
Member

aledbf commented Feb 21, 2017

@bviolier line 60 and 66, if there is no annotation, it returns the value from the configmap.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 46.163% when pulling 203a19c on bviolier:add-whitelistsourcerange-ngix-templ into 5ab0f28 on kubernetes:master.

@bviolier
Copy link
Author

You are right, my bad.

But, it doesn't seem to work in the latest Nginx controller beta though (nginx-ingress-controller:0.9.0-beta.1). With my PR it did work for me.

@aledbf
Copy link
Member

aledbf commented Feb 21, 2017

@bviolier please open an issue to fix it.

@ashb
Copy link
Contributor

ashb commented Mar 28, 2017

Issue has been reported as #473.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. nginx
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants