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.

Sign up for GitHub

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

Jump to bottom

feat(migrations) add preflight defaults for jwt and key-auth #2883

Merged
merged 1 commit into from
Oct 5, 2017
Merged

feat(migrations) add preflight defaults for jwt and key-auth #2883

merged 1 commit into from
Oct 5, 2017

Conversation

Tieske
Copy link
Member

@Tieske Tieske commented Sep 12, 2017 •
edited
Loading

PR #2744 and #2857 implemented the preflight options (for a minor release). This adds the migrations including defaults (for a major release)

@Tieske Tieske self-assigned this Sep 12, 2017
thibaultcha
thibaultcha requested changes Sep 19, 2017
View reviewed changes
Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice stuff! I think this could be commit if we wanted more atomicity (one to introduce the iterator, one to introduce the JWT migration) but well.

The only blocker on my side is the coordinator thing in the Cassandra iterator. Let me know your thoughts.

kong/plugins/jwt/handler.lua Outdated
-- have `nil` and hence will by default start passing the preflight request
-- This should be fixed by a migration to update the actual entries
-- in the datastore
if get_method() == "OPTIONS" and not conf.run_on_preflight then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we check for conf.run_on_preflight first, we can avoid calling get_method() needlessly?

kong/dao/migrations/helpers.lua Outdated
local coro
if dao.db_type == "cassandra" then
coro = coroutine.create(function()
for rows, err in dao.db.cluster:iterate([[
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The recent changes in how we handle migrations in Cassandra introduced a new rule of thumb: we only run migrations on a single coordinator (whether they are DDL or DML queries), and let the elected coordinator propagate the changes to its peers via the inter-nodes Cassandra gossiping.

The cluster:iterate() method will load-balance each query and thus, request each page from a different coordinator. The fix is very simple.

Instead, we should use the coordinator that has been elected to run the migrations. This has been done via dao.db:first_coordinator() in factory.lua#L378. We'll need a new method to retrieve this coordinator, since it is stored in an upvalue. Then, we can simply do:

for rows, err in coordinator:iterate() do

end

Because the iterate() API is the same whether it is used in the cassandra or cassandra.cluster modules.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

kong/dao/migrations/helpers.lua Outdated
end
end

return _M
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty cool iterator 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: also does not respect the 2 lines jump rule (inconsistent with the top of this file where it is respected)

I can fix those issues during merge if you prefer. Or we can build habits...

kong/dao/migrations/helpers.lua Outdated

else
coro = coroutine.create(function()
return nil, nil, "unknown database type: "..tostring(dao.db_type)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: missing spaces around concat operator ..

@thibaultcha thibaultcha added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels Sep 19, 2017
thibaultcha
thibaultcha reviewed Sep 20, 2017
View reviewed changes
kong/dao/migrations/helpers.lua Outdated
return nil, nil, err
end

run_rows(rows)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems run_rows can return an error when we cannot decode the JSON string in row.config, but we are not catching the error here. Should we?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iirc I left it out on purpose, but on second thought it would pass by silently and cause ugly problems later on probably. So added an extra assertion. (only needed for Cassandra, but for consistency added it also on postgres)

thibaultcha
thibaultcha reviewed Sep 20, 2017
View reviewed changes
kong/dao/migrations/helpers.lua Outdated
@@ -0,0 +1,96 @@
local _M = {}
local json_decode = require("cjson.safe").decode
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to say I don't understand why we are not following the usual pattern (in the dozens of files in this codebase) of requiring the modules first, and then declaring the new module?

local json_decode = require("cjson.safe").decode


local _M = {}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand

me neither

thibaultcha
thibaultcha reviewed Sep 20, 2017
View reviewed changes
kong/dao/migrations/helpers.lua
return nil, nil, err
end

assert(run_rows(rows))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 We are never too safe imho

@thibaultcha thibaultcha added pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. pr/status/do not merge (to discuss) labels Sep 21, 2017
@Tieske Tieske changed the base branch from master to next October 5, 2017 14:05
@Tieske Tieske force-pushed the feat/options-migrations branch from 7f99866 to b0e518c Compare October 5, 2017 14:06
@Tieske
feat(migrations) add preflight defaults for jwt and key-auth
8a7934d 
PR #2744 and #2857 implemented the preflight options (for a minor
release). This adds the migrations including defaults (for a
major release)
@Tieske Tieske force-pushed the feat/options-migrations branch from b0e518c to 8a7934d Compare October 5, 2017 14:09
@Tieske Tieske added pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) and removed pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) labels Oct 5, 2017
@thibaultcha thibaultcha merged commit 92cca86 into next Oct 5, 2017
@thibaultcha thibaultcha deleted the feat/options-migrations branch October 5, 2017 18:49
@Tieske Tieske mentioned this pull request Oct 17, 2017
Closed
thibaultcha pushed a commit that referenced this pull request Jan 16, 2018
@Tieske @thibaultcha
feat(*) add run_on_preflight defaults migrations for jwt and key-auth
f7b0c42 
PRs #2744 and #2857 implemented the preflight options (for a minor
release). This adds the migrations including defaults (for a
major release).

From #2883
See #2643 #1292 #1535
thibaultcha pushed a commit that referenced this pull request Jan 19, 2018
@Tieske @thibaultcha
feat(*) add run_on_preflight defaults migrations for jwt and key-auth
f475eeb 
PRs #2744 and #2857 implemented the preflight options (for a minor
release). This adds the migrations including defaults (for a
major release).

From #2883
See #2643 #1292 #1535
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thibaultcha thibaultcha thibaultcha approved these changes

Assignees

@Tieske Tieske

Labels
pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Tieske @thibaultcha