> 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 > >
