You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ambari.apache.org by John Speidel <js...@hortonworks.com> on 2015/02/14 02:00:30 UTC
Review Request 31035: Allow the KDC admin credentials stored in
session to be validated via the REST API
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31035/
-----------------------------------------------------------
Review request for Ambari, Robert Levas, Tom Beerbower, and Yusaku Sako.
Bugs: AMBARI-9640
https://issues.apache.org/jira/browse/AMBARI-9640
Repository: ambari
Description
-------
Add the following properties to the Kerberos service resource which provide the results of the validation of the KDC admin credentials stored in session: "Services/attributes/kdc_validation_result" and "Services/attributes/kdc_validation_failure_details".
See the associated Jira for more details including full api responses.
Diffs
-----
ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelper.java 2e68c7d
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java 40921f5
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ServiceResourceProviderTest.java 210c6a4
Diff: https://reviews.apache.org/r/31035/diff/
Testing
-------
Manual Functional Testing:
- Tested all possible results of the validation
- Tested when no kerberos service existed
Unit Tests:
- Added new unit tests
- Running entire unit test suite now and will post results prior to merging
Thanks,
John Speidel
Re: Review Request 31035: Allow the KDC admin credentials stored in
session to be validated via the REST API
Posted by John Speidel <js...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31035/#review72516
-----------------------------------------------------------
Unit test results:
Tests run: 2691, Failures: 0, Errors: 0, Skipped: 15
...
Total run:594
Total errors:0
Total failures:0
- John Speidel
On Feb. 14, 2015, 9:30 p.m., John Speidel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31035/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2015, 9:30 p.m.)
>
>
> Review request for Ambari, Robert Levas, Tom Beerbower, and Yusaku Sako.
>
>
> Bugs: AMBARI-9640
> https://issues.apache.org/jira/browse/AMBARI-9640
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Add the following properties to the Kerberos service resource which provide the results of the validation of the KDC admin credentials stored in session: "Services/attributes/kdc_validation_result" and "Services/attributes/kdc_validation_failure_details".
>
> See the associated Jira for more details including full api responses.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelper.java 2e68c7d
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java 40921f5
> ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosInvalidConfigurationException.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosMissingAdminCredentialsException.java PRE-CREATION
> ambari-server/src/test/java/org/apache/ambari/server/controller/KerberosHelperTest.java 136615e
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ServiceResourceProviderTest.java 210c6a4
>
> Diff: https://reviews.apache.org/r/31035/diff/
>
>
> Testing
> -------
>
> Manual Functional Testing:
> - Tested all possible results of the validation
> - Tested when no kerberos service existed
>
> Unit Tests:
> - Added new unit tests
> - Running entire unit test suite now and will post results prior to merging
>
>
> Thanks,
>
> John Speidel
>
>
Re: Review Request 31035: Allow the KDC admin credentials stored in
session to be validated via the REST API
Posted by John Speidel <js...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31035/
-----------------------------------------------------------
(Updated Feb. 14, 2015, 9:30 p.m.)
Review request for Ambari, Robert Levas, Tom Beerbower, and Yusaku Sako.
Changes
-------
Addresses issues raised in review.
Bugs: AMBARI-9640
https://issues.apache.org/jira/browse/AMBARI-9640
Repository: ambari
Description
-------
Add the following properties to the Kerberos service resource which provide the results of the validation of the KDC admin credentials stored in session: "Services/attributes/kdc_validation_result" and "Services/attributes/kdc_validation_failure_details".
See the associated Jira for more details including full api responses.
Diffs (updated)
-----
ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelper.java 2e68c7d
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java 40921f5
ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosInvalidConfigurationException.java PRE-CREATION
ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosMissingAdminCredentialsException.java PRE-CREATION
ambari-server/src/test/java/org/apache/ambari/server/controller/KerberosHelperTest.java 136615e
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ServiceResourceProviderTest.java 210c6a4
Diff: https://reviews.apache.org/r/31035/diff/
Testing
-------
Manual Functional Testing:
- Tested all possible results of the validation
- Tested when no kerberos service existed
Unit Tests:
- Added new unit tests
- Running entire unit test suite now and will post results prior to merging
Thanks,
John Speidel
Re: Review Request 31035: Allow the KDC admin credentials stored in
session to be validated via the REST API
Posted by Tom Beerbower <tb...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31035/#review72481
-----------------------------------------------------------
Ship it!
Looks good.
- Tom Beerbower
On Feb. 14, 2015, 1 a.m., John Speidel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31035/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2015, 1 a.m.)
>
>
> Review request for Ambari, Robert Levas, Tom Beerbower, and Yusaku Sako.
>
>
> Bugs: AMBARI-9640
> https://issues.apache.org/jira/browse/AMBARI-9640
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Add the following properties to the Kerberos service resource which provide the results of the validation of the KDC admin credentials stored in session: "Services/attributes/kdc_validation_result" and "Services/attributes/kdc_validation_failure_details".
>
> See the associated Jira for more details including full api responses.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelper.java 2e68c7d
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java 40921f5
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ServiceResourceProviderTest.java 210c6a4
>
> Diff: https://reviews.apache.org/r/31035/diff/
>
>
> Testing
> -------
>
> Manual Functional Testing:
> - Tested all possible results of the validation
> - Tested when no kerberos service existed
>
> Unit Tests:
> - Added new unit tests
> - Running entire unit test suite now and will post results prior to merging
>
>
> Thanks,
>
> John Speidel
>
>
Re: Review Request 31035: Allow the KDC admin credentials stored in
session to be validated via the REST API
Posted by John Speidel <js...@hortonworks.com>.
> On Feb. 14, 2015, 1:12 a.m., Rishi Pidva wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java, line 1446
> > <https://reviews.apache.org/r/31035/diff/1/?file=864036#file864036line1446>
> >
> > How about using ErrorCodes instead of searching for text startsWith()? Something like:
> >
> > ERR-11001: Invalid KDC administrator credentials......
Yeah, I was really just taking the easy route here trying to get this in quickly so I left the exception handling as it was in KerberosHelper when I extracted the new validate method.
I agree that using string comparison on the error message is dangerous and fragile.
I will change the new validate method in KerberosHelper to throw distinct kerberos exceptions.
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31035/#review72468
-----------------------------------------------------------
On Feb. 14, 2015, 1 a.m., John Speidel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31035/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2015, 1 a.m.)
>
>
> Review request for Ambari, Robert Levas, Tom Beerbower, and Yusaku Sako.
>
>
> Bugs: AMBARI-9640
> https://issues.apache.org/jira/browse/AMBARI-9640
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Add the following properties to the Kerberos service resource which provide the results of the validation of the KDC admin credentials stored in session: "Services/attributes/kdc_validation_result" and "Services/attributes/kdc_validation_failure_details".
>
> See the associated Jira for more details including full api responses.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelper.java 2e68c7d
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java 40921f5
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ServiceResourceProviderTest.java 210c6a4
>
> Diff: https://reviews.apache.org/r/31035/diff/
>
>
> Testing
> -------
>
> Manual Functional Testing:
> - Tested all possible results of the validation
> - Tested when no kerberos service existed
>
> Unit Tests:
> - Added new unit tests
> - Running entire unit test suite now and will post results prior to merging
>
>
> Thanks,
>
> John Speidel
>
>
Re: Review Request 31035: Allow the KDC admin credentials stored in
session to be validated via the REST API
Posted by Rishi Pidva <rp...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31035/#review72468
-----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java
<https://reviews.apache.org/r/31035/#comment118535>
How about using ErrorCodes instead of searching for text startsWith()? Something like:
ERR-11001: Invalid KDC administrator credentials......
- Rishi Pidva
On Feb. 13, 2015, 5 p.m., John Speidel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31035/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 5 p.m.)
>
>
> Review request for Ambari, Robert Levas, Tom Beerbower, and Yusaku Sako.
>
>
> Bugs: AMBARI-9640
> https://issues.apache.org/jira/browse/AMBARI-9640
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Add the following properties to the Kerberos service resource which provide the results of the validation of the KDC admin credentials stored in session: "Services/attributes/kdc_validation_result" and "Services/attributes/kdc_validation_failure_details".
>
> See the associated Jira for more details including full api responses.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelper.java 2e68c7d
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java 40921f5
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ServiceResourceProviderTest.java 210c6a4
>
> Diff: https://reviews.apache.org/r/31035/diff/
>
>
> Testing
> -------
>
> Manual Functional Testing:
> - Tested all possible results of the validation
> - Tested when no kerberos service existed
>
> Unit Tests:
> - Added new unit tests
> - Running entire unit test suite now and will post results prior to merging
>
>
> Thanks,
>
> John Speidel
>
>
Re: Review Request 31035: Allow the KDC admin credentials stored in
session to be validated via the REST API
Posted by John Speidel <js...@hortonworks.com>.
> On Feb. 14, 2015, 1:32 a.m., Robert Levas wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java, lines 1441-1457
> > <https://reviews.apache.org/r/31035/diff/1/?file=864036#file864036line1441>
> >
> > It seems like `KerberosHelper.validateKDCCredentials` should throw (more) specific Kerberos*Exceptions rather then the general IllegalArguementAcception. It may take a bit more work to do this, but removes the *guess work* out of determing what is acutally going on. I think of the IllegalArguement exceptions as a interface to an *external* caller not an *internal* caller.
Agreed, it bothered me also so I am going to fix it. See my response to Rishi's similar comment above.
> On Feb. 14, 2015, 1:32 a.m., Robert Levas wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java, line 1437
> > <https://reviews.apache.org/r/31035/diff/1/?file=864036#file864036line1437>
> >
> > Bummer more server-specific logic bleeding in..
yeah, given some time I could have made a generic mechanism where we could register specific handlers based on indentity but it doesn't make sense to do now.
> On Feb. 14, 2015, 1:32 a.m., Robert Levas wrote:
> > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ServiceResourceProviderTest.java, line 263
> > <https://reviews.apache.org/r/31035/diff/1/?file=864037#file864037line263>
> >
> > Your test case needs to mock up krb5-conf and kerberos-env configurations. See `org.apache.ambari.server.controller.KerberosHelperTest#testEnableKerberos(org.apache.ambari.server.serveraction.kerberos.KerberosCredential, boolean, boolean)` for an example.
Thanks Rob I have fixed this and will update a new patch shortly. One note, in the original test there was a lot of usages on ".once()". Using this when it isn't a requirement makes for more fragile tests. For example if you have some state obect with a getName() method, expectations for this method should use ".anyTimes()" and not ".once" because we don't care how many times it is called, it is merely an implementation detail. ".once()" is should only really be used for cases where calling the method more than once would be incorrect or in some cases very expensive.
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31035/#review72471
-----------------------------------------------------------
On Feb. 14, 2015, 1 a.m., John Speidel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31035/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2015, 1 a.m.)
>
>
> Review request for Ambari, Robert Levas, Tom Beerbower, and Yusaku Sako.
>
>
> Bugs: AMBARI-9640
> https://issues.apache.org/jira/browse/AMBARI-9640
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Add the following properties to the Kerberos service resource which provide the results of the validation of the KDC admin credentials stored in session: "Services/attributes/kdc_validation_result" and "Services/attributes/kdc_validation_failure_details".
>
> See the associated Jira for more details including full api responses.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelper.java 2e68c7d
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java 40921f5
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ServiceResourceProviderTest.java 210c6a4
>
> Diff: https://reviews.apache.org/r/31035/diff/
>
>
> Testing
> -------
>
> Manual Functional Testing:
> - Tested all possible results of the validation
> - Tested when no kerberos service existed
>
> Unit Tests:
> - Added new unit tests
> - Running entire unit test suite now and will post results prior to merging
>
>
> Thanks,
>
> John Speidel
>
>
Re: Review Request 31035: Allow the KDC admin credentials stored in
session to be validated via the REST API
Posted by Robert Levas <rl...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31035/#review72471
-----------------------------------------------------------
Ship it!
I think this is a great solution for the issue.
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java
<https://reviews.apache.org/r/31035/#comment118538>
Bummer more server-specific logic bleeding in..
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java
<https://reviews.apache.org/r/31035/#comment118539>
It seems like `KerberosHelper.validateKDCCredentials` should throw (more) specific Kerberos*Exceptions rather then the general IllegalArguementAcception. It may take a bit more work to do this, but removes the *guess work* out of determing what is acutally going on. I think of the IllegalArguement exceptions as a interface to an *external* caller not an *internal* caller.
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ServiceResourceProviderTest.java
<https://reviews.apache.org/r/31035/#comment118540>
Your test case needs to mock up krb5-conf and kerberos-env configurations. See `org.apache.ambari.server.controller.KerberosHelperTest#testEnableKerberos(org.apache.ambari.server.serveraction.kerberos.KerberosCredential, boolean, boolean)` for an example.
- Robert Levas
On Feb. 13, 2015, 8 p.m., John Speidel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31035/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 8 p.m.)
>
>
> Review request for Ambari, Robert Levas, Tom Beerbower, and Yusaku Sako.
>
>
> Bugs: AMBARI-9640
> https://issues.apache.org/jira/browse/AMBARI-9640
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Add the following properties to the Kerberos service resource which provide the results of the validation of the KDC admin credentials stored in session: "Services/attributes/kdc_validation_result" and "Services/attributes/kdc_validation_failure_details".
>
> See the associated Jira for more details including full api responses.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelper.java 2e68c7d
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java 40921f5
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ServiceResourceProviderTest.java 210c6a4
>
> Diff: https://reviews.apache.org/r/31035/diff/
>
>
> Testing
> -------
>
> Manual Functional Testing:
> - Tested all possible results of the validation
> - Tested when no kerberos service existed
>
> Unit Tests:
> - Added new unit tests
> - Running entire unit test suite now and will post results prior to merging
>
>
> Thanks,
>
> John Speidel
>
>