Skip to content

fix!: reject import/management of packages deployed with overridden package name - #343

Open
jasonwashburn wants to merge 17 commits into
mainfrom
fix/tofu-1_handle-uds-cli-name-override-openspec
Open

fix!: reject import/management of packages deployed with overridden package name#343
jasonwashburn wants to merge 17 commits into
mainfrom
fix/tofu-1_handle-uds-cli-name-override-openspec

Conversation

@jasonwashburn

@jasonwashburn jasonwashburn commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

BREAKING CHANGE: uds_package no longer attempts to manage existing package deployments whose deployed name differs from the configured source package's canonical metadata.name. Previously deployed packages who's name's were overridden, for example by uds-cli, cannot be imported by the provider and must be migrated externally before they can be managed under their canonical identity.

Description

This PR prevents uds_package from unsafely adopting, via import, or updating Zarf packages whose deployed name was previously overridden by a UDS bundle and no longer matches the source package's canonical metadata.name.

UDS CLI supports overriding a package's name during deployment so users can deploy multiple copies of the same package and target those deployments individually, such as with a uds-config.yaml. This functionality is not necessary in the OpenTofu provider because each package resource is configured individually. It also differs from Zarf's native behavior by changing the identity under which Zarf manages the deployed package.

Prior to this change, importing an aliased deployment and applying configuration for the canonical package could create a second Zarf package identity while overlapping resources owned by the existing deployment. In addition to leaving an orphaned Zarf package Secret, this could couple both package identities to some of the same deployed resources and result in unintended removal of cluster resources when attempting to clean up the orphaned package.

To prevent this condition, the provider will only perform source-derived management when the deployed package name matches the source package's canonical metadata.name. It also verifies that the deployed identity agrees with the package ID, namespace override, and package metadata recorded by Zarf. If these identities do not agree, cluster-mutating operations are blocked before they occur and the provider returns an actionable error.

Configuration-driven import blocks can validate the configured source and reject an aliased deployment immediately. A CLI-driven import, such as tofu import uds_package.podinfo podinfo:podinfo, does not provide the resource configuration or source package during import. These imports are therefore added to state provisionally. During a subsequent plan or apply, the provider validates the imported identity against the configured source. If the identity is not canonical, the user is instructed to remove the package from Terraform state and migrate the existing deployment before importing its canonical identity.

Comprehensive, package-specific migration guidance is intentionally outside the scope of this PR. Migration requirements can vary based on the package's Helm releases, Kubernetes resources, actions, hooks, and namespace behavior. A follow-up PR will document potential migration paths for existing aliased workloads.

OpenSpec

Additionally, as part of an ongoing trial, OpenSpec was used during the creation of this PR. The resulting spec documentation was left in place in the ./openspec/changes/prevent-unsafe-package-adoption directory. As previously discussed with @joelmccoy and @briantwatson, my current intent is to merge them with the PR, then bootstrap the repository with openspec tooling in a separate PR if/when desired.

Related issue

Related to TOFU-1

Type of change

  • Bug fix
  • New feature
  • Documentation
  • Refactoring or maintenance

Validation

  1. Run unit tests with uds run test-unit.
  2. Run acceptance tests with uds run test-acc.

Acceptance coverage verifies package imports through both CLI import and configuration-driven import blocks after the package is initially deployed with an overridden name through a UDS bundle and UDS CLI.

Checklist

  • Tests, documentation, and ADRs were added or updated as needed
  • Generated provider documentation is up to date
  • I followed the contributor guide

@jasonwashburn

Copy link
Copy Markdown
Contributor Author

@greptileai review

@greptile-apps

greptile-apps Bot commented Sep 8, 2026

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

The PR appears safe to merge; no outstanding correctness, security, or repository-rule violations were identified.

