fix(jwt) preflight: revert workaround, implement migration

[Tieske]

This builds on top of #2744, but targets the next branch. The additional commit reverts the workaround and implements the proper migrations.

[thibaultcha]

The default page size for the driver is (currently) 1000. However, if the page size ever changes, or becomes configurable, or if such an entity has more than a 1000 rows, or simply because we wish to enforce best C* practices to avoid spreading bad code snippets by copy/paste, then we should use the proper way of aggregating all rows:

for rows, err in cluster:iterate("SELECT * FROM plugins WHERE name = 'jwt'") do
  if err then
    return err
  end
  
  if row.config.authenticate_preflight == nil then
    -- do update
  end
end

[thibaultcha]

Additionally, this migration does not belong to the core, but to the JWT plugin.

[Tieske]

This is plugin configuration, which is in the core. The plugins own daos.lua file is what the plugin specific migrations target.

Do you want plugin specific migrations to start updating core entities?

[thibaultcha]

This is how plugins migrations are already handled. The configuration of the plugins themselves does not belong to the core, but to the plugins themselves (and rightfully so).

[Tieske]

yes, figured that out myself. will update

[thibaultcha]

ditto: move to jwt plugin migrations

[thibaultcha]

(whether)

[thibaultcha]

According to this condition, it seems that authenticate_preflight = true means "ignore and proxy upstream".

However, one can interpret authenticate_preflight as to mean "enforce the JWT verification (authentication step) even if preflight". Which is the opposite, and which would mean that authenticate_preflight = true would mean: enforce instead of ignore.

I think the name is thus confusing, and does not carry enough meaning to infer its functionality. In the CORS plugin, we have preflight_continue which, if enabled, will make the plugin ignore, and thus proxy the request upstream. I thus see 2 possibilities:

1. use the same terminology as the CORS plugin: config.preflight_continue = true means: **ignore this plugin on preflight, got directly upstream)

2. Rename to config.run_on_preflight, which is a more explicit name

3. is consistent with the CORS, and 2. carries (I feel) the most meaning.

Note: this is yet another instance of plugins deciding for themselves when to run and when not to. Something they shouldn't be allowed to do in the future :)

[thibaultcha]

Shall we deprecate (close) #2743 since this is the proper long term solution (it follows our policy of running migrations to include a new configuration property).

[Tieske]

the idea here is to merge #2743 to master such that it can go into minor releases of 0.10.x and 0.11.x. And this pr is then (after rebasing) to go to next such that it ends up on 0.12.

If we deprecate #2743 then it will be months before this functionality becomes available.

[thibaultcha]

Understood, but we should also respect the merge windows from now on (especially for 0.11.0). Maybe we can include those old PRs in 0.11.1?

[Tieske]

Understood, but we should also respect the merge windows from now on (especially for 0.11.0).

I had no intent otherwise. Did you get the impression somewhere?

[thibaultcha]

I had no intent otherwise. Did you get the impression somewhere?

Not at all! Just explicitly stating the reason for not merging those today :)