> On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/catalog/AbstractCatalogService.java, > > line 41 > > <https://reviews.apache.org/r/17580/diff/1/?file=457491#file457491line41> > > > > Why is the metaStorePrincipal only in few methods ? Looks like they are > > the initial set of calls into the service and would end up caching the > > client object. But that is not a good assumption to make and can possibly > > introduce errors. Would prefer that either metaStorePrinicipal is in every > > call or allow something similar doAs functionality. > > Seetharam Venkatesh wrote: > Only these 2 methods are invoked from with in falcon server but > everything else is with in oozie namespace which oozie handles. May be I can > add a comment. > > Srikanth Sundarrajan wrote: > Will prefer that a new method say "login" added to AbstractCatalogService > that isAlive or tableExists or any other method can probably use.
Well, thats what is done with getProxiedClient(catalogUrl, metaStorePrincipal); Its not a login. I;m not sure if this makes sense here. > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/entity/store/ConfigurationStore.java, > > line 101 > > <https://reviews.apache.org/r/17580/diff/1/?file=457502#file457502line101> > > > > This would set the umask for all files created henceforth through that > > file system object (which by the way is cached). There may be unwanted side > > effects with this. Can we simply set permission on the storePath dir > > instead ? > > Seetharam Venkatesh wrote: > Thats a good point, I tried with FsPermissions instead but did not work. > Its funny that I have run into issues a few time. But will change. > > Srikanth Sundarrajan wrote: > should be > FsPermission permission = new FsPermission(FsAction.ALL, > FsAction.READ_EXECUTE, FsAction.NONE); > instead of > FsPermission permission = new FsPermission(FsAction.ALL, > FsAction.EXECUTE, FsAction.NONE); > Why? Falcon owns this and others will be able to list but not read. its intentional. > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/util/Preconditions.java, line 1 > > <https://reviews.apache.org/r/17580/diff/1/?file=457510#file457510line1> > > > > Guava ? > > Seetharam Venkatesh wrote: > Thought about it but just for one class? > > Srikanth Sundarrajan wrote: > There are numerous places where Guava can help. We should track this > separately. Meanwhile does it help to use commons-lang:Validate instead of > adding another class ? Another jira pls. > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > prism/src/main/java/org/apache/falcon/security/BasicAuthFilter.java, line 46 > > <https://reviews.apache.org/r/17580/diff/1/?file=457542#file457542line46> > > > > Might be handy to have more java docs in here to explain a new reader > > on what is the role of the Options servlet and how are things wired up > > Seetharam Venkatesh wrote: > Its all documented in Hadoop-auth. If you look at security docs, they > contain a link. > > Srikanth Sundarrajan wrote: > Links here are broken: > http://hadoop.apache.org/docs/current/hadoop-auth/Configuration.html, We > ought to file jira with the hadoop team I guess. why should this block this issue? > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > prism/src/main/java/org/apache/falcon/security/BasicAuthFilter.java, line 63 > > <https://reviews.apache.org/r/17580/diff/1/?file=457542#file457542line63> > > > > Sane defaults have been removed. Is it intentional ? > > Seetharam Venkatesh wrote: > Yes, this was too restrictive and hard to change. Instead, I have it > wired up from the startup props file which is better. > > Srikanth Sundarrajan wrote: > Please remember to add this as an incompatible change in the CHANGES file > if we remove the defaults. adding it in startup.properties should not be hard. Also that list was incomplete anyways. > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > rerun/src/main/java/org/apache/falcon/latedata/LateDataHandler.java, line > > 139 > > <https://reviews.apache.org/r/17580/diff/1/?file=457549#file457549line139> > > > > Except for detectChanges() all other functions are invoked in the MR > > job, which is already running as the workflow user. This might not work > > Seetharam Venkatesh wrote: > Good catch, its unnecessary. > > Seetharam Venkatesh wrote: > Well, after looking deeper, I think this is a necessary evil. > org.apache.falcon.rerun.handler.LateRerunConsumer#detectLate invokes > org.apache.falcon.latedata.LateDataHandler#computeStorageMetric in falcon > server to recompute and see if late needs to be run in > org.apache.falcon.latedata.LateDataHandler#detectChanges. > > A proxy ugi for the login user does not hurt. > > An alternate would be to make 755 for latedata dir under logs dir owned > by falcon and falcon should be able to read the recorded size. makes sense? > > Srikanth Sundarrajan wrote: > Can fs be passed to detectChanges to avoid creating proxies FS in > incorrect places. User may not be able to proxy as themselves unless they are > superuser. FS is not proxied here but is created for that user. > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > rerun/src/main/java/org/apache/falcon/latedata/LateDataHandler.java, line > > 219 > > <https://reviews.apache.org/r/17580/diff/1/?file=457549#file457549line219> > > > > Except for detectChanges() all other functions are invoked in the MR > > job, which is already running as the workflow user. This might not work > > Seetharam Venkatesh wrote: > Good catch, its unnecessary. This is taken care of. > On Jan. 31, 2014, 9:46 a.m., Srikanth Sundarrajan wrote: > > webapp/src/test/java/org/apache/falcon/cli/FalconCLIIT.java, line 514 > > <https://reviews.apache.org/r/17580/diff/1/?file=457572#file457572line514> > > > > This section is commented out. Intentional? > > Seetharam Venkatesh wrote: > Not sure which one here. Line 508 does not have a comment. It it: > Assert.assertEquals(0, > new FalconCLI().run(("admin -stack -url " + > TestContext.BASE_URL).split("\\s"))); > > > Srikanth Sundarrajan wrote: > testGetVersion and some tests in testClientProperties have been disabled. > Any specific reason ? Hmmm....need to see. - Seetharam ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17580/#review33296 ----------------------------------------------------------- On Feb. 14, 2014, 6:20 a.m., Srikanth Sundarrajan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17580/ > ----------------------------------------------------------- > > (Updated Feb. 14, 2014, 6:20 a.m.) > > > Review request for Falcon. > > > Bugs: FALCON-11, FALCON-14, FALCON-15, FALCON-16, FALCON-17, FALCON-18, > FALCON-219, and FALCON-220 > https://issues.apache.org/jira/browse/FALCON-11 > https://issues.apache.org/jira/browse/FALCON-14 > https://issues.apache.org/jira/browse/FALCON-15 > https://issues.apache.org/jira/browse/FALCON-16 > https://issues.apache.org/jira/browse/FALCON-17 > https://issues.apache.org/jira/browse/FALCON-18 > https://issues.apache.org/jira/browse/FALCON-219 > https://issues.apache.org/jira/browse/FALCON-220 > > > Repository: falcon-git > > > Description > ------- > > FALCON-11 Add support for security in Falcon > > > Diffs > ----- > > client/pom.xml a43f7f5 > client/src/main/java/org/apache/falcon/client/FalconClient.java 367fcc5 > common/pom.xml 068a22c > common/src/main/java/org/apache/falcon/catalog/AbstractCatalogService.java > 691d805 > common/src/main/java/org/apache/falcon/catalog/HiveCatalogService.java > 51e4d6e > common/src/main/java/org/apache/falcon/cleanup/AbstractCleanupHandler.java > 644afd2 > common/src/main/java/org/apache/falcon/cleanup/FeedCleanupHandler.java > 7dbac58 > common/src/main/java/org/apache/falcon/entity/CatalogStorage.java 32f7605 > common/src/main/java/org/apache/falcon/entity/ClusterHelper.java 38b5c5b > common/src/main/java/org/apache/falcon/entity/EntityUtil.java a3ad83d > common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java > 68370c7 > common/src/main/java/org/apache/falcon/entity/Storage.java 0634969 > > common/src/main/java/org/apache/falcon/entity/parser/ClusterEntityParser.java > e633838 > common/src/main/java/org/apache/falcon/entity/parser/FeedEntityParser.java > 5c1d9ad > > common/src/main/java/org/apache/falcon/entity/parser/ProcessEntityParser.java > 8647d43 > common/src/main/java/org/apache/falcon/entity/store/ConfigurationStore.java > 18ceb6e > common/src/main/java/org/apache/falcon/hadoop/HadoopClientFactory.java > PRE-CREATION > > common/src/main/java/org/apache/falcon/security/AuthenticationInitializationService.java > PRE-CREATION > common/src/main/java/org/apache/falcon/security/CurrentUser.java 4d2299e > common/src/main/java/org/apache/falcon/security/FalconLoginModule.java > d95e147 > > common/src/main/java/org/apache/falcon/security/FalconSecurityConfiguration.java > b80ab6d > common/src/main/java/org/apache/falcon/security/SecurityConstants.java > 8f7ba4a > common/src/main/java/org/apache/falcon/security/SecurityUtil.java > PRE-CREATION > common/src/main/java/org/apache/falcon/update/UpdateHelper.java 7ed4394 > common/src/main/java/org/apache/falcon/util/Preconditions.java PRE-CREATION > common/src/main/resources/startup.properties 3014418 > common/src/test/java/org/apache/falcon/entity/AbstractTestBase.java 7668c7f > common/src/test/java/org/apache/falcon/entity/FileSystemStorageTest.java > 7b48d2b > common/src/test/java/org/apache/falcon/hadoop/HadoopClientFactoryTest.java > PRE-CREATION > > common/src/test/java/org/apache/falcon/security/AuthenticationInitializationServiceTest.java > PRE-CREATION > common/src/test/java/org/apache/falcon/security/SecurityUtilTest.java > PRE-CREATION > common/src/test/java/org/apache/falcon/util/PreconditionsTest.java > PRE-CREATION > docs/src/site/twiki/Security.twiki PRE-CREATION > docs/src/site/twiki/index.twiki 81c4c3e > docs/src/site/twiki/restapi/AdminConfig.twiki 2841b25 > docs/src/site/twiki/restapi/AdminStack.twiki a241999 > docs/src/site/twiki/restapi/AdminVersion.twiki fbf1405 > docs/src/site/twiki/restapi/EntityDefinition.twiki 955be71 > docs/src/site/twiki/restapi/EntityDelete.twiki 7a7e22a > docs/src/site/twiki/restapi/EntityDependencies.twiki 6daab68 > docs/src/site/twiki/restapi/EntityList.twiki bca84b0 > docs/src/site/twiki/restapi/EntityResume.twiki 223a83f > docs/src/site/twiki/restapi/EntitySchedule.twiki e481613 > docs/src/site/twiki/restapi/EntityStatus.twiki f0e772b > docs/src/site/twiki/restapi/EntitySubmit.twiki e4b608e > docs/src/site/twiki/restapi/EntitySubmitAndSchedule.twiki fb3649d > docs/src/site/twiki/restapi/EntitySuspend.twiki 9d6e9ab > docs/src/site/twiki/restapi/EntityUpdate.twiki 16ec439 > docs/src/site/twiki/restapi/EntityValidate.twiki bc0f508 > docs/src/site/twiki/restapi/InstanceKill.twiki 5c429f6 > docs/src/site/twiki/restapi/InstanceLogs.twiki f84b828 > docs/src/site/twiki/restapi/InstanceRerun.twiki cf35475 > docs/src/site/twiki/restapi/InstanceResume.twiki 2bdd6e1 > docs/src/site/twiki/restapi/InstanceRunning.twiki 6b5ee66 > docs/src/site/twiki/restapi/InstanceStatus.twiki eddc2c8 > docs/src/site/twiki/restapi/InstanceSuspend.twiki 62cf72b > docs/src/site/twiki/restapi/ResourceList.twiki b9ec4b6 > feed/src/main/java/org/apache/falcon/converter/OozieFeedMapper.java 6ca2134 > feed/src/main/resources/config/workflow/replication-workflow.xml 91d0285 > feed/src/main/resources/config/workflow/retention-workflow.xml 8b444f5 > feed/src/test/java/org/apache/falcon/converter/OozieFeedMapperTest.java > a37755b > hadoop-webapp/pom.xml e576310 > html5-ui/js/falcon.js ff3a929 > messaging/pom.xml 9aa5347 > > messaging/src/main/java/org/apache/falcon/messaging/EntityInstanceMessage.java > eb49fd5 > > messaging/src/main/java/org/apache/falcon/messaging/EntityInstanceMessageCreator.java > ecda5eb > messaging/src/main/java/org/apache/falcon/messaging/MessageProducer.java > b37931c > > messaging/src/test/java/org/apache/falcon/messaging/FalconTopicProducerTest.java > da126c7 > messaging/src/test/java/org/apache/falcon/messaging/FeedProducerTest.java > e707567 > > messaging/src/test/java/org/apache/falcon/messaging/ProcessProducerTest.java > 3a40e76 > metrics/src/main/java/org/apache/falcon/aspect/GenericAlert.java 275a725 > > oozie/src/main/java/org/apache/falcon/converter/AbstractOozieEntityMapper.java > cecdeef > oozie/src/main/java/org/apache/falcon/logging/LogMover.java d544311 > oozie/src/main/java/org/apache/falcon/logging/LogProvider.java 48d4589 > > oozie/src/main/java/org/apache/falcon/service/SharedLibraryHostingService.java > 37f8cfa > oozie/src/main/java/org/apache/falcon/workflow/FalconPostProcessing.java > 3f9256c > > oozie/src/main/java/org/apache/falcon/workflow/engine/OozieClientFactory.java > b757531 > > oozie/src/main/java/org/apache/falcon/workflow/engine/OozieHouseKeepingService.java > 068e980 > > oozie/src/main/java/org/apache/falcon/workflow/engine/OozieWorkflowEngine.java > 71ff430 > oozie/src/main/java/org/apache/oozie/client/CustomOozieClient.java c55221e > oozie/src/main/java/org/apache/oozie/client/ProxyOozieClient.java > PRE-CREATION > > oozie/src/test/java/org/apache/falcon/oozie/workflow/FalconPostProcessingTest.java > c6485cd > pom.xml fca01b5 > prism/src/main/java/org/apache/falcon/resource/channel/HTTPChannel.java > 2ba76a7 > prism/src/main/java/org/apache/falcon/security/BasicAuthFilter.java f172e82 > > prism/src/main/java/org/apache/falcon/security/RemoteUserInHeaderBasedAuthenticationHandler.java > PRE-CREATION > prism/src/main/java/org/apache/falcon/service/FalconTopicSubscriber.java > 6ac926d > prism/src/main/java/org/apache/falcon/service/ProcessSubscriberService.java > 1cd7776 > prism/src/test/java/org/apache/falcon/aspect/GenericAlertTest.java 1db71fd > > prism/src/test/java/org/apache/falcon/service/FalconTopicSubscriberTest.java > f1536f4 > process/src/main/java/org/apache/falcon/converter/OozieProcessMapper.java > 87be709 > process/src/main/resources/config/workflow/process-parent-workflow.xml > 494bf20 > > process/src/test/java/org/apache/falcon/converter/OozieProcessMapperTest.java > 61ddbdc > rerun/src/main/java/org/apache/falcon/latedata/LateDataHandler.java 4b35760 > rerun/src/main/java/org/apache/falcon/rerun/event/LaterunEvent.java b5ac121 > rerun/src/main/java/org/apache/falcon/rerun/event/RerunEvent.java baf4601 > rerun/src/main/java/org/apache/falcon/rerun/event/RerunEventFactory.java > 03230f9 > rerun/src/main/java/org/apache/falcon/rerun/event/RetryEvent.java 1396f19 > > rerun/src/main/java/org/apache/falcon/rerun/handler/AbstractRerunConsumer.java > b073117 > > rerun/src/main/java/org/apache/falcon/rerun/handler/AbstractRerunHandler.java > ab7f472 > rerun/src/main/java/org/apache/falcon/rerun/handler/LateRerunConsumer.java > fffd5cd > rerun/src/main/java/org/apache/falcon/rerun/handler/LateRerunHandler.java > 897e7ab > rerun/src/main/java/org/apache/falcon/rerun/handler/RetryConsumer.java > 63dade8 > rerun/src/main/java/org/apache/falcon/rerun/handler/RetryHandler.java > 2b41a7c > rerun/src/main/java/org/apache/falcon/rerun/queue/InMemoryQueue.java > 8234d8a > > rerun/src/test/java/org/apache/falcon/rerun/handler/TestLateRerunHandler.java > e02b495 > rerun/src/test/java/org/apache/falcon/rerun/queue/ActiveMQTest.java 01d0415 > rerun/src/test/java/org/apache/falcon/rerun/queue/InMemoryQueueTest.java > 6aafaa5 > src/bin/falcon d196a5d > src/conf/log4j.xml 58ebd80 > src/conf/startup.properties 79cd211 > test-util/src/main/java/org/apache/falcon/cluster/util/EmbeddedCluster.java > 2b55407 > webapp/pom.xml 8c37409 > webapp/src/conf/oozie/conf/oozie-site.xml e5f404a > webapp/src/main/resources/log4j.xml d133b8e > webapp/src/test/java/org/apache/falcon/catalog/HiveCatalogServiceIT.java > 9909140 > webapp/src/test/java/org/apache/falcon/cli/FalconCLIIT.java 72369c0 > webapp/src/test/java/org/apache/falcon/cli/FalconCLISmokeIT.java 55f240f > webapp/src/test/java/org/apache/falcon/late/LateDataHandlerIT.java 6cfa4e6 > > webapp/src/test/java/org/apache/falcon/lifecycle/FileSystemFeedReplicationIT.java > 058b35c > > webapp/src/test/java/org/apache/falcon/lifecycle/TableStorageFeedEvictorIT.java > 37226e2 > > webapp/src/test/java/org/apache/falcon/lifecycle/TableStorageFeedReplicationIT.java > dbc6442 > webapp/src/test/java/org/apache/falcon/process/PigProcessIT.java 1f4e9e8 > webapp/src/test/java/org/apache/falcon/process/TableStorageProcessIT.java > 2d539c2 > webapp/src/test/java/org/apache/falcon/resource/EntityManagerJerseyIT.java > 6dc1e3f > > webapp/src/test/java/org/apache/falcon/resource/EntityManagerJerseySmokeIT.java > b96aa48 > > webapp/src/test/java/org/apache/falcon/resource/ProcessInstanceManagerIT.java > c0785ba > webapp/src/test/java/org/apache/falcon/resource/TestContext.java 9e10956 > webapp/src/test/java/org/apache/falcon/security/BasicAuthFilterTest.java > 0ff993b > webapp/src/test/java/org/apache/falcon/util/OozieTestUtils.java 3769dde > > webapp/src/test/java/org/apache/falcon/util/ResourcesReflectionUtilTest.java > e54f81c > > webapp/src/test/java/org/apache/falcon/validation/ClusterEntityValidationIT.java > 9299b5b > > webapp/src/test/java/org/apache/falcon/validation/FeedEntityValidationIT.java > db24b9a > > Diff: https://reviews.apache.org/r/17580/diff/ > > > Testing > ------- > > > Thanks, > > Srikanth Sundarrajan > >
