fix(controller): store VM key as annotation so VMs with spaces in name can migrate (#1938)#1949
Open
valentin-pf9 wants to merge 1 commit into
Open
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
valentin-pf9
force-pushed
the
feature/sanitize-vm-key-label
branch
from
44ad2a1 to
bcd7615
Compare
…grate Closes platform9#1938. Migration CR creation fails for VMs whose vCenter display name contains spaces (e.g. 'Win Server 2019'). The controller wrote the raw '<displayName>-<moid>' VM key into the vjailbreak.k8s.pf9.io/vm-key *label*. Kubernetes label values must match '[a-z0-9A-Z]([-a-z0-9A-Z_.]*[a-z0-9A-Z])?'; spaces are rejected by admission, so the create fails. The value cannot simply be sanitized: readers feed it to GetK8sCompatibleVMWareObjectName, whose SHA256 suffix is computed from the exact input bytes. Sanitizing would change the hash and break VMwareMachine lookups for every VM with uppercase/space/underscore in its name. (Thanks to the Devin review bot on the first revision of this PR for catching that.) Fix: store the key as an annotation instead of a label. Annotation values have no character restrictions, so the raw key round-trips intact and all hash-based lookups keep working. Nothing label-selects on vm-key -- every reader uses direct map access, never client.MatchingLabels -- so moving it loses no functionality. Changes: - pkg/common/constants/constants.go: rename MigrationVMKeyLabel -> MigrationVMKeyAnnotation; document why it must be an annotation. - migrationplan_controller.go: write the key into the Migration's Annotations map; read sites updated to .Annotations. - migration_controller.go, credutils.go: read sites updated. - credutils_test.go: fixtures updated. - ui/: four read sites that pull vm-key off a Migration updated from metadata.labels to metadata.annotations. The VMwareMachine read in buildMigrationResourceBundle.ts is left as-is (VMwareMachines do not carry this key). - deploy/installer.yaml + deploy/00crds.yaml: regenerated by the pre-commit make build-installer hook. Test plan: - go build ./... (k8s/migration): OK. - go test ./pkg/utils/... (k8s/migration): credutils tests pass with the annotation fixtures. - Controller envtest suite and UI build left to CI.
valentin-pf9
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #1938.
Migration CR creation fails for VMs whose vCenter display name contains spaces (e.g.
Win Server 2019). The controller wrote the raw<displayName>-<moid>VM key into thevjailbreak.k8s.pf9.io/vm-keylabel. Kubernetes label values must match[a-z0-9A-Z]([-a-z0-9A-Z_.]*[a-z0-9A-Z])?; spaces are rejected by admission, so the create fails.Why not just sanitize the value
My first revision did exactly that — and the Devin review bot correctly caught that it breaks things. Readers feed this value to
GetK8sCompatibleVMWareObjectName, whose SHA256 suffix is computed from the exact input bytes. Sanitizing the stored value changes the hash, so VMwareMachine lookups would fail for every VM with uppercase/space/underscore in its name — the opposite of the goal. Thanks to @devin-ai-integration for flagging it.The fix
Store the key as an annotation instead of a label. Annotation values have no character restrictions, so the raw key round-trips intact and every hash-based lookup keeps working unchanged.
Verified that nothing label-selects on
vm-key— every reader uses direct map access (migration.Labels[...]), neverclient.MatchingLabels— so moving it from label to annotation loses no functionality.Changes
pkg/common/constants/constants.go— renameMigrationVMKeyLabel→MigrationVMKeyAnnotation; docstring explains why it must be an annotation.migrationplan_controller.go— write the key into the Migration'sAnnotationsmap; two read sites updated to.Annotations.migration_controller.go,credutils.go— read sites updated to.Annotations.credutils_test.go— test fixtures updated to set the annotation.ui/— four read sites that pullvm-keyoff a Migration updated frommetadata.labelstometadata.annotations(MigrationDetailModal.tsx,MigrationsTable.tsx,MigrationsPage.tsx,buildMigrationResourceBundle.ts). The VMwareMachine read inbuildMigrationResourceBundle.tsis left as-is — VMwareMachines never carried this key.deploy/installer.yaml+deploy/00crds.yaml— regenerated by the pre-commitmake build-installerhook.Test plan
Controller envtest suite and UI build left to CI.
Backward compatibility
Migrations are short-lived objects. Existing Migration CRs created before this change carry the key in the old label; new ones use the annotation. UI readers like
MigrationsPage.tsxalready fall back tospec.vmName, so display degrades gracefully for any in-flight pre-upgrade migration.