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

Support revocation of tenant keys #2211 #2482

Merged
merged 3 commits into from
Jun 8, 2021
Merged

Support revocation of tenant keys #2211 #2482

merged 3 commits into from
Jun 8, 2021

Conversation

prb112
Copy link
Contributor

@prb112 prb112 commented Jun 7, 2021

Signed-off-by: Paul Bastide pbastide@us.ibm.com

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>
@prb112 prb112 self-assigned this Jun 7, 2021
SQL = "DELETE FROM " + tableName + " WHERE mt_id = ?";
} else {
SQL = "DELETE FROM " + tableName + " WHERE mt_id = ?"
+ " AND tenant_hash = sysibm.hash(tenant_salt || ?, 2)";
Copy link
Member

Choose a reason for hiding this comment

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

is this db2-specific. if so, can it live in the translator?

Copy link
Member

@lmsurpre lmsurpre Jun 8, 2021

Choose a reason for hiding this comment

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

never mind, I think I was mixing up the translator with an "adapter" (the one that adapts the SQL for a specific db type). still the same question though.

Copy link
Contributor Author

@prb112 prb112 Jun 8, 2021

Choose a reason for hiding this comment

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

Right, this is a db2 specific task - we're only multitenant on db2.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, agree it should work just fine. was just a general recommendation/observation to try keeping the db-specific logic in the adapter (in case we ever introduce any other multitenant dbs...which I don't think we will but 🤷 )

Copy link
Member

Choose a reason for hiding this comment

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

maybe just add a comment to indicate that its db2-specific and move on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started from AddTenantKeyDAO, this is consistent with addTenantKeyDAO and the other tenant daos. Are you asking me to update the comments in all of these?

Copy link
Member

Choose a reason for hiding this comment

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

I started from AddTenantKeyDAO, this is consistent with addTenantKeyDAO and the other tenant daos. Are you asking me to update the comments in all of these?

ah, I was wondering just that (how many places we've built in multitenant==db2 logic). I think it would be good to call out all such places, but I guess its really not a big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the top-level comments.

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>
Copy link
Member

@lmsurpre lmsurpre left a comment

Choose a reason for hiding this comment

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

LGTM

@prb112 prb112 added the ci-skip Skips the CI Build label Jun 8, 2021
Signed-off-by: Paul Bastide <pbastide@us.ibm.com>
@prb112 prb112 merged commit 6a6d25a into main Jun 8, 2021
@prb112 prb112 deleted the issue-2211 branch June 8, 2021 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-skip Skips the CI Build persistence
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants