Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/1111
I really like this feature and think it's an important improvement. This
PR is a great start and I don't see any major architectural issues. I've added
some comments inline for the really minor issues. I noticed that
application.yml in metron-config-host and metron-alerts-host is different
between the 2 but I can't think of any reason why. I didn't comment directly
but it's probably worth reviewing.
There are some bigger issues I want to call out that are more complex.
I noticed there is a fairly significant gap with respect to groups and
roles. As far as I can tell it's not supported right now which is a regression
from the JDBC solution currently in place. I'm sure this is expected and will
be solved in follow on tasks in this feature branch. I do think we should set
ourselves up to be able to do that in future PRs though.
In this PR, metron-ui-host and metron-rest use 2 different authentication
mechanisms for testing. I feel they should use the same one and we should
eliminate JDBC or in memory authentication all together. To do this we need to
move this testing infrastructure (the EmbeddedLdap class and supporting files,
Spring properties, etc) to metron-security. This will allow us to switch REST
to a common security model during testing, which is essentially what we're
doing with this feature. I've submitted a PR with an example of how we could
do that (switching REST to embedded ldap isn't included yet and should be done
later to avoid conflicts with other PRs). This will allow us to eliminate
redundant properties across modules but I think the biggest benefit is that our
testing will more accurately match a real installation and allow us to catch
bugs sooner.
I think it's fair to say we need some documentation. I would say at a
minimum the metron-rest README should be updated to reflect the new
authentication approach. Eventually it would be nice to have architectural
level documentation because there are several new components/frameworks being
introduced here. I have no problem with that being done in future PRs.
I also noticed there are several TODOs. We should at some point document
these as follow on tasks so we have a clear understanding of what is expected
to work in each PR.
---
