feat(deployers): implement vcluster factory deduction - #77
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds vCluster provisioning through OpenTofu and Kubernetes, extends provider lifecycle handling, supports configurable Terraform roots, normalizes OpenTofu outputs, persists generated kubeconfigs, and improves OPA remediation readiness checks. ChangesvCluster integration
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟡 Moderate · up to The vcluster deployment changes can fail during provider initialization or chart rendering because incompatible Terraform provider versions remain allowed and the pinned chart rejects a configured key; an unresolved credential-output validation issue and a small file-descriptor leak also remain. The PR is not merge-ready until the deployment blockers are fixed or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant TFDeployer
participant VClusterProvider
participant OpenTofu
participant HostKubernetes
TFDeployer->>VClusterProvider: resolve deployment variables
VClusterProvider->>HostKubernetes: inspect context and detect Service CIDR
TFDeployer->>OpenTofu: apply vCluster stack
OpenTofu-->>TFDeployer: return normalized cluster outputs
TFDeployer->>VClusterProvider: write generated kubeconfig
VClusterProvider->>HostKubernetes: clean orphaned volumes and temporary kubeconfig
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
kubernetes-prow
Bot
added
the
cncf-cla: yes
|
Hi @isadominguez314. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
kubernetes-prow
Bot
added
needs-ok-to-test
|
/hold |
kubernetes-prow
Bot
added
the
do-not-merge/hold
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (8)
tf/prebuilt/vcluster/main.tf (4)
137-158: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSet an explicit
timeoutfor the Helm release.
wait = trueandwait_for_jobs = trueblock until the release is ready, but notimeoutis given, so the provider default of 300 seconds applies. A vCluster control plane on a loaded or local host cluster can exceed that. The apply then fails with a partially deployed release still in the namespace.Add a variable-driven timeout, and consider
cleanup_on_failso a failed release does not leave orphaned objects for the provider cleanup path to find.♻️ Proposed change
wait = true wait_for_jobs = true + timeout = var.helm_timeout_seconds + cleanup_on_fail = trueAdd to
tf/prebuilt/vcluster/variables.tf:variable "helm_timeout_seconds" { description = "Seconds to wait for the vCluster Helm release to become ready." type = number default = 900 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tf/prebuilt/vcluster/main.tf` around lines 137 - 158, Update the helm_release.vcluster resource to set a variable-driven timeout using a new helm_timeout_seconds variable in variables.tf, defaulting to 900 seconds with an appropriate description. Enable cleanup_on_fail for failed releases so partially deployed objects are removed during provider cleanup.
134-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine the external endpoint expression once. The same non-trivial conditional expression is written in both files. The two copies can drift, and any correction, including the empty-
LoadBalancer-address fix, must be applied twice.
tf/prebuilt/vcluster/main.tf#L134-L134: keeplocal.external_endpointas the single definition, and split theLoadBalancerbranch into its own local as proposed in the separate comment on this line.tf/prebuilt/vcluster/outputs.tf#L31-L34: replace the duplicated expression withvalue = local.external_endpoint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tf/prebuilt/vcluster/main.tf` at line 134, In tf/prebuilt/vcluster/main.tf lines 134-134, retain local.external_endpoint as the single definition and extract the LoadBalancer branch into its own local as requested by the related comment. In tf/prebuilt/vcluster/outputs.tf lines 31-34, remove the duplicated conditional expression and set the output value to local.external_endpoint.
126-133: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
nodes[0]selection is unstable and can pick a node that does not serve the NodePort.
data.kubernetes_nodes.host_nodes.nodeshas no guaranteed ordering. Two consequences follow:
- On a multi-node host cluster,
nodes[0]may be a control-plane node whose address is not the one the caller can reach.- If the ordering changes between plans,
local.host_node_ipchanges. That value flows throughtemplatefileintohelm_release.vcluster.values, so the release shows a diff and is updated for no functional reason.Select the node deterministically, for example by sorting the candidate addresses, and prefer a worker node when one exists.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tf/prebuilt/vcluster/main.tf` around lines 126 - 133, Update the host_node_ip expression to avoid relying on nodes[0]: derive candidate addresses from worker nodes when available, fall back to all host nodes otherwise, and sort the candidates before selecting one. Preserve the ExternalIP-over-InternalIP preference and 127.0.0.1 fallback while ensuring stable selection across plans.
86-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider exposing the quota and limit-range sizing as variables.
The CPU, memory, storage, and PVC ceilings are hardcoded. A task that needs a larger or smaller virtual cluster must edit the stack. The values also duplicate the inner
policies.resourceQuotavalues invalues.yaml.tftpl(6 CPU, 24Gi), so the two can drift.Promote the numbers to variables in
tf/prebuilt/vcluster/variables.tfwith the current values as defaults, and derive the template values from the same variables.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tf/prebuilt/vcluster/main.tf` around lines 86 - 121, Promote the hardcoded CPU, memory, storage, PVC, and limit-range ceilings in the vcluster Terraform configuration to variables in variables.tf, preserving the current values as defaults. Update the resource quota and kubernetes_limit_range.vcluster_limits definitions to reference those variables, and derive the corresponding policies.resourceQuota values in values.yaml.tftpl from the same variables so sizing cannot drift.devops_bench/providers/vcluster.py (3)
286-290: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
get_envinstead ofos.environ.getfor consistency.This module already imports
get_envand uses it at Lines 264, 265, 370, and 401. Lines 286 and 290 read the environment directly. The same pattern repeats inresolve_variablesat Lines 373-374 and 379. Use one accessor so environment handling stays uniform.♻️ Proposed change
- host_kubeconfig = vars_dict.get("host_kubeconfig_path") or os.environ.get( - "HOST_KUBECONFIG", "~/.kube/config" - ) + host_kubeconfig = ( + vars_dict.get("host_kubeconfig_path") + or get_env("HOST_KUBECONFIG") + or "~/.kube/config" + ) host_kubeconfig_path = str(Path(host_kubeconfig).expanduser().resolve()) - host_context = vars_dict.get("host_kubecontext") or os.environ.get("HOST_KUBECONTEXT") + host_context = vars_dict.get("host_kubecontext") or get_env("HOST_KUBECONTEXT")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@devops_bench/providers/vcluster.py` around lines 286 - 290, Replace the direct os.environ.get calls for HOST_KUBECONFIG and HOST_KUBECONTEXT in the visible kubeconfig/context resolution logic with the module’s existing get_env accessor. Apply the same change to the corresponding environment lookups in resolve_variables, preserving the current defaults and fallback behavior.
384-393: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGuard against a local host context combined with a
LoadBalanceroverride.The remote gate is clear and the default
service_typeselection is correct. One gap:service_typeusessetdefault, so a task can passservice_type: "LoadBalancer"for a local host context, or"NodePort"for a remote one. The stack then builds a service that never gets a reachable endpoint, andexternal_endpointresolves to an empty string. Consider validating the combination, or at least logging a warning when the caller override contradicts the detected host class.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@devops_bench/providers/vcluster.py` around lines 384 - 393, Validate the caller-provided service_type in the local/remote classification flow around _is_allowlisted_context: reject or clearly warn when a local context specifies LoadBalancer or a remote context specifies NodePort, while preserving the existing defaults when no override is provided. Ensure contradictory overrides cannot silently produce an unreachable endpoint.
149-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog the swallowed parse failure before the name-based fallback.
If kubeconfig parsing fails, the function silently falls back to the context-name allowlist. That fallback is weaker than IP inspection, and the operator gets no signal about why. Add a debug or warning log with the exception.
♻️ Proposed change
- except Exception: - pass + except Exception as exc: # noqa: BLE001 - fall back to name allowlist + _log.warning( + "Failed to inspect kubeconfig %s for context %s; " + "falling back to context-name allowlist: %s", + kubeconfig_path, + context_name, + exc, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@devops_bench/providers/vcluster.py` around lines 149 - 154, Update the kubeconfig parsing exception handler in the surrounding context-checking function to capture the exception and log it at debug or warning level before continuing to the name-based fallback. Preserve the existing fallback return logic using _EXACT_LOCAL_CONTEXTS and _LOCAL_CONTEXT_PREFIXES.Source: Linters/SAST tools
tests/unit/providers/test_vcluster_provider.py (1)
246-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a unique temp file name and remove it deterministically.
_is_safe_scratch_pathrequires the parent directory to equal the system temp directory, sotmp_pathcannot be used here. The fixed nametest-vcluster-clean-vc-config.yamlstill collides between concurrent runs on the same machine, for example two CI jobs orpytest-xdistworkers. If the assertion fails, the file also stays behind.Create the file with
tempfile.mkstempand remove it in afinallyblock.♻️ Proposed change
def test_vcluster_cleanup_deletes_scratch_kubeconfig( mocker: MockerFixture, ) -> None: mocker.patch("devops_bench.providers.vcluster.run") - scratch_file = Path(tempfile.gettempdir()) / "test-vcluster-clean-vc-config.yaml" - scratch_file.write_text("test-kubeconfig", encoding="utf-8") - - info = ClusterInfo( - name="test-cluster", - location="local", - project="local-vcluster", - kubeconfig_path=str(scratch_file), - ) - VClusterProvider().cleanup( - info, - variables={ - "host_kubeconfig_path": "/fake/host", - "host_kubecontext": "kind-host", - }, - ) - - assert not scratch_file.exists() + fd, raw_path = tempfile.mkstemp(prefix="test-vcluster-clean-", suffix=".yaml") + os.close(fd) + scratch_file = Path(raw_path) + try: + scratch_file.write_text("test-kubeconfig", encoding="utf-8") + + info = ClusterInfo( + name="test-cluster", + location="local", + project="local-vcluster", + kubeconfig_path=str(scratch_file), + ) + VClusterProvider().cleanup( + info, + variables={ + "host_kubeconfig_path": "/fake/host", + "host_kubecontext": "kind-host", + }, + ) + + assert not scratch_file.exists() + finally: + scratch_file.unlink(missing_ok=True)Add
import osfor this change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/providers/test_vcluster_provider.py` around lines 246 - 267, Update test_vcluster_cleanup_deletes_scratch_kubeconfig to create a uniquely named file directly under the system temp directory using tempfile.mkstemp, adding the required os import and closing the returned descriptor before writing. Wrap the cleanup invocation and assertion in a finally block that deterministically removes the temporary file, while preserving the existing cleanup assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devops_bench/deployers/factory.py`:
- Around line 38-44: Update the generic deployer docstrings and ConfigError
message in the factory logic to remove provider-specific “gcp” terminology,
replacing it with neutral wording such as “supported provider” or “supported
in-repository stack.” Apply the change at the referenced sections while
preserving the existing provider deduction and validation behavior.
- Around line 61-63: Update the stack-provider deduction logic around stack_path
so relative candidates are resolved against the repository tf/ root and only
auto-deduced when the resolved path remains within that root; otherwise require
explicit provider selection. Preserve the existing kind, vcluster, and gcp name
matching for valid in-root paths.
In `@devops_bench/deployers/tofu.py`:
- Around line 257-280: The teardown flow around the try/finally block must not
invoke destructive provider cleanup unless both initialization and tofu destroy
complete successfully. Track teardown success after the destroy command, pass
that status through the cleanup contract used by provider.cleanup (including
VClusterProvider.cleanup), and ensure missing directories or failed commands
only perform safe local cleanup without deleting provider resources.
In `@devops_bench/providers/vcluster.py`:
- Around line 263-272: Remove or tighten the TF_DATA_DIR condition in the
path-safety return within the cleanup validation so sibling files are not
treated as deletable. Only allow TF_DATA_DIR-related paths when the TF_DATA_DIR
parent is contained by tmp_dir or BENCH_RUN_STATE_ROOT, or when the candidate
path is already within a known run root; preserve the existing tmp_dir and
BENCH_RUN_STATE_ROOT checks.
- Around line 203-238: Update the kubeconfig write path after parent-directory
creation to open raw_target with os.O_NOFOLLOW, preserving the existing write
flags and file mode so the kernel atomically rejects a final-component symlink.
Convert the resulting OSError into the existing ConfigError behavior/message,
and stop relying on the resolved_target symlink re-check for this protection.
- Around line 213-214: Update the NodePort rewrite condition in
ensure_cluster_credentials to depend on service_type being "NodePort" and
node_port being present, rather than location being "local". Add coverage for a
non-local location with NodePort service_type and node_port, asserting the
kubeconfig server URL is rewritten.
In `@tests/unit/deployers/test_deployers_factory.py`:
- Line 307: Add type annotations to all five specified test functions:
tests/unit/deployers/test_deployers_factory.py lines 307, 327, and 344, and
tests/unit/deployers/test_deployers_tofu.py lines 163-165 and 273. Annotate
every fixture parameter (mocker, base_config, and monkeypatch where present)
using the project’s existing fixture types and add a None return annotation.
In `@tests/unit/providers/test_vcluster_provider.py`:
- Around line 315-320: Update the ClusterInfo construction in the affected test
to pass an empty string for kubeconfig_path instead of None, matching the
field’s declared str type while preserving cleanup behavior.
In `@tf/prebuilt/vcluster/main.tf`:
- Line 134: Add a lifecycle precondition to helm_release.vcluster that rejects
local.external_endpoint when it is empty or ends with “:”, with an error
explaining that the LoadBalancer address is not assigned. Keep the existing
external_endpoint resolution unchanged and ensure the Helm release fails before
rendering an unusable kubeconfig.
In `@tf/prebuilt/vcluster/values.yaml.tftpl`:
- Line 75: Remove the hardcoded serviceCIDR from the vcluster values template so
the chart can discover the host cluster range, or wire it to a new optional
service_cidr variable from the vcluster Terraform configuration with an empty
default. Update the corresponding main.tf Helm values flow and variables.tf
declaration, preserving automatic detection when no value is supplied.
- Around line 77-81: Update the vCluster values configuration around
experimental.syncSettings.syncLabels so virtual PVC/PV objects that claim host
PVs receive the devops-bench/cluster-name label populated from cluster_name.
Preserve the existing syncLabels entries, and ensure the label contract remains
effective when the default cluster_name value is empty so
VClusterProvider.cleanup can select the synced PVs.
---
Nitpick comments:
In `@devops_bench/providers/vcluster.py`:
- Around line 286-290: Replace the direct os.environ.get calls for
HOST_KUBECONFIG and HOST_KUBECONTEXT in the visible kubeconfig/context
resolution logic with the module’s existing get_env accessor. Apply the same
change to the corresponding environment lookups in resolve_variables, preserving
the current defaults and fallback behavior.
- Around line 384-393: Validate the caller-provided service_type in the
local/remote classification flow around _is_allowlisted_context: reject or
clearly warn when a local context specifies LoadBalancer or a remote context
specifies NodePort, while preserving the existing defaults when no override is
provided. Ensure contradictory overrides cannot silently produce an unreachable
endpoint.
- Around line 149-154: Update the kubeconfig parsing exception handler in the
surrounding context-checking function to capture the exception and log it at
debug or warning level before continuing to the name-based fallback. Preserve
the existing fallback return logic using _EXACT_LOCAL_CONTEXTS and
_LOCAL_CONTEXT_PREFIXES.
In `@tests/unit/providers/test_vcluster_provider.py`:
- Around line 246-267: Update test_vcluster_cleanup_deletes_scratch_kubeconfig
to create a uniquely named file directly under the system temp directory using
tempfile.mkstemp, adding the required os import and closing the returned
descriptor before writing. Wrap the cleanup invocation and assertion in a
finally block that deterministically removes the temporary file, while
preserving the existing cleanup assertion.
In `@tf/prebuilt/vcluster/main.tf`:
- Around line 137-158: Update the helm_release.vcluster resource to set a
variable-driven timeout using a new helm_timeout_seconds variable in
variables.tf, defaulting to 900 seconds with an appropriate description. Enable
cleanup_on_fail for failed releases so partially deployed objects are removed
during provider cleanup.
- Line 134: In tf/prebuilt/vcluster/main.tf lines 134-134, retain
local.external_endpoint as the single definition and extract the LoadBalancer
branch into its own local as requested by the related comment. In
tf/prebuilt/vcluster/outputs.tf lines 31-34, remove the duplicated conditional
expression and set the output value to local.external_endpoint.
- Around line 126-133: Update the host_node_ip expression to avoid relying on
nodes[0]: derive candidate addresses from worker nodes when available, fall back
to all host nodes otherwise, and sort the candidates before selecting one.
Preserve the ExternalIP-over-InternalIP preference and 127.0.0.1 fallback while
ensuring stable selection across plans.
- Around line 86-121: Promote the hardcoded CPU, memory, storage, PVC, and
limit-range ceilings in the vcluster Terraform configuration to variables in
variables.tf, preserving the current values as defaults. Update the resource
quota and kubernetes_limit_range.vcluster_limits definitions to reference those
variables, and derive the corresponding policies.resourceQuota values in
values.yaml.tftpl from the same variables so sizing cannot drift.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9fd4b167-5c0e-46c1-a1c8-5d3d65a53c90
📒 Files selected for processing (17)
devops_bench/deployers/factory.pydevops_bench/deployers/tofu.pydevops_bench/providers/__init__.pydevops_bench/providers/base.pydevops_bench/providers/gcp.pydevops_bench/providers/kind.pydevops_bench/providers/vcluster.pytasks/vcluster/smoke-test/task.yamltests/unit/deployers/test_deployers_factory.pytests/unit/deployers/test_deployers_tofu.pytests/unit/providers/test_providers.pytests/unit/providers/test_vcluster_provider.pytf/prebuilt/vcluster/.terraform.lock.hcltf/prebuilt/vcluster/main.tftf/prebuilt/vcluster/outputs.tftf/prebuilt/vcluster/values.yaml.tftpltf/prebuilt/vcluster/variables.tf
d1d48bb to
53f65d9
Compare
janetkuo
added
ok-to-test
Implement specification for standalone Loft Labs vCluster provisioning: - Add main.tf with standalone Stage 1 LoadBalancer exposure service and Stage 2 Helm release. - Validate external endpoint, dynamically discover serviceCIDR, inject devops-bench labels. - Wire vcluster into generic tf/modules/cluster. - Update opa-remediation prebuilt to wait for vcluster API server and resolve kubeconfigs dynamically.
1ea4835 to
48e1026
Compare
48e1026 to
49bef7c
Compare
isadominguez314
changed the title
49bef7c to
35ee429
Compare
- Move tf/prebuilt/vcluster to tf/modules/cluster/vcluster to fix inverted dependency. - Update tf/modules/cluster/main.tf to use the new vcluster submodule path. - Change default service_type in vcluster/variables.tf to LoadBalancer to match parent module. - Format opa-remediation main.tf
35ee429 to
59053d9
Compare
59053d9 to
2f37144
Compare
2f37144 to
5d8ecf0
Compare
4a94d2f to
c9dcbc3
Compare
b0fd487 to
63e80df
Compare
63e80df to
5e4fc3f
Compare
kubernetes-prow
Bot
added
lgtm
…tegration Implement specification for CL 2: - Add VClusterProvider with inline no-op account credentials, two-step local allowlist check, and secure kubeconfig writing (0600) / PV cleanup. - Modify TFDeployer to cache ClusterInfo, pass unwrapped outputs to ensure_cluster_credentials(), and call provider.cleanup() on destroy. - Update base Provider, GcpProvider, and KindProvider signatures for CL 2 compatibility. - Add unit tests for VClusterProvider and register provider in PROVIDERS.
…in vcluster provider
…path safety, and claimRef PV cleanup
…t descriptor leak
Implement specification for CL 3: - Update _select_provider() in factory.py to match 'vcluster' and 'gcp' by stack directory name and update ConfigError message. - Add unit tests in test_deployers_factory.py for vcluster stack directory deduction and INFRA_PROVIDER=vcluster override.
…, shared resolve_tf_root, and test assertions
…credential isolation
5e4fc3f to
d8415ea
Compare
kubernetes-prow
Bot
removed
the
lgtm
|
/unhold |
kubernetes-prow
Bot
removed
the
do-not-merge/hold
kubernetes-prow
Bot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: isadominguez314, itssimrank, janetkuo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
Summary
Implements Factory Deduction
Note
Stacked PR: This PR is stacked on top of PR #76 (vcluster provider integration). Please review and merge PR #76 first.
Changes
vclusterprovider by inspecting the stack directory name.test_deployers_factory.py.vcluster/smoke-testtask was intentionally removed from this PR to keep tasks provider agnostic).Verification
Summary by CodeRabbit
New Features
Bug Fixes