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

ARTEMIS-1872 Check for queue exists before creating shared queue #2093

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@michaelandrepearce
Contributor

michaelandrepearce commented May 18, 2018

  1. Add tests case to verify issue and fix, tests also tests for same behavior using CORE, OPENWIRE and AMQP JMS Clients.
  2. Update Core Client to check for queue before creating, sharedQueue as per createQueue logic.
  3. Update ServerSessionPacketHandler to handle packets from old clients to perform to implement the same fix server side for older clients.
  4. Correct AMQP protocol so correct error code is returned on security exception so that amqp jms can correctly throw JMSsecurityException
  5. Correct AMQP protocol to check for queue exists before create
  6. Correct OpenWire protocol to check for address exists before create
@michaelandrepearce

This comment has been minimized.

Show comment
Hide comment
@michaelandrepearce

michaelandrepearce May 18, 2018

Contributor

See for full description of the problem, or see test case.
https://issues.apache.org/jira/browse/ARTEMIS-1872

Contributor

michaelandrepearce commented May 18, 2018

See for full description of the problem, or see test case.
https://issues.apache.org/jira/browse/ARTEMIS-1872

@michaelandrepearce

This comment has been minimized.

Show comment
Hide comment
@michaelandrepearce
Contributor

michaelandrepearce commented May 18, 2018

@clebertsuconic

This comment has been minimized.

Show comment
Hide comment
@clebertsuconic

clebertsuconic May 18, 2018

Contributor

Wouldn’t this make a blocker for 2.6.0? Or this is not that critical ?

I’m reading on the iPhone with limited visibility of the changes. But it seems to warrant a -1 on the current release vote.

Let me know please.

Contributor

clebertsuconic commented May 18, 2018

Wouldn’t this make a blocker for 2.6.0? Or this is not that critical ?

I’m reading on the iPhone with limited visibility of the changes. But it seems to warrant a -1 on the current release vote.

Let me know please.

@michaelandrepearce

This comment has been minimized.

Show comment
Hide comment
@michaelandrepearce

michaelandrepearce May 18, 2018

Contributor

@clebertsuconic well the bug is been there for ages, just more evident/exposed since 2.5.0 where security check was corrected, so made it more apparent. If its easy to re-spin 2.6.0 yes i would probably re-spin it.

It is a blocker for my org, as pre-2.5.0 with the security we have, the bug in security hid the bug in client code, so i would imagine it could be a blocker for other users as well.

Contributor

michaelandrepearce commented May 18, 2018

@clebertsuconic well the bug is been there for ages, just more evident/exposed since 2.5.0 where security check was corrected, so made it more apparent. If its easy to re-spin 2.6.0 yes i would probably re-spin it.

It is a blocker for my org, as pre-2.5.0 with the security we have, the bug in security hid the bug in client code, so i would imagine it could be a blocker for other users as well.

@clebertsuconic

This comment has been minimized.

Show comment
Hide comment
@clebertsuconic

clebertsuconic May 18, 2018

Contributor

Can you bring this up on the voting thread?

Contributor

clebertsuconic commented May 18, 2018

Can you bring this up on the voting thread?

@michaelandrepearce

This comment has been minimized.

Show comment
Hide comment
@michaelandrepearce

michaelandrepearce May 18, 2018

Contributor

@clebertsuconic email is on its way.

Contributor

michaelandrepearce commented May 18, 2018

@clebertsuconic email is on its way.

@franz1981

This comment has been minimized.

Show comment
Hide comment
@franz1981

franz1981 May 18, 2018

Contributor

@michaelandrepearce it looks fine to me: probably I would have performed the check about the queue existence just for the durable ones

Contributor

franz1981 commented May 18, 2018

@michaelandrepearce it looks fine to me: probably I would have performed the check about the queue existence just for the durable ones

@michaelandrepearce

This comment has been minimized.

Show comment
Hide comment
@michaelandrepearce

michaelandrepearce May 18, 2018

Contributor

@franz1981 non durable also needs it. See test cases added, we cover this. As noted logic bug been there, just its more exposed since durable is now correctly being checked.

Reason why its very obvious/more evident now and far more impacting, is its most typical to secure a broker from dynamic creation of durable queue (as these can cause backups when no consumer which can cause broker issue), and less so non-durable, e.g. its typical you allow clients to create these as once client is gone messages wont pile up, so less impacting to the broker, and obviously before the sec check was fixed it was checking security as non-durable no matter if durable or non-durable.

