-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[AMBARI-24616] Disable Kerberos from Ambari UI didn't clean up keytab directories #2334
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Sorry for the large patch, there were lots of changes needed to clean up the code. |
|
Refer to this link for build results (access rights to CI server needed): |
|
|
||
| // ***************************************************************** | ||
| // Create stage to remove principals | ||
| // - this should be the last opterion that deals with principals and keytab files since the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opterion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixing.
|
|
||
| // ***************************************************************** | ||
| // Create stage to delete principals | ||
| // - this should be the last opterion that deals with principals and keytab files since the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opterion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixing.
...-server/src/main/java/org/apache/ambari/server/controller/utilities/RemovableIdentities.java
Show resolved
Hide resolved
| } | ||
|
|
||
| /** | ||
| * KerberosCommandParameterProcessor is an abstract class providing common implementions for processing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implementions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks.. fixing.
|
|
||
| if (keytabFilePath != null) { | ||
| String sha1Keytab = DigestUtils.sha256Hex(keytabFilePath); | ||
| File keytabFile = new File(dataDir + File.separator + hostName + File.separator + sha1Keytab); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might prefer https://docs.oracle.com/javase/8/docs/api/java/nio/file/Paths.html#get-java.lang.String-java.lang.String...- as it is less error-prone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree, but this was the code that was already there and I didn't want to mess with it.
ncole
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple import fixes and nits (optional).
| import org.apache.ambari.server.state.kerberos.KerberosDescriptor; | ||
| import org.apache.ambari.server.state.svccomphost.ServiceComponentHostServerActionEvent; | ||
| import org.apache.ambari.server.utils.StageUtils; | ||
| import org.springframework.util.CollectionUtils; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably be org.apache.commons.collections.CollectionUtils
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I knew I was going to mess this up. Fixed other places where my IDE choose the wrong impl. Nice catch.
| requestParams); | ||
| customCommandExecutionHelper.addExecutionCommandsToStage(actionExecContext, stage, requestParams, null); | ||
| stageContainer.addStage(stage); | ||
| if(!CollectionUtils.isEmpty(hostNames)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: CollectionUtils.isNotEmpty(...)
|
|
||
| ActionExecutionContext actionExecContext = new ActionExecutionContext( | ||
| cluster.getClusterName(), | ||
| "REMOVE_KEYTAB", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a constant somewhere for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup.
| import org.apache.ambari.server.orm.entities.KerberosKeytabPrincipalEntity; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.springframework.util.CollectionUtils; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
org.apache.commons.collections.CollectionUtils
| import org.apache.ambari.server.orm.entities.KerberosKeytabPrincipalEntity; | ||
| import org.apache.ambari.server.orm.entities.KerberosKeytabServiceMappingEntity; | ||
| import org.apache.ambari.server.orm.entities.KerberosPrincipalEntity; | ||
| import org.springframework.util.CollectionUtils; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
org.apache.commons.collections.CollectionUtils
| */ | ||
| public boolean removeIfNotReferenced(KerberosKeytabEntity kerberosKeytabEntity) { | ||
| if (kerberosKeytabEntity != null) { | ||
| if (!CollectionUtils.isEmpty(kerberosKeytabEntity.getKerberosKeytabPrincipalEntities())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: CollectionUtils.isNotEmpty(...)
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
What changes were proposed in this pull request?
Disable Kerberos from Ambari UI didn't clean up keytab directories,
stderr:
This occurred for several reasons related to many iterations of changes to the Kerberos enable and clean up processes. This patch attempts to fix the inconsistencies the lead to the cleanup failures - whether a service, component, or host was removed or Kerberos was being disabled. Now the keytab files, principals/accounts, and Ambari DB records are being properly cleaned up.
How was this patch tested?
Manually tested various scenarios when Kerberos was enabled
Unit tests were updated and all passed.
Please review Ambari Contributing Guide before opening a pull request.