feat(cors) allow for multiple origins #2203

Merged
merged 2 commits into from Mar 15, 2017

Conversation

Projects
None yet
4 participants
@thibaultcha
Member

thibaultcha commented Mar 14, 2017

Summary

Updated PR to handle multiple origins in the CORS plugin.

Full changelog

  • rename origin to origins, which allows a list of origins
  • provide migrations for this change (missing fro m#1973)
  • rebase #1973 on top of the current master and fix tests
  • fix #1973 to also handle * being configured for conf.origins and Allow-Credentials
  • use the PCRE API from ngx.re
  • minor style updates
  • minor perf updates

Issues resolved

Replace #1774
Replace #1973
Fix #1043

TODO

@thibaultcha thibaultcha changed the title from Feat/cors multiple origins to feat/(cors) allow for multiple origins Mar 14, 2017

@nijikokun

This comment has been minimized.

Show comment
Hide comment
@nijikokun

nijikokun Mar 14, 2017

Member

👍

Member

nijikokun commented Mar 14, 2017

👍

kong/plugins/cors/handler.lua
+ "jo")
+ if err then
+ ngx.log(ngx.ERR, "[cors] could not search for domain: ", err)
+ return

This comment has been minimized.

@p0pr0ck5

p0pr0ck5 Mar 14, 2017

Contributor

strictly speaking, this return here seems unnecessary- even if one regex call failed, perhaps others could succeed

@p0pr0ck5

p0pr0ck5 Mar 14, 2017

Contributor

strictly speaking, this return here seems unnecessary- even if one regex call failed, perhaps others could succeed

kong/plugins/cors/handler.lua
+ return
+ end
+
+ local req_origin = req_get_headers()["origin"]

This comment has been minimized.

@p0pr0ck5

p0pr0ck5 Mar 14, 2017

Contributor

using ngx.var as opposed to get_headers would possibly be more performant here

@p0pr0ck5

p0pr0ck5 Mar 14, 2017

Contributor

using ngx.var as opposed to get_headers would possibly be more performant here

@p0pr0ck5

This comment has been minimized.

Show comment
Hide comment
@p0pr0ck5

p0pr0ck5 Mar 14, 2017

Contributor

Also looks like the rockspec is missing the migrations files.

Contributor

p0pr0ck5 commented Mar 14, 2017

Also looks like the rockspec is missing the migrations files.

@thibaultcha

This comment has been minimized.

Show comment
Hide comment
@thibaultcha

thibaultcha Mar 14, 2017

Member

Indeed! That's missing from the rockspec tests...

Member

thibaultcha commented Mar 14, 2017

Indeed! That's missing from the rockspec tests...

Removing approval- rockspec should be updated to include migrations files so unit tests pass

@thibaultcha

This comment has been minimized.

Show comment
Hide comment
@thibaultcha

thibaultcha Mar 14, 2017

Member

Updated .

Member

thibaultcha commented Mar 14, 2017

Updated .

@thibaultcha thibaultcha changed the title from feat/(cors) allow for multiple origins to feat(cors) allow for multiple origins Mar 14, 2017

@thibaultcha thibaultcha referenced this pull request in Kong/docs.konghq.com Mar 15, 2017

Merged

docs(cors) update with multiple origins support #339

@thibaultcha thibaultcha changed the base branch from master to next Mar 15, 2017

thibaultcha added some commits Jan 12, 2017

feat(cors) handle multiple or single origin
* renamed `origin` field to `origins`
* provide migrations for datastore
* use PCRE API for multiple origin matching
* fix compatibility with current upstream changes ensuring "*" support
* minor style updates
* minor perf updates

@thibaultcha thibaultcha merged commit d9f752b into next Mar 15, 2017

1 of 2 checks passed

continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@thibaultcha thibaultcha deleted the feat/cors-multiple-origins branch Mar 15, 2017

@wenqingyu

This comment has been minimized.

Show comment
Hide comment
@wenqingyu

wenqingyu Mar 16, 2017

Guys, cannot wait to use it already, just cut the right time since we are extending our web services.

Guys, cannot wait to use it already, just cut the right time since we are extending our web services.

@thefosk thefosk restored the feat/cors-multiple-origins branch Mar 21, 2017

@thefosk thefosk deleted the feat/cors-multiple-origins branch Mar 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment