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

hotfix(migrations): set default for anonymous with auth plugins #2266

Closed
wants to merge 14 commits into from

Conversation

Tieske
Copy link
Member

@Tieske Tieske commented Mar 27, 2017

The default value for anonymous == "", yet the migrations were missing

fixes #2264 but for all auth plugins.

@subnetmarco
Copy link
Member

subnetmarco commented Mar 27, 2017

This adds a new migration, which could possibly break a running cluster, therefore needs to be merged against next and not master.

@Tieske Tieske modified the milestones: 0.11.0, 0.10.1 Mar 27, 2017
Tieske and others added 8 commits March 27, 2017 21:57
We now avoid reading all request headers via `ngx.req.get_headers()` and
use the `ngx.var` API instead. In the future, we'll distinguish Host
fetching and full headers fetching when arbitrary headers matching will
be added to the router.
When we don't route an API via its `hosts` property (no Host recorded),
this test ensures that we still forward the incoming request `Host`
header when `preserve_host = true`.
…out-hosts

tests(router) ensure preserve_host without hosts matching
perf(router) efficient host header reading
@p0pr0ck5
Copy link
Contributor

@thefosk any way to make an exception here, given that a). this migration is very straightforward, and b). there are issues on 0.10.1 that this will fix? (#2293)

@subnetmarco
Copy link
Member

subnetmarco commented Mar 29, 2017

@p0pr0ck5 it depends. What if this operation fails:

row.config.anonymous = ""
local _, err = dao.plugins:update(row, { id = row.id })

and we have 50% of plugins updated, and the other 50% not updated. How will this affect the cluster?

In my opinion migrations shouldn't be pushed in minor releases.

p0pr0ck5 and others added 5 commits March 29, 2017 14:45
ngx.time uses a cached time struct that obviates the need for the
syscall performed by os.time. additionally, os.time is not JITable.
perf(aws-lambda) use ngx.time in place of os.time
Support user directive of Nginx.

From #2180
Signed-off-by: Wei.ZHAO <zhaowei@qiyi.com>
fix(test) remove the redundant right parentheses
@Tieske
Copy link
Member Author

Tieske commented Mar 30, 2017

What if this operation fails:

In this case, it doesn't matter, you can run the migration again. That said, a migration takes a different upgrade approach, hence I'm with @thefosk that it should not be in a minor release.

@p0pr0ck5
Copy link
Contributor

That said, a migration takes a different upgrade approach, hence I'm with @thefosk that it should not be in a minor release.

So should we update this PR to target next instead of master?

@subnetmarco
Copy link
Member

So should we update this PR to target next instead of master?

+1

@Tieske
Copy link
Member Author

Tieske commented Mar 31, 2017

@p0pr0ck5 @thefosk please check #2313 against next

@Tieske Tieske deleted the fix/authmigration branch April 5, 2017 12:08
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.

5 participants