address review comments

Signed-off-by: Tsvetomir Palashki <[email protected]>

projects/vdk-plugins/vdk-kerberos-auth/requirements.txt

@@ -1,5 +1,5 @@
 vdk-core
-minikerberos
+minikerberos==0.1.0
 requests-kerberos
 
 vdk-test-utils

projects/vdk-plugins/vdk-kerberos-auth/src/vdk/plugin/kerberos/authenticator_factory.py

@@ -1,7 +1,7 @@
 # Copyright 2021 VMware, Inc.
 # SPDX-License-Identifier: Apache-2.0
 import logging
-from typing import Union
+from typing import Optional
 
 from vdk.internal.core import errors
 from vdk.plugin.kerberos.base_authenticator import BaseAuthenticator
@@ -20,7 +20,7 @@ def __init__(self, configuration: KerberosPluginConfiguration):
 
     def create_authenticator(
         self, authentication_type: str
-    ) -> Union[BaseAuthenticator, None]:
+    ) -> Optional[BaseAuthenticator]:
         if authentication_type == "minikerberos":
             return MinikerberosGSSAPIAuthenticator(
                 self.__configuration.keytab_pathname(),

projects/vdk-plugins/vdk-kerberos-auth/src/vdk/plugin/kerberos/base_authenticator.py

@@ -5,7 +5,7 @@
 from abc import ABC
 from abc import abstractmethod
 
-from vdk.internal.control.exception.vdk_exception import VDKException
+from vdk.internal.core import errors
 
 log = logging.getLogger(__name__)
 
@@ -18,20 +18,16 @@ def __init__(
         kerberos_realm: str = None,
         kerberos_kdc_hostname: str = None,
     ):
-
         if not os.path.isfile(keytab_pathname):
             f = os.path.abspath(keytab_pathname)
-            log.warning(
-                f"Cannot locate keytab file {keytab_pathname}. File does not exist. "
-                f"Kerberos authentication is impossible, I'll rethrow this error for processing "
-                f"by my callers and they will decide whether they can recover or not. "
-                f"To avoid this message in the future, please ensure a keytab file at {f}"
-            )
-            raise VDKException(
-                what="Cannot locate keytab file",
-                why=f"Keytab file at {f} does not exist",
-                consequence="Subsequent operation that require authentication will fail.",
-                countermeasure=f"Ensure a keytab file is located at {f}.",
+            errors.log_and_throw(
+                to_be_fixed_by=errors.ResolvableBy.CONFIG_ERROR,
+                log=log,
+                what_happened=f"Cannot locate keytab file {keytab_pathname}.",
+                why_it_happened=f"Keytab file at {f} does not exist",
+                consequences="Kerberos authentication is impossible. "
+                "Subsequent operation that require authentication will fail.",
+                countermeasures=f"Ensure a keytab file is located at {f}.",
             )
         self._keytab_pathname = os.path.abspath(keytab_pathname)
         self._kerberos_principal = kerberos_principal
@@ -72,5 +68,9 @@ def set_authenticated(self):
         self._is_authenticated = True
 
     @abstractmethod
-    def _kinit(self):
-        pass
+    def _kinit(self) -> None:
+        """
+        Obtain and cache a Kerberos ticket-granting ticket (TGT)
+        that will be used for subsequent Kerberos authentication.
+        This method either succeeds or throws an exception.
+        """

projects/vdk-plugins/vdk-kerberos-auth/src/vdk/plugin/kerberos/kerberos_configuration.py

@@ -54,20 +54,32 @@ def add_definitions(config_builder: ConfigurationBuilder) -> None:
     config_builder.add(
         key=KRB_AUTH,
         default_value=None,
+        description="Specifies the Kerberos authentication type to use. "
+        "Possible values are 'minikerberos' and 'kinit'. "
+        "If left empty, the authentication is disabled.",
     )
     config_builder.add(
         key=KEYTAB_FILENAME,
         default_value=None,
+        description="Specifies the name of the keytab file. "
+        "If left empty, the name of the keytab file is assumed to be the same "
+        "as the name of the data job with '.keytab' suffix.",
     )
     config_builder.add(
         key=KEYTAB_PRINCIPAL,
         default_value=None,
+        description="Specifies the Kerberos principal. "
+        "If left empty, the principal will be the job name prepended with 'pa__view_'.",
     )
     config_builder.add(
         key=KEYTAB_REALM,
-        default_value="localhost",
+        default_value="default_realm",
+        description="Specifies the Kerberos realm. This value is used only with "
+        "the 'minikerberos' authentication type. The default value is 'default_realm'.",
     )
     config_builder.add(
         key=KERBEROS_KDC_HOST,
         default_value="localhost",
+        description="Specifies the name of the Kerberos KDC (Key Distribution Center) host. "
+        "This value is used only with the 'minikerberos' authentication type.",
     )

projects/vdk-plugins/vdk-kerberos-auth/src/vdk/plugin/kerberos/kerberos_plugin.py

@@ -2,15 +2,12 @@
 # SPDX-License-Identifier: Apache-2.0
 import logging
 from typing import List
-from typing import Union
 
 from vdk.api.plugin.hook_markers import hookimpl
 from vdk.api.plugin.plugin_registry import IPluginRegistry
 from vdk.internal.builtin_plugins.run.job_context import JobContext
-from vdk.internal.control.exception.vdk_exception import VDKException
 from vdk.internal.core.config import ConfigurationBuilder
 from vdk.plugin.kerberos.authenticator_factory import KerberosAuthenticatorFactory
-from vdk.plugin.kerberos.base_authenticator import BaseAuthenticator
 from vdk.plugin.kerberos.kerberos_configuration import add_definitions
 from vdk.plugin.kerberos.kerberos_configuration import KerberosPluginConfiguration
 
@@ -19,7 +16,6 @@
 
 class KerberosPlugin:
     def __init__(self):
-        self.__authenticator_factory = None
         self.__authenticator = None
 
     @staticmethod
@@ -32,33 +28,22 @@ def initialize_job(self, context: JobContext) -> None:
         kerberos_configuration = KerberosPluginConfiguration(
             context.name, str(context.job_directory), context.core_context.configuration
         )
-        self.__authenticator_factory = KerberosAuthenticatorFactory(
-            kerberos_configuration
-        )
-        authenticator = self.__get_authenticator(
-            kerberos_configuration.authentication_type()
-        )
-        if authenticator:
-            authenticator.authenticate()
-
-    def __get_authenticator(
-        self, authentication_type: str
-    ) -> Union[BaseAuthenticator, None]:
-        if self.__authenticator is None:
-            try:
-                self.__authenticator = (
-                    self.__authenticator_factory.create_authenticator(
-                        authentication_type
-                    )
-                )
-            except VDKException:
-                log.debug(
-                    f"Kerberos authenticator cannot be created. You can provide configuration that specifies "
-                    f"keytab info using the following environment variables: {'VDK_KEYTAB_FOLDER'}, or "
-                    f"{'VDK_KEYTAB_FILENAME'} and {'VDK_KEYTAB_PRINCIPAL'}"
-                )
-                self.__authenticator = None
-        return self.__authenticator
+        authenticator_factory = KerberosAuthenticatorFactory(kerberos_configuration)
+
+        try:
+            self.__authenticator = authenticator_factory.create_authenticator(
+                kerberos_configuration.authentication_type()
+            )
+        except Exception:
+            log.error(
+                f"Kerberos authenticator cannot be created. You can provide configuration that specifies "
+                f"keytab info using the following environment variables: {'VDK_KEYTAB_FOLDER'}, or "
+                f"{'VDK_KEYTAB_FILENAME'} and {'VDK_KEYTAB_PRINCIPAL'}"
+            )
+            self.__authenticator = None
+
+        if self.__authenticator:
+            self.__authenticator.authenticate()
 
 
 @hookimpl

projects/vdk-plugins/vdk-kerberos-auth/src/vdk/plugin/kerberos/kinit_authenticator.py

@@ -12,12 +12,13 @@
 
 
 class KinitGSSAPIAuthenticator(BaseAuthenticator):
-    def __init__(
-        self, keytab_pathname: str, kerberos_principal: str, working_dir: str = None
-    ):
+    """
+    A Kerberos authenticator that uses a 'kinit' call to obtain its ticket-granting ticket (TGT).
+    """
+
+    def __init__(self, keytab_pathname: str, kerberos_principal: str):
         super().__init__(keytab_pathname, kerberos_principal)
 
-        self._working_dir = working_dir
         self.__configure_current_os_process_to_use_own_kerberos_credentials_cache()
 
     def __repr__(self):
@@ -40,7 +41,7 @@ def __configure_current_os_process_to_use_own_kerberos_credentials_cache(self):
             ]  # used for deleting the env variable later
             log.info(f"KRB5CCNAME was already set to {self._oldKRB5CCNAME}")
         except KeyError:
-            tmpfile = tempfile.mktemp(prefix="vdkkrb5cc", dir=self._working_dir)
+            tmpfile = tempfile.NamedTemporaryFile(prefix="vdkkrb5cc", delete=True).name
             os.environ["KRB5CCNAME"] = "FILE:" + tmpfile
             log.info(f"KRB5CCNAME is set to a new file {tmpfile}")
             self._tmp_file = tmpfile
@@ -58,16 +59,14 @@ def __restore_previous_os_process_kerberos_credentials_cache_configuration(self)
             except OSError:
                 pass
 
-    def _kinit(self):
+    def _kinit(self) -> None:
         log.info(
             f"Calling kinit for kerberos principal {self._kerberos_principal} and keytab file {self._keytab_pathname}"
         )
         exitcode = call(
             ["kinit", "-k", "-t", self._keytab_pathname, self._kerberos_principal]
         )
-        if 0 == exitcode:
-            return True
-        else:
+        if exitcode != 0:
             errors.log_and_throw(
                 to_be_fixed_by=errors.ResolvableBy.CONFIG_ERROR,
                 log=log,

projects/vdk-plugins/vdk-kerberos-auth/src/vdk/plugin/kerberos/minikerberos_authenticator.py

@@ -14,6 +14,11 @@
 
 
 class MinikerberosGSSAPIAuthenticator(BaseAuthenticator):
+    """
+    A Kerberos authenticator that connects directly to the KDC (using the minikerberos library)
+    to get its ticket-granting ticket (TGT).
+    """
+
     def __init__(
         self,
         keytab_pathname: str,
@@ -46,7 +51,7 @@ def __exit__(self, *exc):
         except OSError:
             pass
 
-    def _kinit(self):
+    def _kinit(self) -> None:
         log.info(
             "Getting kerberos TGT for principal: %s, realm: %s using keytab file: %s from kdc: %s",
             self._kerberos_principal,
@@ -66,7 +71,6 @@ def _kinit(self):
                 f"Got Kerberos TGT for {self._kerberos_principal}@{self._kerberos_realm} "
                 f"and stored to file: {self._ccache_file}"
             )
-            return self._ccache_file
         except Exception as e:
             errors.log_and_throw(
                 to_be_fixed_by=errors.ResolvableBy.CONFIG_ERROR,