Add credential_namespace variable to harvester-integration

[HiranAdikari]

Summary

• Add credential_namespace variable to the harvester-integration module, allowing the Kubernetes namespace for the cloud credential ServiceAccount and token Secret to be configured (default: kube-system)
• Fix CoreDNS ConfigMap resource to always be present, using a conditional Corefile (patched vs default) instead of count = 0 which would destroy the entire Corefile key and break cluster DNS

Why

The credential_namespace variable is needed for brownfield imports where the SA and Secret were originally created in a namespace other than the default kube-system (e.g., default). Without this variable, terraform plan forces a destroy+recreate of the SA, ClusterRoleBinding, and Secret — which would briefly break cloud credential authentication.

The CoreDNS fix prevents a dangerous plan where patch_coredns = false would destroy the Corefile key entirely, leaving CoreDNS with no configuration and breaking cluster DNS.

Test plan

• terraform plan with credential_namespace = "default" shows no changes for existing SA/Secret in default namespace
• patch_coredns = false restores the standard Corefile (no hosts block) instead of destroying the key
• patch_coredns = true correctly injects rancher_lb_ip → rancher_hostname in the hosts block

Closes #82

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 406ab11a-21a7-4ab3-b314-b491526becc1

📥 Commits

Reviewing files that changed from the base of the PR and between 1b15a74 and 4a1ea73.

📒 Files selected for processing (2)

• modules/management/harvester-integration/main.tf
• modules/management/harvester-integration/variables.tf

🚧 Files skipped from review as they are similar to previous changes (1)

• modules/management/harvester-integration/variables.tf

---

📝 Walkthrough

Overview

Adds a new credential_namespace variable to the harvester-integration module to support configurable Kubernetes namespaces for cloud credential resources, and improves CoreDNS ConfigMap handling to prevent unintended configuration loss.

Changes

New Variable

• Introduces credential_namespace (type: string, default: "kube-system") with validation to reject empty/whitespace-only values. Allows configuring the Kubernetes namespace for the cloud credential ServiceAccount, token Secret, and ClusterRoleBinding.

Cloud Credential Resources

• Updates ServiceAccount, Secret, and ClusterRoleBinding resources to use var.credential_namespace instead of a hard-coded "kube-system", avoiding forced destroy+recreate in brownfield imports where those resources live in a different namespace (e.g., default).

CoreDNS ConfigMap Handling

• Refactors CoreDNS Corefile generation by moving patched and default Corefile bodies into locals.
• Ensures the CoreDNS ConfigMap key (Corefile) always exists; toggling patch_coredns now swaps Corefile content rather than creating/destroying the key.
• Adds a Terraform check (coredns_patch_vars_set) that fails early if patch_coredns = true without both rancher_lb_ip and rancher_hostname, preventing rendering an empty hosts entry.

Test Plan / Behavior

• terraform plan with credential_namespace = "default" shows no changes for existing SA/Secret in default.
• patch_coredns = false restores the standard Corefile (no hosts block) rather than removing the key.
• patch_coredns = true injects the hosts entry mapping rancher_lb_ip → rancher_hostname.

Impact

Prevents Terraform-detected namespace drift and unintended resource replacements in brownfield deployments, and ensures CoreDNS configuration remains intact when CoreDNS patching is toggled. Closes #82.

Walkthrough

