-
Notifications
You must be signed in to change notification settings - Fork 916
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-4780 - fix console when jmx-domain is non default #5028
Conversation
Are these attributes being added to the "security" MBean simply for convenience or are they somehow related to security? If the former, would it be much trouble to add a new MBean (e.g. "ConsoleBootstrap") to hold these attributes so it makes a bit more sense conceptually? |
...a/org/apache/activemq/artemis/core/server/management/impl/HawtioSecurityControlImplTest.java
Outdated
Show resolved
Hide resolved
...a/org/apache/activemq/artemis/core/server/management/impl/HawtioSecurityControlImplTest.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/activemq/artemis/core/server/management/ArtemisMBeanServerGuard.java
Outdated
Show resolved
Hide resolved
For convenience mostly and to avoid having more than 1 bean exposed. I am happy to add a new bean but I have had feedback that we should refrain from adding anything new. Maybe @gtully or @gemmellr have some input |
This exposes the jmx domain and broker name via an mbean to allow discovery when the domain is non default
Sorry, I didnt see that you had commented. When we chatted about this previously and you mentioned you had already discussed with Gary/Dom and decided on adding it to the existing hawtio mbean, I didnt realise that specifically called itself out as a security mbean in its overall naming, so I can definitely see Justin's point there. Though as its not an MBean users really interact with themselves, I'm not sure how bad it would really be to go with it. I also thought their idea was better than adding a new MBean at the time because at least that existing MBean is already 'trusted' for the other stuff and was already clearly hawtio console specific, and this only really being a hawtio console problem. I didnt really want a new MBean for this either as its almost completely superfluous for every normal user, i.e most people simply dont need it so having an mbean for it feels like needless clutter for the regular case. It also needs a known domain and objectname to look up itself, which will almost certainly be something else that can clash with other things at some point. In retrospect this also only works for one broker 'jmx domain' at a time (same is true of using the existing hawtio mbean), which in some ways is odd as the entire reason this weird 'variable mbean jmx domain' stuff was added ~15 years ago was to support multiple broker being managed in the same VM using multiple different jmx domains. Thus far only 1 person has asked about hitting a mismatch here it seems, and that is only coming up because they are shading the broker. So to me it seems reasonable enough theyd also need to similarly update the console as well, which they already indicated theyd be up for doing with a custom build (since the bit needing changed is in a file in a jar, in the war) if that was the approach deemed necessary. I did suggest perhaps there is some way we could make that easier, e.g take the custom value from a specific file, which they could then more easily add/update/swap out than currently as part of the general console files themselves. I think I'm back to thinking that is preferable than mechanics like this to do it automatically. A specific new MBean for it, that doesnt even work for the original use case of varying the mbean domain, and is entirely superfluous for almost every user, feels like perhaps the worst option for auto-fixing a self-created problem from shading the broker but not similarly updating the console. |
What you suggest @gemmellr using a external file is easy in the new console but difficult to do in th eold console because of the way HawtIO builds and authenticates. I could just fix it in th enew console and leave this as is (apart for the mbean guard fix). wdyt? |
I didn't particularly mean an external file, just a specific file. One targeted file used only for that mechanism of identifying the domain, rather than the current string-literal-across-several-main-console-files. Something that could then be simply updated in isolation, e.g overwrite it while repacking the plugin war we already publish to create their own version along with their custom broker. That said, I'd also be fine with just addressing it in the new console. |
This exposes the jmx domain and broker name via an mbean to allow discovery when the domain is non default.
Currently if the jmx domain is non default the console is broken. The console does currently load in the jmx domain from local storage but there are 2 problems with this:
A better approach is to expose the jmx domain via a well known mBean that never changes. Artemis already has one of these that is used by the console for RBAC and decorating the console. This PR add the JMX Domain and the broker name on that mbean. The console looks this up as its 1st step on loading the plugin and uses it for locating, inspecting and executing methods for the duraion.
There is also a fix I have added where the MBean Guard Handler was throwing an NPE which was swallowed by hawtIO, I have added a comment but for completeness here it is:
HawtIO calls invoke(...) with a null operationName as a coarse grained way of authenticating against all the operations on an mbean. Until this addition this was throwing a null pointer on operationName later in this call which was swallowed by HawtIO. Since fine grained checks are carried out against every operation this was never an issue however the new console based on HawtIO 4 passes this exception back to the console which breaks it. Since it is just an optimisation it is fine to always return true. Note that the alternative ArtemisRbacInvocationHandler does allow the ability to restrict a while mbean