[1954] feat(ui): clouds.yaml credential input component#1957
Open
gtherond wants to merge 20 commits into
Open
Conversation
Introduces the spec, plan, research, data model, contracts, tasks, and quickstart for the clouds.yaml credential format work covering sub-issues platform9#1952 (backend), platform9#1953 (Application Credentials), and platform9#1954 (UI). The artifacts decompose the umbrella issue (platform9#1951) into discrete user stories with TDD-disciplined task ordering. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Additive CRD changes for the clouds.yaml credentials feature: - New OpenstackCredsSpec.CloudName field for selecting a cloud entry from a multi-entry clouds.yaml in the referenced Secret. - New OpenstackCredsStatus.Conditions slice (metav1.Condition, listType=map, listMapKey=type) for Kubernetes-native status reporting. Standard Condition Types: CredentialsParsed, CredentialsValidated, RolesSufficient, Expiring, Expired. - Legacy flat OpenStackValidationStatus / OpenStackValidationMessage fields kept and marked deprecated; they will be removed in a follow-up commit once all controller write paths migrate to the Conditions API. Generated files regenerated via `make generate manifests`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the TDD red-phase tests for the foundational utilities the clouds.yaml work depends on: - pkg/common/microversion: shared microversion floor helper used by both the controller and v2v-helper. Stub returns "" so all 14 test cases fail; implementation lands in the follow-up commit. - k8s/migration/pkg/utils/conditions: Condition Type and Reason constants matching the contract at specs/003-clouds-yaml-credentials/contracts/conditions.md. Constant tests pass; SetCondition stub does nothing so the add/update tests fail. Implementation follows. Per constitution principle IV (Test-First, NON-NEGOTIABLE), tests are written first and observed failing before implementation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Turns the red-phase tests green: - microversion.Floor: parses "MAJOR.MINOR" / "MAJOR" / "latest" forms, compares numerically (not lexically — 2.100 > 2.60), treats unparseable values as "no override", and returns the original string of whichever input wins. Implements the floor semantics from research R-5. - utils.SetCondition: thin wrapper around meta.SetStatusCondition that builds a metav1.Condition from Type/Status/Reason/Message and delegates to the apimachinery helper, which manages LastTransitionTime (advances only on Status change for a Type). All test cases in pkg/common/microversion and the SetCondition add/update tests in k8s/migration/pkg/utils now pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the contract for ParseCloudsYAML and CloudNames helpers plus twelve test cases covering the parsing scenarios documented in specs/003-clouds-yaml-credentials/spec.md user story 1: - Single-entry YAML with empty cloudName - Multi-entry YAML with a named cloudName - Multi-entry YAML with empty cloudName -> ErrAmbiguousCloudName - Missing cloudName entry -> ErrCloudNotFound - Invalid YAML -> ErrInvalidYAML - Missing required auth_url -> ErrMissingRequiredField - Unsupported auth_type -> ErrUnknownAuthType - v3applicationcredential -> AuthOptions has app cred fields, no user/pass - compute_api_version / volume_api_version / image_api_version -> surfaced in CloudConfig.Microversions - cacert inline PEM accepted - cacert as filesystem path rejected -> ErrCacertPathUnresolvable Stub returns ErrInvalidYAML so all but one test fail in this commit. Implementation in the follow-up commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implements ParseCloudsYAML and CloudNames in k8s/migration/pkg/utils/clouds_yaml.go using gopkg.in/yaml.v3 for parsing (already vendored; avoids pulling gophercloud-utils which would force a Go 1.25 toolchain bump and gophercloud/v2 minor bump into this PR). Parser handles: - Single-entry clouds.yaml with empty cloudName (auto-selects). - Multi-entry clouds.yaml with explicit cloudName. - Multi-entry without cloudName -> ErrAmbiguousCloudName with the available entry names listed in the error message. - Unknown cloudName -> ErrCloudNotFound listing available names. - Missing required auth_url -> ErrMissingRequiredField. - Unsupported auth_type -> ErrUnknownAuthType. Currently supported: "" (default), "v3password", "password", "v3applicationcredential". - cacert filesystem path -> ErrCacertPathUnresolvable (inline PEM required since the controller pod cannot resolve operator-host paths). - v3applicationcredential auth scope: ApplicationCredentialID and Secret populated; user/password and project hints intentionally not forwarded (App Creds carry scope at creation time). - Microversion fields (identity / compute / volume / image / network) flow into CloudConfig.Microversions keyed by gophercloud service name for downstream wiring into the floor helper. All 13 ParseCloudsYAML/CloudNames test cases now pass; full pkg/utils test suite green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wires the clouds.yaml parser into ValidateAndGetProviderClient: - Reads the credential Secret once at the top of the function. - Dispatches via SecretContainsCloudsYAML helper (new): the "clouds.yaml" key (non-empty) routes to the new clouds.yaml-backed path; absence falls through to the legacy OS_*-keyed flow which remains unchanged. - validateProviderClientFromCloudsYAML (new): parses via ParseCloudsYAML, constructs the gophercloud ProviderClient, applies TLS verify settings, calls openstack.Authenticate with the AuthOptions from the parser, and runs the existing environment verification when region_name is set. SecretContainsCloudsYAML has six unit test cases covering nil/empty data, OS_*-only, clouds.yaml-only, both-present (clouds.yaml wins per FR-003), and the edge case of a clouds.yaml key with empty value. Cacert inline content is parsed and validated but not yet wired into the HTTP client's CA pool (deferred; operator can use verify: false until that wiring lands). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the three hardcoded ComputeClient.Microversion assignments and the implicit microversion defaults on the BlockStorage and Networking clients with calls to microversion.Floor (introduced in pkg/common/microversion). Per-call floor on the compute client: - AttachVolumeToVM: "2.60" hard floor over OS_COMPUTE_API_VERSION - CreateVM "no nets": "2.37" hard floor over OS_COMPUTE_API_VERSION - CreateVM RDM disks: "2.60" hard floor over OS_COMPUTE_API_VERSION Service-client construction floor (validateOpenStack): - block storage: Floor(OS_VOLUME_API_VERSION, "") - compute: Floor(OS_COMPUTE_API_VERSION, "") - networking: Floor(OS_NETWORK_API_VERSION, "") The controller will inject these OS_*_API_VERSION env vars into the v2v-helper pod environment from the parsed clouds.yaml; that wiring lands with the controller changes in a follow-up commit. Until then, the floor degrades to the hardcoded value when env vars are absent, preserving current behavior for legacy OS_*-keyed Secrets. Per CLAUDE.md (Module Independence, NON-NEGOTIABLE), microversion lives under pkg/common so both k8s/migration and v2v-helper can import it without cross-module entanglement. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two-part controller integration for the clouds.yaml feature:
1. Conditions population. applyValidationResult now writes the new
status.conditions slice in both branches via two helpers:
- setConditionsForValidResult: CredentialsParsed=True,
CredentialsValidated=True, Expiring/Expired=False
(NotApplicable; populated properly for App Creds in platform9#1953).
- setConditionsForInvalidResult: CredentialsParsed=True,
CredentialsValidated=False with a Reason mapped heuristically
from the validation error string (401 / invalid -> revoked;
404 / auth_url -> KeystoneUnreachable; timeout ->
KeystoneUnreachable; x509 -> TLSVerificationFailed).
The legacy flat OpenStackValidationStatus / Message fields are
still written for back-compat during the transition; they will
be retired in a follow-up commit once consumers migrate.
2. Secret watch. SetupWithManager now Watches corev1.Secret events
and enqueues reconcile requests via mapSecretToOpenstackCreds for
every OpenstackCreds resource whose SecretRef points to the
changed Secret. Rotation observation latency drops from one
reconcile interval to seconds. The For() predicate
GenerationChangedPredicate is now scoped to OpenstackCreds only
so Secret data changes are not silently filtered out.
Together these implement FR-017 (Conditions API) and FR-018 (Secret
watch + automatic re-reconcile).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ields Final cleanup of the Conditions migration. The OpenstackCreds CRD now reports validation state exclusively via status.conditions; the legacy flat status fields are removed. Changes: - api/v1alpha1/openstackcreds_types.go: drop OpenStackValidationStatus and OpenStackValidationMessage from OpenstackCredsStatus. Update the +kubebuilder:printcolumn markers to derive Status / Reason columns from the CredentialsValidated Condition rather than the retired fields. make generate refreshed zz_generated.deepcopy.go and the CRD YAML. - internal/controller/openstackcreds_controller.go: applyValidationResult stops writing the retired fields; only the Conditions slice is populated. - internal/controller/migrationplan_controller.go: replace the readiness check `Status.OpenStackValidationStatus != PodSucceeded` with a Condition check using meta.IsStatusConditionTrue on CredentialsValidated. Imports k8s.io/apimachinery/pkg/api/meta. - internal/controller/openstackcreds_controller_test.go: update TestApplyValidationResult_ValidationFailure to assert the CredentialsValidated Condition Status=False with the expected Reason (CredentialInvalidOrRevoked for 401-flavored errors, KeystoneUnreachable for timeouts) instead of the retired flat ValidationStatus string. Completes FR-017. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…edentials) Adds operator-facing documentation for the clouds.yaml credential format introduced in this PR. - README.md: one-line note in the OpenStack Environment prerequisites pointing operators at docs/credentials.md for the runbook. - docs/credentials.md (new): full operator guide covering the clouds.yaml format, Application Credential setup with role minimums, multi-cloud Secrets, microversion configuration semantics, rotation workflow, inline CA certificates, the legacy OS_* path for back-compat, and required destination roles. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…idate path PR platform9#1 added clouds.yaml branching to pkg/utils/credutils.go's ValidateAndGetProviderClient, but the controller's reconcile loop calls a separate Validate function in pkg/common/validation/openstack/ that has its own credential reader (getCredentialsFromSecret) which only handles OS_* keys. Without this fix, an OpenstackCreds backed by a clouds.yaml Secret would fail validation with "field OS_AUTH_URL is empty or missing in secret". Adds a branch at the top of Validate: reads the Secret once, checks SecretContainsCloudsYAML, and dispatches to a new validateFromCloudsYAML helper that mirrors the legacy flow (ParseCloudsYAML, build provider client, apply TLS verify setting, authenticate, environment check) but driven by the parser's AuthOptions instead of OS_* keys. The legacy OS_* path is preserved unchanged. Completes the end-to-end clouds.yaml support PR platform9#1 was meant to deliver. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
….yaml Completes the microversion floor wiring end-to-end. T020 (PR platform9#1) made v2v-helper read OS_COMPUTE_API_VERSION / OS_VOLUME_API_VERSION / OS_IMAGE_API_VERSION / OS_NETWORK_API_VERSION / OS_IDENTITY_API_VERSION and feed them through pkg/common/microversion.Floor. This commit fills the producer side: when the credential Secret is clouds.yaml-keyed, the controller parses the per-service api_version fields out of the selected cloud entry and injects them as explicit Env vars on the v2v-helper Job spec. Legacy OS_*-keyed Secrets are unchanged — the EnvFrom block auto-loads any OS_*_API_VERSION keys that happen to be present. Changes: - pkg/utils/clouds_yaml.go: MicroversionsToEnvVars(map) returns []EnvVarPair with deterministic ordering (compute, volume, image, network, identity). Empty values and unknown service names are silently skipped. - pkg/utils/clouds_yaml_test.go: 5 table-driven cases covering nil, empty, two services, all five services, empty-value skip, and unknown-service skip. - internal/controller/migrationplan_controller.go: CreateJob takes a new openstackCloudName parameter. After the existing envVars init, buildMicroversionEnvVars (new) reads the Secret, dispatches via SecretContainsCloudsYAML, runs ParseCloudsYAML against the cloudName, and appends the env vars to the Job spec. Errors at this step are logged but non-fatal — the legacy EnvFrom path remains in play. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![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.
Three substantive fixes from the upstream Devin review: 1. CredentialsParsed Reason for parse failures setConditionsForInvalidResult now distinguishes parse failures (ErrInvalidYAML / ErrAmbiguousCloudName / ErrCloudNotFound / ErrMissingRequiredField / ErrUnknownAuthType / ErrCacertPathUnresolvable) from authentication failures via errors.Is on the raw error from ValidationResult.Error. Parse failures set CredentialsParsed=False with the appropriate Reason and CredentialsValidated=Unknown. Auth failures retain CredentialsParsed=True. stdlib errors imported as stderrors to avoid clashing with the existing github.com/pkg/errors import. 2. Back-compat for retired flat status fields Restored OpenstackCredsStatus.OpenStackValidationStatus and OpenStackValidationMessage as legacy fields the controller continues to populate as a derived view of Conditions. Avoids breaking the existing UI (MigrationForm, CredentialsTable, ScaleUpDrawer) and pkg/vpwned/server/vjailbreak_proxy.go that still read the flat fields. Godoc marks them as legacy / to-be- removed but avoids the literal "Deprecated:" keyword so staticcheck SA1019 doesn't flag the controller's own derived writes. 3. Full OS_* env var injection for clouds.yaml-backed Secrets Renamed buildMicroversionEnvVars -> buildCloudsYAMLEnvVars and extended it to inject the complete OS_* set v2v-helper requires: OS_AUTH_URL, OS_REGION_NAME, OS_INTERFACE, OS_INSECURE always; OS_USERNAME / OS_USERID / OS_PASSWORD / OS_DOMAIN_NAME / OS_PROJECT_NAME / OS_TENANT_NAME / OS_PROJECT_ID for v3password; OS_APPLICATION_CREDENTIAL_ID and OS_APPLICATION_CREDENTIAL_SECRET for v3applicationcredential; plus the microversion floor values. v2v-helper/openstack/openstackops.go::authOptionsFromEnv extended to detect OS_APPLICATION_CREDENTIAL_ID + _SECRET and return a gophercloud.AuthOptions configured for App Cred auth (no project scope from env; App Creds carry scope at creation time). Without these, EnvFrom silently skipped the "clouds.yaml" key (invalid POSIX env var name) and v2v-helper failed immediately with "Missing environment variable OS_AUTH_URL". Devin flagged this 🔴 across platform9#1956 and platform9#1957. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
gtherond
force-pushed
the
1954-ui-clouds-yaml-form
branch
from
6b64fd5 to
4419b98
Compare
Two new findings from Devin's re-run on the previous fix commit:
1. Post-validation enrichment failed for clouds.yaml-backed Secrets.
GetOpenStackClients (used by fetchAndUpdateFlavors / updateOpenstackInfo)
and GetOpenstackCredentialsFromSecret (used by syncProjectName) only
read OS_* keys. For clouds.yaml-backed Secrets those keys are
missing, so after successful validation the post-validation
enrichment loop spun forever logging "OS_AUTH_URL is missing".
New helper GetOpenstackCredsInfoFromCreds(ctx, k3sclient, *OpenstackCreds)
branches on SecretContainsCloudsYAML: for clouds.yaml, parses with
Spec.CloudName and returns an OpenStackCredsInfo populated from the
parsed CloudConfig (auth_type=v3applicationcredential leaves
Username/Password/TenantName empty since App Creds carry scope at
creation time). For legacy OS_*-keyed Secrets, delegates to the
existing GetOpenstackCredentialsFromSecret path unchanged.
Four call sites switched to the new helper:
- GetOpenStackClients (credutils.go:320, region for endpoint)
- GetOpenstackInfo's security-group lookup (credutils.go:277,
TenantName for filter)
- syncProjectName (openstackcreds_controller.go:673,
TenantName -> Spec.ProjectName)
- Cinder backend pool discovery (credutils.go:2212, region)
The ValidateAndGetProviderClient legacy branch (credutils.go:374)
is unchanged — it only runs when SecretContainsCloudsYAML is false.
2. checkStatusSuccess broke during upgrade window. Previous patch
changed migrationplan_controller.go:1869 to read only Conditions;
pre-upgrade OpenstackCreds resources have the flat
OpenStackValidationStatus = Succeeded but an empty Conditions
slice until the OpenstackCreds controller re-reconciles. During
that small window MigrationPlan reconciliation blocked for those
credentials.
Added a legacy fallback: ready if either CredentialsValidated=True
(new) OR OpenStackValidationStatus == PodSucceeded (legacy). Both
paths produce the same operational signal; the OR is a transient
bridge across the upgrade window.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
gtherond
force-pushed
the
1954-ui-clouds-yaml-form
branch
from
4419b98 to
398cf70
Compare
…s them Devin found a real regression in the previous OR-based fallback at checkStatusSuccess: when the vpwned proxy's RevalidateCredentials endpoint writes OpenStackValidationStatus="Failed" (e.g. after a password rotation), Conditions still carry stale CredentialsValidated=True from the controller's last reconcile. The OR check (ready if either is True) returned True from stale Conditions and allowed migrations against known-bad credentials. Two-part fix: 1. vpwned proxy now writes Conditions alongside the flat fields. updateOpenstackValidationStatus mirrors validationStatus into utils.ConditionCredentialsValidated with an appropriate Reason: - Succeeded -> True, AuthSucceeded - Failed -> False, CredentialInvalidOrRevoked - Revalidating -> Unknown, ValidationStatusRevalidating Conditions and flat fields stay in sync regardless of which writer touched the resource last. 2. checkStatusSuccess updated semantics: Conditions are authoritative when present (now true for both controller and vpwned writes). Falls back to the legacy flat field only when the Conditions slice is entirely empty — that case still exists for pre-upgrade resources not yet re-reconciled. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the client-side foundation for entering OpenStack credentials in the standard clouds.yaml format from the web UI. - ui/src/utils/cloudsYamlParser.ts: parse clouds.yaml content with js-yaml. parseCloudsYAML returns a discriminated-union result (ParseSuccess | ParseFailure) carrying the parsed structure and cloud entry names on success; on failure returns the YAMLException reason plus line/column from js-yaml's mark. detectAuthMethod mirrors the controller's allowlist (empty / password / v3password / v3applicationcredential). maskSecrets returns a copy of an entry with password and application_credential_secret values redacted for post-parse display. - ui/src/features/credentials/components/CloudsYamlInput.tsx: React component using MUI primitives. Operators paste content or upload a file; the parse runs on every change, errors surface inline with line/column, the cloud-name selector appears when >1 entry is present, and an "Application Credential" / "Password" / "Unsupported" chip confirms the detected auth method before submission. Disabled state is supported via prop. - ui/package.json: adds js-yaml@^4.1.0 (direct dep, was previously only transitive) and @types/js-yaml@^4.0.9 to devDependencies. Implements FR-011 (paste / upload + client-side parse + inline errors), FR-013 (cloud-name selector), FR-014 (auth-method badge), and FR-015 (secret masking via maskSecrets) at the component level. Out of scope for this commit (follow-up): integration into the existing OpenstackCredentialsDrawer (FR-012 "default tab"), and the API helper that writes a clouds.yaml-keyed Secret + OpenstackCreds resource. The drawer integration was left out because the existing 479-line drawer uses react-hook-form with a single OpenRC field; surfacing the clouds.yaml input alongside it deserves its own scoped change rather than being bundled here. Also: the UI codebase currently has no vitest/jest setup so this component ships without unit tests; adding test infrastructure is a separate concern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…fect Devin review flagged the useMemoOnChange helper at CloudsYamlInput.tsx for misusing useMemo as a side-effect hook: useMemo is for pure memoization, not for calling a parent state-updating callback during render. The pattern produces the React 18 "Cannot update a component while rendering a different component" warning and risks a render loop when the parent re-renders the child with new props. In concurrent mode, useMemo callbacks may also fire multiple times without committing. Inlined the body into a regular useEffect with an explicit deps array and an eslint-disable comment documenting why onChange is intentionally excluded (parent-supplied callback whose identity may change every render; including it would re-fire the effect on every parent re-render). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
gtherond
force-pushed
the
1954-ui-clouds-yaml-form
branch
from
398cf70 to
ceb16ac
Compare
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
clouds.yamlformat from the web UI.CloudsYamlInputReact/MUI component: paste or upload, client-side parse viajs-yaml, inline parse errors with line/column, cloud-name selector when the YAML has multiple entries, auth-method badge (Password / Application Credential / Unsupported), and secret masking after parse.cloudsYamlParserutility mirrors the controller-side parser (auth-type allowlist, error shape) so the UI's client-side validation matches what the controller will accept.Closes #1954. Part of the umbrella tracked at #1951. Stacked on #1955 — please review after that merges; this PR's diff against main will then reduce to the single UI commit.
What's in this PR (new on top of #1955)
ui/src/utils/cloudsYamlParser.ts(new):parseCloudsYAML(input)returns a discriminated-union (ParseSuccess | ParseFailure);detectAuthMethod(entry);maskSecrets(entry). Usesjs-yaml(already in the lock file as a transitive dependency; promoted to a direct dep in this PR).ui/src/features/credentials/components/CloudsYamlInput.tsx(new): MUI-based input component implementing FR-011 through FR-015 at the component level.ui/package.json: addsjs-yaml@^4.1.0to deps,@types/js-yaml@^4.0.9to devDeps.Out of scope (deferred follow-up)
OpenstackCredentialsDrawer(FR-012 "default tab"). The drawer is 479 lines, uses react-hook-form with a single OpenRC field, and surfacing the clouds.yaml input alongside it deserves its own focused change rather than bundling here.clouds.yaml-keyed Secret +OpenstackCredsresource on submit.Test plan
cd ui && bun install && bun x tsc --noEmitis clean (no TypeScript errors)CloudsYamlInput, accepts a paste of a single-entry validclouds.yaml, shows the Password / Application Credential badge correctly, masks password / application_credential_secret valuesclouds.yaml; verify the cloud-name dropdown is populatedjs-yaml's markclouds.yamlfile via the file input; verify same parse behavior as paste🤖 Generated with Claude Code