Contributor

michaelandrepearce commented May 18, 2018

@franz1981 non durable also needs it. See test cases added, we cover this. As noted logic bug been there, just its more exposed since durable is now correctly being checked.

Reason why its very obvious/more evident now and far more impacting, is its most typical to secure a broker from dynamic creation of durable queue (as these can cause backups when no consumer which can cause broker issue), and less so non-durable, e.g. its typical you allow clients to create these as once client is gone messages wont pile up, so less impacting to the broker, and obviously before the sec check was fixed it was checking security as non-durable no matter if durable or non-durable.

@franz1981

This comment has been minimized.

Show comment
Hide comment
@franz1981

franz1981 May 21, 2018

Contributor

@michaelandrepearce Yep, makes sense indeed. I will probably implemented it in the same you've done (on Core). I haven't finished to read the AMQP part yet 👍

Contributor

franz1981 commented May 21, 2018

@michaelandrepearce Yep, makes sense indeed. I will probably implemented it in the same you've done (on Core). I haven't finished to read the AMQP part yet 👍

@clebertsuconic

This comment has been minimized.

Show comment
Hide comment
@clebertsuconic

clebertsuconic May 21, 2018

Contributor

I'm running the testsuite first before merging this...

Contributor

clebertsuconic commented May 21, 2018

I'm running the testsuite first before merging this...

@clebertsuconic

This comment has been minimized.

Show comment
Hide comment
@clebertsuconic

clebertsuconic May 22, 2018

Contributor

These following tests are failing:

Test Result (14 failures / +14)
org.apache.activemq.artemis.tests.integration.amqp.JMSConnectionWithSecurityTest.testCreateTemporaryQueueNotAuthorized
org.apache.activemq.artemis.tests.integration.amqp.JMSConnectionWithSecurityTest.testCreateTemporaryTopicNotAuthorized
org.apache.activemq.artemis.tests.integration.jms.consumer.JmsConsumerTest.testValidateExceptionsThroughSharedConsumers
org.apache.activemq.artemis.tests.integration.jms.jms2client.SharedConsumerTest.sharedNonDurableSubOnDifferentSelector
org.apache.activemq.artemis.tests.integration.jms.jms2client.SharedConsumerTest.sharedDurableSubOnDifferentSelector
org.apache.activemq.artemis.tests.integration.jms.jms2client.SharedConsumerTest.sharedDurableSubOnDifferentTopic
org.apache.activemq.artemis.tests.integration.jms.jms2client.SharedConsumerTest.sharedNonDurableSubOnDifferentSelectorSrcFilterNull
org.apache.activemq.artemis.tests.integration.jms.jms2client.SharedConsumerTest.sharedDurableSubOnDifferentSelectorSrcFilterNull
org.apache.activemq.artemis.tests.integration.jms.jms2client.SharedConsumerTest.sharedDurableSubOnDifferentSelectorTargetFilterNull
org.apache.activemq.artemis.tests.integration.jms.jms2client.SharedConsumerTest.sharedNonDurableSubOnDifferentSelectorTargetFilterNull

These tests also failed when I ran but they are probably part of the intermittent failures:
org.apache.activemq.artemis.protocol.amqp.util.CreditsSemaphoreTest.testDownAndUp
org.apache.activemq.artemis.tests.integration.cluster.failover.ReplicatedLargeMessageWithDelayFailoverTest.testFailThenReceiveMoreMessagesAfterFailover2
org.apache.activemq.artemis.tests.integration.cluster.reattach.MultiThreadRandomReattachTest.testD
org.apache.activemq.artemis.tests.integration.cluster.reattach.NettyMultiThreadRandomReattachTest.testE

