Migrate cluster and nodepool handlers from hyperfleet-api/maestro to fleet-db#116
Draft
typeid wants to merge 1 commit into
Draft
Migrate cluster and nodepool handlers from hyperfleet-api/maestro to fleet-db#116typeid wants to merge 1 commit into
typeid wants to merge 1 commit into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: typeid 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 |
openshift-ci
Bot
added
approved
|
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. |
Contributor
WalkthroughThis PR replaces Maestro and Hyperfleet HTTP/gRPC clients with a new ChangesFleetDB client migration and handler rewiring
Deployment artifact removal
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant Handler as ClusterHandler / NodePoolHandler
participant FleetDB as fleetdb.Client
participant tokenProvider as tokenProvider (STS cache)
participant K8sAPI as EKS fleet-db API
Client->>Handler: POST /clusters (create request)
Handler->>FleetDB: ListClusters(accountID) — uniqueness check
FleetDB->>tokenProvider: get bearer token
tokenProvider-->>FleetDB: cached / presigned STS token
FleetDB->>K8sAPI: List Cluster CRs in namespace
K8sAPI-->>FleetDB: ClusterList
FleetDB-->>Handler: cluster list
Handler->>FleetDB: CreateCluster(accountID, clusterCR)
FleetDB->>K8sAPI: ensureNamespace + Create CR
K8sAPI-->>FleetDB: created CR
FleetDB-->>Handler: nil error
Handler-->>Client: 201 + platform Cluster JSON
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
|
Replace Maestro with HyperFleetManifest and fleet-db for ZOA dispatch and MC registration. Add nodepool CRUD E2E tests via rosactl CLI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/zoa/reconciler.go (1)
160-181: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist terminal state before deleting the Manifest.
Deleting the Manifest first removes the only source of HFM job status. If
UpdateCompletionfails, the next reconcile sees not-found and cannot recover the successful/failed result.🤖 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 `@pkg/zoa/reconciler.go` around lines 160 - 181, Persist the terminal state before removing the Manifest: in Reconciler.handleCompletion, call store.UpdateCompletion before deleteManifest so the HFM-derived status is saved even if manifest deletion later fails. Keep the duration/status computation in the same method, but reorder the completion flow and preserve the existing terminal status fields (status, outputStatus, runnerSeconds, uploadSeconds) when updating Execution.pkg/zoa/reconciler_test.go (1)
297-345: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe “DeletionFails” cases never exercise a delete failure.
One test creates an HFM that the fake client can delete successfully, and the other relies on
NotFound, whichdeleteManifesttreats as success. That leaves the real non-404 delete-error branch uncovered while the test names claim otherwise.Also applies to: 604-630
🤖 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 `@pkg/zoa/reconciler_test.go` around lines 297 - 345, The DeletionFails tests do not actually cover a failing delete path, because the current setup only exercises successful deletion or a NotFound case that deleteManifest treats as success. Update the test setup around reconcileExecution and deleteManifest so it triggers a real delete error from the fake client, and assert the non-404 failure branch is handled correctly. Use the existing mockExecutionStore, NewReconciler, and fake FleetDB/manifest fixture patterns to force the error case while keeping the test names aligned with the behavior being verified.
🟡 Minor comments (11)
test/e2e-api/e2e_test.go-115-115 (1)
115-115: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a higher-entropy test management-cluster ID.
Line 115 only has second-level uniqueness, so two fast retries or parallel jobs can hit the persistent ConfigMap with the same
test-mc-*ID and fail with a conflict.Proposed change
- mcID := fmt.Sprintf("test-mc-%s", time.Now().Format("20060102150405")) + mcID := fmt.Sprintf("test-mc-%d", time.Now().UnixNano())🤖 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 `@test/e2e-api/e2e_test.go` at line 115, The test management-cluster ID generation in e2e_test.go is only second-level unique, which can collide across retries or parallel runs. Update the mcID creation in the e2e test flow to use a higher-entropy value instead of time.Now().Format alone, and keep the change localized to the test setup that builds the test-mc-* identifier so each run gets a truly unique ConfigMap name.test/e2e-cli/cluster_test.go-415-421 (1)
415-421: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail fast when the OIDC issuer URL is unavailable.
If neither the cluster response nor
HCP_ROSA_ISSUER_URLprovides a value, this spec still invokesrosactl cluster-oidc createwith an empty--oidc-issuer-url, producing a less actionable failure.Proposed change
if oidcIssuerURL == "" { oidcIssuerURL = os.Getenv("HCP_ROSA_ISSUER_URL") } + Expect(oidcIssuerURL).ToNot(BeEmpty(), "oidcIssuerURL missing from cluster create response; set HCP_ROSA_ISSUER_URL when running this spec standalone") GinkgoWriter.Printf("HCP cluster OIDC issuer URL: %s\n", oidcIssuerURL)🤖 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 `@test/e2e-cli/cluster_test.go` around lines 415 - 421, The cluster-oidc create spec is falling through with an empty issuer URL when both the cluster response and HCP_ROSA_ISSUER_URL are missing. Update the test around the cluster-oidc creation flow to fail fast before invoking exec.Command if oidcIssuerURL is still empty, using the existing clusterName/oidcIssuerURL setup in the It block. Add a clear assertion or fatal failure in that path so the test stops with an actionable message instead of calling rosactl cluster-oidc create with an empty --oidc-issuer-url.test/e2e-cli/cluster_test.go-643-656 (1)
643-656: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFilter nodepool readiness polling by cluster ID.
The nodepool list endpoint defaults pagination to 50 and supports
clusterId; polling the unfiltered account list can miss this cluster’s nodepool in shared e2e accounts.Proposed change
- resp, err := customerApiClient.Get("/api/v0/nodepools", customerAccountID) + resp, err := customerApiClient.Get("/api/v0/nodepools?clusterId="+url.QueryEscape(id)+"&limit=100", customerAccountID)Also restore the
net/urlimport.🤖 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 `@test/e2e-cli/cluster_test.go` around lines 643 - 656, The polling in the nodepool readiness check is using the unfiltered /api/v0/nodepools account list, which can miss the target nodepool in shared accounts. Update the request in the cluster test helper around the Eventually block to pass the cluster ID filter through customerApiClient.Get, using the same id value as the clusterId query parameter, and restore the net/url import needed to build that filtered request.pkg/handlers/nodepool.go-67-69 (1)
67-69: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn an empty array for out-of-range pages.
nodepools = nilserializes as JSONnull; list endpoints should return[]for emptyitems, matching the cluster handler behavior.Proposed fix
if offset >= len(nodepools) { - nodepools = nil + nodepools = []*types.NodePool{} } else {🤖 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 `@pkg/handlers/nodepool.go` around lines 67 - 69, The pagination branch in nodepool list handling is returning nil for out-of-range pages, which serializes to JSON null instead of an empty array. Update the logic in the nodepool handler’s listing path so that the items slice is set to an empty slice when offset is beyond len(nodepools), matching the cluster handler behavior and keeping list responses consistent.pkg/handlers/cluster_test.go-27-31 (1)
27-31: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCheck scheme registration errors in the test helper.
Avoid discarding
AddToSchemefailures; make the helper take*testing.Tand fail fast.As per path instructions, Go code should “Never ignore error returns.”Proposed fix
-func newTestScheme() *runtime.Scheme { +func newTestScheme(t *testing.T) *runtime.Scheme { + t.Helper() s := runtime.NewScheme() - _ = corev1.AddToScheme(s) - _ = hyperfleetv1alpha1.AddToScheme(s) + if err := corev1.AddToScheme(s); err != nil { + t.Fatalf("add corev1 to scheme: %v", err) + } + if err := hyperfleetv1alpha1.AddToScheme(s); err != nil { + t.Fatalf("add hyperfleet to scheme: %v", err) + } return s }🤖 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 `@pkg/handlers/cluster_test.go` around lines 27 - 31, The test helper newTestScheme is ignoring scheme registration errors from corev1.AddToScheme and hyperfleetv1alpha1.AddToScheme. Update newTestScheme to accept *testing.T, check both AddToScheme returns, and fail fast with t.Fatalf if either registration fails so the test suite never silently proceeds with a broken scheme.Source: Path instructions
pkg/clients/fleetdb/convert.go-329-334 (1)
329-334: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
UpdatedAtandLastUpdateTimenever reflect real updates.For every non-deleting object,
metaTimereturns the creation timestamp. That makes active Cluster and NodePool responses report creation time as both “updated” and “lastUpdateTime” forever. If FleetDB does not expose a true update timestamp, derive this from status transition times or leave it unset instead of returning stale data.🤖 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 `@pkg/clients/fleetdb/convert.go` around lines 329 - 334, The metaTime helper is using the creation timestamp for all non-deleting objects, which makes UpdatedAt and LastUpdateTime stale. Update metaTime in convert.go so it does not default to GetCreationTimestamp().Time for active objects; instead derive a real update time from available status transition timestamps or return an unset/zero value when no true update timestamp exists. Keep the deletion-timestamp branch intact and adjust the callers that populate Cluster and NodePool response fields to use the revised behavior.docs/index.html-423-423 (1)
423-423: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove stale
/workdocumentation.
openapi/openapi.yamlremoved the/workpath andWorkRequestschema, butdocs/index.htmlstill documents them. Regenerate this file from the updated OpenAPI spec or delete the stale sections.Also applies to: 2620-2620
🤖 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 `@docs/index.html` at line 423, The docs in docs/index.html still include stale /work API content and the WorkRequest schema even though they were removed from the OpenAPI spec. Update the generated documentation by regenerating docs/index.html from the current openapi/openapi.yaml, or manually remove the /work path and WorkRequest sections so the published docs match the spec.docs/api/zoa-endpoints.md-684-685 (1)
684-685: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThis still describes the old propagation path.
The migration here is through FleetDB HyperFleetManifest CRs, so saying resources are applied on the MC “via DynamoDB desires” is stale and will mislead operators about where state is flowing now.
🤖 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 `@docs/api/zoa-endpoints.md` around lines 684 - 685, Update the status descriptions in the Zoa endpoints docs to reflect the FleetDB-based migration path rather than the old DynamoDB desires flow. In the pending/running bullets near the HyperFleetManifest state descriptions, revise the wording so state propagation is described as going through FleetDB HyperFleetManifest CRs and the runner Job, and remove any mention that resources are applied on the MC via DynamoDB desires. Use the existing status labels and nearby HyperFleetManifest references to keep the wording consistent with the rest of the document.README.md-43-50 (1)
43-50: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the config table to match
main.go.--oidc-issuer-base-urlis missing,--aws-regionis no longer a flag, and--dynamodb-regiondefaults to the auto-detected region when unset, notsame as aws-region.🤖 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 `@README.md` around lines 43 - 50, Update the README config table to match the actual CLI defined in main.go: add the missing --oidc-issuer-base-url entry, remove --aws-region since it is no longer a supported flag, and correct --dynamodb-region so its default reflects the auto-detected region when unset rather than “same as aws-region.” Keep the table entries aligned with the flag names and defaults used by the argument parsing logic.pkg/handlers/zoa_test.go-84-87 (1)
84-87: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFail fast if scheme registration breaks.
newFakeFleetDBcurrently discardshyperfleetv1alpha1.AddToSchemeerrors, so a broken test fixture setup turns into a later, noisier fake-client failure. Please threadtinto this helper and assert here instead of ignoring it. As per path instructions,**/*.go: "Never ignore error returns".Suggested change
-func newFakeFleetDB() *fleetdb.Client { +func newFakeFleetDB(t *testing.T) *fleetdb.Client { + t.Helper() scheme := runtime.NewScheme() - _ = hyperfleetv1alpha1.AddToScheme(scheme) + require.NoError(t, hyperfleetv1alpha1.AddToScheme(scheme)) return fleetdb.NewClientFrom(fake.NewClientBuilder().WithScheme(scheme).Build(), testZoaLogger()) }🤖 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 `@pkg/handlers/zoa_test.go` around lines 84 - 87, The helper newFakeFleetDB is ignoring the error from hyperfleetv1alpha1.AddToScheme, which should fail fast in the test setup. Update newFakeFleetDB to accept the testing handle and assert the AddToScheme call succeeds instead of discarding the error, so fixture registration failures are caught immediately; keep the change localized to newFakeFleetDB and its call sites in zoa_test.go.Source: Path instructions
pkg/zoa/reconciler_test.go-91-119 (1)
91-119: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDon’t swallow fixture-construction errors in these helpers.
Both
newFakeFleetDBandjobStatusignore errors during test setup, which makes broken schemes/status payloads fail much later and much less clearly. As per path instructions,**/*.go: "Never ignore error returns".Suggested change
-func newFakeFleetDB(objs ...client.Object) *fleetdb.Client { +func newFakeFleetDB(t *testing.T, objs ...client.Object) *fleetdb.Client { + t.Helper() scheme := runtime.NewScheme() - _ = hyperfleetv1alpha1.AddToScheme(scheme) + require.NoError(t, hyperfleetv1alpha1.AddToScheme(scheme)) ... } func jobStatus(succeeded, failed int32, startTime, completionTime string) runtime.RawExtension { ... - raw, _ := json.Marshal(status) + raw, err := json.Marshal(status) + if err != nil { + panic(err) + } return runtime.RawExtension{Raw: raw} }🤖 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 `@pkg/zoa/reconciler_test.go` around lines 91 - 119, The helper functions newFakeFleetDB and jobStatus are swallowing setup errors, which can hide broken test fixtures and fail later in confusing ways. Update these helpers to return errors instead of discarding them: have newFakeFleetDB propagate any failure from scheme/client construction around fake.NewClientBuilder and fleetdb.NewClientFrom, and have jobStatus propagate the JSON marshal failure rather than ignoring it. Then update the test call sites in reconciler_test.go to handle these errors explicitly so fixture creation fails fast and clearly.Source: Path instructions
🤖 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 `@cmd/rosa-regional-platform-api/main.go`:
- Around line 181-183: The startup setup in main.go is using
ctrl.GetConfigOrDie() and discarding the result of corev1.AddToScheme, which can
hide initialization failures and terminate the process unexpectedly. Update the
setup around scheme creation and ctrlclient.New to call ctrl.GetConfig() and
return or propagate any error instead of exiting, and check the error from
corev1.AddToScheme before constructing the client so initialization failures are
handled cleanly within runServe.
- Around line 107-112: Reuse the detected AWS region for ZOA when
--dynamodb-region is not provided by ensuring the ZOA startup path in main.go
uses the same resolved region as cfg.Authz.AWSRegion. Update the ZOA block to
reference the effective region value (dynamodbRegion if set, otherwise
awsCfg.Region) instead of the raw flag variable, so ZOA starts with a non-empty
AWS region when ZOA_ENABLED is true. Focus the change around the region
resolution logic in main and the ZOA initialization code.
In `@Dockerfile`:
- Line 3: The builder stage is using a mutable non-Red Hat image tag, so update
the Dockerfile’s FROM statement to use an approved catalog.redhat.com builder
image or pin the existing golang:1.26-alpine image by digest. Keep the change
scoped to the builder base image reference and ensure it follows the
repository’s base image requirements.
In `@docs/api/zoa-endpoints.md`:
- Around line 197-202: The endpoint description is too strong about execution
state: `pkg/handlers/zoa.go` returns `dispatch-error` after a best-effort
`UpdateStatus(..., failed, ...)` that can be ignored, so `status: failed` is not
guaranteed. Either update `zoa.go` to নিশ্চিতly persist the failed status before
returning, or revise the `docs/api/zoa-endpoints.md` wording to say the
execution record exists but may not be persisted as failed.
In `@pkg/clients/fleetdb/convert.go`:
- Around line 122-129: The Cluster update merge in convert.go is only doing a
shallow top-level overwrite in the spec merge path. Update the merge logic
around specToMap, the req.Spec loop, and mapToSpec so nested ClusterSpec
sections are merged field-by-field or with a true deep merge instead of
replacing whole sections. Keep sibling fields in objects like networking intact
when only one nested field is patched, and preserve the existing error handling
on merge cluster spec.
- Around line 85-87: The FleetDB conversion logic in convert.go currently only
fills spec.AccountID when it is empty, which still allows a request body to
override the intended account mapping and diverge from metadata.namespace.
Update the conversion path in the function that handles spec.AccountID
assignment so it always overwrites the incoming value with the authoritative
accountID, rather than conditionally defaulting it. Make sure the fix is applied
where spec.AccountID is set before ClusterCRToPlatform uses it as
TargetProjectID.
- Around line 226-237: The conversion/defaulting logic is treating explicit zero
and false values as missing, so `spec.Replicas` and `spec.Management.AutoRepair`
are being overwritten with defaults and the update path also ignores `replicas:
0`. Update the request model and the conversion code in the `convert.go` path to
use presence-aware fields for scalar inputs, then apply defaults only when the
field is actually unset. Make sure both the create/defaulting branch and the
update handling preserve explicit `0` replicas and explicit `false` for
auto-repair.
In `@pkg/clients/fleetdb/eksauth.go`:
- Around line 29-49: Guard the `DescribeCluster` response before dereferencing
nested fields in `NewConfig`/the EKS auth setup: `out.Cluster`,
`out.Cluster.CertificateAuthority`, `out.Cluster.CertificateAuthority.Data`, and
`out.Cluster.Endpoint` can be nil, so add explicit nil checks after
`eksClient.DescribeCluster` and return a wrapped error instead of panicking.
Keep the existing flow for decoding CA data and building `rest.Config`, but only
proceed once those fields are confirmed present.
- Around line 75-80: The presign setup in PresignGetCallerIdentity is
incorrectly treating X-Amz-Expires as a signed header, so remove that header
signing from the sts.PresignOptions path and let the presigned request carry its
expiry normally. Also harden the EKS response handling in the same flow by
adding nil checks for out.Cluster, out.Cluster.CertificateAuthority, Data, and
Endpoint before dereferencing them, using the existing presignClient and
cluster-related logic as the place to update.
In `@pkg/handlers/cluster_test.go`:
- Line 64: The cluster handler tests are constructing requests without a
context, which triggers the noctx lint. Update the request creation in the
cluster test helpers and cases that use httptest.NewRequest so they use
httptest.NewRequestWithContext directly with the test context instead of
creating the request first and calling WithContext afterward. Apply this in the
request setup for the cluster-related test cases so the pattern is consistent
across the affected tests.
In `@pkg/handlers/cluster.go`:
- Around line 210-216: `UpdateCluster` currently allows
`fleetdb.ApplyPlatformUpdateToClusterCR` to change `Spec.Name` without
rechecking uniqueness, so add a duplicate-name guard in the cluster update flow
before calling `h.fleetDB.UpdateCluster`. Use the existing cluster update path
in `cluster.go` to compare the merged `cr.Spec.Name` against other clusters in
the same account and return the same validation error style used by create when
a conflict is found. Keep the fix localized around
`ApplyPlatformUpdateToClusterCR` and `UpdateCluster` so both create and update
enforce the same name uniqueness rule.
In `@pkg/handlers/management_cluster.go`:
- Around line 204-205: Initialize the ConfigMap data map before writing the
management cluster entry to avoid a panic when existing.Data is nil. In the
update path in management_cluster.go, make sure the existing ConfigMap’s Data
field is created if missing before assigning mcConfigMapKey, then proceed with
the rcClient.Update call. Use the existing variable and mcConfigMapKey in the
same flow so the fix stays localized to the ConfigMap update logic.
- Around line 90-94: The ManagementCluster creation in the handler is trusting
req.AccountID from the request body instead of the authenticated account from
context. Update the logic around the ManagementCluster assembly to derive the
account ID from the authenticated context value (or explicitly validate
req.AccountID matches it) before persisting, so cross-account registrations are
rejected. Use the existing request handling flow and the ManagementCluster
struct assignment as the place to enforce this trust boundary.
In `@pkg/handlers/nodepool.go`:
- Around line 112-133: The nodepool creation flow is overwriting the
user-provided name because `Create` builds the CR with
`fleetdb.PlatformCreateToNodePoolCR` and then returns
`fleetdb.NodePoolCRToPlatform(cr)`, which currently exposes the generated CR ID
as the name. Update the `PlatformCreateToNodePoolCR`/`NodePoolCRToPlatform` path
so the resulting platform object preserves `req.Name` in the response while
still using the UUID only as the internal nodepool ID, and keep the logging in
`h.logger.Info` aligned with the preserved name.
In `@pkg/handlers/zoa.go`:
- Around line 213-223: Persist the manifest name before dispatching the action
by setting and saving it via UpdateManifestWorkName immediately after
CreateManifest succeeds, using exec.ManifestWorkName/hfm.Name in the same flow.
If UpdateManifestWorkName fails, treat it as a dispatch failure by logging the
error, updating status to failed, and returning the error response instead of
continuing so the reconciler won’t skip the execution with an empty manifest
name.
In `@pkg/zoa/jobbuilder.go`:
- Line 473: The Job spec in the runner action builder is allowing pod retries
via backoffLimit: 5, which can rerun the trusted-action script and duplicate
side effects. Update the Job configuration in the runner action path so the
backoffLimit is 0, keeping runner actions single-shot; use the job-building code
in jobbuilder.go as the place to adjust this setting.
In `@test/e2e-api/e2e_test.go`:
- Around line 107-109: Avoid logging customer account IDs in the management
cluster listing output. Update the GinkgoWriter.Printf call inside the loop over
list.Items in e2e_test.go to keep the useful id and region context but remove
the accountId field from the log message. Make the change at the logging
statement that currently references item["accountId"].
---
Outside diff comments:
In `@pkg/zoa/reconciler_test.go`:
- Around line 297-345: The DeletionFails tests do not actually cover a failing
delete path, because the current setup only exercises successful deletion or a
NotFound case that deleteManifest treats as success. Update the test setup
around reconcileExecution and deleteManifest so it triggers a real delete error
from the fake client, and assert the non-404 failure branch is handled
correctly. Use the existing mockExecutionStore, NewReconciler, and fake
FleetDB/manifest fixture patterns to force the error case while keeping the test
names aligned with the behavior being verified.
In `@pkg/zoa/reconciler.go`:
- Around line 160-181: Persist the terminal state before removing the Manifest:
in Reconciler.handleCompletion, call store.UpdateCompletion before
deleteManifest so the HFM-derived status is saved even if manifest deletion
later fails. Keep the duration/status computation in the same method, but
reorder the completion flow and preserve the existing terminal status fields
(status, outputStatus, runnerSeconds, uploadSeconds) when updating Execution.
---
Minor comments:
In `@docs/api/zoa-endpoints.md`:
- Around line 684-685: Update the status descriptions in the Zoa endpoints docs
to reflect the FleetDB-based migration path rather than the old DynamoDB desires
flow. In the pending/running bullets near the HyperFleetManifest state
descriptions, revise the wording so state propagation is described as going
through FleetDB HyperFleetManifest CRs and the runner Job, and remove any
mention that resources are applied on the MC via DynamoDB desires. Use the
existing status labels and nearby HyperFleetManifest references to keep the
wording consistent with the rest of the document.
In `@docs/index.html`:
- Line 423: The docs in docs/index.html still include stale /work API content
and the WorkRequest schema even though they were removed from the OpenAPI spec.
Update the generated documentation by regenerating docs/index.html from the
current openapi/openapi.yaml, or manually remove the /work path and WorkRequest
sections so the published docs match the spec.
In `@pkg/clients/fleetdb/convert.go`:
- Around line 329-334: The metaTime helper is using the creation timestamp for
all non-deleting objects, which makes UpdatedAt and LastUpdateTime stale. Update
metaTime in convert.go so it does not default to GetCreationTimestamp().Time for
active objects; instead derive a real update time from available status
transition timestamps or return an unset/zero value when no true update
timestamp exists. Keep the deletion-timestamp branch intact and adjust the
callers that populate Cluster and NodePool response fields to use the revised
behavior.
In `@pkg/handlers/cluster_test.go`:
- Around line 27-31: The test helper newTestScheme is ignoring scheme
registration errors from corev1.AddToScheme and hyperfleetv1alpha1.AddToScheme.
Update newTestScheme to accept *testing.T, check both AddToScheme returns, and
fail fast with t.Fatalf if either registration fails so the test suite never
silently proceeds with a broken scheme.
In `@pkg/handlers/nodepool.go`:
- Around line 67-69: The pagination branch in nodepool list handling is
returning nil for out-of-range pages, which serializes to JSON null instead of
an empty array. Update the logic in the nodepool handler’s listing path so that
the items slice is set to an empty slice when offset is beyond len(nodepools),
matching the cluster handler behavior and keeping list responses consistent.
In `@pkg/handlers/zoa_test.go`:
- Around line 84-87: The helper newFakeFleetDB is ignoring the error from
hyperfleetv1alpha1.AddToScheme, which should fail fast in the test setup. Update
newFakeFleetDB to accept the testing handle and assert the AddToScheme call
succeeds instead of discarding the error, so fixture registration failures are
caught immediately; keep the change localized to newFakeFleetDB and its call
sites in zoa_test.go.
In `@pkg/zoa/reconciler_test.go`:
- Around line 91-119: The helper functions newFakeFleetDB and jobStatus are
swallowing setup errors, which can hide broken test fixtures and fail later in
confusing ways. Update these helpers to return errors instead of discarding
them: have newFakeFleetDB propagate any failure from scheme/client construction
around fake.NewClientBuilder and fleetdb.NewClientFrom, and have jobStatus
propagate the JSON marshal failure rather than ignoring it. Then update the test
call sites in reconciler_test.go to handle these errors explicitly so fixture
creation fails fast and clearly.
In `@README.md`:
- Around line 43-50: Update the README config table to match the actual CLI
defined in main.go: add the missing --oidc-issuer-base-url entry, remove
--aws-region since it is no longer a supported flag, and correct
--dynamodb-region so its default reflects the auto-detected region when unset
rather than “same as aws-region.” Keep the table entries aligned with the flag
names and defaults used by the argument parsing logic.
In `@test/e2e-api/e2e_test.go`:
- Line 115: The test management-cluster ID generation in e2e_test.go is only
second-level unique, which can collide across retries or parallel runs. Update
the mcID creation in the e2e test flow to use a higher-entropy value instead of
time.Now().Format alone, and keep the change localized to the test setup that
builds the test-mc-* identifier so each run gets a truly unique ConfigMap name.
In `@test/e2e-cli/cluster_test.go`:
- Around line 415-421: The cluster-oidc create spec is falling through with an
empty issuer URL when both the cluster response and HCP_ROSA_ISSUER_URL are
missing. Update the test around the cluster-oidc creation flow to fail fast
before invoking exec.Command if oidcIssuerURL is still empty, using the existing
clusterName/oidcIssuerURL setup in the It block. Add a clear assertion or fatal
failure in that path so the test stops with an actionable message instead of
calling rosactl cluster-oidc create with an empty --oidc-issuer-url.
- Around line 643-656: The polling in the nodepool readiness check is using the
unfiltered /api/v0/nodepools account list, which can miss the target nodepool in
shared accounts. Update the request in the cluster test helper around the
Eventually block to pass the cluster ID filter through customerApiClient.Get,
using the same id value as the clusterId query parameter, and restore the
net/url import needed to build that filtered request.
🪄 Autofix (Beta)
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: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a0e0c504-842d-4154-a016-b78ff9890828
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (57)
CLAUDE.mdDockerfileREADME.mdcmd/rosa-regional-platform-api/main.gocmd/rosa-regional-platform-api/main_test.godeployment/argocd/application-helm-hook.yamldeployment/argocd/application.yamldeployment/helm/rosa-regional-frontend/.helmignoredeployment/helm/rosa-regional-frontend/Chart.yamldeployment/helm/rosa-regional-frontend/README.mddeployment/helm/rosa-regional-frontend/templates/_helpers.tpldeployment/helm/rosa-regional-frontend/templates/configmap.yamldeployment/helm/rosa-regional-frontend/templates/deployment.yamldeployment/helm/rosa-regional-frontend/templates/post-sync-hook-job.yamldeployment/helm/rosa-regional-frontend/templates/post-sync-hook-rbac.yamldeployment/helm/rosa-regional-frontend/templates/service.yamldeployment/helm/rosa-regional-frontend/templates/serviceaccount.yamldeployment/helm/rosa-regional-frontend/templates/targetgroupbinding.yamldeployment/helm/rosa-regional-frontend/values.yamldeployment/manifests/api.yamldocs/api/zoa-endpoints.mddocs/index.htmlgo.modinternal/test/aws/api_client.goopenapi/openapi.yamlpkg/clients/fleetdb/client.gopkg/clients/fleetdb/convert.gopkg/clients/fleetdb/eksauth.gopkg/clients/hyperfleet/client.gopkg/clients/hyperfleet/client_test.gopkg/clients/hyperfleet/types.gopkg/clients/maestro/client.gopkg/clients/maestro/client_test.gopkg/clients/maestro/interface.gopkg/clients/maestro/types.gopkg/config/config.gopkg/config/config_test.gopkg/handlers/cluster.gopkg/handlers/cluster_test.gopkg/handlers/management_cluster.gopkg/handlers/nodepool.gopkg/handlers/resource_bundle.gopkg/handlers/resource_bundle_test.gopkg/handlers/work.gopkg/handlers/work_test.gopkg/handlers/zoa.gopkg/handlers/zoa_test.gopkg/server/server.gopkg/server/server_test.gopkg/types/cluster.gopkg/zoa/jobbuilder.gopkg/zoa/reconciler.gopkg/zoa/reconciler_test.gopkg/zoa/templates_test.gotest/e2e-api/e2e_test.gotest/e2e-cli/cluster_test.gotest/e2e-zoa/suite_test.go
💤 Files with no reviewable changes (29)
- deployment/helm/rosa-regional-frontend/Chart.yaml
- pkg/clients/maestro/interface.go
- deployment/helm/rosa-regional-frontend/README.md
- deployment/helm/rosa-regional-frontend/.helmignore
- deployment/helm/rosa-regional-frontend/templates/service.yaml
- deployment/helm/rosa-regional-frontend/templates/post-sync-hook-rbac.yaml
- CLAUDE.md
- pkg/clients/maestro/client_test.go
- deployment/manifests/api.yaml
- pkg/handlers/resource_bundle.go
- deployment/helm/rosa-regional-frontend/templates/post-sync-hook-job.yaml
- cmd/rosa-regional-platform-api/main_test.go
- deployment/helm/rosa-regional-frontend/templates/_helpers.tpl
- pkg/clients/maestro/types.go
- deployment/argocd/application.yaml
- deployment/helm/rosa-regional-frontend/templates/targetgroupbinding.yaml
- deployment/helm/rosa-regional-frontend/templates/configmap.yaml
- deployment/helm/rosa-regional-frontend/templates/serviceaccount.yaml
- pkg/handlers/work_test.go
- pkg/handlers/work.go
- pkg/clients/hyperfleet/client.go
- pkg/handlers/resource_bundle_test.go
- deployment/helm/rosa-regional-frontend/templates/deployment.yaml
- deployment/argocd/application-helm-hook.yaml
- pkg/clients/hyperfleet/types.go
- pkg/clients/hyperfleet/client_test.go
- deployment/helm/rosa-regional-frontend/values.yaml
- pkg/clients/maestro/client.go
- pkg/config/config_test.go
Comment on lines
+107
to
+112
| // Set DynamoDB region: --dynamodb-region if set, otherwise fall back to auto-detected region | ||
| if dynamodbRegion != "" { | ||
| cfg.Authz.AWSRegion = dynamodbRegion | ||
| logger.Info("using DynamoDB region from flag", "region", dynamodbRegion) | ||
| } else { | ||
| cfg.Authz.AWSRegion = awsCfg.Region |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reuse the detected region for ZOA when --dynamodb-region is omitted.
cfg.Authz.AWSRegion falls back to awsCfg.Region, but the ZOA block still uses the raw dynamodbRegion. With ZOA_ENABLED=true and no flag, ZOA starts with an empty AWS region.
Proposed fix
if dynamodbRegion != "" {
cfg.Authz.AWSRegion = dynamodbRegion
logger.Info("using DynamoDB region from flag", "region", dynamodbRegion)
} else {
cfg.Authz.AWSRegion = awsCfg.Region
}
+ cfg.Zoa.AWSRegion = cfg.Authz.AWSRegion🤖 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 `@cmd/rosa-regional-platform-api/main.go` around lines 107 - 112, Reuse the
detected AWS region for ZOA when --dynamodb-region is not provided by ensuring
the ZOA startup path in main.go uses the same resolved region as
cfg.Authz.AWSRegion. Update the ZOA block to reference the effective region
value (dynamodbRegion if set, otherwise awsCfg.Region) instead of the raw flag
variable, so ZOA starts with a non-empty AWS region when ZOA_ENABLED is true.
Focus the change around the region resolution logic in main and the ZOA
initialization code.
Comment on lines
+181
to
+183
| scheme := runtime.NewScheme() | ||
| _ = corev1.AddToScheme(scheme) | ||
| rcClient, err := ctrlclient.New(ctrl.GetConfigOrDie(), ctrlclient.Options{Scheme: scheme}) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and nearby startup code
git ls-files cmd/rosa-regional-platform-api/main.go
wc -l cmd/rosa-regional-platform-api/main.go
sed -n '140,230p' cmd/rosa-regional-platform-api/main.go
# Find the runServe definition and any existing error handling around startup
rg -n "func .*runServe|GetConfigOrDie|AddToScheme|ctrlclient.New|GetConfig\\(" cmd/rosa-regional-platform-api/main.goRepository: openshift-online/rosa-hyperfleet-api
Length of output: 3157
Avoid GetConfigOrDie() in startup setup. ctrl.GetConfigOrDie() can terminate the process before runServe returns, so use ctrl.GetConfig() and propagate the error; handle corev1.AddToScheme instead of discarding its return.
🤖 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 `@cmd/rosa-regional-platform-api/main.go` around lines 181 - 183, The startup
setup in main.go is using ctrl.GetConfigOrDie() and discarding the result of
corev1.AddToScheme, which can hide initialization failures and terminate the
process unexpectedly. Update the setup around scheme creation and ctrlclient.New
to call ctrl.GetConfig() and return or propagate any error instead of exiting,
and check the error from corev1.AddToScheme before constructing the client so
initialization failures are handled cleanly within runServe.
Source: Path instructions
| # Build stage | ||
| FROM golang:1.25-alpine AS builder | ||
| ARG TARGETPLATFORM | ||
| FROM --platform=$BUILDPLATFORM golang:1.26-alpine AS builder |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Use an approved or digest-pinned builder image.
golang:1.26-alpine is a mutable non-Red Hat image tag. Switch to an approved catalog.redhat.com builder image or pin the non-RH image by digest.
As per path instructions, “Base image: UBI minimal or distroless from catalog.redhat.com” and “non-RH images: pin by digest.”
🤖 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 `@Dockerfile` at line 3, The builder stage is using a mutable non-Red Hat image
tag, so update the Dockerfile’s FROM statement to use an approved
catalog.redhat.com builder image or pin the existing golang:1.26-alpine image by
digest. Keep the change scoped to the builder base image reference and ensure it
follows the repository’s base image requirements.
Source: Path instructions
Comment on lines
+197
to
+202
| "code": "dispatch-error", | ||
| "reason": "Failed to dispatch trusted action" | ||
| } | ||
| ``` | ||
|
|
||
| Indicates Maestro gRPC call failed. The execution record exists in DynamoDB with `status: failed`. | ||
| Indicates HyperFleetManifest creation on fleet-db failed. The execution record exists in DynamoDB with `status: failed`. |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don’t document status: failed as guaranteed here.
pkg/handlers/zoa.go does a best-effort UpdateStatus(..., failed, ...) and ignores that error before returning dispatch-error, so the record is guaranteed to exist but not guaranteed to be persisted as failed. Either harden the handler or soften this sentence.
🤖 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 `@docs/api/zoa-endpoints.md` around lines 197 - 202, The endpoint description
is too strong about execution state: `pkg/handlers/zoa.go` returns
`dispatch-error` after a best-effort `UpdateStatus(..., failed, ...)` that can
be ignored, so `status: failed` is not guaranteed. Either update `zoa.go` to
নিশ্চিতly persist the failed status before returning, or revise the
`docs/api/zoa-endpoints.md` wording to say the execution record exists but may
not be persisted as failed.
Comment on lines
+85
to
+87
| if spec.AccountID == "" { | ||
| spec.AccountID = accountID | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not trust spec.accountID from the request body.
Lines 85-87 only default spec.AccountID; they do not reject or overwrite a conflicting value from the request. That allows metadata.namespace and spec.accountID to diverge, and ClusterCRToPlatform later exposes spec.AccountID back as TargetProjectID. Force spec.AccountID = accountID here.
🤖 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 `@pkg/clients/fleetdb/convert.go` around lines 85 - 87, The FleetDB conversion
logic in convert.go currently only fills spec.AccountID when it is empty, which
still allows a request body to override the intended account mapping and diverge
from metadata.namespace. Update the conversion path in the function that handles
spec.AccountID assignment so it always overwrites the incoming value with the
authoritative accountID, rather than conditionally defaulting it. Make sure the
fix is applied where spec.AccountID is set before ClusterCRToPlatform uses it as
TargetProjectID.
Comment on lines
+204
to
+205
| existing.Data[mcConfigMapKey] = string(yamlData) | ||
| return h.rcClient.Update(ctx, existing) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Initialize ConfigMap.Data before assigning into it.
If the ConfigMap exists without a data section, existing.Data[mcConfigMapKey] = ... panics.
Proposed fix
+ if existing.Data == nil {
+ existing.Data = map[string]string{}
+ }
existing.Data[mcConfigMapKey] = string(yamlData)
return h.rcClient.Update(ctx, existing)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| existing.Data[mcConfigMapKey] = string(yamlData) | |
| return h.rcClient.Update(ctx, existing) | |
| if existing.Data == nil { | |
| existing.Data = map[string]string{} | |
| } | |
| existing.Data[mcConfigMapKey] = string(yamlData) | |
| return h.rcClient.Update(ctx, existing) |
🤖 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 `@pkg/handlers/management_cluster.go` around lines 204 - 205, Initialize the
ConfigMap data map before writing the management cluster entry to avoid a panic
when existing.Data is nil. In the update path in management_cluster.go, make
sure the existing ConfigMap’s Data field is created if missing before assigning
mcConfigMapKey, then proceed with the rcClient.Update call. Use the existing
variable and mcConfigMapKey in the same flow so the fix stays localized to the
ConfigMap update logic.
Comment on lines
+112
to
+133
| nodepoolID := uuid.New().String() | ||
|
|
||
| h.logger.Info("creating nodepool", "account_id", accountID, "cluster_id", req.ClusterID, "nodepool_name", req.Name, "nodepool_id", nodepoolID) | ||
|
|
||
| nodepool, err := h.maestroClient.CreateNodePool(ctx, accountID, userEmail, &req) | ||
| cr, err := fleetdb.PlatformCreateToNodePoolCR(nodepoolID, accountID, &req) | ||
| if err != nil { | ||
| h.logger.Error("failed to convert nodepool spec", "error", err, "account_id", accountID) | ||
| h.writeError(w, http.StatusBadRequest, "NODEPOOLS-MGMT-CREATE-002", "Invalid nodepool spec") | ||
| return | ||
| } | ||
|
|
||
| if err := h.fleetDB.CreateNodePool(ctx, accountID, cr); err != nil { | ||
| h.logger.Error("failed to create nodepool", "error", err, "account_id", accountID) | ||
| if fleetdb.IsAlreadyExists(err) { | ||
| h.writeError(w, http.StatusConflict, "NODEPOOLS-MGMT-CREATE-003", "NodePool already exists") | ||
| return | ||
| } | ||
| h.writeError(w, http.StatusInternalServerError, "NODEPOOLS-MGMT-CREATE-003", "Failed to create nodepool") | ||
| return | ||
| } | ||
|
|
||
| h.writeJSON(w, http.StatusCreated, nodepool) | ||
| h.writeJSON(w, http.StatusCreated, fleetdb.NodePoolCRToPlatform(cr)) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve the requested nodepool name.
Create requires and logs req.Name, but the FleetDB conversion returns name from the generated CR ID. The API response will show the UUID as the nodepool name and lose the user-provided name.
🤖 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 `@pkg/handlers/nodepool.go` around lines 112 - 133, The nodepool creation flow
is overwriting the user-provided name because `Create` builds the CR with
`fleetdb.PlatformCreateToNodePoolCR` and then returns
`fleetdb.NodePoolCRToPlatform(cr)`, which currently exposes the generated CR ID
as the name. Update the `PlatformCreateToNodePoolCR`/`NodePoolCRToPlatform` path
so the resulting platform object preserves `req.Name` in the response while
still using the UUID only as the internal nodepool ID, and keep the logging in
`h.logger.Info` aligned with the preserved name.
Comment on lines
+213
to
223
| if err := h.fleetDB.CreateManifest(ctx, zoa.JobNamespace, hfm); err != nil { | ||
| h.logger.Error("failed to create manifest on fleet-db", "error", err, "execution_id", execID) | ||
| _ = h.store.UpdateStatus(ctx, execID, zoa.StatusFailed, time.Now().UTC().Format(time.RFC3339), 0) | ||
| h.writeError(w, http.StatusBadGateway, "maestro-error", "Failed to dispatch trusted action") | ||
| h.writeError(w, http.StatusBadGateway, "dispatch-error", "Failed to dispatch trusted action") | ||
| return | ||
| } | ||
|
|
||
| exec.ManifestWorkName = result.Name | ||
| if err := h.store.UpdateManifestWorkName(ctx, execID, result.Name); err != nil { | ||
| h.logger.Error("failed to update manifestwork name", "error", err, "execution_id", execID) | ||
| exec.ManifestWorkName = hfm.Name | ||
| if err := h.store.UpdateManifestWorkName(ctx, execID, hfm.Name); err != nil { | ||
| h.logger.Error("failed to update manifest name", "error", err, "execution_id", execID) | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Persist the manifest name before dispatching it.
If UpdateManifestWorkName fails after CreateManifest succeeds, the response is still accepted, but the reconciler skips executions with an empty manifest name, leaving the dispatched job untracked.
Proposed fix
+ exec.ManifestWorkName = hfm.Name
+ if err := h.store.UpdateManifestWorkName(ctx, execID, hfm.Name); err != nil {
+ h.logger.Error("failed to update manifest name", "error", err, "execution_id", execID)
+ _ = h.store.UpdateStatus(ctx, execID, zoa.StatusFailed, time.Now().UTC().Format(time.RFC3339), 0)
+ h.writeError(w, http.StatusInternalServerError, "store-error", "Failed to record dispatched manifest")
+ return
+ }
+
if err := h.fleetDB.CreateManifest(ctx, zoa.JobNamespace, hfm); err != nil {
h.logger.Error("failed to create manifest on fleet-db", "error", err, "execution_id", execID)
_ = h.store.UpdateStatus(ctx, execID, zoa.StatusFailed, time.Now().UTC().Format(time.RFC3339), 0)
h.writeError(w, http.StatusBadGateway, "dispatch-error", "Failed to dispatch trusted action")
return
}
-
- exec.ManifestWorkName = hfm.Name
- if err := h.store.UpdateManifestWorkName(ctx, execID, hfm.Name); err != nil {
- h.logger.Error("failed to update manifest name", "error", err, "execution_id", execID)
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err := h.fleetDB.CreateManifest(ctx, zoa.JobNamespace, hfm); err != nil { | |
| h.logger.Error("failed to create manifest on fleet-db", "error", err, "execution_id", execID) | |
| _ = h.store.UpdateStatus(ctx, execID, zoa.StatusFailed, time.Now().UTC().Format(time.RFC3339), 0) | |
| h.writeError(w, http.StatusBadGateway, "maestro-error", "Failed to dispatch trusted action") | |
| h.writeError(w, http.StatusBadGateway, "dispatch-error", "Failed to dispatch trusted action") | |
| return | |
| } | |
| exec.ManifestWorkName = result.Name | |
| if err := h.store.UpdateManifestWorkName(ctx, execID, result.Name); err != nil { | |
| h.logger.Error("failed to update manifestwork name", "error", err, "execution_id", execID) | |
| exec.ManifestWorkName = hfm.Name | |
| if err := h.store.UpdateManifestWorkName(ctx, execID, hfm.Name); err != nil { | |
| h.logger.Error("failed to update manifest name", "error", err, "execution_id", execID) | |
| } | |
| exec.ManifestWorkName = hfm.Name | |
| if err := h.store.UpdateManifestWorkName(ctx, execID, hfm.Name); err != nil { | |
| h.logger.Error("failed to update manifest name", "error", err, "execution_id", execID) | |
| _ = h.store.UpdateStatus(ctx, execID, zoa.StatusFailed, time.Now().UTC().Format(time.RFC3339), 0) | |
| h.writeError(w, http.StatusInternalServerError, "store-error", "Failed to record dispatched manifest") | |
| return | |
| } | |
| if err := h.fleetDB.CreateManifest(ctx, zoa.JobNamespace, hfm); err != nil { | |
| h.logger.Error("failed to create manifest on fleet-db", "error", err, "execution_id", execID) | |
| _ = h.store.UpdateStatus(ctx, execID, zoa.StatusFailed, time.Now().UTC().Format(time.RFC3339), 0) | |
| h.writeError(w, http.StatusBadGateway, "dispatch-error", "Failed to dispatch trusted action") | |
| return | |
| } |
🤖 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 `@pkg/handlers/zoa.go` around lines 213 - 223, Persist the manifest name before
dispatching the action by setting and saving it via UpdateManifestWorkName
immediately after CreateManifest succeeds, using exec.ManifestWorkName/hfm.Name
in the same flow. If UpdateManifestWorkName fails, treat it as a dispatch
failure by logging the error, updating status to failed, and returning the error
response instead of continuing so the reconciler won’t skip the execution with
an empty manifest name.
| "spec": map[string]interface{}{ | ||
| "ttlSecondsAfterFinished": ctx.Config.TTLSeconds, | ||
| "backoffLimit": 0, | ||
| "backoffLimit": 5, |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Kubernetes Job backoffLimit failed pods Job status failed succeeded semantics
💡 Result:
In Kubernetes, a Job's completion status is determined by tracking the success and failure of its associated Pods [1][2]. Job Status Semantics The Job controller monitors two primary counters in the Job's status: -.status.succeeded: The number of Pods that have reached the Succeeded phase [3][2]. When this reaches the desired number of completions (defined by.spec.completions, which defaults to 1), the Job is considered complete [3][4]. -.status.failed: The number of Pods that have reached the Failed phase [3][2]. This value increases monotonically as Pods fail [3][2]. The Job itself is considered failed if it reaches a terminal state with a "Failed" condition, which occurs if the number of failed Pods meets or exceeds the.spec.backoffLimit [1][2][4]. Role of backoffLimit The.spec.backoffLimit specifies the maximum number of Pod failures a Job will tolerate before it is marked as failed [1][5]. - Default Behavior: When a Pod fails, the failure is counted towards the backoffLimit, and the Job controller recreates the Pod with an exponential back-off delay [1]. - Pod Failure Policy: You can override the default behavior using a PodFailurePolicy [1][3]. This allows you to distinguish between retriable and non-retriable failures [6]. For example, you can configure the Job to immediately fail (FailJob), fail only a specific index (FailIndex), or ignore specific failures so they do not increment the.status.failed counter toward the backoffLimit [6]. Terminal Conditions A Job is considered finished when it has a terminal condition of either "Complete" or "Failed" [2]. These conditions are mutually exclusive; a Job cannot have both, and they cannot be disabled [2]. You should check the.status.conditions field to reliably determine if a Job has finished, rather than relying solely on the integer counters [2][4]. Summary Table Key Field | Purpose.spec.backoffLimit | Maximum tolerated Pod failures before marking the Job as failed [1][5]..status.succeeded | Monotonically increasing count of successfully completed Pods [2][4]..status.failed | Monotonically increasing count of failed Pods [2][4]..status.conditions | Contains the definitive "Complete" or "Failed" status of the Job [2]. Top Results: [1][3][6][2]
Citations:
- 1: https://kubernetes.io/docs/concepts/workloads/controllers/job/
- 2: https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/batch/types.go
- 3: https://kubernetes.io/docs/reference/kubernetes-api/batch/job-v1/
- 4: https://stackoverflow.com/questions/50054222/how-to-determine-if-a-job-is-failed
- 5: https://www.baeldung.com/ops/kubernetes-backofflimit
- 6: https://kubernetes.io/blog/2024/08/19/kubernetes-1-31-pod-failure-policy-for-jobs-goes-ga/
Keep runner actions single-shot. backoffLimit: 5 lets Kubernetes retry the pod and rerun the trusted-action script, which can duplicate side effects if the action isn't idempotent. If retries aren't required, set this to 0.
🤖 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 `@pkg/zoa/jobbuilder.go` at line 473, The Job spec in the runner action builder
is allowing pod retries via backoffLimit: 5, which can rerun the trusted-action
script and duplicate side effects. Update the Job configuration in the runner
action path so the backoffLimit is 0, keeping runner actions single-shot; use
the job-building code in jobbuilder.go as the place to adjust this setting.
Comment on lines
107
to
109
| for _, item := range list.Items { | ||
| labels, _ := item["labels"].(map[string]interface{}) | ||
| clusterType := "" | ||
| if labels != nil { | ||
| clusterType, _ = labels["cluster_type"].(string) | ||
| } | ||
| GinkgoWriter.Printf("management cluster id=%v name=%v cluster_type=%s\n", item["id"], item["name"], clusterType) | ||
| GinkgoWriter.Printf("management cluster id=%v region=%v accountId=%v\n", item["id"], item["region"], item["accountId"]) | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid logging customer account IDs.
Line 108 prints accountId for every returned management cluster; keep the useful ID/region context without exposing customer account data in CI logs.
Proposed change
- GinkgoWriter.Printf("management cluster id=%v region=%v accountId=%v\n", item["id"], item["region"], item["accountId"])
+ GinkgoWriter.Printf("management cluster id=%v region=%v\n", item["id"], item["region"])As per coding guidelines, “Flag logging that may expose ... customer data.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for _, item := range list.Items { | |
| labels, _ := item["labels"].(map[string]interface{}) | |
| clusterType := "" | |
| if labels != nil { | |
| clusterType, _ = labels["cluster_type"].(string) | |
| } | |
| GinkgoWriter.Printf("management cluster id=%v name=%v cluster_type=%s\n", item["id"], item["name"], clusterType) | |
| GinkgoWriter.Printf("management cluster id=%v region=%v accountId=%v\n", item["id"], item["region"], item["accountId"]) | |
| } | |
| for _, item := range list.Items { | |
| GinkgoWriter.Printf("management cluster id=%v region=%v\n", item["id"], item["region"]) | |
| } |
🤖 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 `@test/e2e-api/e2e_test.go` around lines 107 - 109, Avoid logging customer
account IDs in the management cluster listing output. Update the
GinkgoWriter.Printf call inside the loop over list.Items in e2e_test.go to keep
the useful id and region context but remove the accountId field from the log
message. Make the change at the logging statement that currently references
item["accountId"].
Source: Coding guidelines
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
Continuation of #113 on a new branch.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
id,region, andaccountIdfields.Bug Fixes
Documentation