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.

Sign up for GitHub

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

Jump to bottom

ARTEMIS-4924 Proper handling of invalid messages in SNF queues #5091

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

ARTEMIS-4924 Proper handling of invalid messages in SNF queues #5091

wants to merge 1 commit into from

Conversation

gaohoward
Copy link
Contributor

No description provided.

@gtully
Copy link
Contributor

gtully commented Jul 17, 2024

I think we already have a way to prevent sending with rbac if that is the problem.
But we may just need to make use of a dlq for invalid messages in this case. This cause seems like a good match for a dlq as it blocks the head.

@gaohoward
Copy link
Contributor Author

gaohoward commented Jul 23, 2024 •
edited

@gtully @jbertram I've upated the PR. Now when a cluster connection is activated, a Role is added so that when security is enabled any client will be denied of sending messages to store and forward queues. When security is disabled, or user overrides the role in security settings, messages are sent to snf queue will be moved to DLQ when messages are to be added to snf queue.

@clebertsuconic
Copy link
Contributor

I'm running the whole test suite now. if it's good it LGTM.

@gtully let me know what you think... I will post the results for the test suite here. We should probably merge this before the release if it's all good.

@gaohoward
Copy link
Contributor Author

I'm running the whole test suite now. if it's good it LGTM.

@gtully let me know what you think... I will post the results for the test suite here. We should probably merge this before the release if it's all good.

Thanks @clebertsuconic

@gaohoward gaohoward force-pushed the d_sf_reject-9143 branch 2 times, most recently from 44bc79d to 1a1f0f5 Compare July 26, 2024 13:25
@gaohoward
Copy link
Contributor Author

@gtully PR updated. I added a bit documentation.

@gaohoward gaohoward requested a review from gtully July 26, 2024 13:25
@gaohoward gaohoward force-pushed the d_sf_reject-9143 branch 2 times, most recently from 9419454 to a611107 Compare July 30, 2024 03:29
@gaohoward gaohoward force-pushed the d_sf_reject-9143 branch 2 times, most recently from c1749d8 to bdb0fd1 Compare August 7, 2024 11:29
@gaohoward gaohoward force-pushed the d_sf_reject-9143 branch 3 times, most recently from abba07a to e67f6f5 Compare August 20, 2024 03:23
gtully
gtully approved these changes Aug 20, 2024
View reviewed changes
Copy link
Contributor

@gtully gtully left a comment

Choose a reason for hiding this comment

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

This looks good to me. Adding the detail property is a nice addition to help trouble shooting.

@gtully
Copy link
Contributor

gtully commented Aug 20, 2024

I think the commit message needs to be updated to better reflect what it is now doing.

@howardgao
ARTEMIS-4924 Proper handling of invalid messages in SNF queues
222f01a 
  - Send invalid messages in SNF queues to DLQ
  - Add documentation for store and forward queue proper usage
@gaohoward gaohoward changed the title ARTEMIS-4924 Do not allow sending messages directly to store-and-forward queues ARTEMIS-4924 Proper handling of invalid messages in SNF queues Aug 22, 2024
@gaohoward
Copy link
Contributor Author

@gtully I've updated the commit messages as well as the jira title. Thanks!

@gaohoward gaohoward requested a review from gtully August 22, 2024 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gtully gtully gtully approved these changes

@clebertsuconic clebertsuconic Awaiting requested review from clebertsuconic

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@gaohoward @gtully @clebertsuconic @howardgao