(Although I believe CreditsSemaphoreTest failed because of these changes.. I'm not 100% sure)

Contributor

clebertsuconic commented May 22, 2018

These following tests are failing:

Test Result (14 failures / +14)
org.apache.activemq.artemis.tests.integration.amqp.JMSConnectionWithSecurityTest.testCreateTemporaryQueueNotAuthorized
org.apache.activemq.artemis.tests.integration.amqp.JMSConnectionWithSecurityTest.testCreateTemporaryTopicNotAuthorized
org.apache.activemq.artemis.tests.integration.jms.consumer.JmsConsumerTest.testValidateExceptionsThroughSharedConsumers
org.apache.activemq.artemis.tests.integration.jms.jms2client.SharedConsumerTest.sharedNonDurableSubOnDifferentSelector
org.apache.activemq.artemis.tests.integration.jms.jms2client.SharedConsumerTest.sharedDurableSubOnDifferentSelector
org.apache.activemq.artemis.tests.integration.jms.jms2client.SharedConsumerTest.sharedDurableSubOnDifferentTopic
org.apache.activemq.artemis.tests.integration.jms.jms2client.SharedConsumerTest.sharedNonDurableSubOnDifferentSelectorSrcFilterNull
org.apache.activemq.artemis.tests.integration.jms.jms2client.SharedConsumerTest.sharedDurableSubOnDifferentSelectorSrcFilterNull
org.apache.activemq.artemis.tests.integration.jms.jms2client.SharedConsumerTest.sharedDurableSubOnDifferentSelectorTargetFilterNull
org.apache.activemq.artemis.tests.integration.jms.jms2client.SharedConsumerTest.sharedNonDurableSubOnDifferentSelectorTargetFilterNull

These tests also failed when I ran but they are probably part of the intermittent failures:
org.apache.activemq.artemis.protocol.amqp.util.CreditsSemaphoreTest.testDownAndUp
org.apache.activemq.artemis.tests.integration.cluster.failover.ReplicatedLargeMessageWithDelayFailoverTest.testFailThenReceiveMoreMessagesAfterFailover2
org.apache.activemq.artemis.tests.integration.cluster.reattach.MultiThreadRandomReattachTest.testD
org.apache.activemq.artemis.tests.integration.cluster.reattach.NettyMultiThreadRandomReattachTest.testE

(Although I believe CreditsSemaphoreTest failed because of these changes.. I'm not 100% sure)

@clebertsuconic

This comment has been minimized.

Show comment
Hide comment
@clebertsuconic

clebertsuconic May 22, 2018

Contributor

CreditsSemaphoreTest passed under same github commit on another box where I ran.. so non related... but the Sharedconsumer ones and the queue related seem broken by this change.

Contributor

clebertsuconic commented May 22, 2018

CreditsSemaphoreTest passed under same github commit on another box where I ran.. so non related... but the Sharedconsumer ones and the queue related seem broken by this change.

@michaelandrepearce

This comment has been minimized.

Show comment
Hide comment
@michaelandrepearce

michaelandrepearce May 22, 2018

Contributor

Ill have a look i think i know what it is (need to add filter check at same point as exists check and should be super simple.

Contributor

michaelandrepearce commented May 22, 2018

Ill have a look i think i know what it is (need to add filter check at same point as exists check and should be super simple.

@michaelandrepearce

This comment has been minimized.

Show comment
Hide comment
@michaelandrepearce

michaelandrepearce May 22, 2018

Contributor

@clebertsuconic Should be sorted now.

Added checks for address and filterstring along side checking the queue exists.

Ive run locally SharedConsumerTest you mentioned and with this it now passes.

Contributor

michaelandrepearce commented May 22, 2018

@clebertsuconic Should be sorted now.

Added checks for address and filterstring along side checking the queue exists.

Ive run locally SharedConsumerTest you mentioned and with this it now passes.

@jbertram

This comment has been minimized.

Show comment
Hide comment
@jbertram

jbertram May 22, 2018

Contributor

Looks like a checkstyle violation.

Contributor

jbertram commented May 22, 2018

Looks like a checkstyle violation.

ARTEMIS-1872 Check for queue exists before creating shared queue
1. Add tests case to verify issue and fix, tests also tests for same behavior using CORE, OPENWIRE and AMQP JMS Clients.
2. Update Core Client to check for queue before creating, sharedQueue as per createQueue logic.
3. Update ServerSessionPacketHandler to handle packets from old clients to perform to implement the same fix server side for older clients.
4. Correct AMQP protocol so correct error code is returned on security exception so that amqp jms can correctly throw JMSsecurityException
5. Correct AMQP protocol to check for queue exists before create
6. Correct OpenWire protocol to check for address exists before create
@michaelandrepearce

This comment has been minimized.

Show comment
Hide comment
@michaelandrepearce

michaelandrepearce May 22, 2018

Contributor

@jbertram fixed checkstyle introduced in last change, to address franz's review comment.

Contributor

michaelandrepearce commented May 22, 2018

@jbertram fixed checkstyle introduced in last change, to address franz's review comment.

@jbertram

This comment has been minimized.

Show comment
Hide comment
@jbertram

jbertram May 22, 2018

Contributor

This looks good to me. @clebertsuconic, what do you think?

Contributor

jbertram commented May 22, 2018

This looks good to me. @clebertsuconic, what do you think?

@clebertsuconic

This comment has been minimized.

Show comment
Hide comment
@clebertsuconic

clebertsuconic May 22, 2018

Contributor

I took a day off after the release:) If you are happy with it feel free to merge it. We can amend later if anything is failing.

Or I can merge later tonight or tomorrow morning.

— typing on the iPhone. From a beach. :)

Contributor

clebertsuconic commented May 22, 2018

I took a day off after the release:) If you are happy with it feel free to merge it. We can amend later if anything is failing.

Or I can merge later tonight or tomorrow morning.

— typing on the iPhone. From a beach. :)

