aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Kevin Sweeney" <kevi...@apache.org>
Subject Re: Review Request 32559: Add Kerberos support to the scheduler
Date Fri, 03 Apr 2015 21:18:19 GMT


> On April 2, 2015, 10:23 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizeHeaderToken.java,
line 46
> > <https://reviews.apache.org/r/32559/diff/4/?file=908315#file908315line46>
> >
> >     What does this mean?  Perhaps the missing context is "<other component x>
will do this part".

Reworded.


> On April 2, 2015, 10:23 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizeHeaderToken.java,
line 53
> > <https://reviews.apache.org/r/32559/diff/4/?file=908315#file908315line53>
> >
> >     Marking this method as deprecated is slightly confusing.  I assume it's only
because you don't want other code in the project to call it?  If that's the case, i suggest
you don't use deprecated as the signal and instead docs + code review.

Done. I was attempting to follow the guava convention that uses this to bring attention to
potential errors with this annotation, but will not overload its use here.


> On April 2, 2015, 10:23 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/IniShiroRealmModule.java,
line 34
> > <https://reviews.apache.org/r/32559/diff/4/?file=908316#file908316line34>
> >
> >     `<p>`

Fixed.


> On April 2, 2015, 10:23 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/IniShiroRealmModule.java,
line 35
> > <https://reviews.apache.org/r/32559/diff/4/?file=908316#file908316line35>
> >
> >     "it" is ambiguous.  Consider rewording: "Another filter may still be used for
authentication, in which case the ini file can still be used to provide authorization configuration."

done.


> On April 2, 2015, 10:23 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5Realm.java,
line 36
> > <https://reviews.apache.org/r/32559/diff/4/?file=908317#file908317line36>
> >
> >     comment formatting is off

fixed.


> On April 2, 2015, 10:23 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5Realm.java,
line 46
> > <https://reviews.apache.org/r/32559/diff/4/?file=908317#file908317line46>
> >
> >     nonNull?

fixed.


> On April 2, 2015, 10:23 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java,
lines 56-57
> > <https://reviews.apache.org/r/32559/diff/4/?file=908318#file908318line56>
> >
> >     These are pure magic to me.  Please doc.

Doc'd.


> On April 2, 2015, 10:23 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java,
lines 114-115
> > <https://reviews.apache.org/r/32559/diff/4/?file=908318#file908318line114>
> >
> >     This is a question of convention - what's the upside to addError/return as opposed
to throwing?

addError lets you get a bunch of errors at once whereas throwing aborts injector creation.


> On April 2, 2015, 10:23 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java,
line 113
> > <https://reviews.apache.org/r/32559/diff/4/?file=908318#file908318line113>
> >
> >     Why is this `Optional` if it's required?  Ditto below.

The command-line argument is nullable (can't use constraints since that would make them required
even if you weren't using Kerberos). The alternative is doing requireNonNull in the constructor,
which will produce a worse error message than the one below. Optional was chosen over a null
check to be explicit.


> On April 2, 2015, 10:23 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java,
line 147
> > <https://reviews.apache.org/r/32559/diff/4/?file=908318#file908318line147>
> >
> >     s/jass/jaas/

fixed.


> On April 2, 2015, 10:23 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java,
lines 124-141
> > <https://reviews.apache.org/r/32559/diff/4/?file=908318#file908318line124>
> >
> >     Given the behavior i see, i'd prefer a constant and String.format over the resource
file and use of StringTemplate.
> >     
> >     If there are reasons for retaining this complexity, please leave a comment justifying
it.

Ok dropped.


> On April 2, 2015, 10:23 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java,
lines 182-186
> > <https://reviews.apache.org/r/32559/diff/4/?file=908318#file908318line182>
> >
> >     Why is this in a `finally`?  Seems like any exception is fatal, and there's
no more work to do.  Use of a finally block makes the expectations of this code confusing.

I'm patching a global system property and unpatching it when I leave this block since it's
not my resource to manage. This is similar to using a contextmanager in python - the effect
of the property change shouldn't be visible once we've left this code block no matter how
we left it


> On April 2, 2015, 10:23 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java,
line 200
> > <https://reviews.apache.org/r/32559/diff/4/?file=908318#file908318line200>
> >
> >     remove newline

done.


> On April 2, 2015, 10:23 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosAuthenticationFilter.java,
line 75
> > <https://reviews.apache.org/r/32559/diff/4/?file=908320#file908320line75>
> >
> >     Please use a consistent exception logging convention - the exception logged
on :67 is different.

made consistent


> On April 2, 2015, 10:23 a.m., Bill Farner wrote:
> > src/test/sh/org/apache/aurora/e2e/test_kerberos_end_to_end.sh, line 1
> > <https://reviews.apache.org/r/32559/diff/4/?file=908329#file908329line1>
> >
> >     Please invoke this from the existing end-to-end test script to avoid another
qualification step for developers.

Done, but that script appears to be broken on master.


- Kevin


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


On March 27, 2015, 1:50 p.m., Kevin Sweeney wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32559/
> -----------------------------------------------------------
> 
> (Updated March 27, 2015, 1:50 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-812
>     https://issues.apache.org/jira/browse/AURORA-812
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Support authenticating to the scheduler API with Kerberos.
> 
> 
> Diffs
> -----
> 
>   config/findbugs/excludeFilter.xml 5ff5f87d6bb7d8bf3d9ecb1bc6c6b1a524d1bf15 
>   examples/vagrant/provision-dev-cluster.sh ae500436e703140065e5c16fc0e38dbe3214e69f

>   examples/vagrant/upstart/aurora-scheduler-kerberos.conf PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java
ec6a02c4086ee0d5a7529083030d978ea889f677 
>   src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizeHeaderToken.java
PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/http/api/security/IniShiroRealmModule.java
58b559e96402ef282dc0e5ac2de0930906256487 
>   src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5Realm.java PRE-CREATION

>   src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java
PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/http/api/security/KerberosPrincipalParser.java
PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosAuthenticationFilter.java
PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroUtils.java PRE-CREATION

>   src/main/python/apache/aurora/client/hooks/BUILD 49d73dfddbcee97ddde1a483f6697865d2566508

>   src/main/resources/org/apache/aurora/scheduler/http/api/security/jaas.conf.st PRE-CREATION

>   src/test/java/org/apache/aurora/scheduler/http/api/security/AuthorizeHeaderTokenTest.java
PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModuleTest.java
PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/api/security/KerberosPrincipalParserTest.java
PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosAuthenticationFilterTest.java
PRE-CREATION 
>   src/test/python/apache/aurora/client/hooks/BUILD 43f59bc5a2397ad851211e064d83ef6e6de81972

>   src/test/sh/org/apache/aurora/e2e/test_kerberos_end_to_end.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/32559/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/test_kerberos_end_to_end.sh
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>


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