HDDS-14380. The user who starts Recon process will have administrator privilege

[echonesis]

What changes were proposed in this pull request?

This PR enhances the Recon administrator access control by automatically granting administrator privileges to the user who starts the Recon process, in addition to any configured administrators.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-14380

How was this patch tested?

GitHub Actions CI: https://github.com/echonesis/ozone/actions/runs/21105670646

[devmadhuu]

Thanks @echonesis for the patch. Largely looks good, but just few minor comments.

[devmadhuu]

why we need reconServer instance to be passes explicitly to ReconControllerModule ? can we not use Singleton ?

[echonesis]

This follows the same pattern as OM and SCM. In OM, the starter user and admin information are stored as instance fields in the OzoneManager object itself (https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L680-L682). Similarly, we store this information in the ReconServer instance and make it accessible to other components via Guice.

Using bind(ReconServer.class).toInstance(reconServer) allows other components to inject and access the starter user information, just like how OM exposes this through its instance methods.

[devmadhuu]

Okay, your implementation is correct for the current architecture. The toInstance() pattern is appropriate here because ReconServer is created and initialized before Guice, and we need to bind the specific initialized instance. While OM/SCM store admin info similarly, they don't use Guice, so in a way, the comparison isn't directly applicable.

[devmadhuu]

Instead of calling Admins and AdminsGroups method separately here, can we use this method directly and then add recon admins and recon admin groups. this will minimise the code lines.

[echonesis]

Thanks for the suggestion!
Since we need to add Recon-specific admins from OZONE_RECON_ADMINISTRATORS and OZONE_RECON_ADMINISTRATORS_GROUPS, using addAll() seemed like a straightforward approach.

[devmadhuu]

Okay, can we refactor and extract to a helper method instead of inline code directly in call() method:

private OzoneAdmins createReconAdmins(OzoneConfiguration conf) {
  String starterUser = UserGroupInformation.getCurrentUser().getShortUserName();
  Collection<String> adminUsers =
      OzoneAdmins.getOzoneAdminsFromConfig(conf, starterUser);
  adminUsers.addAll(
      conf.getStringCollection(ReconConfigKeys.OZONE_RECON_ADMINISTRATORS));
  
  Collection<String> adminGroups =
      OzoneAdmins.getOzoneAdminsGroupsFromConfig(conf);
  adminGroups.addAll(
      conf.getStringCollection(ReconConfigKeys.OZONE_RECON_ADMINISTRATORS_GROUPS));
  
  return new OzoneAdmins(adminUsers, adminGroups);
}

[devmadhuu]

Any specific usecase or need to define these admin and admin group getter methods ?

[echonesis]

Thanks @devmadhuu !
These methods follow the same pattern as OM's getOmAdminUsernames() and getOmAdminGroups() for consistency. They're not currently used in Recon.
I will remove them in a follow-up commit if preferred.

[devmadhuu]

Thanks @echonesis for clarifying the comments. Just a nit: if you can handle. Overall LGTM +1

[devmadhuu]

Okay, your implementation is correct for the current architecture. The toInstance() pattern is appropriate here because ReconServer is created and initialized before Guice, and we need to bind the specific initialized instance. While OM/SCM store admin info similarly, they don't use Guice, so in a way, the comparison isn't directly applicable.

[devmadhuu]

Okay, can we refactor and extract to a helper method instead of inline code directly in call() method:

private OzoneAdmins createReconAdmins(OzoneConfiguration conf) {
  String starterUser = UserGroupInformation.getCurrentUser().getShortUserName();
  Collection<String> adminUsers =
      OzoneAdmins.getOzoneAdminsFromConfig(conf, starterUser);
  adminUsers.addAll(
      conf.getStringCollection(ReconConfigKeys.OZONE_RECON_ADMINISTRATORS));
  
  Collection<String> adminGroups =
      OzoneAdmins.getOzoneAdminsGroupsFromConfig(conf);
  adminGroups.addAll(
      conf.getStringCollection(ReconConfigKeys.OZONE_RECON_ADMINISTRATORS_GROUPS));
  
  return new OzoneAdmins(adminUsers, adminGroups);
}

[priyeshkaratha]

Thanks @echonesis for the patch. Overall changes LGTM. I also suggest to move to private method as Devesh suggested and have one minor comment.

[priyeshkaratha]

The call to UserGroupInformation.getCurrentUser() can technically throw an IOException if there are issues with the Kerberos ticket or OS-level user retrieval.

So I am suggesting to ensure the ReconServer startup sequence gracefully handles or logs a clear error message if the current user cannot be identified