@clebertsuconic

This comment has been minimized.

Show comment
Hide comment
@clebertsuconic

clebertsuconic May 23, 2018

Contributor

@michaelandrepearce Maybe I'm being slow.. but why do you need to check for version here?

shouldn't this be a simple fix on checking the address before?

from what I udnerstood from the fix is that you always remove the queue when creating a shared subscription?

Contributor

clebertsuconic commented May 23, 2018

@michaelandrepearce Maybe I'm being slow.. but why do you need to check for version here?

shouldn't this be a simple fix on checking the address before?

from what I udnerstood from the fix is that you always remove the queue when creating a shared subscription?

@clebertsuconic

This comment has been minimized.

Show comment
Hide comment
@clebertsuconic

clebertsuconic May 23, 2018

Contributor

that would mean older clients will still hit the bug if you keep the version check

Contributor

clebertsuconic commented May 23, 2018

that would mean older clients will still hit the bug if you keep the version check

@clebertsuconic

This comment has been minimized.

Show comment
Hide comment
@clebertsuconic

clebertsuconic May 23, 2018

Contributor

compatibility tests are passing with this.. do you know any cases where this would brea compatibility and the current tests have missed it? if that's the case we need to add a test on the compatibility...

Contributor

clebertsuconic commented May 23, 2018

compatibility tests are passing with this.. do you know any cases where this would brea compatibility and the current tests have missed it? if that's the case we need to add a test on the compatibility...

@clebertsuconic

This comment has been minimized.

Show comment
Hide comment
@clebertsuconic

clebertsuconic May 23, 2018

Contributor

the more I digg.. the more I'm convinced there's no need for the version check... if we can speak on IRC before we merge this?

The version check was basically for wire compatibility... for semantic you just change the server... there's no wire changes here. .just keep it this way?

and if this is breaking the client->server contract in a way it needs a raise to the properties. .this would bump the release into 2.7.0...

We can't make changes to versioning on point releases... if later on we need to make any other changes into 2.7.0... it will be some extra complexity on bumping the release there again.

It seems we can simplify this a lot?

Contributor

clebertsuconic commented May 23, 2018

the more I digg.. the more I'm convinced there's no need for the version check... if we can speak on IRC before we merge this?

The version check was basically for wire compatibility... for semantic you just change the server... there's no wire changes here. .just keep it this way?

and if this is breaking the client->server contract in a way it needs a raise to the properties. .this would bump the release into 2.7.0...

We can't make changes to versioning on point releases... if later on we need to make any other changes into 2.7.0... it will be some extra complexity on bumping the release there again.

It seems we can simplify this a lot?

clebertsuconic added a commit to clebertsuconic/activemq-artemis that referenced this pull request May 23, 2018

clebertsuconic added a commit to clebertsuconic/activemq-artemis that referenced this pull request May 23, 2018

@michaelandrepearce

This comment has been minimized.

Show comment
Hide comment
@michaelandrepearce

michaelandrepearce May 23, 2018

Contributor

Ill close this if you merge yours, im happy with yours.

Contributor

michaelandrepearce commented May 23, 2018

Ill close this if you merge yours, im happy with yours.

@clebertsuconic

This comment has been minimized.

Show comment
Hide comment
@clebertsuconic

clebertsuconic May 23, 2018

Contributor

I am merging your changes.. however I'm reverting the portion of the version properties check. we may revisit this later if you see any other issues.

To make eventual cherry-picking into 2.6.1 easier.. I'm making this a single commit.

Contributor

clebertsuconic commented May 23, 2018

I am merging your changes.. however I'm reverting the portion of the version properties check. we may revisit this later if you see any other issues.

To make eventual cherry-picking into 2.6.1 easier.. I'm making this a single commit.

@asfgit asfgit closed this in 88b2399 May 23, 2018

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