fix: fixed the targetCluster cannot be set per event issue in maestro client transportation #44
Conversation
WalkthroughThis PR adds an optional per-request TransportContext to the transport client API and updates GetResource/DiscoverResources signatures across transport_client, k8s_client, maestro_client, executor, mocks, and tests to accept the extra parameter (callers pass nil where unused). Maestro client is refactored to use the TransportContext for routing: it derives consumer information and searches across ManifestWorks, and builds ManifestWorks from templates per request. New manifest utilities ParseManifestWork and LoadManifestWork (with tests) are added. Minor logging level/format tweaks applied. Sequence Diagram(s)sequenceDiagram
participant Caller
participant TransportClient
participant K8sClient
participant MaestroClient
participant ManifestWorkStore
participant API_Server
Note over Caller,TransportClient: Caller invokes GetResource/DiscoverResources with optional TransportContext (nil => k8s path)
Caller->>TransportClient: GetResource(gvk, ns, name, target)
alt target == nil
TransportClient->>K8sClient: GetResource(gvk, ns, name, nil)
K8sClient->>API_Server: Read object
API_Server-->>K8sClient: Unstructured / NotFound
K8sClient-->>TransportClient: result
TransportClient-->>Caller: result
else target != nil
TransportClient->>MaestroClient: GetResource(gvk, ns, name, target)
MaestroClient->>ManifestWorkStore: List ManifestWorks(consumer from target)
ManifestWorkStore-->>MaestroClient: ManifestWork list
MaestroClient->>MaestroClient: Search manifests for resource (gvk, ns, name)
alt found
MaestroClient-->>TransportClient: matched Unstructured
TransportClient-->>Caller: result
else not found
MaestroClient-->>TransportClient: NotFound
TransportClient-->>Caller: NotFound
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/executor/resource_executor.go`:
- Around line 182-183: The log now emits operation values in lowercase because
the Operation constants (OperationCreate, OperationUpdate, OperationRecreate,
OperationSkip in internal/manifest/generation.go) are lowercase strings and
strings.ToUpper was removed; restore the previous uppercase log format by
uppercasing the operation when logging (e.g., in resource_executor.go where
re.log.Infof is called for Resource processing) so logs remain consistent with
downstream parsers—apply strings.ToUpper(result.Operation) (or format
accordingly) in the log message to convert the operation to uppercase before
logging.
🧹 Nitpick comments (3)
internal/maestro_client/client.go (3)
456-469: Clarify: All resources must share the same TransportContext.The implementation extracts
TransportContextonly fromresources[0].Targetand applies all resources to that single consumer/ManifestWork. If callers pass resources with differentTargetvalues, those targets will be silently ignored.Consider either:
- Documenting this constraint explicitly in the method comment (line 441-443)
- Validating that all resources have consistent
Targetvalues📝 Suggested documentation update
// ApplyResources applies multiple resources by bundling them into a ManifestWork. // All resources are stored in a single ManifestWork for the target cluster. // Requires a *maestro_client.TransportContext with ConsumerName and ManifestWork template. +// NOTE: All resources must share the same TransportContext (taken from resources[0].Target). +// Resources with different Target values will be bundled into the same ManifestWork. func (c *Client) ApplyResources(
545-558: Silent error handling may hide data corruption issues.When
manifestToUnstructuredfails (line 547-550), the error is silently continued. This could mask data corruption or malformed manifests in ManifestWorks, making debugging difficult.Consider logging the error at debug/warn level before continuing:
📝 Suggested improvement
for _, m := range workList.Items[i].Spec.Workload.Manifests { obj, err := manifestToUnstructured(m) if err != nil { + c.log.Debugf(ctx, "Skipping malformed manifest in work %s: %v", workList.Items[i].Name, err) continue }
588-593: Consider logging errors during discovery iteration.Similar to
GetResource, errors fromDiscoverManifestInWorkare silently continued. Adding debug logging would aid troubleshooting.📝 Suggested improvement
for i := range workList.Items { list, err := c.DiscoverManifestInWork(&workList.Items[i], discovery) if err != nil { + c.log.Debugf(ctx, "Skipping work %s during discovery: %v", workList.Items[i].Name, err) continue }
| re.log.Infof(successCtx, "Resource[%s] processed: operation=%s reason=%s", | ||
| resource.Name, strings.ToUpper(string(result.Operation)), result.OperationReason) | ||
| resource.Name, result.Operation, result.OperationReason) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find where Operation constants are defined
find . -name "*.go" -type f | head -20Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 837
🏁 Script executed:
# Search for Operation constant definitions in the manifest package
rg -n "Operation\s*=" --type=go -A1 internal/manifest/Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 893
🏁 Script executed:
# Also search for Operation type definitions and constants more broadly
rg -n "const\s+.*Operation" --type=go | head -20Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 65
🏁 Script executed:
# Check the actual values assigned to Operation constants
rg -n "Operation.*=\s*\"" --type=goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 674
Logging format change confirmed: operation values are now lowercase.
The Operation constants (OperationCreate, OperationUpdate, OperationRecreate, OperationSkip) are defined as lowercase strings ("create", "update", "recreate", "skip" in internal/manifest/generation.go). By removing the strings.ToUpper call, the log output format changes from uppercase (e.g., "CREATE") to lowercase (e.g., "create"). This affects log consistency and any downstream log parsing that may depend on the previous uppercase format.
🤖 Prompt for AI Agents
In `@internal/executor/resource_executor.go` around lines 182 - 183, The log now
emits operation values in lowercase because the Operation constants
(OperationCreate, OperationUpdate, OperationRecreate, OperationSkip in
internal/manifest/generation.go) are lowercase strings and strings.ToUpper was
removed; restore the previous uppercase log format by uppercasing the operation
when logging (e.g., in resource_executor.go where re.log.Infof is called for
Resource processing) so logs remain consistent with downstream parsers—apply
strings.ToUpper(result.Operation) (or format accordingly) in the log message to
convert the operation to uppercase before logging.
There was a problem hiding this comment.
This is my first version. No need to keep consistency
There was a problem hiding this comment.
@xueli181114, understood! Since this is the first version, lowercase operation logging makes sense. Thank you for clarifying.
🧠 Learnings used
Learnt from: xueli181114
Repo: openshift-hyperfleet/hyperfleet-adapter PR: 39
File: pkg/logger/context.go:35-35
Timestamp: 2026-02-06T06:53:45.757Z
Learning: In the openshift-hyperfleet/hyperfleet-adapter repository, log field keys should be descriptive rather than generic. Use `cluster_id` instead of generic `id` for cluster identifiers in log fields (ClusterIDKey constant and WithClusterID function) to maintain consistency with other resource fields like `resource_id` and improve log searchability and observability.
xueli181114
force-pushed
the
HYPERFLEET-629
branch
from
ed1fb64 to
877b2e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/maestro_client/client.go (1)
603-625:⚠️ Potential issue | 🟠 MajorTemplate manifests can be dropped when any resource has a manifest.
The code skips nil manifests but then replaces template manifests whenever any non-nil manifest exists, which contradicts the “skip means template manifests are already defined” comment.🔧 Safer merge preserving template manifests
- manifests := make([]workv1.Manifest, 0, len(resources)) + manifests := make([]workv1.Manifest, 0, len(resources)+len(work.Spec.Workload.Manifests)) + manifests = append(manifests, work.Spec.Workload.Manifests...) for _, r := range resources { if r.Manifest == nil { continue // Skip resources with no manifest. It means manifests already defined in the manifestWork template } raw, err := json.Marshal(r.Manifest.Object) @@ - if len(manifests) > 0 { - work.Spec.Workload.Manifests = manifests - } + if len(manifests) > 0 { + work.Spec.Workload.Manifests = manifests + }
🤖 Fix all issues with AI agents
In `@internal/maestro_client/client.go`:
- Around line 456-469: ApplyResources currently resolves transportCtx only from
the first resource which can silently misroute others; update ApplyResources to
call resolveTransportContext for each ResourceToApply and verify that every
resource's Target resolves to a non-nil *TransportContext and that its
ConsumerName and ManifestWork match the first resolved transportCtx (or return
an error referencing ResourceToApply.Target, TransportContext.ConsumerName, and
TransportContext.ManifestWork on mismatch/nil). Ensure you use the existing
resolveTransportContext function and produce clear errors naming the offending
resource when a mismatch or missing fields are detected.
🧹 Nitpick comments (1)
internal/manifest/manifestwork.go (1)
12-33: Consider validating Kind/APIVersion/metadata to fail fast.
{}(or wrong Kind) parses cleanly and can defer errors to later apply paths. If you want a stricter contract, validate required fields after unmarshal.📌 Possible validation (inline)
// Try JSON first if err := json.Unmarshal(data, work); err == nil { - return work, nil + if work.APIVersion == "" || work.Kind != "ManifestWork" || work.Name == "" { + return nil, fmt.Errorf("invalid ManifestWork: apiVersion/kind/metadata.name are required") + } + return work, nil } @@ if err := json.Unmarshal(jsonData, work); err != nil { return nil, fmt.Errorf("failed to parse ManifestWork: %w", err) } + if work.APIVersion == "" || work.Kind != "ManifestWork" || work.Name == "" { + return nil, fmt.Errorf("invalid ManifestWork: apiVersion/kind/metadata.name are required") + } return work, nil
| // Resolve maestro transport context from first resource | ||
| transportCtx := c.resolveTransportContext(resources[0].Target) | ||
| if transportCtx == nil { | ||
| return nil, fmt.Errorf("maestro TransportContext is required: set ResourceToApply.Target to *maestro_client.TransportContext") | ||
| } | ||
|
|
||
| consumerName := transportCtx.ConsumerName | ||
| if consumerName == "" { | ||
| return nil, fmt.Errorf("consumer name (target cluster) is required: set TransportContext.ConsumerName") | ||
| } | ||
|
|
||
| if transportCtx.ManifestWork == nil { | ||
| return nil, fmt.Errorf("ManifestWork template is required: set TransportContext.ManifestWork") | ||
| } |
There was a problem hiding this comment.
Ensure all resources share the same target context.
ApplyResources resolves only the first resource’s Target; mismatches in later resources would silently route to the wrong consumer/template.
✅ Suggested guard
// Resolve maestro transport context from first resource
transportCtx := c.resolveTransportContext(resources[0].Target)
if transportCtx == nil {
return nil, fmt.Errorf("maestro TransportContext is required: set ResourceToApply.Target to *maestro_client.TransportContext")
}
consumerName := transportCtx.ConsumerName
if consumerName == "" {
return nil, fmt.Errorf("consumer name (target cluster) is required: set TransportContext.ConsumerName")
}
if transportCtx.ManifestWork == nil {
return nil, fmt.Errorf("ManifestWork template is required: set TransportContext.ManifestWork")
}
+
+ for _, r := range resources {
+ tc := c.resolveTransportContext(r.Target)
+ if tc == nil || tc.ConsumerName != consumerName || tc.ManifestWork != transportCtx.ManifestWork {
+ return nil, fmt.Errorf("mixed TransportContext in ApplyResources for resource %s", r.Name)
+ }
+ }📝 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.
| // Resolve maestro transport context from first resource | |
| transportCtx := c.resolveTransportContext(resources[0].Target) | |
| if transportCtx == nil { | |
| return nil, fmt.Errorf("maestro TransportContext is required: set ResourceToApply.Target to *maestro_client.TransportContext") | |
| } | |
| consumerName := transportCtx.ConsumerName | |
| if consumerName == "" { | |
| return nil, fmt.Errorf("consumer name (target cluster) is required: set TransportContext.ConsumerName") | |
| } | |
| if transportCtx.ManifestWork == nil { | |
| return nil, fmt.Errorf("ManifestWork template is required: set TransportContext.ManifestWork") | |
| } | |
| // Resolve maestro transport context from first resource | |
| transportCtx := c.resolveTransportContext(resources[0].Target) | |
| if transportCtx == nil { | |
| return nil, fmt.Errorf("maestro TransportContext is required: set ResourceToApply.Target to *maestro_client.TransportContext") | |
| } | |
| consumerName := transportCtx.ConsumerName | |
| if consumerName == "" { | |
| return nil, fmt.Errorf("consumer name (target cluster) is required: set TransportContext.ConsumerName") | |
| } | |
| if transportCtx.ManifestWork == nil { | |
| return nil, fmt.Errorf("ManifestWork template is required: set TransportContext.ManifestWork") | |
| } | |
| for _, r := range resources { | |
| tc := c.resolveTransportContext(r.Target) | |
| if tc == nil || tc.ConsumerName != consumerName || tc.ManifestWork != transportCtx.ManifestWork { | |
| return nil, fmt.Errorf("mixed TransportContext in ApplyResources for resource %s", r.Name) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@internal/maestro_client/client.go` around lines 456 - 469, ApplyResources
currently resolves transportCtx only from the first resource which can silently
misroute others; update ApplyResources to call resolveTransportContext for each
ResourceToApply and verify that every resource's Target resolves to a non-nil
*TransportContext and that its ConsumerName and ManifestWork match the first
resolved transportCtx (or return an error referencing ResourceToApply.Target,
TransportContext.ConsumerName, and TransportContext.ManifestWork on
mismatch/nil). Ensure you use the existing resolveTransportContext function and
produce clear errors naming the offending resource when a mismatch or missing
fields are detected.
There was a problem hiding this comment.
Yes. All resources should be put in 1 manifestwork and they will use same target.
There was a problem hiding this comment.
@xueli181114, thanks for confirming! Since all resources must share the same ManifestWork and target cluster, it's important to always validate inputs to avoid leaving yourself open to future headaches, annoyances and bugs.
Adding the guard would catch API misuse early (e.g., if a caller accidentally mixes resources with different ConsumerName or ManifestWork values) and return a clear error message instead of silently applying resources to the wrong cluster or failing later with a confusing error.
The validation is lightweight—just a loop comparing pointers and strings—and ensures data integrity, prevents security vulnerabilities, and provides meaningful feedback to API consumers. Would you like to add it as a safeguard, or are you confident the callers will always provide consistent targets?
… client transportation
xueli181114
force-pushed
the
HYPERFLEET-629
branch
from
877b2e6 to
e0576ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/maestro_client/client.go`:
- Around line 534-537: GetResource returns apierrors.NewNotFound when
consumerName is empty, while DiscoverResources returns an empty list; make the
behaviors consistent by changing DiscoverResources to return the same NotFound
error when consumerName == "" (construct the same schema.GroupResource using
gvk.Group and gvk.Kind and call apierrors.NewNotFound with the same name), so
callers get identical error semantics as GetResource instead of an empty slice.
🧹 Nitpick comments (3)
internal/manifest/manifestwork.go (1)
14-33: Consider logging or validating when JSON fails before YAML fallback.When
json.Unmarshalfails at Line 18, the error is silently discarded before trying YAML conversion. This could mask issues where the input is malformed JSON that happens to parse as valid (but incorrect) YAML.♻️ Optional: preserve context about fallback
func ParseManifestWork(data []byte) (*workv1.ManifestWork, error) { work := &workv1.ManifestWork{} // Try JSON first - if err := json.Unmarshal(data, work); err == nil { + jsonErr := json.Unmarshal(data, work) + if jsonErr == nil { return work, nil } // Fall back to YAML → JSON → ManifestWork jsonData, err := yaml.YAMLToJSON(data) if err != nil { - return nil, fmt.Errorf("failed to convert ManifestWork YAML to JSON: %w", err) + return nil, fmt.Errorf("failed to parse ManifestWork (JSON error: %v, YAML error: %w)", jsonErr, err) }internal/maestro_client/client.go (2)
545-558: Silent error suppression when parsing manifests.When
manifestToUnstructuredfails (Line 548-550), errors are silently skipped viacontinue. This could hide corrupted or malformed manifests in the ManifestWork. Consider at least logging these errors for debugging purposes.🔍 Suggested improvement: log parse failures
for i := range workList.Items { for _, m := range workList.Items[i].Spec.Workload.Manifests { obj, err := manifestToUnstructured(m) if err != nil { + c.log.Debugf(ctx, "Failed to parse manifest in work %s: %v", workList.Items[i].Name, err) continue }
608-612: Clarify the nil Manifest skip behavior.The comment "manifests already defined in manifestWork template" may be misleading. If a
ResourceToApplyhas a nilManifest, it's unclear whether:
- The resource should use what's already in the template
- It's an error condition
- The resource should be intentionally skipped
Consider adding validation or clearer documentation about when nil manifests are expected.
| if consumerName == "" { | ||
| gr := schema.GroupResource{Group: gvk.Group, Resource: gvk.Kind} | ||
| return nil, apierrors.NewNotFound(gr, name) | ||
| } |
There was a problem hiding this comment.
Inconsistent behavior when consumerName is empty.
GetResource returns NotFound error when consumerName is empty (Line 535-536), but DiscoverResources returns an empty list (Line 578). This inconsistency could lead to unexpected behavior for callers.
Consider aligning the behavior - either both should return an error or both should return empty/not-found.
🔧 Option 1: Make DiscoverResources return an error
if consumerName == "" {
- return &unstructured.UnstructuredList{}, nil
+ return nil, fmt.Errorf("consumer name (target cluster) is required for DiscoverResources")
}Also applies to: 577-579
🤖 Prompt for AI Agents
In `@internal/maestro_client/client.go` around lines 534 - 537, GetResource
returns apierrors.NewNotFound when consumerName is empty, while
DiscoverResources returns an empty list; make the behaviors consistent by
changing DiscoverResources to return the same NotFound error when consumerName
== "" (construct the same schema.GroupResource using gvk.Group and gvk.Kind and
call apierrors.NewNotFound with the same name), so callers get identical error
semantics as GetResource instead of an empty slice.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rh-amarin 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-merge-bot
bot
merged commit ed01ba1
into
openshift-hyperfleet:main
2 participants
Summary by CodeRabbit
New Features
Changes
Tests