switch to using generic storage for externalauths#5218
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k 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 |
There was a problem hiding this comment.
Pull request overview
This PR continues the multi-step migration to store ExternalAuth resources using the generic Cosmos document format (instead of a bespoke database.ExternalAuth wrapper), updating database CRUD/listers, conversion plumbing, and the associated integration test artifacts.
Changes:
- Switch
ExternalAuthstorage, listers, and transaction decoding todatabase.GenericDocument[api.HCPOpenShiftClusterExternalAuth]and remove the legacydatabase.ExternalAuthtypes/converters. - Embed
CosmosMetadatadirectly in the internalHCPOpenShiftClusterExternalAuthtype and update read-only copying + API version conversions accordingly. - Update numerous integration artifacts to the new stored document shape and relax integration comparisons for redundant Cosmos metadata fields.
Reviewed changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test-integration/utils/databasemutationhelpers/per_resource_comparer.go | Ignore additional redundant/variable Cosmos metadata fields during resource comparisons. |
| test-integration/frontend/artifacts/FrontendCRUD/Migration/read-new-data/18-cosmosCompare-confirm/externalauth-default.json | Update expected stored ExternalAuth document shape for migration read-new-data flow. |
| test-integration/frontend/artifacts/FrontendCRUD/Migration/read-new-data/01-load-old-data/externalauth-default.json | Update old-data load artifact to new inline/generic storage shape. |
| test-integration/frontend/artifacts/FrontendCRUD/Migration/migrate-old-data/99-cosmosCompare-confirm-migration/externalauth-default.json | Update migration confirmation artifact for new storage shape. |
| test-integration/frontend/artifacts/FrontendCRUD/Migration/migrate-old-data/01-load-old-data/externalauth-default.json | Update migrate-old-data load artifact for new storage shape. |
| test-integration/frontend/artifacts/FrontendCRUD/ExternalAuth/read-old-data/09-cosmosCompare-confirm-update/externalauth-default.json | Update read-old-data update confirmation artifact for new storage shape. |
| test-integration/frontend/artifacts/FrontendCRUD/ExternalAuth/read-old-data/01-load-old-data/externalauth-default.json | Update read-old-data load artifact for new storage shape. |
| test-integration/frontend/artifacts/FrontendCRUD/ExternalAuth/create-current/06-cosmosCompare-ending-content/externalauth-default.json | Update create-current expected ending Cosmos content for new storage shape. |
| test-integration/frontend/artifacts/DatabaseCRUD/UntypedCRUD/basic/07-untypedListRecursive-resourcegroup-via-child/basic-external-auth-auth.json | Remove legacy nested internalState payload from untyped CRUD artifacts. |
| test-integration/frontend/artifacts/DatabaseCRUD/UntypedCRUD/basic/05-untypedListRecursive-subscription/basic-external-auth-auth.json | Remove legacy nested internalState payload from untyped CRUD artifacts. |
| test-integration/frontend/artifacts/DatabaseCRUD/UntypedCRUD/basic/04-untypedListRecursive-resourcegroup/basic-external-auth-auth.json | Remove legacy nested internalState payload from untyped CRUD artifacts. |
| test-integration/frontend/artifacts/DatabaseCRUD/UntypedCRUD/basic/01-load-initial/basic-external-auth-auth.json | Remove legacy nested internalState payload from initial load artifacts. |
| test-integration/backend/controllers/mismatches/artifacts/nodepool/remove_orphaned_nodepool_descendents/99-cosmosCompare-end-state/externalauth-default.json | Update mismatch-controller artifacts to new stored ExternalAuth shape. |
| test-integration/backend/controllers/mismatches/artifacts/nodepool/remove_orphaned_nodepool_descendents/00-load-initial-state/externalauth-default.json | Update mismatch-controller initial-state artifacts to new stored ExternalAuth shape. |
| test-integration/backend/controllers/mismatches/artifacts/externalauth/remove_orphaned_externalauth_descendents/00-load-initial-state/externalauth-default.json | Update mismatch-controller artifacts to new stored ExternalAuth shape. |
| test-integration/backend/controllers/mismatches/artifacts/externalauth/present_externalauth/99-cosmosCompare-end-state/externalauth-default.json | Update mismatch-controller artifacts to new stored ExternalAuth shape. |
| test-integration/backend/controllers/mismatches/artifacts/externalauth/present_externalauth/00-load-initial-state/externalauth-default.json | Update mismatch-controller artifacts to new stored ExternalAuth shape. |
| test-integration/backend/controllers/mismatches/artifacts/cluster/remove_orphaned_cluster_descendents/00-load-initial-state/externalauth-default.json | Update mismatch-controller artifacts to new stored ExternalAuth shape. |
| internal/validation/validate_externalauth_comprehensive_test.go | Ensure test fixtures include CosmosMetadata.ResourceID for ExternalAuth. |
| internal/databasetesting/mock_resources_global_lister.go | Update mock global lister to use GenericDocument[ExternalAuth]. |
| internal/databasetesting/mock_resources_db_client.go | Update mock transaction result decoding to CosmosGenericToInternal. |
| internal/databasetesting/mock_resources_crud.go | Update mock CRUD wiring to store ExternalAuth as GenericDocument. |
| internal/database/types_externalauth.go | Remove legacy Cosmos wrapper type for ExternalAuth. |
| internal/database/transaction.go | Transaction result decoding now expects GenericDocument[ExternalAuth]. |
| internal/database/global_lister.go | Global lister now reads ExternalAuth from GenericDocument. |
| internal/database/crud_hcpcluster.go | Cluster-scoped ExternalAuthsCRUD now uses GenericDocument. |
| internal/database/convert_generic.go | Apply EnsureDefaults() during generic Cosmos->internal conversion via a small Defaulter interface. |
| internal/database/convert_externalauth.go | Remove bespoke ExternalAuth conversion helpers. |
| internal/database/convert_externalauth_test.go | Remove bespoke round-trip tests tied to the legacy wrapper type. |
| internal/database/convert_defaults_consistency_test.go | Update defaulting consistency test to exercise CosmosGenericToInternal for ExternalAuth. |
| internal/database/convert_cluster_test.go | Adjust round-trip helper to clear ExistingCosmosUID from embedded CosmosMetadata for ExternalAuth. |
| internal/database/convert_any.go | Remove explicit ExternalAuth conversion routing; rely on generic conversion. |
| internal/conversion/readonly_externalauth.go | Preserve CosmosMetadata (including ETag/ResourceID) during replace update merges. |
| internal/api/zz_generated.deepcopy.go | Deep-copy CosmosMetadata inside HCPOpenShiftClusterExternalAuth. |
| internal/api/v20251223preview/external_auth_methods.go | Emit/ingest CosmosMetadata.ResourceID during versioned conversions. |
| internal/api/v20251223preview/conversion_fuzz_test.go | Teach fuzzing about CosmosMetadata fields and round-trip expectations. |
| internal/api/v20240610preview/external_auth_methods.go | Emit/ingest CosmosMetadata.ResourceID during versioned conversions. |
| internal/api/v20240610preview/conversion_fuzz_test.go | Teach fuzzing about CosmosMetadata fields and round-trip expectations. |
| internal/api/types_externalauth.go | Embed CosmosMetadata in the internal ExternalAuth type; remove legacy Cosmos ETag plumbing. |
| frontend/pkg/frontend/external_auth.go | Set/validate CosmosMetadata.ResourceID and use it for resourceID consistency checks. |
| backend/pkg/listertesting/slice_listers_test.go | Ensure test ExternalAuth objects set CosmosMetadata.ResourceID. |
| backend/pkg/informers/informers_test.go | Ensure informer tests create ExternalAuth with CosmosMetadata.ResourceID. |
| backend/pkg/controllers/operationcontrollers/test_helpers_test.go | Ensure controller fixtures set CosmosMetadata.ResourceID. |
| backend/pkg/controllers/metricscontrollers/resource_metrics_controller_test.go | Ensure metrics tests set CosmosMetadata.ResourceID. |
Comments suppressed due to low confidence (1)
frontend/pkg/frontend/external_auth.go:701
- After validating the stored
cosmosMetadata.resourceIDmatches the request URL, the code overwrites onlyinternalExternalAuth.IDwith the URL-casedresourceID. Since external serialization now usesCosmosMetadata.ResourceIDfor theidfield, consider also settinginternalExternalAuth.SetResourceID(resourceID)here so GET/PUT/PATCH responses (and subsequent updates that copy CosmosMetadata) preserve the most recent URL casing per ARM requirements.
if internalExternalAuth.ResourceID == nil {
return nil, fmt.Errorf("stored externalauth document is missing cosmosMetadata.resourceID")
}
if !strings.EqualFold(internalExternalAuth.ResourceID.String(), resourceID.String()) {
return nil, fmt.Errorf("unexpected resourceID: %s", internalExternalAuth.ResourceID.String())
}
internalExternalAuth.ID = resourceID
return internalExternalAuth, nil
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| idString := "" | ||
| if from.ID != nil { | ||
| idString = from.ID.String() | ||
| if from.ResourceID != nil { | ||
| idString = from.ResourceID.String() | ||
| } | ||
|
|
| idString := "" | ||
| if from.ID != nil { | ||
| idString = from.ID.String() | ||
| if from.ResourceID != nil { | ||
| idString = from.ResourceID.String() | ||
| } |
| // CosmosMetadata is read-only on the API surface; carry over so the | ||
| // case-preserving ResourceID and CosmosETag survive the replace round-trip. | ||
| dest.CosmosMetadata = *src.CosmosMetadata.DeepCopy() | ||
|
|
|
@deads2k: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
PR needs rebase. 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. |
|
replaced by #5395 |
part of #5215, requires #5216 to go to prod before merge
/hold for #5216 to get to prod