Summary

  • Verifies package ID, namespace, deployed metadata, and canonical source name before update or deletion.
  • Treats confirmed absence separately from identity lookup failures.
  • Adds provisional CLI-import handling and actionable migration guidance.
  • Expands unit and acceptance coverage for aliased deployments, malformed identities, failed lookups, and mutation boundaries.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Terraform state or import ID] --> B[Look up deployed Zarf package]
    B -->|Absent| C[Apply lifecycle-specific absence handling]
    B -->|Lookup or identity inconsistency| D[Block mutation and return diagnostic]
    B -->|Identity verified| E[Load configured source metadata]
    E -->|Load failure| D
    E --> F{Canonical name matches deployed name?}
    F -->|No| G[Reject aliased deployment and provide migration guidance]
    F -->|Yes| H[Allow update or deletion]
Loading

@jasonwashburn jasonwashburn self-assigned this Sep 8, 2026
@jasonwashburn
jasonwashburn requested a lite review from Copilot September 8, 2026 22:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It introduces breaking, cross-lifecycle behavior changes (import/read/plan/update/delete) that warrant final human review despite strong test coverage.

Pull request overview

This PR hardens uds_package adoption/import behavior by refusing to manage (plan/apply mutate) Zarf packages whose deployed identity was aliased (overridden name) and no longer matches the source package’s canonical metadata.name, preventing unsafe “takeover” scenarios and identity drift.

Changes:

  • Add verified deployed-identity checks (ID/name/namespace/metadata agreement) at Read/Update/Delete boundaries and block mutations when identity can’t be safely validated.
  • Enforce canonical-name matching for any source-derived management (plan-time when enabled, and always pre-mutation during apply), while keeping import read-only and provisional for CLI imports.
  • Expand unit + acceptance coverage and update generated resource docs with migration-safe guidance (tofu state rm for provisional alias imports).
File summaries
File Description
test/acc/package_test.go Adds acceptance coverage for aliased vs canonical imports and ensures no unsafe adoption occurs.
test/acc/fixtures/package_adoption/uds-bundle.yaml New bundle fixture that deploys a package under an aliased name.
test/acc/fixtures/package_adoption/package/zarf.yaml New minimal Zarf package fixture with canonical metadata.name.
test/acc/fixtures/package_adoption/package/manifests/marker.yaml New manifest used to assert fixture deployment behavior.
templates/resources/package.md.tmpl Updates docs template with canonical-name import rules and safe state-removal guidance.
openspec/config.yaml Introduces OpenSpec repository context and workflow guidance.
openspec/changes/prevent-unsafe-package-adoption/tasks.md Tracks implementation tasks for adoption-safety change.
openspec/changes/prevent-unsafe-package-adoption/specs/package-adoption-safety/spec.md Defines the behavioral contract for identity verification and canonical adoption safety.
openspec/changes/prevent-unsafe-package-adoption/proposal.md Captures motivation and intended behavior change (breaking).
openspec/changes/prevent-unsafe-package-adoption/design.md Documents design decisions and lifecycle-boundary validation strategy.
openspec/changes/prevent-unsafe-package-adoption/.openspec.yaml Registers the OpenSpec change bundle.
internal/provider/resource_package.go Implements verified identity model + canonical-name guards across lifecycle operations.
internal/provider/resource_package_test.go Adds/updates unit & framework tests to cover identity/canonical validation and non-mutation guarantees.
docs/resources/package.md Updates generated resource documentation for new import/adoption behavior.
Review details
  • Files reviewed: 14/14 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread internal/provider/resource_package.go
@jasonwashburn jasonwashburn changed the title fix: reject import/management of packages deployed with overridden package name fix!: reject import/management of packages deployed with overridden package name Sep 8, 2026
@jasonwashburn
jasonwashburn requested a lite review from Copilot September 8, 2026 23:15
@jasonwashburn
jasonwashburn marked this pull request as ready for review September 8, 2026 23:15
@jasonwashburn
jasonwashburn requested a review from a team as a code owner September 8, 2026 23:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It changes core lifecycle behavior (read/plan/update/delete) for a stateful Kubernetes/Zarf-managed resource and warrants final human verification despite strong test coverage.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

test/acc/package_test.go:180

  • buildPackageAdoptionFixtures() reads the entire package archive into memory (os.ReadFile) just to duplicate it under an aliased filename. These fixture archives can be large, so this increases memory usage and can slow/flake acceptance runs unnecessarily; a streaming file copy avoids buffering the whole archive.
  • Files reviewed: 14/14 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants