Skip to content

[proposal] perf(basic-auth-v2) introduces basic-auth-v2 plugin with better performance#5914

Closed
Abhishekvrshny wants to merge 1 commit intoKong:masterfrom
Abhishekvrshny:basic-auth-v2
Closed

[proposal] perf(basic-auth-v2) introduces basic-auth-v2 plugin with better performance#5914
Abhishekvrshny wants to merge 1 commit intoKong:masterfrom
Abhishekvrshny:basic-auth-v2

Conversation

@Abhishekvrshny
Copy link
Copy Markdown
Contributor

Summary

The original basic-auth plugin has id as the primary key, with a
secondary index on username. While this works well with postgres,
there are performance issues with cassandra as lookups always happen
on username and secondary indexes on cassandra are inefficient.

A new plugin is introduced as changing the primary key in cassandra
is non-trivial and hence updating the existing plugin is not possible.

This plugin makes username as the partition key and id as the
clustering in cassandra. In case of postgres, both form composite
primary key.

Signed-off-by: Abhishek Varshney abhishek.varshney@razorpay.com

Issues resolved

https://discuss.konghq.com/t/primary-key-for-cassandra-in-basic-auth-plugin/6169

…rmance

The original basic-auth plugin has id as the primary key, with a
secondary index on username. While this works well with postgres,
there are performance issues with cassandra as lookups always happen
on username and secondary indexes on cassandra are inefficient.

A new plugin is introduced as changing the primary key in cassandra
is non-trivial and hence updating the existing plugin is not possible.

This plugin makes username as the partition key and id as the
clustering in cassandra. In case of postgres, both form composite
primary key.

Signed-off-by: Abhishek Varshney <abhishek.varshney@razorpay.com>
@kikito
Copy link
Copy Markdown
Member

kikito commented Jan 20, 2021

Hello, thanks for your contribution.

changing the primary key in cassandra
is non-trivial and hence updating the existing plugin is not possible

While I agree in that it is not-trivial, I think updating the existing plugin is perfectly possible. Changes similar to the one you are mentioning on this PR have already been done to Kong entities in the past. This kind of transition is done in the migration segments. Recently, for example, we had to add a new core entity for several tables. This meant changing the pagination of several key entities. You can see the relevant functions here:

https://github.com/Kong/kong/blob/8ee1db2811fd04c992b17e83ea073f954c90fd71/kong/db/migrations/operations/200_to_210.lua

They were used to re-partition several tables in the corresponding migration:

https://github.com/Kong/kong/blob/8ee1db2811fd04c992b17e83ea073f954c90fd71/kong/db/migrations/core/009_200_to_210.lua

It stands to reason that a similar approach could be taken here. It is not a trivial one, for sure, but it is doable. I think in the long run it would be less work than maintaining two separate basic-auth plugins.

@gszr gszr changed the base branch from next to master April 15, 2021 18:20
@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Abhishek Varshney seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@hbagdi
Copy link
Copy Markdown
Contributor

hbagdi commented Oct 25, 2022

Cassandra is deprecated as a supported data-store. Furthermore, Hybrid mode is our recommended mode of operation which doesn't have this problem.
Our guidance is to have a custom plugin to get around these limitations.
Closing this PR for now. Please re-open if needed.

@hbagdi hbagdi closed this Oct 25, 2022
@gabybeitler
Copy link
Copy Markdown

gabybeitler commented Oct 25, 2022 via email

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants