[improve][txn] Allow superusers to abort transactions #19467
Conversation
| if (service.isAuthenticationEnabled() && service.isAuthorizationEnabled()) { | ||
| return getBrokerService() | ||
| .getAuthorizationService() | ||
| .isSuperUser(getPrincipal(), getAuthenticationData()); |
There was a problem hiding this comment.
Does this work correctly for with proxy and original auth data? I will try to review more closely tomorrow.
There was a problem hiding this comment.
Yes, I've tested it manually. We always use the originalPrincipal - if present - as owner
There was a problem hiding this comment.
One issue with this code is that we should always get this data from the netty event loop that the channel runs on.
Super users must be always allowed to abort a transaction even if they're not the original owner. * Check that only owner or superusers are allowed to perform txn operations (end, add partition and add subscription) (cherry picked from commit 459a7a5)
Super users must be always allowed to abort a transaction even if they're not the original owner. * Check that only owner or superusers are allowed to perform txn operations (end, add partition and add subscription) (cherry picked from commit 459a7a5)
| return failedFutureTxnNotOwned(txnID); | ||
| } | ||
| return transactionMetadataStoreService | ||
| .addProducedPartitionToTxn(txnID, command.getPartitionsList()); |
There was a problem hiding this comment.
This is an unsafe call. We must copy the value because command is mutable.
| if (service.isAuthenticationEnabled() && service.isAuthorizationEnabled()) { | ||
| return getBrokerService() | ||
| .getAuthorizationService() | ||
| .isSuperUser(checkOwner, getAuthenticationData()); |
There was a problem hiding this comment.
This is also unsafe. We should not call getAuthenticationData() from another thread. We can update the thenCompose to thenComposeAsync and run it on the ctx.executor().
| return failedFutureTxnTcNotAllowed(txnID); | ||
| } | ||
| return subscription.endTxn(txnidMostBits, txnidLeastBits, txnAction, lowWaterMark); | ||
| }).whenComplete((ignored, e) -> { |
There was a problem hiding this comment.
This is a nit, but I've been looking closely at the netty implementation, and I noticed that when we write to the channel, netty will schedule an event on the ctx.executor() if the calling thread is not the event loop (the executor). Would it make sense to prevent context switching and just schedule this to run on the ctx's event loop?
|
@michaeljmarshall I've addressed your comments here: #19517 |
…ache#19467) (apache#19473) Co-authored-by: Nicolò Boschi <boschi1997@gmail.com> (cherry picked from commit cb91c4a)
I completely agree, thanks for addressing my feedback. |
### Motivation This PR builds on #19467. When we modify/abort transactions, we need to make sure that authorization is checked for both the proxy and the client. ### Modifications * Add a second authorization check when `originalPrincipal` is set in the `ServerCnx`. * Fix a bug where we were not doing a deep copy of the `SubscriptionsList` object. (Tests caught this bug!) ### Verifying this change Added a new test to cover some of the changes. ### Does this pull request potentially affect one of the following parts: This is an internal change. ### Documentation - [x] `doc-not-needed` ### Matching PR in forked repository PR in forked repository: michaeljmarshall#38
This PR builds on #19467. When we modify/abort transactions, we need to make sure that authorization is checked for both the proxy and the client. * Add a second authorization check when `originalPrincipal` is set in the `ServerCnx`. * Fix a bug where we were not doing a deep copy of the `SubscriptionsList` object. (Tests caught this bug!) Added a new test to cover some of the changes. This is an internal change. - [x] `doc-not-needed` PR in forked repository: michaeljmarshall#38 (cherry picked from commit f76beda)
This PR builds on #19467. When we modify/abort transactions, we need to make sure that authorization is checked for both the proxy and the client. * Add a second authorization check when `originalPrincipal` is set in the `ServerCnx`. * Fix a bug where we were not doing a deep copy of the `SubscriptionsList` object. (Tests caught this bug!) Added a new test to cover some of the changes. This is an internal change. - [x] `doc-not-needed` PR in forked repository: michaeljmarshall#38 (cherry picked from commit f76beda) (cherry picked from commit 5a180f78d7636537198a758e1c9416e58d80bf42)
This PR builds on apache#19467. When we modify/abort transactions, we need to make sure that authorization is checked for both the proxy and the client. * Add a second authorization check when `originalPrincipal` is set in the `ServerCnx`. * Fix a bug where we were not doing a deep copy of the `SubscriptionsList` object. (Tests caught this bug!) Added a new test to cover some of the changes. This is an internal change. - [x] `doc-not-needed` PR in forked repository: michaeljmarshall#38 (cherry picked from commit f76beda) (cherry picked from commit 5a180f78d7636537198a758e1c9416e58d80bf42) (cherry picked from commit 716db37)
Motivation
Super users must be always allowed to abort a transaction even if they're not the original owner.
Modifications
Verifying this change
Documentation
docdoc-requireddoc-not-neededdoc-complete