You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "nicoloboschi (via GitHub)" <gi...@apache.org> on 2023/02/08 20:38:00 UTC
[GitHub] [pulsar] nicoloboschi opened a new pull request, #19467: [improve][txn] Allow superusers to abort transactions
nicoloboschi opened a new pull request, #19467:
URL: https://github.com/apache/pulsar/pull/19467
### Motivation
Super users must be always allowed to abort a transaction even if they're not the original owner.
### Modifications
* Check that only owner or superusers are allowed to perform txn operations (end, add partition and add subscription)
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. Please attach the local preview screenshots (run `sh start.sh` at `pulsar/site2/website`) to your PR description, or else your PR might not get merged. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nicoloboschi commented on pull request #19467: [improve][txn] Allow superusers to abort transactions
Posted by "nicoloboschi (via GitHub)" <gi...@apache.org>.
nicoloboschi commented on PR #19467:
URL: https://github.com/apache/pulsar/pull/19467#issuecomment-1429585877
@michaeljmarshall I've addressed your comments here: https://github.com/apache/pulsar/pull/19517
The last one is very interesting but I think we should do it for all the ServerCnx methods in one shot in another pull request.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19467: [improve][txn] Allow superusers to abort transactions
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #19467:
URL: https://github.com/apache/pulsar/pull/19467#discussion_r1101815870
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -2443,9 +2444,15 @@ protected void handleAddPartitionToTxn(CommandAddPartitionToTxn command) {
TransactionMetadataStoreService transactionMetadataStoreService =
service.pulsar().getTransactionMetadataStoreService();
- service.pulsar().getTransactionMetadataStoreService().addProducedPartitionToTxn(txnID,
- command.getPartitionsList())
- .whenComplete(((v, ex) -> {
+ verifyTxnOwnership(txnID)
+ .thenCompose(isOwner -> {
+ if (!isOwner) {
+ return failedFutureTxnNotOwned(txnID);
+ }
+ return transactionMetadataStoreService
+ .addProducedPartitionToTxn(txnID, command.getPartitionsList());
Review Comment:
This is an unsafe call. We must copy the value because `command` is mutable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 merged pull request #19467: [improve][txn] Allow superusers to abort transactions
Posted by "congbobo184 (via GitHub)" <gi...@apache.org>.
congbobo184 merged PR #19467:
URL: https://github.com/apache/pulsar/pull/19467
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #19467: [improve][txn] Allow superusers to abort transactions
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #19467:
URL: https://github.com/apache/pulsar/pull/19467#issuecomment-1430733126
> The last one is very interesting but I think we should do it for all the ServerCnx methods in one shot in another pull request.
I completely agree, thanks for addressing my feedback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19467: [improve][txn] Allow superusers to abort transactions
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #19467:
URL: https://github.com/apache/pulsar/pull/19467#discussion_r1101830009
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -2596,23 +2660,28 @@ protected void handleEndTxnOnSubscription(CommandEndTxnOnSubscription command) {
Commands.newEndTxnOnSubscriptionResponse(requestId, txnidLeastBits, txnidMostBits));
return;
}
-
- CompletableFuture<Void> completableFuture =
- subscription.endTxn(txnidMostBits, txnidLeastBits, txnAction, lowWaterMark);
- completableFuture.whenComplete((ignored, e) -> {
- if (e != null) {
- log.error("handleEndTxnOnSubscription fail ! topic: {}, subscription: {}"
- + "txnId: [{}], txnAction: [{}]", topic, subName,
- txnID, TxnAction.valueOf(txnAction), e.getCause());
- writeAndFlush(Commands.newEndTxnOnSubscriptionResponse(
- requestId, txnidLeastBits, txnidMostBits,
- BrokerServiceException.getClientErrorCode(e),
- "Handle end txn on subscription failed: " + e.getMessage()));
- return;
- }
- writeAndFlush(
- Commands.newEndTxnOnSubscriptionResponse(requestId, txnidLeastBits, txnidMostBits));
- });
+ // we only accept super user becase this endpoint is reserved for tc to broker communication
+ verifyTxnOwnershipForTCToBrokerCommands()
+ .thenCompose(isOwner -> {
+ if (!isOwner) {
+ return failedFutureTxnTcNotAllowed(txnID);
+ }
+ return subscription.endTxn(txnidMostBits, txnidLeastBits, txnAction, lowWaterMark);
+ }).whenComplete((ignored, e) -> {
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19467: [improve][txn] Allow superusers to abort transactions
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #19467:
URL: https://github.com/apache/pulsar/pull/19467#discussion_r1101834879
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -2496,6 +2525,34 @@ protected void handleEndTxn(CommandEndTxn command) {
});
}
+ private CompletableFuture<Boolean> verifyTxnOwnershipForTCToBrokerCommands() {
+ if (service.isAuthenticationEnabled() && service.isAuthorizationEnabled()) {
+ return getBrokerService()
+ .getAuthorizationService()
+ .isSuperUser(getPrincipal(), getAuthenticationData());
Review Comment:
One issue with this code is that we should always get this data from the netty event loop that the channel runs on.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19467: [improve][txn] Allow superusers to abort transactions
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #19467:
URL: https://github.com/apache/pulsar/pull/19467#discussion_r1100868791
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -2496,6 +2525,34 @@ protected void handleEndTxn(CommandEndTxn command) {
});
}
+ private CompletableFuture<Boolean> verifyTxnOwnershipForTCToBrokerCommands() {
+ if (service.isAuthenticationEnabled() && service.isAuthorizationEnabled()) {
+ return getBrokerService()
+ .getAuthorizationService()
+ .isSuperUser(getPrincipal(), getAuthenticationData());
Review Comment:
Does this work correctly for with proxy and original auth data? I will try to review more closely tomorrow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19467: [improve][txn] Allow superusers to abort transactions
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #19467:
URL: https://github.com/apache/pulsar/pull/19467#discussion_r1101827289
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -2496,6 +2525,34 @@ protected void handleEndTxn(CommandEndTxn command) {
});
}
+ private CompletableFuture<Boolean> verifyTxnOwnershipForTCToBrokerCommands() {
+ if (service.isAuthenticationEnabled() && service.isAuthorizationEnabled()) {
+ return getBrokerService()
+ .getAuthorizationService()
+ .isSuperUser(getPrincipal(), getAuthenticationData());
+ } else {
+ return CompletableFuture.completedFuture(true);
+ }
+ }
+
+ private CompletableFuture<Boolean> verifyTxnOwnership(TxnID txnID) {
+ final String checkOwner = getPrincipal();
+ return service.pulsar().getTransactionMetadataStoreService()
+ .verifyTxnOwnership(txnID, checkOwner)
+ .thenCompose(isOwner -> {
+ if (isOwner) {
+ return CompletableFuture.completedFuture(true);
+ }
+ if (service.isAuthenticationEnabled() && service.isAuthorizationEnabled()) {
+ return getBrokerService()
+ .getAuthorizationService()
+ .isSuperUser(checkOwner, getAuthenticationData());
Review 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()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nicoloboschi commented on a diff in pull request #19467: [improve][txn] Allow superusers to abort transactions
Posted by "nicoloboschi (via GitHub)" <gi...@apache.org>.
nicoloboschi commented on code in PR #19467:
URL: https://github.com/apache/pulsar/pull/19467#discussion_r1101791884
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -2496,6 +2525,34 @@ protected void handleEndTxn(CommandEndTxn command) {
});
}
+ private CompletableFuture<Boolean> verifyTxnOwnershipForTCToBrokerCommands() {
+ if (service.isAuthenticationEnabled() && service.isAuthorizationEnabled()) {
+ return getBrokerService()
+ .getAuthorizationService()
+ .isSuperUser(getPrincipal(), getAuthenticationData());
Review Comment:
Yes, I've tested it manually. We always use the `originalPrincipal` - if present - as owner
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org