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

Cors features improvements #1553

Merged
merged 1 commit into from
Oct 22, 2017
Merged

Conversation

rikatz
Copy link
Contributor

@rikatz rikatz commented Oct 20, 2017

What this PR does / why we need it: This PR adds new forms to control CORS in a server, allowing user to specify not only if CORS is enabled, but also what are the allowed METHODS, HEADERS, the ORIGIN and credentials behaviour

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #1171, fixes #676, fixes #625

Special notes for your reviewer:

This PR adds 4 new annotations that allow user to control CORS behaviour in his ingress.

This code inserts some regex validations, as it's not desired that user is allowed to insert values as $nginx_version in allowed methods, as this is going to be returned directly to the final user and can cause an internal information leak, and also Ingress misconfiguration.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 20, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 20, 2017
@rikatz
Copy link
Contributor Author

rikatz commented Oct 20, 2017

Please do not merge yet, need to test this in a production environment.

@rikatz
Copy link
Contributor Author

rikatz commented Oct 20, 2017

I'm just finishing some other tests, maybe tomorrow I'll release this.

@rikatz
Copy link
Contributor Author

rikatz commented Oct 22, 2017

Need some help here.

It seems controller is not returning any values to template. Although everything is working 'fine', any value used in ingress is discarded.

I've put a comment in here but even setting the right annotation (ngress.kubernetes.io/enable-cors: "true") doesn't seems to work fine.

Probably is something small that I'm missing.

The temporal image is rpkatz/nginx-ingress-controller:katz-cors7

Thanks!

CORS annotations improvements

Cors improvements

Cors improevements

Cors improvements

Cors improvements
@rikatz rikatz changed the title WIP: Cors features improvements Cors features improvements Oct 22, 2017
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 22, 2017
@rikatz
Copy link
Contributor Author

rikatz commented Oct 22, 2017

Finished the PR.

@aledbf Thanks for the help!

@aledbf
Copy link
Member

aledbf commented Oct 22, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 22, 2017
@aledbf
Copy link
Member

aledbf commented Oct 22, 2017

@rikatz thanks!

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
4 participants