----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64118/#review192040 -----------------------------------------------------------
This is looking realy good. You need to add upgrade logic and fix the ignored tests. ambari-server/src/main/java/org/apache/ambari/server/agent/HeartBeatHandler.java Lines 614-616 (original), 616-618 (patched) <https://reviews.apache.org/r/64118/#comment270028> It seems like `service`, `component`, and `principal` are not needed here anymore. Maybe we should remove them from the Heartbeat request and response structures. This will require some changes on the agent side as well. ambari-server/src/main/java/org/apache/ambari/server/agent/HeartBeatHandler.java Lines 640-642 (original), 650-652 (patched) <https://reviews.apache.org/r/64118/#comment270029> It seems like `service`, `component`, and `principal` are not needed here anymore. Maybe we should remove them from the Heartbeat request and response structures. This will require some changes on the agent side as well. ambari-server/src/main/java/org/apache/ambari/server/agent/HeartbeatProcessor.java Line 446 (original), 451 (patched) <https://reviews.apache.org/r/64118/#comment270027> It seems like `principal` may not be needed here anymore. Maybe we should remove it from the Heartbeat request and response structures. ambari-server/src/main/java/org/apache/ambari/server/agent/HeartbeatProcessor.java Lines 458-459 (original), 466-467 (patched) <https://reviews.apache.org/r/64118/#comment270031> This seems incorrect (I know it was pre-existing code). The same path for some keytab files may be on several hosts. If only a single host has a keytab file removed, it seems like the DB will be updated incorrectly and remove all entries for a keytab file path regardless of host. ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelperImpl.java Lines 470-471 (original), 480-481 (patched) <https://reviews.apache.org/r/64118/#comment270032> This should be reverted back. The existing implementation is correct. I assume this is a merge issue. ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelperImpl.java Lines 1902-1903 (original), 1921-1953 (patched) <https://reviews.apache.org/r/64118/#comment270034> It seems like this could be simplified using more _JPA-like_ logic. Someone with more experience with JPA (than me) should probably take a look. ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelperImpl.java Line 2421 (original), 2437-2441 (patched) <https://reviews.apache.org/r/64118/#comment270037> This syntax is typcially avoided. Are there any `com.google.common.collect` utilities that can be used? ambari-server/src/main/java/org/apache/ambari/server/orm/dao/KerberosKeytabDAO.java Line 57 (original), 60-68 (patched) <https://reviews.apache.org/r/64118/#comment270039> Cascading should be setup to avoid having to do this. ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/ConfigureAmbariIdentitiesServerAction.java Line 120 (original), 120 (patched) <https://reviews.apache.org/r/64118/#comment270055> Use `RootService.AMBARI.name()` instead of `"AMBARI"` ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/FinalizeKerberosServerAction.java Lines 63-64 (original), 59-60 (patched) <https://reviews.apache.org/r/64118/#comment270057> The logic below is needed when Kerberos is enabled during a Blueprint install. Due to the order of operations, any shared keytab files (like spnego.service.keytab) will be initially owned and accessible only by the user executing Ambari. This is due to the lack of accounts and groups on the Ambari server host. For example, when enabling Kerberos the Ambari server host's SPNEGO principal is created for Ambari as well as some services that may reside on that host. At the time the Ambari server's keytab files are created, the needed users and groups may not have been created. Like `${cluster-env/user_group}` which defaults to `hadoop`. ambari-server/src/main/resources/Ambari-DDL-Derby-CREATE.sql Lines 938 (patched) <https://reviews.apache.org/r/64118/#comment270070> Technially, this _unnatural_ key is not necessary since you have a valid _natural_ key - principal, keytab, component, service. ambari-server/src/main/resources/Ambari-DDL-Derby-CREATE.sql Lines 945 (patched) <https://reviews.apache.org/r/64118/#comment270071> Should host_id be part of this? ambari-server/src/main/resources/Ambari-DDL-MySQL-CREATE.sql Line 934 (original), 940 (patched) <https://reviews.apache.org/r/64118/#comment270062> This should be renamed to `PK_kerberos_keytab` ambari-server/src/main/resources/Ambari-DDL-MySQL-CREATE.sql Lines 963-965 (patched) <https://reviews.apache.org/r/64118/#comment270072> Test this on MySQL.. The creation of this table may fail due to lack of indices on the related fields. ambari-server/src/main/resources/Ambari-DDL-Oracle-CREATE.sql Line 913 (original), 919 (patched) <https://reviews.apache.org/r/64118/#comment270063> This should be renamed to `PK_kerberos_keytab` ambari-server/src/main/resources/Ambari-DDL-Postgres-CREATE.sql Line 916 (original), 922 (patched) <https://reviews.apache.org/r/64118/#comment270064> This should be renamed to `PK_kerberos_keytab` ambari-server/src/main/resources/Ambari-DDL-SQLAnywhere-CREATE.sql Line 912 (original), 918 (patched) <https://reviews.apache.org/r/64118/#comment270065> This should be renamed to `PK_kerberos_keytab` ambari-server/src/main/resources/Ambari-DDL-SQLServer-CREATE.sql Line 936 (original), 942 (patched) <https://reviews.apache.org/r/64118/#comment270066> This should be renamed to `PK_kerberos_keytab` - Robert Levas On Nov. 28, 2017, 8:17 a.m., Eugene Chekanskiy wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64118/ > ----------------------------------------------------------- > > (Updated Nov. 28, 2017, 8:17 a.m.) > > > Review request for Ambari, Attila Magyar, Balázs Bence Sári, and Robert Levas. > > > Bugs: AMBARI-22530 > https://issues.apache.org/jira/browse/AMBARI-22530 > > > Repository: ambari > > > Description > ------- > > Moved out of data files for principal generation. > Now before every kerberos action all info written to database. > > Server actions determine what principals to process based on filters passed > to action, this allow: > > * selective generation > * split generation process per-host/per-service/per-component basis and make > generation concurrent for seperate parts > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/agent/HeartBeatHandler.java > 53cceb0ded > > ambari-server/src/main/java/org/apache/ambari/server/agent/HeartbeatProcessor.java > 83d2c9808d > > ambari-server/src/main/java/org/apache/ambari/server/controller/DeleteIdentityHandler.java > a7b9d80df0 > > ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelper.java > 749943dc00 > > ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelperImpl.java > ab85aa1d7c > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/KerberosKeytabDAO.java > a8723b7bfa > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/KerberosKeytabPrincipalDAO.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/HostGroupComponentEntityPK.java > 0898133bb5 > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/KerberosKeytabEntity.java > a25931b946 > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/KerberosKeytabPrincipalEntity.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/KerberosKeytabPrincipalEntityPK.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/KerberosPrincipalHostEntity.java > d4e80c65d2 > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/AbstractPrepareKerberosServerAction.java > b8affb4e19 > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/CleanupServerAction.java > 002076d85c > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/ConfigureAmbariIdentitiesServerAction.java > 338415280f > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/CreateKeytabFilesServerAction.java > 5ec4c1011e > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/CreatePrincipalsServerAction.java > 0c906592af > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/DestroyPrincipalsServerAction.java > 4c80bd425e > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/FinalizeKerberosServerAction.java > bfd5e4036d > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosServerAction.java > ff5f5cef1c > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/PrepareEnableKerberosServerAction.java > 671ad95c8f > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/PrepareKerberosIdentitiesServerAction.java > 83a2106afd > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/stageutils/KerberosKeytabController.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/stageutils/ResolvedKerberosKeytab.java > 17e484ad10 > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/stageutils/ResolvedKerberosPrincipal.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/upgrades/PreconfigureKerberosAction.java > ca78dbb8a2 > ambari-server/src/main/resources/Ambari-DDL-Derby-CREATE.sql 7045240b30 > ambari-server/src/main/resources/Ambari-DDL-MySQL-CREATE.sql c950c7ef83 > ambari-server/src/main/resources/Ambari-DDL-Oracle-CREATE.sql 537ae196c5 > ambari-server/src/main/resources/Ambari-DDL-Postgres-CREATE.sql b4952c2e86 > ambari-server/src/main/resources/Ambari-DDL-SQLAnywhere-CREATE.sql > 4fb0d0981a > ambari-server/src/main/resources/Ambari-DDL-SQLServer-CREATE.sql 8a88aba905 > ambari-server/src/main/resources/META-INF/persistence.xml 686c8312cd > > ambari-server/src/test/java/org/apache/ambari/server/agent/TestHeartbeatHandler.java > b4ff5c10ab > > ambari-server/src/test/java/org/apache/ambari/server/controller/KerberosHelperTest.java > ee87d24d8a > ambari-server/src/test/java/org/apache/ambari/server/orm/db/DDLTests.java > 96cf64e53c > > ambari-server/src/test/java/org/apache/ambari/server/serveraction/kerberos/FinalizeKerberosServerActionTest.java > c9301f3b7e > > ambari-server/src/test/java/org/apache/ambari/server/serveraction/kerberos/KerberosServerActionTest.java > e6f0868360 > > ambari-server/src/test/java/org/apache/ambari/server/serveraction/upgrades/PreconfigureKerberosActionTest.java > a08f7a0a25 > > > Diff: https://reviews.apache.org/r/64118/diff/1/ > > > Testing > ------- > > manual testing, all kind of regenerate keytabs > unit tests updating in progress > > > Thanks, > > Eugene Chekanskiy > >
