Send advisory messages using Broker connection context#2071
Conversation
This updates the AdvisoryBroker to always publish advisory messages that were generated by other events to use the Broker's own ConnectionContext. Before this change the AdvisoryBroker was using the original ConnectionContext that used used for the action that triggered the advisory. This doesn't make sense because its actually the broker itself firing the advisory message and not the original connection. It also meant requiring all users to be given access to create new advisory topics that could be created on demand. After this update, all users no longer need permission to create advisory destinations which was required previously. Users only need read access to the temporary destination advisories for the AMQ client as the broker itself will now use its own context going forward to create all the destinations on demand and for publishing.
cshannon
changed the title
tabish121
left a comment
tabish121
left a comment
There was a problem hiding this comment.
One consequence of this change seems to be that there isn't a means of preventing a client from generating advisories which could be an uncommon thing to want to do but seems like it was possible under the existing model as those users could be assigned a role that didn't allow then write access. Have you given any thought to whether there is some consequence to not having this ability now?
So this is very confusing but this is actually a non issue because of how the AdvisoryBroker is configured and these changes also help clarify this. The AdvisoryBroker is set up to bypass authorization checks for publish. The advisory broker is created before any broker plugins are applied, so it always delegates to the region broker which does not include the authorization plugins which get initialized later here. This means that any advisory message sent by the AdvisoryBroker never has permissions checked and is always allowed to be sent as it bypasses auth checks. This is fine and expected because its the broker itself. So trying to block advisories being sent with permissions wouldn't work anyways. So what are we fixing here? The main problem this fixes is destination creation when messages are published to topics that don't exist yet. On creation there are interceptors that cause the creation to go back through the full broker plugin stack and it does get processed by any authorization plugins using the provided connection context. This means creation would get blocked if the user isn't given permission. So by using the broker admin context the creation won't get blocked either and we can take away the requirement to grant create permissions. With these changes it would be possible if we wanted to refactor to include the authorization plugin in the broker stack that is passed to AdvisoryBroker and it would work fine (because it uses the admin context) but there isn't any good reason to do that because we are an admin anyways so we don't need to check. |
| // We never want to use flow control for advisories | ||
| result.setProducerFlowControl(false); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
Must we throw an Exception at all for Advisories?
Thinking of call chain.. if a message or other activity results in an advisory firing and the advisory fails, I don't think we'd want to propagate an exception back to caller -- specifically it being sent back to client.
Feels like we should log, and add a failed advisory counter so this could be monitored (should never error) instead of throwing an exception.
There was a problem hiding this comment.
The idea here is we are lazy loading one time and re-using the context so this wouldn't be done every time, although I guess in theory if it throws an exception it could keep retrying.
Ultimately, if we can't load the broker admin context that's really bad. I am going to look at reworking this because this really should be a fatal error.
I first tried to do this in the constructor but ran into the stack overflow issue because creating that context requires the Broker filter chain to be complete so we can't call that during the construction. I next tried to add a start() method override and do it during start but that was also a problem. It turns out that the broker loads destinations and creates them which triggers the advisory callbacks before start() is called so I ended up with this lazy load idea.
However I don't really like this design. This object should really be initialized on start and re-used for the lifecycle. If we can't load it it should fail fast as that's a fatal error. I'm going to look more into it because I realized that while the broker may be trying to send advisories during destination loading before start() is called, is that really doing anything? At that point there would be no consumers for the advisories anyways so I need to investigate this more.
There was a problem hiding this comment.
However I don't really like this design. This object should really be initialized on start and re-used for the lifecycle. If we can't load it it should fail fast as that's a fatal error. I'm going to look more into it because I realized that while the broker may be trying to send advisories during destination loading before start() is called, is that really doing anything? At that point there would be no consumers for the advisories anyways so I need to investigate this more.
Yep it is indeed important that the advisory broker has its callback called during destination add even without consumers because it captures all the information for replay when the advisory consumers do connect. So we can't get rid of this and i'm looking now how to handle it. I am looking into if we need to keep the lazy loading or not.
There was a problem hiding this comment.
- Destinations send advisories -- all methods except 'onMessagegWithNoConsumer' process through the Broker API.
protected void onMessageWithNoConsumers(ConnectionContext context, Message msg) throws Exception {
...
-
StatisticsBroker sends advisories -- using the client connection context here seems appropriate
-
Scheduler jobs are technical advisory messages. This too looks ok as it sends it back through the broker.
@mattrpav - Very good catch! I missed that, the solution here is pretty simple, we need to just move this code inside the AdvisoryBroker, I have no idea why it's not there in the first place so I will fix this.
Both of theses send the messages to temporary destinations and by default the broker allows all users to create and use temporary destinations unless restricted so it probably doesn't matter one way or the other. I guess an argument could be made that both could/should use the broker context as well as the broker is sending the messages back but due to the temporary target I don't think it matters and maybe we leave it alone for now. |
Remove the lazy loading and leverage the callback that exists
|
@mattrpav - This was refactored to remove the lazy loading supplier and to leverage the existing callback that is called when the admin context is created that I didn't realize existed. I updated BrokerService start cycle to always guarantee this is called before it is needed (previously it was only called if destinations needed to be loaded). This refactoring ensures the object is created before it is attempted to be used. An extra sanity check is done during start() just to make sure it isn't null so we fail fast if someone changes something in the future and the initialization gets removed by mistake. |
| * @param context connection context | ||
| * @param messageReference message reference | ||
| */ | ||
| void messageNoConsumers(ConnectionContext context, MessageReference messageReference); |
There was a problem hiding this comment.
This is an API breaking change that we are talking about putting into point releases -- thoughts on making it a default method or other approach to solve for the compatibility issue?
I'm not opposed to telling people in the release notes they need to update their plugins for this point release as a one time deal.
There was a problem hiding this comment.
My assumption is that almost everyone is using BrokerFilter for writing plugins and just extending that. I doubt many people are implementing this interface as then they would have to implement every method themselves. We could make the method have a no-op default but I don't really like that because then anyone who is implementing directly wouldn't know about it.
The way to avoid this is to not move the logic into the AdvisoryBroker in the minor releases, and instead just look up the admin context from the broker each time one of those advisories is fired.
I can revert that change and then make a follow on when this is merged that will move this logic into the AdvisoryBroker and add the new method for version 6.3.0 only.
There was a problem hiding this comment.
@mattrpav and I talked offline and I am going to keep the new API updates and we will document it as it's a much better and cleaner solution.
mattrpav
left a comment
mattrpav
left a comment
There was a problem hiding this comment.
Approved with API change to Broker
3 participants
This updates the AdvisoryBroker to always publish advisory messages that were generated by other events to use the Broker's own ConnectionContext. Before this change the AdvisoryBroker was using the original ConnectionContext that used used for the action that triggered the advisory. This doesn't make sense because its actually the broker itself firing the advisory message and not the original connection. It also meant requiring all users to be given access to create new advisory topics that could be created on demand.
After this update, all users no longer need permission to create advisory destinations which was required previously. Users only need read access to the temporary destination advisories for the AMQ client as the broker itself will now use its own context going forward to create all the destinations on demand and for publishing.