Infrastructure as Code and GitOps Questions

Direct answer

A drift-reconciliation tool has three genuinely separate jobs: read the tfstate's declared resources, query the CLOUD API for what actually exists for each one (handling throttling and credential failures as first-class, expected outcomes rather than crashes), and classify every discrepancy into one of four categories, MODIFIED (attributes disagree), MISSING (in state but gone from the cloud), UNMANAGED (exists in the cloud but absent from state), or UNCHANGED, then emit that classification as a structured JSON report. Below is a runnable Python implementation of all three, using a pinned FAKE cloud API (clearly marked as such) so the reconciliation LOGIC is exercised deterministically without depending on live AWS access.

Approach

1. Parse tfstate resources. Each tracked resource carries an address, type, id, and its DECLARED attributes.
2. Query the cloud API per resource, with bounded retry. describe_with_retry wraps the API call in a bounded exponential-backoff loop (attempt count tracked explicitly, backoff modeled structurally rather than with real sleeps so this demo's output stays fast and deterministic) that retries a RateLimitError up to max_attempts times, but does NOT retry a CredentialError, since retrying an expired or invalid credential just burns the retry budget on a guaranteed failure; credential errors are reported distinctly in the output instead.
3. Classify each tracked resource. None returned from the API means the resource is MISSING (gone from the cloud, still in state). A non-None result gets attribute-by-attribute compared against the declared values; any disagreement is recorded as MODIFIED with the specific field-level diff; no disagreement means UNCHANGED.
4. Detect unmanaged resources. For every resource TYPE the account is known to manage (not just the types present in THIS particular tfstate, a type with zero currently-tracked resources can still have untracked ones), query for resources of that type and flag any whose ID is not in the tracked set as UNMANAGED.
5. Emit a structured JSON drift report with four top-level categories plus an errors list for anything that failed to reconcile at all (credential failures, exhausted retries), so those are visible and actionable rather than silently dropped from the report.

Code

import json
import random
import time

class RateLimitError(Exception):
    pass

class CredentialError(Exception):
    pass

class FakeCloudAPI:
    """Stands in for a real AWS SDK client (e.g. boto3). Real code would call
    ec2.describe_instances / s3.list_buckets / etc; this fixture returns a
    FIXED, pinned live-resource map so the reconciliation logic below is
    exercised deterministically. It also deliberately injects a rate-limit
    error on the FIRST call for one resource type and a credential error for
    one specific lookup, so the retry/backoff and auth-error paths are
    actually exhausted by this run, not just written and never hit.
    """
    def __init__(self, seed=42):
        self.rng = random.Random(seed)
        self._ec2_call_count = 0
        self.live_resources = {
            # matches tfstate below except: sg-app has a drifted tag,
            # i-abandoned is MISSING (terminated outside Terraform),
            # bucket-untracked is UNMANAGED (exists in AWS, absent from state),
            # legacy is CredentialError (its account's read role has expired)
            "aws_instance.web": {"id": "i-0abc123", "type": "aws_instance",
                                  "attributes": {"instance_type": "t3.medium", "tags": {"env": "prod"}}},
            "aws_instance.abandoned": None,  # terminated outside Terraform -> MISSING
            "aws_security_group.app": {"id": "sg-0def456", "type": "aws_security_group",
                                        "attributes": {"ingress_ports": [443], "tags": {"env": "prod", "owner": "manual-edit"}}},
            "aws_s3_bucket.untracked": {"id": "bucket-untracked", "type": "aws_s3_bucket",
                                         "attributes": {"versioning": True}},
        }

    def describe(self, resource_type, resource_id, tf_address):
        if tf_address == "aws_instance.legacy":
            raise CredentialError("ExpiredTokenException: the security token included in the request is expired")
        if resource_type == "aws_instance" and self._ec2_call_count == 0:
            self._ec2_call_count += 1
            raise RateLimitError(f"Throttling: rate exceeded describing {resource_type}")
        return self.live_resources.get(tf_address)

    def list_untracked(self, resource_type):
        # returns resources of this type that exist in the cloud but were
        # never in the tfstate map we were given (an "unmanaged" resource)
        if resource_type == "aws_s3_bucket":
            return [{"id": "bucket-untracked", "type": "aws_s3_bucket",
                     "attributes": self.live_resources["aws_s3_bucket.untracked"]["attributes"]}]
        return []


TFSTATE = {
    "resources": [
        {"address": "aws_instance.web", "type": "aws_instance", "id": "i-0abc123",
         "attributes": {"instance_type": "t3.medium", "tags": {"env": "prod"}}},
        {"address": "aws_instance.abandoned", "type": "aws_instance", "id": "i-0stale789",
         "attributes": {"instance_type": "t3.small", "tags": {"env": "staging"}}},
        {"address": "aws_security_group.app", "type": "aws_security_group", "id": "sg-0def456",
         "attributes": {"ingress_ports": [443], "tags": {"env": "prod"}}},
        {"address": "aws_instance.legacy", "type": "aws_instance", "id": "i-legacy999",
         "attributes": {"instance_type": "t2.micro", "tags": {"env": "legacy"}}},
    ]
}


def describe_with_retry(api, resource_type, resource_id, tf_address, max_attempts=3):
    """Bounded retry with exponential backoff for transient throttling.
    Credential errors are NOT retried (retrying an expired/invalid
    credential just wastes the remaining attempts on a guaranteed failure);
    they are raised immediately so the caller can report them distinctly
    from a genuine drift finding."""
    attempt = 0
    while True:
        attempt += 1
        try:
            return api.describe(resource_type, resource_id, tf_address)
        except RateLimitError as e:
            if attempt >= max_attempts:
                raise
            # backoff modeled structurally (attempt number recorded), not
            # timed, so this demo's output is deterministic and fast
            continue
        except CredentialError:
            raise


def reconcile(tfstate, api, known_resource_types):
    """known_resource_types: every resource TYPE this account manages, not just
    the ones present in tfstate -- a type with ZERO tracked resources of that
    kind can still have untracked ones in the cloud, so it has to be scanned
    for "unmanaged" resources even though the loop below never visits it."""
    report = {"modified": [], "missing": [], "unmanaged": [], "errors": [], "unchanged": []}
    tracked_ids_by_type = {t: set() for t in known_resource_types}

    for res in tfstate["resources"]:
        addr, rtype, rid = res["address"], res["type"], res["id"]
        tracked_ids_by_type.setdefault(rtype, set()).add(rid)
        try:
            live = describe_with_retry(api, rtype, rid, addr)
        except CredentialError as e:
            report["errors"].append({"address": addr, "error": str(e)})
            continue
        except RateLimitError as e:
            report["errors"].append({"address": addr, "error": f"exhausted retries: {e}"})
            continue

        if live is None:
            report["missing"].append({"address": addr, "id": rid,
                                       "note": "in tfstate but not found in the cloud API response"})
            continue

        diffs = {}
        for key, declared_value in res["attributes"].items():
            live_value = live["attributes"].get(key)
            if live_value != declared_value:
                diffs[key] = {"declared": declared_value, "live": live_value}
        if diffs:
            report["modified"].append({"address": addr, "id": rid, "diffs": diffs})
        else:
            report["unchanged"].append(addr)

    for rtype, tracked_ids in tracked_ids_by_type.items():
        for live_res in api.list_untracked(rtype):
            if live_res["id"] not in tracked_ids:
                report["unmanaged"].append(live_res)

    return report


if __name__ == "__main__":
    api = FakeCloudAPI(seed=42)
    report = reconcile(TFSTATE, api, known_resource_types=["aws_instance", "aws_security_group", "aws_s3_bucket"])
    print(json.dumps(report, indent=2, sort_keys=True))

    print("\n=== Assertions ===")
    assert len(report["modified"]) == 1 and report["modified"][0]["address"] == "aws_security_group.app"
    print("security group tag drift detected:", report["modified"][0]["diffs"])
    assert len(report["missing"]) == 1 and report["missing"][0]["address"] == "aws_instance.abandoned"
    print("terminated-outside-Terraform instance detected as MISSING:", report["missing"][0]["address"])
    assert len(report["unmanaged"]) == 1 and report["unmanaged"][0]["id"] == "bucket-untracked"
    print("untracked bucket detected as UNMANAGED:", report["unmanaged"][0]["id"])
    assert "aws_instance.web" in report["unchanged"]
    print("unchanged instance correctly reported as no drift:", "aws_instance.web" in report["unchanged"])
    assert len(report["errors"]) == 1 and report["errors"][0]["address"] == "aws_instance.legacy"
    print("credential error correctly reported as its own category, not folded into drift or silently dropped:",
          report["errors"][0])
    print("\nBoth the rate-limit retry path (first EC2 describe call raised RateLimitError and was retried "
          "successfully) and the credential-error path (aws_instance.legacy) were genuinely exercised by this run.")

Output (actually executed with python3 s43_drift.py)

{
  "errors": [
    {
      "address": "aws_instance.legacy",
      "error": "ExpiredTokenException: the security token included in the request is expired"
    }
  ],
  "missing": [
    {
      "address": "aws_instance.abandoned",
      "id": "i-0stale789",
      "note": "in tfstate but not found in the cloud API response"
    }
  ],
  "modified": [
    {
      "address": "aws_security_group.app",
      "diffs": {
        "tags": {
          "declared": {
            "env": "prod"
          },
          "live": {
            "env": "prod",
            "owner": "manual-edit"
          }
        }
      },
      "id": "sg-0def456"
    }
  ],
  "unchanged": [
    "aws_instance.web"
  ],
  "unmanaged": [
    {
      "attributes": {
        "versioning": true
      },
      "id": "bucket-untracked",
      "type": "aws_s3_bucket"
    }
  ]
}

=== Assertions ===
security group tag drift detected: {'tags': {'declared': {'env': 'prod'}, 'live': {'env': 'prod', 'owner': 'manual-edit'}}}
terminated-outside-Terraform instance detected as MISSING: aws_instance.abandoned
untracked bucket detected as UNMANAGED: bucket-untracked
unchanged instance correctly reported as no drift: True
credential error correctly reported as its own category, not folded into drift or silently dropped: {'address': 'aws_instance.legacy', 'error': 'ExpiredTokenException: the security token included in the request is expired'}

Both the rate-limit retry path (first EC2 describe call raised RateLimitError and was retried successfully) and the credential-error path (aws_instance.legacy) were genuinely exercised by this run.


The fixture is pinned to exercise all four classification paths plus the retry and credential-handling paths in one run: `aws_security_group.app`'s live tags include an extra `owner: manual-edit` key not in the declared attributes, correctly reported as MODIFIED with the exact field diff; `aws_instance.abandoned` returns `None` from the fake API (modeling a termination outside Terraform), correctly reported as MISSING; `bucket-untracked` exists in the fake API's inventory but has no corresponding tfstate entry, correctly reported as UNMANAGED; `aws_instance.web` matches exactly and is reported UNCHANGED; the fake API is rigged to raise `RateLimitError` on the FIRST EC2-type describe call specifically, which the retry logic caught and recovered from transparently; and `aws_instance.legacy` is rigged to raise `CredentialError` on every call, correctly recorded as its own `errors` entry rather than being retried or folded into `missing`, confirming both the retry path and the credential-error path actually executed in this run rather than being written but never exercised.

## Key points

- Classifying MISSING versus UNMANAGED requires querying in BOTH directions, tracked-resource-by-resource lookups catch MISSING; a separate listing call per resource type catches UNMANAGED. A tool that only does the first direction (which is the more obvious one to implement) systematically misses every untracked resource, silently under-reporting real drift.
- Credential errors and rate-limit errors need DIFFERENT handling, not a single generic "retry on any exception" catch-all: retrying a credential error wastes the retry budget on something retrying cannot fix, while NOT retrying a genuine transient throttle would report false drift/errors for what was actually a recoverable, momentary condition.
- Reporting errors as their OWN category, separate from MODIFIED/MISSING/UNMANAGED, matters because a resource the tool COULD NOT CHECK is not the same claim as a resource confirmed unchanged; conflating "we don't know" with "no drift" would produce a report that looks more complete and reassuring than the run actually was.

## Complexity

- Time: $$O(R \cdot A)$$ where $$R$$ is the number of tracked resources and $$A$$ is the maximum retry attempts per resource, plus $$O(T)$$ for the untracked-resource listing calls where $$T$$ is the number of distinct resource types managed.
- Space: $$O(R + U)$$ for the report, where $$U$$ is the number of unmanaged resources discovered, both bounded by the actual inventory size, not by anything unbounded in the algorithm itself.

## Edge cases

- **A resource type with zero tracked resources in this particular tfstate but real untracked resources in the cloud:** correctly caught, since `known_resource_types` is passed explicitly rather than derived only from what happens to be in THIS tfstate.
- **Every retry attempt for a resource exhausted without success:** the exception propagates out of `describe_with_retry` and is caught at the reconciliation level, recorded in `errors` rather than crashing the whole reconciliation run over one resource's persistent failure.
- **A resource with an empty attributes dict on either side:** the field-by-field comparison loop simply finds no keys to compare, correctly falling through to UNCHANGED rather than erroring on an empty iteration.

Direct answer

Service-level objectives (SLOs) and error budgets should be represented as PLAIN, VERSION-CONTROLLED DECLARATIVE ARTIFACTS (a YAML or similar spec defining the objective, the underlying service-level indicator query, and the burn-rate alerting thresholds) living in the SAME repository, and going through the SAME PR-and-review workflow, as the infrastructure they govern, deployed to the alerting/automation system by the SAME reconciliation mechanism, not a separate, dashboard-configured, un-versioned side channel. Because an SLO artifact directly controls automated behavior (paging thresholds, and in some organizations automated rollback triggers), its review bar and rollback path need to be at least as rigorous as the infrastructure changes it governs, arguably more so, since a wrong SLO can either cause alert fatigue (too tight) or silently miss real degradation (too loose).

Structured elaboration

Representation. A structured spec per service: the SLI (service-level indicator) query defining what is actually measured (a specific latency percentile, a specific error-rate calculation), the objective threshold and measurement window, and the burn-rate alert thresholds derived from it (fast-burn and slow-burn windows, following the widely-used multi-window burn-rate alerting pattern). This spec is data, not code, so it is reviewable as a clean diff the same way any other declarative configuration is.

Versioning. Committed to Git alongside (or in a clearly cross-referenced sibling path to) the service's own infrastructure definition, so git log on the SLO spec answers "when did this objective change and why" the same way it would for any other infra change, and a service's SLO history is auditable independent of whoever happens to remember the reasoning.

Deployment via Git workflows. A GitOps controller (or a dedicated SLO-config sync job, if the alerting platform is not itself Kubernetes-native) reconciles the deployed alerting-platform configuration to match the Git-declared spec, the same pull-based, continuously-reconciled model already used for application infrastructure generally, so a manually-edited alert threshold in the alerting platform's own UI gets treated as drift, not silently tolerated as a parallel source of truth.

Validation steps. Before merge: schema validation (is the spec well-formed), a SANITY check on the objective itself (is the target achievable given recent actual measured performance, catching an SLO set so tight it would page constantly from day one), and, where the alerting platform supports it, a DRY-RUN evaluation against recent historical data showing what the proposed burn-rate thresholds WOULD have alerted on over, say, the last 30 days, so a reviewer sees concretely whether the change would have caused excessive noise or a dangerous blind spot before it goes live.

Who should review SLO changes. The OWNING team proposes (they have the domain context for what "acceptable" performance means for their service), but a change LOOSENING an existing objective, or changing what a downstream automated system does in response to burn (see below), should require a second reviewer from OUTSIDE the owning team, typically the SRE or platform function with cross-service context, specifically because a team under pressure has an incentive to loosen its own SLO to escape being paged, which is exactly the review-conflict this second-reviewer requirement exists to catch.

How changes propagate to alerting and automation. The reconciler updates the ALERTING PLATFORM's configuration (the actual burn-rate alert rules) directly from the Git-declared spec; any DOWNSTREAM automation keyed off SLO/error-budget state (an automated deployment-freeze trigger when a budget is exhausted, for instance) reads the SAME reconciled objective, so there is exactly one source of truth for "what counts as within budget" feeding every consumer, rather than each consumer holding its own copy that could drift out of sync.

Rolling back an SLO change that causes undesired automation behavior. Because the spec is just a Git-tracked file, rollback is the same git revert plus reconciliation pattern as any other infra change; the practical difference is URGENCY, an SLO change causing a pager storm or, worse, an incorrectly-triggered automated rollback/freeze needs the same fast-path emergency-change mechanism as any infrastructure change: a short-lived, audited bypass with mandatory reconciliation back into Git afterward, since waiting for a normal-cadence PR review while pages fire is not acceptable.

Trade-offs and pitfalls

• Common mistake: configuring SLO thresholds directly in the alerting platform's UI "to iterate quickly," planning to formalize into Git later. This almost never gets formalized in practice, and it recreates exactly the un-auditable, un-reviewable side channel this whole answer exists to prevent; the dry-run-against-historical-data validation step exists specifically to make the Git-first path fast enough that UI-first iteration stops being tempting.
• The second-reviewer requirement for loosening an SLO is the single most-skipped control on this list under deadline pressure, precisely because it exists to catch the exact situation (a team wanting relief from its own pages) where the proposing team has the least incentive to enforce it on themselves.
• A DRY-RUN validation is only as good as the historical window it evaluates against. A recent quiet period can make an objectively too-loose threshold look safe; validating against a window that includes at least one known past incident, where the team can check "would this new threshold have caught it," is a meaningfully stronger check than an arbitrary recent-N-days window.
• Downstream automation consuming SLO state (deployment freezes, automated rollback triggers) needs to fail SAFE if the SLO reconciliation itself is stale or broken, an automation system that silently treats "no recent SLO data" as "budget is fine" can mask exactly the kind of problem it exists to catch; explicit staleness detection on the SLO data feed itself is a real, easy-to-omit requirement.

Direct answer

Immutable digests (image@sha256:...) guarantee that the manifest committed to Git and the bytes actually pulled and run are the SAME image forever, which is exactly what reconciliation, reproducibility, and rollback correctness all depend on; mutable tags (:latest, :v1 reused across builds) let the SAME manifest silently resolve to DIFFERENT bytes at different times, breaking the core GitOps guarantee that Git fully describes what is running. The practical policy: digests (or at minimum strictly immutable, never-reused tags) in every environment a GitOps controller actually reconciles, with a separate, explicit "update the digest" commit as the mechanism for shipping a new version, never implicit re-resolution of a mutable tag; mutable tags are acceptable ONLY in inner-loop developer environments that are explicitly outside the GitOps-reconciled path.

Structured elaboration

Effect on reconciliation. A GitOps controller's reconciliation loop compares the manifest's declared image reference against the cluster's running state. With a digest, "does the running Pod match desired state" is an unambiguous, stable comparison, the digest either matches or it doesn't, and once it matches, reconciliation correctly does nothing further. With a mutable tag, the CONTROLLER sees no drift (the tag string in the manifest hasn't changed), but the ACTUAL running bytes can differ from what was originally deployed if a Pod restarts and re-pulls a tag that has since been overwritten upstream, a form of drift the reconciliation loop is structurally blind to because it only compares the STRING in the manifest, not the resolved content.

Effect on security (CVE, Common Vulnerabilities and Exposures, remediation). A patched image (fixing a CVE) pushed under the SAME mutable tag changes what NEW pods pull without any Git commit recording that change, so there is no audit trail of when the fix actually rolled out, and existing running pods do not automatically pick it up (they keep running the old, pulled bytes until they restart), creating an unpredictable, unrecorded window of mixed-version exposure. A digest-pinned deployment makes CVE remediation an explicit, auditable act: bump the digest in Git, and reconciliation deploys exactly that patched image, deterministically, everywhere, with a Git commit as the permanent record of when the fix went out.

Effect on reproducibility. "Reproducible" means the SAME Git commit always produces the SAME running system. Digests give this property unconditionally. Mutable tags give it only until someone pushes a new image under that tag, at which point the SAME Git commit now resolves differently than it did before, which specifically breaks rollback (reverting to an OLD commit that references :v1 does not guarantee you get back the ORIGINAL :v1 bytes if :v1 has since been overwritten) and breaks any forensic "what was actually running at time T" investigation.

Effect on developer iteration. For an INNER-LOOP dev environment (a developer's own sandbox, rapidly rebuilding and testing), requiring a fresh digest and a Git commit for every single iteration is genuine friction that does not serve any of the guarantees above, since nobody is trying to prove reproducibility or audit a dev sandbox's history the way they would a production deployment.

Worked example

A recommended tiered policy:

The transition from a build artifact to a production digest reference should be an EXPLICIT step in the pipeline (CI resolves the newly built image's digest and commits a manifest update referencing it), not something a human copies by hand, since a manually-typed digest is exactly the kind of error-prone step this policy exists to eliminate.

Trade-offs and pitfalls

• Common mistake: using a "unique-looking" tag (a commit SHA or build number) and assuming it is equivalent to a digest. It usually is, in practice, AS LONG AS the registry and CI pipeline genuinely never reuse it; but this is a PROCESS guarantee (nobody re-pushes that tag), not a CRYPTOGRAPHIC one the way a digest is (the digest IS the content hash, so it cannot silently point to different bytes by definition). A digest removes the reliance on that process discipline entirely.
• Common mistake: enforcing digest-only policy in production manifests but leaving the ADMISSION path open to mutable tags, so a manifest applied outside the normal GitOps flow (a manual kubectl apply, a different pipeline) can still introduce a mutable-tag image; an admission-controller policy (Kyverno/Gatekeeper) rejecting any Pod spec without a resolved digest closes this gap regardless of HOW the manifest arrived.
• Digests make manifests harder for a human to read at a glance (sha256:a1b2c3... conveys no version information the way :v2.3.1 does); a common, workable mitigation is keeping a human-readable tag in the commit message or a companion annotation/label while the actual image: field uses the digest, giving both machine-verifiable immutability and human-readable context.
• Rollback via Git revert only fully works if EVERY commit in history referenced digests, not mutable tags. A rollback to a historical commit that referenced :v1 is not a true rollback if :v1 has since been overwritten; digest pinning is what makes "revert the commit" and "actually get back the old running state" the same operation.

Direct answer

Enforce release gates at TWO complementary layers: the pull request itself (peer/approver review, required status checks, branch protection rules) as the primary compliance-auditable approval, and Argo CD's sync policy (manual sync for production, or SyncWindows restricting when auto-sync can fire) as a technical backstop that prevents a merged-but-not-yet-approved-for-deployment change from auto-applying. The key design constraint: human approval happens on the PR (a Git-native, naturally auditable event with reviewer identity and timestamp already captured), never as an out-of-band manual step performed against the live cluster, which is what would actually break the declarative, Git-is-source-of-truth principle.

Structured elaboration

Where gates are enforced. CI enforces AUTOMATED gates (linting, terraform plan/manifest-diff generation, policy-as-code checks, security scanning) as required status checks that must pass before merge is even possible. The PR itself enforces the HUMAN approval gate via required reviewers (branch protection rules requiring N approvals from a CODEOWNERS-defined group for production-targeting paths). Argo CD enforces a FINAL technical gate: production Application objects configured with syncPolicy.automated OMITTED (manual sync only) or restricted via SyncWindows, so even a merged commit doesn't immediately roll out until an authorized operator (or an automated promotion step gated on its own separate approval) triggers the sync.

What metadata should live in Git. The PR itself is the metadata record: title/description linking to the change ticket, the diff itself (what actually changed), the list of approvers and their approval timestamps (captured natively by the Git hosting platform, exportable via API for audit), and any required-context status check results (policy-as-code pass/fail, security scan results) attached to the commit. For compliance frameworks requiring a documented "why" beyond the diff, a structured commit message or PR template field (change ticket ID, risk assessment, rollback plan) keeps that context IN the same auditable artifact rather than in a separate system that can drift out of sync with what was actually deployed.

Implementing human approval without breaking declarative principles. The subtlety: "declarative" means the DESIRED STATE is fully specified in Git and reconciliation is automatic given that state, not that every state transition must be fully automatic with no human in the loop. A human approval gate is compatible with GitOps as long as the approval ITSELF is captured declaratively (the merged PR, an approved state) and the reconciler then acts deterministically on whatever is in Git; what would BREAK the principle is a human directly editing the live cluster to "approve" a change out of band, since that reintroduces exactly the drift-inducing manual intervention GitOps exists to eliminate. A common, clean pattern: a promotion PR (bumping an image tag or config value in the production overlay) requires its own separate, higher-bar approval from the PR that merged the change into staging, so "promote to production" is itself a reviewed, auditable Git event, not a live cluster action.

Trade-offs and pitfalls

• Common mistake: implementing the "human approval" step as a manual argocd app sync performed by an operator outside of any recorded process. This satisfies "a human looked at it" but produces NO durable audit record of WHO approved WHAT and WHY beyond whatever is (or isn't) in a chat log; the PR-based approval model is specifically what makes the approval auditable without extra tooling, since the Git platform already records it.
• Common mistake: conflating CI's automated checks with the compliance-required human approval. A passing lint/policy-as-code check is necessary but not sufficient for most compliance frameworks, which specifically require a human decision-maker's sign-off; treating "all checks green" as equivalent to "approved" removes the human judgment step the compliance requirement exists to capture.
• SyncWindows and manual-sync settings are a technical backstop, not a substitute for PR-level review. They prevent an ALREADY-MERGED change from auto-deploying at the wrong time, but by the time a change is merged, the PR-level approval should already have happened; relying on the sync gate as the PRIMARY approval mechanism (skipping rigorous PR review because "someone will catch it before syncing") inverts the intended order and risks a rushed, under-reviewed sync-time approval replacing a properly reviewed PR.
• CODEOWNERS-based required-reviewer rules need to be scoped to the RIGHT paths. A blanket "any 2 approvers" rule does not satisfy most compliance requirements for segregation of duties if the two approvers can be the same person who wrote the change plus a rubber-stamping teammate; production-path changes typically need a reviewer who is NOT the author and, in stricter regimes, a reviewer from a DIFFERENT team or a designated compliance role.

Direct answer

Helm packages Kubernetes manifests as a TEMPLATED chart (Go template syntax over YAML, plus a values.yaml providing parameters) with a release/versioning model and a package registry ecosystem; Kustomize takes plain, valid YAML manifests as a BASE and applies structural, patch-based OVERLAYS per environment with no templating language at all. For a multi-team organization, the practical recommendation is Kustomize for INTERNAL application manifests owned and reviewed by the teams that run them (overlays are easy to read as plain YAML diffs in a PR), and Helm specifically for THIRD-PARTY or widely-reused software (anything you install FROM someone else's chart, or genuinely reusable internal building blocks you want to parameterize and publish once for many independent consumers).

Structured elaboration

Parameterization and environment overlays, Helm. A chart defines {{ .Values.X }} template placeholders throughout its manifests; each environment supplies its own values-<env>.yaml overriding just the fields that differ. This is powerful (conditionals, loops, helper functions, subchart composition) but the templates themselves are not valid YAML until rendered, so reviewing a raw chart change means mentally executing the template logic, not just reading YAML.

Parameterization and environment overlays, Kustomize. A base/ directory holds plain, directly-appliable YAML; each environment's overlays/<env>/kustomization.yaml lists STRATEGIC MERGE PATCHES or JSON patches against that base (bump replica count, change an image tag, add an env-specific label). Every file at every layer is always valid, renderable YAML on its own, so a reviewer can read a base manifest and an overlay patch and understand the FULL resulting object without running anything.

Pros and cons.

Recommendation for a multi-team organization. Use Kustomize as the default for TEAM-OWNED application manifests: base manifests live in the service's own repo, environment overlays are small, auditable patch files that show up as clean diffs in pull requests, and a reviewer with zero Helm-template-rendering context can read exactly what changes between staging and production. Reserve Helm for two specific cases: consuming THIRD-PARTY software (installing Prometheus, cert-manager, ingress controllers from their published charts, since fighting the ecosystem's Helm-only distribution model is not worth it), and building genuinely reusable INTERNAL platform building blocks meant to be parameterized and consumed by many independent, less-GitOps-sophisticated teams, where Helm's stronger parameterization primitives (conditionals, defaults, schema validation via values.schema.json) earn their complexity. A common, workable hybrid: use helm template to render third-party charts into plain YAML, then apply Kustomize overlays ON TOP of the rendered output for environment-specific tweaks, getting Kustomize's review-friendliness for the parts a team actually edits while still consuming the Helm ecosystem for upstream software.

Trade-offs and pitfalls

• Common mistake: adopting Helm organization-wide "because it's the ecosystem standard" without weighing the code-review cost. A templated chart's diff in a pull request shows changes to TEMPLATE LOGIC or VALUES, not the resulting Kubernetes objects; a reviewer who does not mentally render the template can approve a change without fully understanding what will actually be applied, a real, recurring review-quality gap Kustomize's plain-YAML model avoids by construction.
• Common mistake: assuming Kustomize overlays scale cleanly to many environments without discipline. A patch-based model with no templating can accumulate subtle, hard-to-track divergence across a dozen environment overlays if there is no convention for what belongs in the base versus what belongs in each overlay; Kustomize's simplicity is a review-time advantage, not a substitute for a deliberate base/overlay design convention.
• Security: template injection is a REAL risk specific to Helm when chart values are ever sourced from something less trusted than the chart author (a CI variable an external contributor could influence, for instance), since Go template execution can, in pathological cases, be abused; Kustomize's patch-only model has no equivalent template-execution attack surface, a genuine, structural security argument in Kustomize's favor for team-owned manifests.
• The two tools are not mutually exclusive, and the strongest real-world setups typically use both, Helm for what the ecosystem forces (third-party software) and Kustomize for what teams actually author and review day to day; treating this as an either/or organizational mandate usually produces friction in whichever direction was NOT chosen.