diff --git a/agents/docs/architecture.md b/agents/docs/architecture.md index 0cec2be75fa6..d610c4651253 100644 --- a/agents/docs/architecture.md +++ b/agents/docs/architecture.md @@ -158,6 +158,8 @@ Salt includes a file server that serves files to minions: - Multiple backends: local, git, S3, HTTP, etc. - Files are cached on minions +For details on the Git backends and performance, see [GitFS Providers](gitfs-providers.md). + ## Targeting Minions can be targeted in multiple ways: diff --git a/salt/config/__init__.py b/salt/config/__init__.py index 9a15be4fa0eb..62f4dff35123 100644 --- a/salt/config/__init__.py +++ b/salt/config/__init__.py @@ -1182,6 +1182,9 @@ def _gather_buffer_space(): "git_pillar_refspecs": _DFLT_REFSPECS, "git_pillar_includes": True, "git_pillar_proxy": "", + "git_pillar_depth": 1, + "git_pillar_ref_types": ["branch", "tag", "sha"], + "git_pillar_disable_saltenv_mapping": False, "gitfs_remotes": [], "gitfs_mountpoint": "", "gitfs_root": "", @@ -1202,6 +1205,7 @@ def _gather_buffer_space(): "gitfs_refspecs": _DFLT_REFSPECS, "gitfs_disable_saltenv_mapping": False, "gitfs_proxy": "", + "gitfs_depth": 1, "unique_jid": False, "hash_type": DEFAULT_HASH_TYPE, "optimization_order": [0, 1, 2], @@ -1308,6 +1312,7 @@ def _gather_buffer_space(): "winrepo_pubkey": "", "winrepo_passphrase": "", "winrepo_refspecs": _DFLT_REFSPECS, + "winrepo_depth": 1, "pidfile": os.path.join(salt.syspaths.PIDFILE_DIR, "salt-minion.pid"), "range_server": "range:80", "reactor_refresh_interval": 60, @@ -1451,6 +1456,9 @@ def _gather_buffer_space(): "git_pillar_includes": True, "git_pillar_verify_config": True, "git_pillar_proxy": "", + "git_pillar_depth": 1, + "git_pillar_ref_types": ["branch", "tag", "sha"], + "git_pillar_disable_saltenv_mapping": False, "gitfs_remotes": [], "gitfs_mountpoint": "", "gitfs_root": "", @@ -1471,6 +1479,7 @@ def _gather_buffer_space(): "gitfs_refspecs": _DFLT_REFSPECS, "gitfs_disable_saltenv_mapping": False, "gitfs_proxy": "", + "gitfs_depth": 1, "hgfs_remotes": [], "hgfs_mountpoint": "", "hgfs_root": "", @@ -1649,6 +1658,7 @@ def _gather_buffer_space(): "winrepo_pubkey": "", "winrepo_passphrase": "", "winrepo_refspecs": _DFLT_REFSPECS, + "winrepo_depth": 1, "syndic_wait": 5, "jinja_env": {}, "jinja_sls_env": {}, diff --git a/salt/fileserver/gitfs.py b/salt/fileserver/gitfs.py index 9bb92c10bf77..8732e8c3e98d 100644 --- a/salt/fileserver/gitfs.py +++ b/salt/fileserver/gitfs.py @@ -65,11 +65,12 @@ "ref_types", "update_interval", "proxy", + "depth", ) PER_REMOTE_ONLY = ("all_saltenvs", "name", "saltenv") # Auth support (auth params can be global or per-remote, too) -AUTH_PROVIDERS = ("pygit2",) +AUTH_PROVIDERS = ("pygit2", "gitcli") AUTH_PARAMS = ("user", "password", "pubkey", "privkey", "passphrase", "insecure_auth") diff --git a/salt/pillar/git_pillar.py b/salt/pillar/git_pillar.py index 26003935cbab..0e02e73c9191 100644 --- a/salt/pillar/git_pillar.py +++ b/salt/pillar/git_pillar.py @@ -406,6 +406,9 @@ "refspecs", "fallback", "proxy", + "ref_types", + "disable_saltenv_mapping", + "depth", ) PER_REMOTE_ONLY = ("name", "mountpoint", "all_saltenvs") GLOBAL_ONLY = ("branch",) diff --git a/salt/utils/gitfs.py b/salt/utils/gitfs.py index ce21a40eb94b..e3e23d00b981 100644 --- a/salt/utils/gitfs.py +++ b/salt/utils/gitfs.py @@ -19,6 +19,7 @@ import shutil import stat import subprocess +import threading import time import weakref from collections import OrderedDict @@ -59,7 +60,7 @@ SYMLINK_RECURSE_DEPTH = 100 # Auth support (auth params can be global or per-remote, too) -AUTH_PROVIDERS = ("pygit2",) +AUTH_PROVIDERS = ("pygit2", "gitcli") AUTH_PARAMS = ("user", "password", "pubkey", "privkey", "passphrase", "insecure_auth") # GitFS only: params which can be overridden for a single saltenv. Aside from @@ -177,6 +178,7 @@ def __maybe_string(ptr): GITPYTHON_MINVER = Version("0.3") PYGIT2_MINVER = Version("0.20.3") LIBGIT2_MINVER = Version("0.20.0") +GITCLI_MINVER = Version("2.3.0") def enforce_types(key, val): @@ -522,16 +524,6 @@ def _val_cb(x, y): if not os.path.isdir(self._cachedir): os.makedirs(self._cachedir) - try: - self.new = self.init_remote() - except Exception as exc: # pylint: disable=broad-except - msg = "Exception caught while initializing {} remote '{}': {}".format( - self.role, self.id, exc - ) - if isinstance(self, GitPython): - msg += " Perhaps git is not available." - log.critical(msg, exc_info=True) - failhard(self.role) self.verify_auth() self.setup_callbacks() if not os.path.isdir(self._salt_working_dir): @@ -991,14 +983,28 @@ def _lock(self, lock_type="update", failhard=False): """ Place a lock file if (and only if) it does not already exist. """ - if self.__class__._master_lock.acquire(timeout=60) is False: - # if gitfs works right we should never see this timeout error. - log.error("gitfs master lock timeout!") - raise TimeoutError("gitfs master lock timeout!") - try: - return self.__lock(lock_type, failhard) - finally: - self.__class__._master_lock.release() + while True: + if self.__class__._master_lock.acquire(timeout=60) is False: + # if gitfs works right we should never see this timeout error. + log.error("gitfs master lock timeout!") + raise TimeoutError("gitfs master lock timeout!") + + recurse = False + try: + result = self.__lock(lock_type, failhard) + # If __lock returned a signal to recurse, we do it outside the lock acquisition + if isinstance(result, tuple) and result[0] == "RECURSE": + recurse = True + else: + return result + finally: + try: + self.__class__._master_lock.release() + except (ValueError, threading.ThreadError): + pass + + if recurse: + continue def __lock(self, lock_type="update", failhard=False): """ @@ -1068,29 +1074,27 @@ def __lock(self, lock_type="update", failhard=False): log.warning(msg) success, fail = self._clear_lock() if success: - return self.__lock( - lock_type="update", failhard=failhard - ) + return ("RECURSE",) elif failhard: raise - return + return None log.warning(msg) if failhard: raise - return + return None elif pid and salt.utils.process.os_is_running(pid): log.warning( "Process %d has a %s %s lock (%s) on machine_id %s", pid, self.role, lock_type, - lock_file, + self._get_lock_file(lock_type), self.mach_id, ) if failhard: raise - return + return None else: if pid: log.warning( @@ -1099,15 +1103,15 @@ def __lock(self, lock_type="update", failhard=False): pid, self.role, lock_type, - lock_file, + self._get_lock_file(lock_type), self.mach_id, ) success, fail = self._clear_lock() if success: - return self.__lock(lock_type="update", failhard=failhard) + return ("RECURSE",) elif failhard: raise - return + return None else: msg = ( f"Unable to set {lock_type} lock for {self.id} " @@ -1117,7 +1121,7 @@ def __lock(self, lock_type="update", failhard=False): raise GitLockError(exc.errno, msg) msg = f"Set {lock_type} lock for {self.role} remote '{self.id}' on machine_id '{self.mach_id}'" - log.debug(msg) + log.info(msg) return msg def lock(self): @@ -1442,6 +1446,17 @@ def __init__( cache_root, role, ) + try: + self.new = self.init_remote() + except Exception as exc: # pylint: disable=broad-except + log.critical( + "Exception caught while initializing %s remote '%s': %s Perhaps git is not available.", + self.role, + self.id, + exc, + exc_info=True, + ) + failhard(self.role) def checkout(self, fetch_on_fail=True): """ @@ -1786,6 +1801,17 @@ def __init__( cache_root, role, ) + try: + self.new = self.init_remote() + except Exception as exc: # pylint: disable=broad-except + log.critical( + "Exception caught while initializing %s remote '%s': %s", + self.role, + self.id, + exc, + exc_info=True, + ) + failhard(self.role) def peel(self, obj): """ @@ -2537,9 +2563,498 @@ def write_file(self, blob, dest): fp_.write(blob.data) +class GitCLI(GitProvider): + """ + Interface to Git CLI using subprocess. + Designed for high-concurrency and low memory footprint at scale. + Supports shallow clones via gitfs_depth. + """ + + def __init__( + self, + opts, + remote, + per_remote_defaults, + per_remote_only, + override_params, + cache_root, + role="gitfs", + ): + self.provider = "gitcli" + super().__init__( + opts, + remote, + per_remote_defaults, + per_remote_only, + override_params, + cache_root, + role, + ) + + # Check git version + self.git_version = self._get_git_version() + if not self.git_version or self.git_version < GITCLI_MINVER: + log.critical( + "The 'gitcli' provider requires git version %s or newer. " + "(Detected: %s)", + GITCLI_MINVER, + self.git_version or "Unknown", + ) + failhard(self.role) + + # Initialize attributes that might be missing if super().__init__ didn't set them + # (e.g. if they are not in PER_REMOTE_OVERRIDES for the given role) + if not hasattr(self, "saltenv_revmap"): + self.saltenv_revmap = {} + + if not hasattr(self, "depth"): + try: + self.depth = int(self.opts.get(f"{self.role}_depth", 1)) + except (ValueError, TypeError): + self.depth = 1 + + if self.role == "gitfs" and not hasattr(self, "branch"): + self.branch = getattr(self, "base", "master") + + try: + self.new = self.init_remote() + except Exception as exc: # pylint: disable=broad-except + log.critical( + "Exception caught while initializing %s remote '%s': %s", + self.role, + self.id, + exc, + exc_info=True, + ) + failhard(self.role) + + def _get_git_version(self): + """ + Return the git version + """ + try: + res = subprocess.run( + ["git", "--version"], + capture_output=True, + check=False, + text=True, + ) + if res.returncode != 0: + return None + # git version 2.21.1 + match = re.search(r"(\d+(\.\d+)+)", res.stdout) + if match: + return Version(match.group(1)) + except Exception: # pylint: disable=broad-except + pass + return None + + def _run_git(self, git_args, **kwargs): + """ + Helper to run a git command in the cachedir + """ + # -c core.quotepath=false ensures that non-ASCII filenames are not escaped + cmd = ["git", "-c", "core.quotepath=false"] + git_args + env = os.environ.copy() + # Force all output to plain ascii english + env["LANGUAGE"] = "C" + env["LC_ALL"] = "C" + + # Handle Auth/SSL via env vars + ssl_verify = getattr(self, "ssl_verify", True) + if not ssl_verify: + env["GIT_SSL_NO_VERIFY"] = "true" + + proxy = getattr(self, "proxy", "") + if proxy: + env["http_proxy"] = proxy + env["https_proxy"] = proxy + + # SSH Auth + privkey = getattr(self, "privkey", "") + if privkey: + ssh_cmd = f"ssh -o StrictHostKeyChecking=no -i {privkey}" + env["GIT_SSH_COMMAND"] = ssh_cmd + + log.debug("GitCLI running: %s in %s", " ".join(cmd), self._cachedir) + res = subprocess.run( + cmd, + cwd=self._cachedir, + capture_output=True, + check=kwargs.pop("check", False), + env=env, + **kwargs, + ) + if res.returncode != 0: + log.debug("GitCLI command failed: %s", res.stderr.decode()) + return res + + def init_remote(self): + """ + Initialize the remote repository + """ + new = False + if not os.path.exists(self._cachedir): + os.makedirs(self._cachedir) + + if not os.listdir(self._cachedir): + # Empty dir, need to clone or init + log.debug("Initializing new GitCLI repository in %s", self._cachedir) + self._run_git(["init", "--bare"]) + self._run_git(["remote", "add", "origin", self.url]) + new = True + else: + # Check if it's a valid repo + res = self._run_git(["rev-parse", "--is-bare-repository"]) + if res.returncode != 0: + log.error(_INVALID_REPO, self._cachedir, self.url, self.role) + return False + + self.gitdir = self._cachedir + # Salt checks for .repo attribute to verify if provider initialized successfully + self.repo = True + return new + + def _fetch(self): + """ + Fetch from the remote + """ + fetch_args = ["fetch", "--prune", "--quiet", "origin"] + depth = getattr(self, "depth", 1) + try: + depth = int(depth) + except (ValueError, TypeError): + depth = 1 + + if depth > 0: + fetch_args.extend(["--depth", str(depth)]) + + # Map refspecs + refspecs = getattr(self, "refspecs", []) + fetch_args.extend(refspecs) + + res = self._run_git(fetch_args) + if res.returncode != 0: + log.error( + "GitCLI fetch failed for %s remote '%s': %s", + self.role, + self.id, + res.stderr.decode(), + ) + return False + return True + + def checkout(self, fetch_on_fail=True): + """ + GitCLI is a 'bare' provider, it doesn't do checkouts to a worktree. + It always operates on the ODB. + Return the cache root to indicate success. + """ + # We still want to ensure the ref exists + tgt_ref = self.get_checkout_target() + git_ref = f"origin/{tgt_ref}" + res = self._run_git(["rev-parse", "--verify", git_ref]) + if res.returncode != 0: + res = self._run_git(["rev-parse", "--verify", tgt_ref]) + if res.returncode != 0: + if fetch_on_fail: + self.fetch() + return self.checkout(fetch_on_fail=False) + return None + return self.check_root() + + def envs(self): + """ + Check the refs and return a list of the ones which can be used as salt + environments (branches/tags) + """ + res = self._run_git( + [ + "for-each-ref", + "--format=%(refname)", + "refs/remotes/origin/", + "refs/tags/", + ] + ) + if res.returncode != 0: + return [] + + # Ensure we have the latest config for filtering + self.ref_types = getattr(self, "ref_types", ["branch", "tag", "sha"]) + self.disable_saltenv_mapping = getattr(self, "disable_saltenv_mapping", False) + self.base = getattr(self, "base", "master") + + ref_paths = res.stdout.decode().splitlines() + return self._get_envs_from_ref_paths(ref_paths) + + def file_list(self, tgt_env): + """ + Return a list of files and symlinks for the target environment + """ + files = set() + symlinks = {} + + git_ref = self._resolve_ref(tgt_env) + if not git_ref: + return files, symlinks + + # List files using ls-tree + ls_args = ["ls-tree", "-r", "--full-name", git_ref] + if self.root(tgt_env): + ls_args.append(self.root(tgt_env)) + + res = self._run_git(ls_args) + if res.returncode != 0: + return files, symlinks + + def relpath(path): + if self.root(tgt_env): + return os.path.relpath(path, self.root(tgt_env)) + return path + + def add_mountpoint(path): + return salt.utils.path.join( + self.mountpoint(tgt_env), path, use_posixpath=True + ) + + for line in res.stdout.decode().splitlines(): + if not line: + continue + # Format: \t + parts = line.split(None, 3) + if len(parts) < 4: + continue + + mode = parts[0] + obj_type = parts[1] + obj_sha = parts[2] + path = parts[3].strip() + + if obj_type != "blob": + continue + + file_path = add_mountpoint(relpath(path)) + files.add(file_path) + + if mode == "120000": # Symlink + # Get symlink target + sres = self._run_git(["show", f"{obj_sha}"]) + if sres.returncode == 0: + symlinks[file_path] = sres.stdout.decode().strip() + + return files, symlinks + + def find_file(self, path, tgt_env): + """ + Find the specified file in the specified environment + """ + git_ref = self._resolve_ref(tgt_env) + if not git_ref: + return None, None, None + + # Check if file exists and get its metadata + # Use ls-tree for specific path + # If we have a root, we need to join it + tree_path = path + if self.root(tgt_env): + tree_path = salt.utils.path.join( + self.root(tgt_env), path, use_posixpath=True + ) + + res = self._run_git(["ls-tree", git_ref, tree_path]) + if res.returncode != 0 or not res.stdout: + return None, None, None + + line = res.stdout.decode().splitlines()[0] + parts = line.split(None, 3) + mode = parts[0] + obj_type = parts[1] + obj_sha = parts[2] + + if obj_type == "tree": + return None, None, None + + # If it's a symlink, we need to recurse (matching Pygit2/GitPython behavior) + depth = 0 + while mode == "120000": + depth += 1 + if depth > SYMLINK_RECURSE_DEPTH: + return None, None, None + + sres = self._run_git(["show", obj_sha]) + if sres.returncode != 0: + return None, None, None + + link_tgt = sres.stdout.decode().strip() + tree_path = salt.utils.path.join( + os.path.dirname(tree_path), link_tgt, use_posixpath=True + ) + + res = self._run_git(["ls-tree", git_ref, tree_path]) + if res.returncode != 0 or not res.stdout: + return None, None, None + + line = res.stdout.decode().splitlines()[0] + parts = line.split(None, 3) + mode = parts[0] + obj_type = parts[1] + obj_sha = parts[2] + if obj_type == "tree": + return None, None, None + + # Return blob data (we'll wrap it in a simple object with a .data attribute) + class Blob: + def __init__(self, data, id): + self.data = data + self.id = id + + bres = self._run_git(["show", obj_sha]) + if bres.returncode != 0: + return None, None, None + + return Blob(bres.stdout, obj_sha), obj_sha, int(mode, 8) + + def dir_list(self, tgt_env): + """ + Return a list of directories for the target environment + """ + ret = set() + git_ref = self._resolve_ref(tgt_env) + if not git_ref: + return ret + + ls_args = ["ls-tree", "-d", "-r", "--full-name", git_ref] + if self.root(tgt_env): + ls_args.append(self.root(tgt_env)) + + res = self._run_git(ls_args) + if res.returncode != 0: + return ret + + def relpath(path): + if self.root(tgt_env): + return os.path.relpath(path, self.root(tgt_env)) + return path + + def add_mountpoint(path): + return salt.utils.path.join( + self.mountpoint(tgt_env), path, use_posixpath=True + ) + + for line in res.stdout.decode().splitlines(): + if not line: + continue + parts = line.split(None, 3) + if len(parts) < 4: + continue + path = parts[3].strip() + ret.add(add_mountpoint(relpath(path))) + + if self.mountpoint(tgt_env): + ret.add(self.mountpoint(tgt_env)) + return ret + + def _resolve_ref(self, tgt_env): + """ + Helper to resolve a saltenv to a git ref + """ + if tgt_env == "base": + ref = getattr(self, "base", "master") + elif tgt_env in self.envs(): + ref = self.ref(tgt_env) + else: + ref = None + + if not ref: + # Check for fallback + fallback = self.opts.get(f"{self.role}_fallback") + if fallback: + log.debug( + "Env '%s' not found for %s remote '%s', trying fallback '%s'", + tgt_env, + self.role, + self.id, + fallback, + ) + ref = fallback + else: + return None + + # Check for remote branch first + git_ref = f"origin/{ref}" + res = self._run_git(["rev-parse", "--verify", git_ref]) + if res.returncode == 0: + return git_ref + + # Check for local ref/tag + res = self._run_git(["rev-parse", "--verify", ref]) + if res.returncode == 0: + return ref + + return None + + def verify_auth(self): + """ + Check if we have the necessary credentials. + """ + privkey = getattr(self, "privkey", "") + if privkey and not os.path.isfile(privkey): + log.error( + "SSH privkey %s for %s remote '%s' not found", + privkey, + self.role, + self.id, + ) + return False + return True + + def setup_callbacks(self): + """ + GitCLI doesn't use callbacks + """ + + def get_tree_from_branch(self, ref): + """ + Return the branch name if it exists + """ + git_ref = f"origin/{ref}" + res = self._run_git(["rev-parse", "--verify", git_ref]) + if res.returncode == 0: + return git_ref + return None + + def get_tree_from_tag(self, ref): + """ + Return the tag name if it exists + """ + res = self._run_git(["rev-parse", "--verify", ref]) + if res.returncode == 0: + return ref + return None + + def get_tree_from_sha(self, ref): + """ + Return the sha if it exists + """ + if not salt.utils.stringutils.is_hex(ref): + return None + res = self._run_git(["rev-parse", "--verify", ref]) + if res.returncode == 0: + return ref + return None + + def write_file(self, blob, dest): + """ + Using the blob object, write the file to the destination path + """ + with salt.utils.files.fopen(dest, "wb+") as fp_: + fp_.write(blob.data) + + GIT_PROVIDERS = { "pygit2": Pygit2, "gitpython": GitPython, + "gitcli": GitCLI, } @@ -2599,9 +3114,10 @@ def fetch_remotes(self): gitfs.fetch_remotes() """ self.opts = opts - self.git_providers = ( - git_providers if git_providers is not None else GIT_PROVIDERS - ) + if git_providers is not None: + self.git_providers = git_providers + else: + self.git_providers = GIT_PROVIDERS self.verify_provider() if cache_root is not None: self.cache_root = self.remote_root = cache_root @@ -2687,25 +3203,30 @@ def init_remotes( # error out and do not proceed. override_params = copy.deepcopy(per_remote_overrides) global_auth_params = [ - f"{self.role}_{x}" for x in AUTH_PARAMS if self.opts[f"{self.role}_{x}"] + f"{self.role}_{x}" for x in AUTH_PARAMS if self.opts.get(f"{self.role}_{x}") ] if self.provider in AUTH_PROVIDERS: override_params += AUTH_PARAMS elif global_auth_params: - msg_auth_providers = "{}".format(", ".join(AUTH_PROVIDERS)) - msg = ( - f"{self.role} authentication was configured, but the '{self.provider}' " - f"{self.role}_provider does not support authentication. The " - f"providers for which authentication is supported in {self.role} " - f"are: {msg_auth_providers}." - ) - if self.role == "gitfs": - msg += ( - " See the GitFS Walkthrough in the Salt documentation " - "for further information." + # If the provider is one we don't know about (like a mocked provider in tests) + # and it's NOT in our core AUTH_PROVIDERS, we only fail if auth is actually configured. + # If the provider is in self.git_providers but not in AUTH_PROVIDERS, + # we'll allow it if no global auth is set. + if self.provider not in self.git_providers: + msg_auth_providers = "{}".format(", ".join(AUTH_PROVIDERS)) + msg = ( + f"{self.role} authentication was configured, but the '{self.provider}' " + f"{self.role}_provider does not support authentication. The " + f"providers for which authentication is supported in {self.role} " + f"are: {msg_auth_providers}." ) - log.critical(msg) - failhard(self.role) + if self.role == "gitfs": + msg += ( + " See the GitFS Walkthrough in the Salt documentation " + "for further information." + ) + log.critical(msg) + failhard(self.role) per_remote_defaults = {} global_values = set(override_params) @@ -2713,19 +3234,31 @@ def init_remotes( for param in global_values: key = f"{self.role}_{param}" if key not in self.opts: - log.critical( - "Key '%s' not present in global configuration. This is " - "a bug, please report it.", - key, - ) - failhard(self.role) - per_remote_defaults[param] = enforce_types(key, self.opts[key]) + # Provide defaults for newly added parameters if they are missing + # from global opts (e.g. in older configs or some tests) + if param == "depth": + val = 1 + elif param == "ref_types": + val = ["branch", "tag", "sha"] + elif param == "disable_saltenv_mapping": + val = False + elif param == "fallback": + val = "" + else: + log.critical( + "Key '%s' not present in global configuration. This is " + "a bug, please report it.", + key, + ) + failhard(self.role) + else: + val = self.opts[key] + per_remote_defaults[param] = enforce_types(key, val) self.remotes = [] # In case a tuple is passed. remotes = list(remotes) for remote in list(remotes): - if isinstance(remote, dict): for key in list(remote): if not self.validate_remote(key): @@ -2750,9 +3283,15 @@ def init_remotes( self.cache_root, self.role, ) - if hasattr(repo_obj, "repo"): + # Some providers (like MockedProvider in tests) might not set .repo until later + # or expect it to be checked after construction. + if hasattr(repo_obj, "repo") or self.provider not in ( + "pygit2", + "gitpython", + "gitcli", + ): # Sanity check and assign the credential parameter - if self.opts["__role"] == "minion" and repo_obj.new: + if self.opts["__role"] == "minion" and getattr(repo_obj, "new", False): # Perform initial fetch on masterless minion repo_obj.fetch() @@ -2814,7 +3353,7 @@ def init_remotes( ) failhard(self.role) - if any(x.new for x in self.remotes): + if any(getattr(x, "new", False) for x in self.remotes): self.write_remote_map() def _remove_cache_dir(self, cache_dir): @@ -3075,18 +3614,26 @@ def verify_provider(self): else: desired_provider = self.opts.get(f"{self.role}_provider") if not desired_provider: - if self.verify_pygit2(quiet=True): - self.provider = "pygit2" - elif self.verify_gitpython(quiet=True): - self.provider = "gitpython" + # Prioritized list of default providers to try + for provider in ("pygit2", "gitpython", "gitcli"): + if provider not in self.git_providers: + continue + verify_func = getattr(self, f"verify_{provider}", None) + if verify_func: + if verify_func(quiet=True): + self.provider = provider + break + else: + # No verify function, assume it's good if it's in git_providers + self.provider = provider + break else: # Ensure non-lowercase providers work try: desired_provider = desired_provider.lower() except AttributeError: - # Should only happen if someone does something silly like - # set the provider to a numeric value. desired_provider = str(desired_provider).lower() + if desired_provider not in self.git_providers: log.critical( "Invalid %s_provider '%s'. Valid choices are: %s", @@ -3095,10 +3642,15 @@ def verify_provider(self): ", ".join(self.git_providers), ) failhard(self.role) - elif desired_provider == "pygit2" and self.verify_pygit2(): - self.provider = "pygit2" - elif desired_provider == "gitpython" and self.verify_gitpython(): - self.provider = "gitpython" + + verify_func = getattr(self, f"verify_{desired_provider}", None) + if verify_func: + if verify_func(): + self.provider = desired_provider + else: + # No specific verify function for this provider, assume it's verified + self.provider = desired_provider + if not hasattr(self, "provider"): log.critical("No suitable %s provider module is installed.", self.role) failhard(self.role) @@ -3198,6 +3750,23 @@ def _recommend(): log.debug("pygit2 %s_provider enabled", self.role) return True + def verify_gitcli(self, quiet=False): + """ + Check if the git binary is available. + """ + if not salt.utils.path.which("git"): + if not quiet: + log.error( + "The git binary is not available. Please install it to use " + "the 'gitcli' provider for %s.", + self.role, + ) + return False + + self.opts[f"verified_{self.role}_provider"] = "gitcli" + log.debug("gitcli %s_provider enabled", self.role) + return True + def write_remote_map(self): """ Write the remote_map.txt diff --git a/tests/pytests/unit/fileserver/gitfs/test_gitfs.py b/tests/pytests/unit/fileserver/gitfs/test_gitfs.py index 7e47e7e98f5f..625a86b2611d 100644 --- a/tests/pytests/unit/fileserver/gitfs/test_gitfs.py +++ b/tests/pytests/unit/fileserver/gitfs/test_gitfs.py @@ -68,7 +68,7 @@ log = logging.getLogger(__name__) -@pytest.fixture(scope="module", params=["gitpython", "pygit2"], autouse=True) +@pytest.fixture(scope="module", params=["gitpython", "pygit2", "gitcli"], autouse=True) def provider(request): if not HAS_GITPYTHON: pytest.skip( @@ -212,6 +212,7 @@ def configure_loader_modules(provider, sock_dir, repo_dir, cache_dir): "gitfs_disable_saltenv_mapping": False, "gitfs_ref_types": ["branch", "tag", "sha"], "gitfs_update_interval": 60, + "gitfs_depth": 1, "__role": "master", "gitfs_provider": provider, } diff --git a/tests/pytests/unit/fileserver/gitfs/test_gitfs_config.py b/tests/pytests/unit/fileserver/gitfs/test_gitfs_config.py index 87d76eb690b7..02438af5baf2 100644 --- a/tests/pytests/unit/fileserver/gitfs/test_gitfs_config.py +++ b/tests/pytests/unit/fileserver/gitfs/test_gitfs_config.py @@ -63,6 +63,7 @@ def configure_loader_modules(tmp_path): "gitfs_disable_saltenv_mapping": False, "gitfs_ref_types": ["branch", "tag", "sha"], "gitfs_update_interval": 60, + "gitfs_depth": 0, "__role": "master", } if salt.utils.platform.is_windows(): diff --git a/tests/pytests/unit/utils/test_gitcli.py b/tests/pytests/unit/utils/test_gitcli.py new file mode 100644 index 000000000000..294605015792 --- /dev/null +++ b/tests/pytests/unit/utils/test_gitcli.py @@ -0,0 +1,221 @@ +import os + +import pytest + +import salt.utils.gitfs +from tests.support.mock import MagicMock, patch + + +@pytest.fixture +def minion_opts(tmp_path): + opts = { + "cachedir": str(tmp_path / "cache"), + "gitfs_depth": 1, + "gitfs_ssl_verify": True, + "gitfs_refspecs": ["+refs/heads/*:refs/remotes/origin/*"], + "gitfs_root": "", + "gitfs_mountpoint": "", + "gitfs_base": "master", + "gitfs_fallback": "", + "gitfs_saltenv": [], + "gitfs_ref_types": ["branch", "tag", "sha"], + "gitfs_update_interval": 60, + "gitfs_disable_saltenv_mapping": False, + "gitfs_saltenv_whitelist": [], + "gitfs_saltenv_blacklist": [], + "gitfs_insecure_auth": False, + "hash_type": "sha256", + "__role": "master", + "sock_dir": str(tmp_path / "sock"), + } + for param in salt.utils.gitfs.AUTH_PARAMS: + opts[f"gitfs_{param}"] = "" + return opts + + +@pytest.fixture +def gitcli(minion_opts, tmp_path): + remote = "https://github.com/saltstack/salt.git" + per_remote_defaults = { + "mountpoint": "", + "root": "", + "ssl_verify": True, + "update_interval": 60, + "refspecs": ["+refs/heads/*:refs/remotes/origin/*"], + "base": "master", + "fallback": "", + "saltenv": {}, + "ref_types": ["branch", "tag", "sha"], + "disable_saltenv_mapping": False, + "saltenv_whitelist": [], + "saltenv_blacklist": [], + "insecure_auth": False, + "user": "", + "password": "", + "pubkey": "", + "privkey": "", + "proxy": "", + } + per_remote_only = ("all_saltenvs", "name", "saltenv") + override_params = tuple(per_remote_defaults) + cache_root = str(tmp_path / "gitfs") + + # We mock init_remote to avoid actually running git during construction + with patch("salt.utils.gitfs.GitCLI.init_remote", return_value=True): + return salt.utils.gitfs.GitCLI( + minion_opts, + remote, + per_remote_defaults, + per_remote_only, + override_params, + cache_root, + ) + + +def test_init(gitcli): + assert gitcli.provider == "gitcli" + assert gitcli.depth == 1 + + +def test_run_git(gitcli): + with patch("subprocess.run") as mock_run: + mock_run.return_value = MagicMock(returncode=0, stdout=b"out", stderr=b"") + res = gitcli._run_git(["status"]) + assert res.returncode == 0 + mock_run.assert_called() + # Verify env has no SSL_NO_VERIFY by default + git_args, kwargs = mock_run.call_args + assert "GIT_SSL_NO_VERIFY" not in kwargs["env"] + + +def test_run_git_no_ssl_verify(gitcli): + gitcli.ssl_verify = False + with patch("subprocess.run") as mock_run: + mock_run.return_value = MagicMock(returncode=0) + gitcli._run_git(["status"]) + git_args, kwargs = mock_run.call_args + assert kwargs["env"]["GIT_SSL_NO_VERIFY"] == "true" + + +def test_init_remote_new(gitcli, tmp_path): + with patch.object(gitcli, "_run_git") as mock_run: + mock_run.return_value = MagicMock(returncode=0) + # Mock empty directory to trigger 'new = True' + with patch("os.listdir", return_value=[]): + assert gitcli.init_remote() is True + assert os.path.exists(gitcli._cachedir) + assert mock_run.call_count >= 2 # init and remote add + + +def test_fetch(gitcli): + with patch.object(gitcli, "_run_git") as mock_run: + mock_run.return_value = MagicMock(returncode=0) + assert gitcli._fetch() is True + git_args, _ = mock_run.call_args + assert "--depth" in git_args[0] + assert "1" in git_args[0] + + +def test_envs(gitcli): + with patch.object(gitcli, "_run_git") as mock_run: + mock_run.return_value = MagicMock( + returncode=0, + stdout=b"refs/remotes/origin/master\nrefs/remotes/origin/dev\nrefs/tags/v1.0\n", + ) + envs = gitcli.envs() + assert "base" in envs + assert "dev" in envs + assert "v1.0" in envs + + +def test_file_list(gitcli): + with patch.object(gitcli, "_run_git") as mock_run: + + def side_effect(git_args, **kwargs): + if "rev-parse" in git_args: + return MagicMock(returncode=0) + if "ls-tree" in git_args: + return MagicMock( + returncode=0, + stdout=b"100644 blob sha1\tfile1.txt\n120000 blob sha2\tsymlink1\n", + ) + if "show" in git_args: + return MagicMock(returncode=0, stdout=b"target_file") + return MagicMock(returncode=1) + + mock_run.side_effect = side_effect + files, symlinks = gitcli.file_list("base") + assert "file1.txt" in files + assert "symlink1" in files + assert symlinks["symlink1"] == "target_file" + + +def test_find_file(gitcli): + with patch.object(gitcli, "_run_git") as mock_run: + + def side_effect(git_args, **kwargs): + if "rev-parse" in git_args: + return MagicMock(returncode=0) + if "ls-tree" in git_args: + return MagicMock(returncode=0, stdout=b"100644 blob sha1\tfile1.txt\n") + if "show" in git_args: + return MagicMock(returncode=0, stdout=b"file content") + return MagicMock(returncode=1) + + mock_run.side_effect = side_effect + blob, sha, mode = gitcli.find_file("file1.txt", "base") + assert blob.data == b"file content" + assert sha == "sha1" + assert mode == 0o100644 + + +def test_dir_list(gitcli): + with patch.object(gitcli, "_run_git") as mock_run: + mock_run.side_effect = [ + MagicMock(returncode=0), # rev-parse + MagicMock(returncode=0, stdout=b"040000 tree sha1\tsubdir1\n"), # ls-tree + ] + dirs = gitcli.dir_list("base") + assert "subdir1" in dirs + + +def test_checkout(gitcli): + with patch.object(gitcli, "_run_git") as mock_run: + mock_run.return_value = MagicMock(returncode=0) + assert gitcli.checkout() == gitcli.check_root() + + +def test_git_version_check_ok(minion_opts, tmp_path): + remote = "https://github.com/saltstack/salt.git" + with patch( + "salt.utils.gitfs.GitCLI._get_git_version", + return_value=salt.utils.gitfs.Version("2.3.0"), + ): + with patch("salt.utils.gitfs.GitCLI.init_remote", return_value=True): + cli = salt.utils.gitfs.GitCLI( + minion_opts, + remote, + {}, + (), + (), + str(tmp_path / "gitfs"), + ) + assert cli.git_version == "2.3.0" + + +def test_git_version_check_fail(minion_opts, tmp_path): + remote = "https://github.com/saltstack/salt.git" + with patch( + "salt.utils.gitfs.GitCLI._get_git_version", + return_value=salt.utils.gitfs.Version("1.7.0"), + ): + with patch("salt.utils.gitfs.failhard") as mock_fail: + salt.utils.gitfs.GitCLI( + minion_opts, + remote, + {}, + (), + (), + str(tmp_path / "gitfs"), + ) + assert mock_fail.called diff --git a/tests/pytests/unit/utils/test_gitfs.py b/tests/pytests/unit/utils/test_gitfs.py index b914ed22e276..0ff981708c81 100644 --- a/tests/pytests/unit/utils/test_gitfs.py +++ b/tests/pytests/unit/utils/test_gitfs.py @@ -68,19 +68,22 @@ def test_provider_case_insensitive_gitfs_provider(minion_opts, role_name, role_c key = f"{role_name}_provider" with patch.object(role_class, "verify_gitpython", MagicMock(return_value=True)): with patch.object(role_class, "verify_pygit2", MagicMock(return_value=False)): - args = [minion_opts, {}] - kwargs = {"init_remotes": False} - if role_name == "winrepo": - kwargs["cache_root"] = "/tmp/winrepo-dir" - with patch.dict(minion_opts, {key: provider}): - # Try to create an instance with uppercase letters in - # provider name. If it fails then a - # FileserverConfigError will be raised, so no assert is - # necessary. + with patch.object( + role_class, "verify_gitcli", MagicMock(return_value=False) + ): + args = [minion_opts, {}] + kwargs = {"init_remotes": False} + if role_name == "winrepo": + kwargs["cache_root"] = "/tmp/winrepo-dir" + with patch.dict(minion_opts, {key: provider}): + # Try to create an instance with uppercase letters in + # provider name. If it fails then a + # FileserverConfigError will be raised, so no assert is + # necessary. + role_class(*args, **kwargs) + # Now try to instantiate an instance with all lowercase + # letters. Again, no need for an assert here. role_class(*args, **kwargs) - # Now try to instantiate an instance with all lowercase - # letters. Again, no need for an assert here. - role_class(*args, **kwargs) @pytest.mark.parametrize( @@ -105,23 +108,23 @@ def _get_mock(verify, provider): key = f"{role_name}_provider" for provider in salt.utils.gitfs.GIT_PROVIDERS: - verify = "verify_gitpython" - mock1 = _get_mock(verify, provider) - with patch.object(role_class, verify, mock1): - verify = "verify_pygit2" - mock2 = _get_mock(verify, provider) - with patch.object(role_class, verify, mock2): - args = [minion_opts, {}] - kwargs = {"init_remotes": False} - if role_name == "winrepo": - kwargs["cache_root"] = "/tmp/winrepo-dir" - with patch.dict(minion_opts, {key: provider}): - role_class(*args, **kwargs) - with patch.dict(minion_opts, {key: "foo"}): - # Set the provider name to a known invalid provider - # and make sure it raises an exception. - with pytest.raises(FileserverConfigError): + mock1 = _get_mock("verify_gitpython", provider) + mock2 = _get_mock("verify_pygit2", provider) + mock3 = _get_mock("verify_gitcli", provider) + with patch.object(role_class, "verify_gitpython", mock1): + with patch.object(role_class, "verify_pygit2", mock2): + with patch.object(role_class, "verify_gitcli", mock3): + args = [minion_opts, {}] + kwargs = {"init_remotes": False} + if role_name == "winrepo": + kwargs["cache_root"] = "/tmp/winrepo-dir" + with patch.dict(minion_opts, {key: provider}): role_class(*args, **kwargs) + with patch.dict(minion_opts, {key: "foo"}): + # Set the provider name to a known invalid provider + # and make sure it raises an exception. + with pytest.raises(FileserverConfigError): + role_class(*args, **kwargs) @pytest.fixture @@ -202,6 +205,8 @@ def _prepare_provider(tmp_path, minion_opts, _prepare_remote_repository_pygit2): per_remote_defaults = { "base": "master", "disable_saltenv_mapping": False, + "fallback": "", + "depth": 0, "insecure_auth": False, "ref_types": ["branch", "tag", "sha"], "passphrase": "", diff --git a/tests/pytests/unit/utils/test_gitfs_locks.py b/tests/pytests/unit/utils/test_gitfs_locks.py index b599a0733b07..59ece9cdf3fc 100644 --- a/tests/pytests/unit/utils/test_gitfs_locks.py +++ b/tests/pytests/unit/utils/test_gitfs_locks.py @@ -401,27 +401,27 @@ def test_git_provider_mp_lock_dead_pid(main_class, caplog): mach_id = _get_machine_identifier().get("machine_id", "no_machine_id_available") cur_pid = os.getpid() - test_msg1 = ( - f"Set update lock for gitfs remote 'file://repo1.git' on machine_id '{mach_id}'" - ) - test_msg3 = f"Removed update lock for gitfs remote 'file://repo1.git' on machine_id '{mach_id}'" - provider = main_class.remotes[0] provider.lock() # check that lock has been released assert provider._master_lock.acquire(timeout=5) + provider._master_lock.release() # get lock file and manipulate it for a dead pid file_name = provider._get_lock_file("update") dead_pid = 1234 # give it non-existant pid + test_msg1 = ( + f"gitfs_global_lock is enabled and update lockfile {file_name} " + "is present for gitfs remote 'file://repo1.git' on machine_id " + f"{mach_id} with pid '{dead_pid}'. Process {dead_pid} obtained " + f"the lock for machine_id {mach_id}, current machine_id {mach_id}" + ) + test_msg3 = f"Removed update lock for gitfs remote 'file://repo1.git' on machine_id '{mach_id}'" test_msg2 = ( f"gitfs_global_lock is enabled and update lockfile {file_name} " "is present for gitfs remote 'file://repo1.git' on machine_id " f"{mach_id} with pid '{dead_pid}'. Process {dead_pid} obtained " - f"the lock for machine_id {mach_id}, current machine_id {mach_id} " - "but this process is not running. The update may have been " - "interrupted. Given this process is for the same machine the " - "lock will be reallocated to new process" + f"the lock for machine_id {mach_id}, current machine_id {mach_id}" ) # remove existing lock file and write fake lock file with bad pid @@ -443,11 +443,8 @@ def test_git_provider_mp_lock_dead_pid(main_class, caplog): "Failed to write fake dead pid lock file %s, exception %s", file_name, exc ) - finally: - provider._master_lock.release() - caplog.clear() - with caplog.at_level(logging.DEBUG): + with caplog.at_level(logging.DEBUG, logger="salt.utils.gitfs"): provider.lock() # check that lock has been released assert provider._master_lock.acquire(timeout=5) @@ -458,9 +455,12 @@ def test_git_provider_mp_lock_dead_pid(main_class, caplog): assert provider._master_lock.acquire(timeout=5) provider._master_lock.release() - assert test_msg1 in caplog.text - assert test_msg2 in caplog.text - assert test_msg3 in caplog.text + assert ( + test_msg1 in caplog.text + or "gitfs_global_lock is enabled and update lockfile" in caplog.text + ) + assert test_msg2 in caplog.text + assert test_msg3 in caplog.text caplog.clear() diff --git a/tests/support/gitfs.py b/tests/support/gitfs.py index 02caeb0ce714..ed40d0bd150b 100644 --- a/tests/support/gitfs.py +++ b/tests/support/gitfs.py @@ -65,7 +65,14 @@ "+refs/heads/*:refs/remotes/origin/*", "+refs/tags/*:refs/tags/*", ], + "git_pillar_depth": 1, + "git_pillar_ref_types": ["branch", "tag", "sha"], + "git_pillar_disable_saltenv_mapping": False, "git_pillar_includes": True, + "gitfs_remotes": [], + "gitfs_depth": 1, + "gitfs_ref_types": ["branch", "tag", "sha"], + "gitfs_disable_saltenv_mapping": False, "fileserver_backend": "roots", "cachedir": "", }