This pull request adds a new input variable credential_namespace (default "kube-system") to the harvester-integration module and uses it as the namespace for the cloud credential ServiceAccount and token Secret, allowing brownfield imports to avoid forced replacements. It refactors CoreDNS Corefile handling by moving patched and default Corefile contents into locals, makes the CoreDNS ConfigMap data always present (switching contents via var.patch_coredns instead of creating/destroying the key), and adds a Terraform check that requires rancher_lb_ip or rancher_hostname when patch_coredns=true.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Terraform
    participant HarvesterModule as "harvester-integration module"
    participant KubernetesAPI as "Kubernetes API (cluster)"
    participant Rancher as "Rancher (optional)"

    User->>Terraform: terraform apply
    Terraform->>HarvesterModule: render resources with vars (credential_namespace, patch_coredns, rancher_*)
    HarvesterModule->>KubernetesAPI: create/update ServiceAccount & Secret in credential_namespace
    HarvesterModule->>KubernetesAPI: create/update ClusterRoleBinding (namespace-aware)
    alt patch_coredns = true
        HarvesterModule->>KubernetesAPI: update CoreDNS ConfigMap with patched Corefile (includes rancher_lb_ip/hostname)
        note right of HarvesterModule: Terraform check ensures rancher_lb_ip or rancher_hostname set
    else patch_coredns = false
        HarvesterModule->>KubernetesAPI: update CoreDNS ConfigMap with default Corefile
    end
    KubernetesAPI->>Terraform: acknowledge resource state
    Terraform->>User: apply complete

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and accurately summarizes the primary change—adding a credential_namespace variable to the harvester-integration module.
Description check	✅ Passed	The description covers the main changes, rationale, and test plan effectively, though it does not follow the template structure with labeled sections like Purpose, Goals, Approach, etc.
Linked Issues check	✅ Passed	The PR implements the core requirement from issue #82: adds credential_namespace variable and applies it to the three resource namespace fields. Additionally refactors CoreDNS handling to prevent destructive plans.
Out of Scope Changes check	✅ Passed	The CoreDNS refactoring and validation check are complementary to the primary objective and address preventing dangerous operational states when the variable is used.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Review rate limit: 0/1 reviews remaining, refill in 60 minutes.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

modules/management/harvester-integration/variables.tf (1)

19-23: Add validation for credential_namespace input.

This variable is configurable, but invalid empty values are not rejected early. A small validation block will fail fast and prevent apply-time provider errors.

Proposed patch

 variable "credential_namespace" {
   type        = string
   description = "Kubernetes namespace for the cloud credential ServiceAccount and token Secret on Harvester. Override to match an existing namespace when importing brownfield state."
   default     = "kube-system"
+  validation {
+    condition     = trim(var.credential_namespace) != ""
+    error_message = "credential_namespace must be a non-empty Kubernetes namespace."
+  }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@modules/management/harvester-integration/variables.tf` around lines 19 - 23,
Add a Terraform validation block to the variable "credential_namespace" to
reject empty values and fail fast; update the variable "credential_namespace"
declaration to include a validation { condition =
length(trim(var.credential_namespace)) > 0 error_message = "credential_namespace
must not be empty" } so blank or whitespace-only inputs are refused before
plan/apply.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@modules/management/harvester-integration/main.tf`:
- Around line 113-123: The CoreDNS patch template (_coredns_corefile_patched) is
interpolating var.rancher_lb_ip and var.rancher_hostname even when they are
empty; modify the logic that applies the patch (guarded by patch_coredns) to
also require non-empty var.rancher_lb_ip and var.rancher_hostname (e.g., add a
conditional check or use a Terraform validation/conditional expression) so that
when patch_coredns = true but either var.rancher_lb_ip or var.rancher_hostname
is empty the patch is blocked and a clear error is raised; update references
around patch_coredns, local._coredns_corefile_patched and any resource that uses
that local to respect this guard.

---

Nitpick comments:
In `@modules/management/harvester-integration/variables.tf`:
- Around line 19-23: Add a Terraform validation block to the variable
"credential_namespace" to reject empty values and fail fast; update the variable
"credential_namespace" declaration to include a validation { condition =
length(trim(var.credential_namespace)) > 0 error_message = "credential_namespace
must not be empty" } so blank or whitespace-only inputs are refused before
plan/apply.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a14e71f1-f8d5-417b-97c8-19d42e11af7c

📥 Commits

Reviewing files that changed from the base of the PR and between 291c8a3 and 1b15a74.

📒 Files selected for processing (2)

• modules/management/harvester-integration/main.tf
• modules/management/harvester-integration/variables.tf

[HiranAdikari]

Both review comments addressed in the latest commit:

• credential_namespace validation: Added a validation block that rejects empty or whitespace-only values with a clear error message before plan/apply.
• CoreDNS patch guard: Added a check "coredns_patch_vars_set" block (consistent with the existing harvester_kubeconfig_has_embedded_ca check) that asserts patch_coredns = false || (rancher_lb_ip != "" && rancher_hostname != ""), so setting patch_coredns = true without both variables fails fast with an actionable error instead of silently rendering an empty hosts entry.