geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kirk Lund <kirk.l...@gmail.com>
Subject Re: Review Request 60375: GEODE-3117: fix Gateway authentication with legacy security
Date Thu, 29 Jun 2017 17:49:34 GMT


> On June 26, 2017, 11:58 p.m., Jinmei Liao wrote:
> > geode-core/src/main/java/org/apache/geode/cache/client/internal/ConnectionFactoryImpl.java
> > Line 82 (original), 82 (patched)
> > <https://reviews.apache.org/r/60375/diff/1/?file=1763105#file1763105line82>
> >
> >     since we already pass in the entire sys, do we need to pass in sys.getSecurityService()
as well? Can we change the signaure of the constructor to take an InternalDistributedSystem,
and directly uses the security service inside the passed sys?
> 
> Kirk Lund wrote:
>     I'm trying to separate our major services instead of folding them in together. For
example, I don't want to go to Cache to get DistributedSystem (but you can) and I don't want
our code to go to DistributedSystem to get SecurityService. The link between DistributedSystem
and SecurityService is tenuous at best -- DistributedSystem USES SecurityService but it doesn't
logically own it except for the fact that DS is the 1st object in Geode that needs to use
it. So from the point-of-view of an object like HandShake, I'm trying to give it individual
services. This also has the side effect of making classes such as HandShake easier to unit
test when mocking the collaborators. Another reason I'm doing this is because it makes ALL
of the collorators visible at the constructor level which is a good practice to follow.
>     
>     If we want to start using Dependency Injection (which I want to), then we need to
do even more of this -- passing in every collaborator individually rather than passing in
a god object to invoke getters against on it to get the other dependencies.
>     
>     Or to summarize even more, I'm trying to steer away from bad code to good code (good
object-oriented design) and god objects are one of the worst anti-patterns.
> 
> Jinmei Liao wrote:
>     If you envisioned that security service will be rippsed out of ds in the future,
and wants to gear towards that way, then I can see your point. But from the current state
of things I would not call it bad o-o design though. If I see any method call that's in the
shape of callXYZ(o, o.getX(), o.getY(), o.getZ()), then I would question why bother pssing
in the last three parameters. To me, that's anti-pattern.

I'd much prefer to discuss object-oriented design principles outside of a simple bug fix.
You seem to not want me to make subtle improvements to class design. YOu need to understand
that I can't fix every class in Geode all at once -- I can only do it very gradually.

DistributedSystem is a god object [1] which is a pattern that we want to move away from.

Requiring HandShake to know that DistributedSystem (2nd arg to constructor) has a SecurityService
breaks the Law of Demeter [2].

As we change classes to use Dependency Injection [3], one visible difference. is that you're
going to see every logically separate dependency as a separate argument being passed into
the constructor.

Now ask yourself, is SecurityService logically a part of DistributedSystem? Such that it doesn't
make sense to use Security outside of the membership and distribution code?

Last but not least, we have organized our teams to utilize what upper management calls Inverse
Conway's Manuever [4]. Unfortunately this point has more to do with Pivotal and GemFire than
Geode. The point of Inverse Conway's Manuever (according to Elisabeth and Michael) is that
our code will gradually separate from the code owned by other teams and eventually become
so modular that we can even have separate git repos. The Communications team owns membership
and distribution (org.apache.geode.distributed). The Storage team owns caching (org.apache.geode.cache).
The M&M team owns Security (org.apache.geode.security) and Configuration. Unfortunately
Configuration is currently spaghettied throughout DistributedSystem and Cache (distributed
and cache packages). The only reason why InternalDistributedSystem currently touches SecurityService
is because it needs to inject SecurityService into the membership Services. We would potentially
create a new Configuration entity which handles the li
 fecycle of configuration in Geode instead of it being spread out within DistributedSystem
and Cache. So aside from breaking Law of Demeter, using system.getSecurityService() instead
of simply passing in SecurityService adds one more point of code that we have to change when
we tease apart SecurityService from both DistributedSystem and Cache.

I honestly don't know what else to tell you regarding this. I believe we need to stop having
these reviews break down into philosophical discussions about proper object oriented coding
and how each person involved on Geode wants to guide the code as it evolves towards better
modularity. Any questions about why I'm trying to do this?

[1] https://en.wikipedia.org/wiki/God_object
[2] https://en.wikipedia.org/wiki/Law_of_Demeter
[3] https://en.wikipedia.org/wiki/Dependency_injection
[4] https://www.thoughtworks.com/radar/techniques/inverse-conway-maneuver


- Kirk


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60375/#review178940
-----------------------------------------------------------


On June 29, 2017, 5:29 p.m., Kirk Lund wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60375/
> -----------------------------------------------------------
> 
> (Updated June 29, 2017, 5:29 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Ken Howe, and Patrick
Rhomberg.
> 
> 
> Bugs: GEODE-3117
>     https://issues.apache.org/jira/browse/GEODE-3117
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-3117: fix Gateway authentication with legacy security
> 
> * add GatewayLegacyAuthenticationRegressionTest to reproduce bug
> * fix authentication of Gateway sender/receiver with SECURITY_CLIENT_AUTHENTICATOR
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/cache/client/internal/ConnectionFactoryImpl.java
a419d575010d39d4dab6c5c8f9748928c1764344 
>   geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/HandShake.java
32735b9ab17fe9467cea46096bd177902145e4bd 
>   geode-wan/src/test/java/org/apache/geode/internal/cache/wan/misc/GatewayLegacyAuthenticationRegressionTest.java
PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60375/diff/2/
> 
> 
> Testing
> -------
> 
> * new test reproduces GEODE-3117: GatewayLegacyAuthenticationRegressionTest
> * precheckin passes
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message