HYPERFLEET-569 - feat: Implement maestro DSL loader #41
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
rh-amarin
force-pushed
the
resource-transport
branch
from
e904536 to
8a0c194
Compare
WalkthroughThis pull request introduces a transport abstraction layer to support multiple deployment backends (Kubernetes and Maestro). Key changes include: a new factory pattern for client creation (API, Kubernetes, Maestro); a TransportClient interface decoupling executor logic from backend-specific implementations; enhanced manifest handling with named manifests and Maestro-specific configuration; refactored executor to branch resource application by transport type; new utility modules for manifest generation, rendering, and conversion; and updated Helm examples demonstrating both Kubernetes and Maestro deployments. Configuration templates are removed in favor of schema-based validation, and several K8s client methods are eliminated in favor of unified transport-based resource application. Sequence DiagramsequenceDiagram
participant Adapter as Adapter Startup
participant Factory as Client Factory
participant TransportC as TransportClient
participant APIClient as HyperFleet API
participant K8sClient as Kubernetes
participant MaestroClient as Maestro
participant Executor as Executor
participant ResExec as Resource Executor
Adapter->>Factory: CreateAPIClient(config)
Factory->>APIClient: NewClient(options)
APIClient-->>Factory: API Client
Factory-->>Adapter: API Client
alt Maestro Config Present
Adapter->>Factory: CreateMaestroClient(config)
Factory->>MaestroClient: NewMaestroClient(ctx, config)
MaestroClient-->>Factory: Maestro Client
Factory-->>Adapter: Maestro Client (TransportClient)
Adapter->>Executor: WithTransportClient(Maestro)
else Kubernetes Path
Adapter->>Factory: CreateK8sClient(ctx, config)
Factory->>K8sClient: NewClient(ctx, config)
K8sClient-->>Factory: K8s Client
Factory-->>Adapter: K8s Client (TransportClient)
Adapter->>Executor: WithTransportClient(K8s)
end
Adapter->>Executor: Execute(event, config)
Executor->>ResExec: executeResource(resource, execCtx)
alt Kubernetes Transport
ResExec->>ResExec: buildManifestK8s()
ResExec->>TransportC: ApplyResources([manifest])
TransportC->>K8sClient: ApplyResource()
K8sClient-->>TransportC: ApplyResult
else Maestro Transport
ResExec->>ResExec: buildManifestsMaestro()
ResExec->>TransportC: ApplyResources([manifests])
TransportC->>MaestroClient: ApplyResources()
MaestroClient->>MaestroClient: buildManifestWork()
MaestroClient-->>TransportC: ApplyResourcesResult
end
TransportC-->>ResExec: Result
ResExec-->>Executor: ExecutionContext (status)
Executor-->>Adapter: Complete
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
190-196:⚠️ Potential issue | 🟡 MinorFix typo: “overriden” → “overridden.”
Minor doc polish.✏️ Proposed fix
- - Configure topic/subscription in the `adapter-config.yaml` but can be overriden with env variables or cli params + - Configure topic/subscription in the `adapter-config.yaml` but can be overridden with env variables or CLI params
🤖 Fix all issues with AI agents
In `@internal/executor/resource_executor.go`:
- Around line 356-366: The generation annotation is being lost because
work.SetAnnotations is called twice and the second call in
applyManifestWorkSettings overwrites all annotations; instead merge annotations:
when copying generation from manifest (using manifestAnnotations and
constants.AnnotationGeneration) read existing work.GetAnnotations(), create a
merged map that preserves existing keys (including any metadata.annotations from
RefContent), set/overwrite the generation key in that merged map, and then call
work.SetAnnotations once with the merged map; similarly, update
applyManifestWorkSettings to merge incoming metadata.annotations with
work.GetAnnotations() rather than replacing them so existing generation and
other annotations are preserved.
🧹 Nitpick comments (3)
internal/executor/types.go (1)
63-67: Add config-time validation for MaestroClient when maestro transport is used.While
applyResourceMaestroalready validates at runtime thatmaestroClientis not nil, validation at config initialization time would catch the issue earlier. Consider adding a check invalidateExecutorConfigto iterate over resources and requireMaestroClientwhen any resource usesTransportClientMaestro.internal/executor/resource_executor.go (2)
388-405: Labels frommetadata.labelsoverwrite rather than merge.On line 403,
work.SetLabels(labelMap)overwrites any existing labels. This is inconsistent with the root-level labels handling (lines 436-444) which merges with existing. Consider using the same merge pattern for consistency:♻️ Suggested fix for consistency
if labels, ok := metadata["labels"].(map[string]interface{}); ok { labelMap := make(map[string]string) for k, v := range labels { if str, ok := v.(string); ok { rendered, err := renderTemplate(str, params) if err != nil { return fmt.Errorf("failed to render label value for key %s: %w", k, err) } labelMap[k] = rendered } } - work.SetLabels(labelMap) + // Merge with existing labels + existing := work.GetLabels() + if existing == nil { + existing = make(map[string]string) + } + for k, v := range labelMap { + existing[k] = v + } + work.SetLabels(existing) re.log.Debugf(ctx, "Applied labels: %v", labelMap) }
297-299: Operation result is alwaysOperationCreatebut the actual operation is an upsert.The comment correctly states "ManifestWork apply is always an upsert", but the operation is recorded as
OperationCreate. This could be misleading in logs and metrics. Consider using a more accurate operation type or adding a new constant likeOperationApplyorOperationUpsert.
| // Copy the generation annotation from the manifest to the ManifestWork | ||
| // This is required by the maestro client for generation-based idempotency | ||
| manifestAnnotations := manifest.GetAnnotations() | ||
| if manifestAnnotations != nil { | ||
| if gen, ok := manifestAnnotations[constants.AnnotationGeneration]; ok { | ||
| work.SetAnnotations(map[string]string{ | ||
| constants.AnnotationGeneration: gen, | ||
| }) | ||
| re.log.Debugf(ctx, "Set ManifestWork generation annotation: %s", gen) | ||
| } | ||
| } |
There was a problem hiding this comment.
Generation annotation may be overwritten by RefContent settings.
The generation annotation is set on lines 361-364, but applyManifestWorkSettings calls work.SetAnnotations() (line 419) which overwrites all annotations. This could lose the generation annotation if metadata.annotations is present in RefContent.
Consider merging annotations instead of overwriting:
🐛 Proposed fix
// Apply annotations from metadata
if annotations, ok := metadata["annotations"].(map[string]interface{}); ok {
annotationMap := make(map[string]string)
for k, v := range annotations {
if str, ok := v.(string); ok {
rendered, err := renderTemplate(str, params)
if err != nil {
return fmt.Errorf("failed to render annotation value for key %s: %w", k, err)
}
annotationMap[k] = rendered
}
}
- work.SetAnnotations(annotationMap)
+ // Merge with existing annotations to preserve generation annotation
+ existing := work.GetAnnotations()
+ if existing == nil {
+ existing = make(map[string]string)
+ }
+ for k, v := range annotationMap {
+ existing[k] = v
+ }
+ work.SetAnnotations(existing)
re.log.Debugf(ctx, "Applied annotations: %v", annotationMap)
}Also applies to: 403-421
🤖 Prompt for AI Agents
In `@internal/executor/resource_executor.go` around lines 356 - 366, The
generation annotation is being lost because work.SetAnnotations is called twice
and the second call in applyManifestWorkSettings overwrites all annotations;
instead merge annotations: when copying generation from manifest (using
manifestAnnotations and constants.AnnotationGeneration) read existing
work.GetAnnotations(), create a merged map that preserves existing keys
(including any metadata.annotations from RefContent), set/overwrite the
generation key in that merged map, and then call work.SetAnnotations once with
the merged map; similarly, update applyManifestWorkSettings to merge incoming
metadata.annotations with work.GetAnnotations() rather than replacing them so
existing generation and other annotations are preserved.
cmd/adapter/main.go
Outdated
| } | ||
|
|
||
| // Configure TLS if auth type is "tls" | ||
| if maestroConfig.Auth.Type == "tls" && maestroConfig.Auth.TLSConfig != nil { |
There was a problem hiding this comment.
This could lead to a confusing scenario where a user
configures auth.type: tls but forgets to provide tlsConfig — the client would silently
connect without mTLS, potentially exposing traffic.
Consider returning an explicit error when the configuration is inconsistent:
if maestroConfig.Auth.Type == "tls" {
if maestroConfig.Auth.TLSConfig == nil {
return nil, fmt.Errorf("maestro auth type is 'tls' but tlsConfig is not provided")
}
clientConfig.CAFile = maestroConfig.Auth.TLSConfig.CAFile
clientConfig.ClientCertFile = maestroConfig.Auth.TLSConfig.CertFile
clientConfig.ClientKeyFile = maestroConfig.Auth.TLSConfig.KeyFile
}
| } | ||
|
|
||
| // Error if env var is not set and no default provided | ||
| if envValue == "" && param.Default == nil { |
There was a problem hiding this comment.
Previously, a parameter like:
- name: myParam
source: "env.MY_REQUIRED_VAR"
required: true
would fail validation if MY_REQUIRED_VAR was not set and no default was provided. Now it
passes validation regardless of whether the env var exists.
Was this change intentional? If so, it would be good to document why the runtime env check was
removed (e.g., validation runs at build time, not in the target environment). If not, this is
a regression that could lead to silent failures at runtime when required env vars are
missing.
There was a problem hiding this comment.
You are right, I confused the moment when then adapter env gets evaluated.
At runtime, the env for the Adapter instance will not change, which is different for the Adapter Task itself, for example a job is created in the resources phase, where the manifest can introduce new env variables.
|
|
||
| # Resources with valid K8s manifests | ||
| resources: | ||
| - name: "maestro" |
There was a problem hiding this comment.
This kind of configuration lose the manifestwork ref. And for there should be multiple manifests in the manifestwork.
There was a problem hiding this comment.
And we should put manifests in same manifestwork together. Manifests number cannot be changed once the manifestwork created.
| hyperfleet.io/cluster-name: "{{ .clusterName }}" | ||
| annotations: | ||
| hyperfleet.io/generation: "{{ .generationSpec }}" | ||
| discovery: |
There was a problem hiding this comment.
Discovery is manifest level not manifestwork.
There was a problem hiding this comment.
the discovery can help us locate the manifest attributes with jsonPath
cmd/adapter/main.go
Outdated
| } | ||
|
|
||
| // createMaestroClient creates a Maestro client from the config | ||
| func createMaestroClient(ctx context.Context, maestroConfig *config_loader.MaestroClientConfig, log logger.Logger) (*maestro_client.Client, error) { |
There was a problem hiding this comment.
How about move the createXXXClient functions to another file to simplify main.go file?
| transport: | ||
| client: "maestro" | ||
| maestro: | ||
| targetCluster: cluster1 |
There was a problem hiding this comment.
manifestwork.ref here should be something required and have a validation
| // Replace manifest with loaded content | ||
| resource.Manifest = content | ||
| // Load transport.maestro.manifestWork.ref | ||
| if resource.Transport != nil && resource.Transport.Maestro != nil && resource.Transport.Maestro.ManifestWork != nil { |
There was a problem hiding this comment.
ManifestWork must not be nil.
| } | ||
|
|
||
| // Validate transport.maestro.manifestWork.ref in spec.resources | ||
| if resource.Transport != nil && resource.Transport.Maestro != nil && resource.Transport.Maestro.ManifestWork != nil { |
There was a problem hiding this comment.
how about have a splited function for maestro validation only?
| } | ||
|
|
||
| // validateTransportConfig validates transport configuration for all resources | ||
| func (v *TaskConfigValidator) validateTransportConfig() { |
There was a problem hiding this comment.
This function only validate maestro.
| // Step 2: Delegate to applyResource which handles discovery, generation comparison, and operations | ||
| return re.applyResource(ctx, resource, manifest, execCtx) | ||
| // Step 2: Check transport type and route to appropriate client | ||
| clientType := resource.Transport.GetClientType() |
There was a problem hiding this comment.
Can we have resource.Transport.GetClient()? The we just return maestroClient or k8sClient. My executor is having an interface defined transportClient that can accept both maestroClient and k8sClient
| } | ||
|
|
||
| // Validate maestro transport config is present | ||
| if resource.Transport == nil || resource.Transport.Maestro == nil { |
There was a problem hiding this comment.
I assume this condition checkout should happen outside of this function. It should happen before calling applyResourceMaestro,
|
|
||
| // applyResourceMaestro handles resource delivery via Maestro ManifestWork. | ||
| // It builds a ManifestWork containing the manifest and applies it to the target cluster. | ||
| func (re *ResourceExecutor) applyResourceMaestro(ctx context.Context, resource config_loader.Resource, manifest *unstructured.Unstructured, execCtx *ExecutionContext) (ResourceResult, error) { |
There was a problem hiding this comment.
In my PR, I refactored this part. Let me shrink the PR to split out this part and make it merged first instead of stuck to steps implemenation
rh-amarin
force-pushed
the
resource-transport
branch
from
8a0c194 to
0d78626
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
191-196:⚠️ Potential issue | 🟡 MinorFix minor typo in broker override note.
✏️ Suggested edit
- - Configure topic/subscription in the `adapter-config.yaml` but can be overriden with env variables or cli params + - Configure topic/subscription in the `adapter-config.yaml` but can be overridden with env variables or CLI params
🤖 Fix all issues with AI agents
In `@charts/examples/adapter-config.yaml`:
- Line 48: Update the example to use secure defaults by changing the adapter
configuration key insecure from true to false (replace "insecure: true" with
"insecure: false" in the adapter-config.yaml example) and add a brief comment or
note near the insecure setting explaining that insecure can be enabled
explicitly for local development only; ensure the example still documents how to
opt into plaintext for dev environments so maintainers know how to override
insecure when needed.
- Around line 14-16: Change the example to avoid enabling full merged-config
debug output by setting debugConfig to false (do not log secrets), and keep
log.level at debug if desired; update the adapter-config.yaml entry for the
debugConfig key and add a short inline comment next to debugConfig explaining to
enable it only for local troubleshooting to avoid leaking secrets in real
deployments.
In `@cmd/adapter/main.go`:
- Around line 287-313: The executor can be built even when resources require
Maestro transport but no Maestro client is provided; update
validateExecutorConfig to detect this by scanning config.Spec.Resources (or
wherever resource transports are defined) for any resource with
TransportClientMaestro and return an error if maestroClient is nil, and then
call validateExecutorConfig (pass the config and the maestroClient from
execBuilder or the local maestroClient variable) before calling
execBuilder.Build(); reference validateExecutorConfig(), applyResourceMaestro(),
TransportClientMaestro, MaestroClient, executor.NewBuilder(),
execBuilder.WithMaestroClient(), and execBuilder.Build() when locating the
change.
In `@internal/client_factory/maestro_client.go`:
- Around line 13-39: Add a nil-check at the start of CreateMaestroClient to
validate maestroConfig (and return a clear error) before any field access to
avoid panics; specifically, check if maestroConfig == nil and return an error
like "nil maestroConfig provided" from CreateMaestroClient, and also guard
accesses to maestroConfig.Auth (e.g., check maestroConfig.Auth != nil before
reading Auth.Type or TLSConfig) so clientConfig population and the TLS branch
(TLSConfig, CAFile, CertFile, KeyFile) never dereference a nil pointer.
In `@internal/executor/resource_executor.go`:
- Around line 203-206: The failure logging currently dumps the full manifest
(manifest.Object) via re.log.Debugf which can leak Secret data; update the
ResourceExecutor failure path to either skip logging for sensitive kinds (e.g.,
"Secret") or to redact sensitive fields before rendering (mask common secret
keys and data/strings) and then log the sanitized YAML instead of the raw
manifest, and apply the same change to the Maestro failure logging codepath so
both use the sanitized output rather than dumping manifest.Object or raw
manifests.
🧹 Nitpick comments (2)
charts/examples/adapter-task-config.yaml (1)
79-84: Avoid hardcoding the Maestro target cluster in the example.Using a static
cluster1makes the example easy to misapply; consider templating it to the event/lookup result so it routes correctly.✏️ Suggested change
- maestro: - targetCluster: cluster1 + maestro: + targetCluster: "{{ .clusterName }}"internal/config_loader/validator.go (1)
544-574: Flag maestro config whentransport.clientisn’t maestro. This avoids silently defaulting to Kubernetes if users set a maestro block but forget the client field.♻️ Suggested validation tweak
- // Validate maestro config is present when client=maestro - if resource.Transport.Client == TransportClientMaestro { + clientType := resource.Transport.GetClientType() + // Validate maestro config is present when client=maestro + if clientType == TransportClientMaestro { if resource.Transport.Maestro == nil { v.errors.Add(basePath, "maestro configuration is required when client is 'maestro'") continue } // ... + } else if resource.Transport.Maestro != nil { + v.errors.Add(basePath+"."+FieldMaestro, "maestro config is set but transport.client is not 'maestro'") }
| debugConfig: true | ||
| log: | ||
| level: debug |
There was a problem hiding this comment.
Avoid enabling full config debug logging in the example.
debugConfig: true logs the full merged config and can leak secrets when users copy this example into real deployments. Consider defaulting it to false and leaving a comment for troubleshooting.
🔒 Suggested change
- debugConfig: true
- log:
- level: debug
+ debugConfig: false
+ log:
+ level: info📝 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.
| debugConfig: true | |
| log: | |
| level: debug | |
| debugConfig: false | |
| log: | |
| level: info |
🤖 Prompt for AI Agents
In `@charts/examples/adapter-config.yaml` around lines 14 - 16, Change the example
to avoid enabling full merged-config debug output by setting debugConfig to
false (do not log secrets), and keep log.level at debug if desired; update the
adapter-config.yaml entry for the debugConfig key and add a short inline comment
next to debugConfig explaining to enable it only for local troubleshooting to
avoid leaking secrets in real deployments.
| # Client identifier (defaults to sourceId if not specified) | ||
| # Environment variable: HYPERFLEET_MAESTRO_CLIENT_ID | ||
| clientId: "hyperfleet-adapter-client" | ||
| insecure: true |
There was a problem hiding this comment.
Prefer secure defaults for Maestro in the example.
insecure: true makes the example opt into plaintext. Safer to default to false and allow enabling insecure mode explicitly for local dev.
🔧 Suggested change
- insecure: true
+ insecure: false📝 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.
| insecure: true | |
| insecure: false |
🤖 Prompt for AI Agents
In `@charts/examples/adapter-config.yaml` at line 48, Update the example to use
secure defaults by changing the adapter configuration key insecure from true to
false (replace "insecure: true" with "insecure: false" in the
adapter-config.yaml example) and add a brief comment or note near the insecure
setting explaining that insecure can be enabled explicitly for local development
only; ensure the example still documents how to opt into plaintext for dev
environments so maintainers know how to override insecure when needed.
| // CreateMaestroClient creates a Maestro client from the config | ||
| func CreateMaestroClient(ctx context.Context, maestroConfig *config_loader.MaestroClientConfig, log logger.Logger) (*maestro_client.Client, error) { | ||
| clientConfig := &maestro_client.Config{ | ||
| MaestroServerAddr: maestroConfig.HTTPServerAddress, | ||
| GRPCServerAddr: maestroConfig.GRPCServerAddress, | ||
| SourceID: maestroConfig.SourceID, | ||
| Insecure: maestroConfig.Insecure, | ||
| } | ||
|
|
||
| // Parse timeout if specified | ||
| if maestroConfig.Timeout != "" { | ||
| timeout, err := time.ParseDuration(maestroConfig.Timeout) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid maestro timeout %q: %w", maestroConfig.Timeout, err) | ||
| } | ||
| clientConfig.HTTPTimeout = timeout | ||
| } | ||
|
|
||
| // Configure TLS if auth type is "tls" | ||
| if maestroConfig.Auth.Type == "tls" { | ||
| if maestroConfig.Auth.TLSConfig == nil { | ||
| return nil, fmt.Errorf("maestro auth type is 'tls' but tlsConfig is not provided") | ||
| } | ||
| clientConfig.CAFile = maestroConfig.Auth.TLSConfig.CAFile | ||
| clientConfig.ClientCertFile = maestroConfig.Auth.TLSConfig.CertFile | ||
| clientConfig.ClientKeyFile = maestroConfig.Auth.TLSConfig.KeyFile | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "type MaestroClientConfig|type Maestro.*Auth|TLSConfig" internal/config_loader/types.go -C 3Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 338
🏁 Script executed:
cat -n internal/config_loader/types.go | head -100Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 4215
🏁 Script executed:
rg -n "type MaestroClientConfig|type.*Auth|TLSConfig" internal/config_loader/types.go -A 10Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1633
🏁 Script executed:
rg -n "CreateMaestroClient" internal/ -B 2 -A 2Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 663
🏁 Script executed:
cat -n internal/client_factory/maestro_client.go | head -50Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1871
Add nil guard for maestroConfig to prevent panic. Exported factory functions should validate input parameters; without this guard, nil config will panic on field access.
Proposed nil check
func CreateMaestroClient(ctx context.Context, maestroConfig *config_loader.MaestroClientConfig, log logger.Logger) (*maestro_client.Client, error) {
+ if maestroConfig == nil {
+ return nil, fmt.Errorf("maestro config is nil")
+ }
clientConfig := &maestro_client.Config{
MaestroServerAddr: maestroConfig.HTTPServerAddress,
GRPCServerAddr: maestroConfig.GRPCServerAddress,
SourceID: maestroConfig.SourceID,
Insecure: maestroConfig.Insecure,
}📝 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.
| // CreateMaestroClient creates a Maestro client from the config | |
| func CreateMaestroClient(ctx context.Context, maestroConfig *config_loader.MaestroClientConfig, log logger.Logger) (*maestro_client.Client, error) { | |
| clientConfig := &maestro_client.Config{ | |
| MaestroServerAddr: maestroConfig.HTTPServerAddress, | |
| GRPCServerAddr: maestroConfig.GRPCServerAddress, | |
| SourceID: maestroConfig.SourceID, | |
| Insecure: maestroConfig.Insecure, | |
| } | |
| // Parse timeout if specified | |
| if maestroConfig.Timeout != "" { | |
| timeout, err := time.ParseDuration(maestroConfig.Timeout) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid maestro timeout %q: %w", maestroConfig.Timeout, err) | |
| } | |
| clientConfig.HTTPTimeout = timeout | |
| } | |
| // Configure TLS if auth type is "tls" | |
| if maestroConfig.Auth.Type == "tls" { | |
| if maestroConfig.Auth.TLSConfig == nil { | |
| return nil, fmt.Errorf("maestro auth type is 'tls' but tlsConfig is not provided") | |
| } | |
| clientConfig.CAFile = maestroConfig.Auth.TLSConfig.CAFile | |
| clientConfig.ClientCertFile = maestroConfig.Auth.TLSConfig.CertFile | |
| clientConfig.ClientKeyFile = maestroConfig.Auth.TLSConfig.KeyFile | |
| } | |
| // CreateMaestroClient creates a Maestro client from the config | |
| func CreateMaestroClient(ctx context.Context, maestroConfig *config_loader.MaestroClientConfig, log logger.Logger) (*maestro_client.Client, error) { | |
| if maestroConfig == nil { | |
| return nil, fmt.Errorf("maestro config is nil") | |
| } | |
| clientConfig := &maestro_client.Config{ | |
| MaestroServerAddr: maestroConfig.HTTPServerAddress, | |
| GRPCServerAddr: maestroConfig.GRPCServerAddress, | |
| SourceID: maestroConfig.SourceID, | |
| Insecure: maestroConfig.Insecure, | |
| } | |
| // Parse timeout if specified | |
| if maestroConfig.Timeout != "" { | |
| timeout, err := time.ParseDuration(maestroConfig.Timeout) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid maestro timeout %q: %w", maestroConfig.Timeout, err) | |
| } | |
| clientConfig.HTTPTimeout = timeout | |
| } | |
| // Configure TLS if auth type is "tls" | |
| if maestroConfig.Auth.Type == "tls" { | |
| if maestroConfig.Auth.TLSConfig == nil { | |
| return nil, fmt.Errorf("maestro auth type is 'tls' but tlsConfig is not provided") | |
| } | |
| clientConfig.CAFile = maestroConfig.Auth.TLSConfig.CAFile | |
| clientConfig.ClientCertFile = maestroConfig.Auth.TLSConfig.CertFile | |
| clientConfig.ClientKeyFile = maestroConfig.Auth.TLSConfig.KeyFile | |
| } |
🤖 Prompt for AI Agents
In `@internal/client_factory/maestro_client.go` around lines 13 - 39, Add a
nil-check at the start of CreateMaestroClient to validate maestroConfig (and
return a clear error) before any field access to avoid panics; specifically,
check if maestroConfig == nil and return an error like "nil maestroConfig
provided" from CreateMaestroClient, and also guard accesses to
maestroConfig.Auth (e.g., check maestroConfig.Auth != nil before reading
Auth.Type or TLSConfig) so clientConfig population and the TLS branch
(TLSConfig, CAFile, CertFile, KeyFile) never dereference a nil pointer.
| // Log the full manifest for debugging | ||
| if manifestYAML, marshalErr := yaml.Marshal(manifest.Object); marshalErr == nil { | ||
| re.log.Debugf(errCtx, "Resource[%s] failed manifest:\n%s", resource.Name, string(manifestYAML)) | ||
| } |
There was a problem hiding this comment.
Avoid logging full manifests that may include secrets.
Dumping full YAML on failure can leak Secret data; the same risk appears in the Maestro failure logs. Consider redaction or skipping for sensitive kinds.
🔒 Suggested guard (apply similarly in the Maestro failure path)
- if manifestYAML, marshalErr := yaml.Marshal(manifest.Object); marshalErr == nil {
- re.log.Debugf(errCtx, "Resource[%s] failed manifest:\n%s", resource.Name, string(manifestYAML))
- }
+ if manifest.GetKind() != "Secret" {
+ if manifestYAML, marshalErr := yaml.Marshal(manifest.Object); marshalErr == nil {
+ re.log.Debugf(errCtx, "Resource[%s] failed manifest:\n%s", resource.Name, string(manifestYAML))
+ }
+ } else {
+ re.log.Debugf(errCtx, "Resource[%s] failed manifest: <redacted Secret>", resource.Name)
+ }🤖 Prompt for AI Agents
In `@internal/executor/resource_executor.go` around lines 203 - 206, The failure
logging currently dumps the full manifest (manifest.Object) via re.log.Debugf
which can leak Secret data; update the ResourceExecutor failure path to either
skip logging for sensitive kinds (e.g., "Secret") or to redact sensitive fields
before rendering (mask common secret keys and data/strings) and then log the
sanitized YAML instead of the raw manifest, and apply the same change to the
Maestro failure logging codepath so both use the sanitized output rather than
dumping manifest.Object or raw manifests.
rh-amarin
force-pushed
the
resource-transport
branch
from
0d78626 to
851b84b
Compare
|
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. |
|
@rh-amarin: The following tests 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. |
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In `@charts/examples/kubernetes/README.md`:
- Around line 98-99: The sentence "In this example is overly permissive since is
creating deployments and jobs" is grammatically awkward; update it (in
charts/examples/kubernetes/README.md) to a clearer phrasing such as "This
example is overly permissive because it creates deployments and jobs." Replace
the existing sentence text wherever it appears (search for that exact phrase)
and ensure tense and subject are correct (use "This example" + "because it
creates deployments and jobs").
- Line 146: The helm example has a malformed placeholder in the flag --set
broker.googlepubsub.subscriptionId=<gcp-subscription? \; update the placeholder
to close the angle bracket and remove the stray question mark so it reads --set
broker.googlepubsub.subscriptionId=<gcp-subscription> \ (edit the README.md
example line containing broker.googlepubsub.subscriptionId).
In `@charts/examples/maestro/values.yaml`:
- Around line 40-50: The rbac block has inconsistent indentation: change the
"resources:" key under the rbac mapping (symbol "rbac" and its child
"resources") to use 2-space indentation (and align its list items) instead of
the current 3-space indent so the "rbac:" section matches the file's top-level
2-space indentation convention.
In `@internal/config_loader/loader_test.go`:
- Around line 97-98: The comment above the assertion in loader_test.go is
misleading; update or remove it so it reflects that metadata is taken from the
adapter config via Merge (it uses adapterCfg.Metadata in types.go). Replace
"Metadata comes from task config" with "Metadata comes from adapter config" (or
remove the comment) to match the actual behaviour asserted by
config.Metadata.Name.
In `@internal/executor/utils.go`:
- Around line 233-260: The code silently falls back to returning the raw
apiCallURL when url.Parse(baseURL) fails; update the function that builds the
API URL (the block using execCtx.Config.Spec.Clients.HyperfleetAPI.BaseURL and
calling url.Parse) to validate BaseURL and surface errors instead of returning
silently—either (a) return an error up the call chain when url.Parse(baseURL)
fails, or (b) log a clear error via the executor logger (include baseURL and
parse error) and avoid returning a potentially misrouted relative URL; ensure
the change touches the code paths that construct origin/basePath and the final
fmt.Sprintf call so callers receive a failure or see explicit logs when BaseURL
is malformed.
In `@internal/maestro_client/mock.go`:
- Around line 258-309: The ApplyResources method is appending res.Manifest
without checking for nil, which can cause buildManifestWork to panic; update
MockMaestroClient.ApplyResources to validate each resource (iterate resources
and check res.Manifest != nil) and either return a clear error (e.g., "resource
manifest at index X is nil") or skip nil entries before building manifests, then
proceed to call buildManifestWork and ApplyManifestWork as before; reference
symbols: ApplyResources, res.Manifest, buildManifestWork, ApplyManifestWork.
- Around line 144-158: Update MockMaestroClient.GetManifestWork so it returns a
Kubernetes NotFound error when no result is present (instead of returning nil,
nil) to match real client behavior checked by apierrors.IsNotFound(); use the
same input identifiers (consumerName, workName) in the error message to help
debugging and keep existing behavior where GetManifestWorkError is returned if
set and GetManifestWorkResult.DeepCopy() is returned when present; reference
MockMaestroClient, GetManifestWorkError, GetManifestWorkResult, and
workv1.ManifestWork when locating the method to change.
In `@internal/maestro_client/operations_test.go`:
- Around line 3-4: Update the top comment that currently reads "Tests for
manifest.ValidateGeneration, manifest.ValidateGenerationFromUnstructured, and
manifest.ValidateManifestWorkGeneration are in
internal/generation/generation_test.go" to point to the new location
"internal/manifest/generation_test.go"; locate the comment containing the phrase
"Tests for manifest.ValidateGeneration" in operations_test.go and change the
referenced path accordingly so it no longer points to the old file.
In `@internal/manifest/manifest.go`:
- Around line 11-23: ValidateManifest currently dereferences obj without
checking for nil; add an early nil guard at the top of ValidateManifest that
returns a descriptive error (e.g., "manifest is nil" or similar) when obj == nil
to avoid a panic, then proceed with existing checks; reference ValidateManifest
and ensure behavior remains same for GetGenerationFromUnstructured and
constants.AnnotationGeneration checks.
In `@pkg/utils/convert.go`:
- Around line 78-88: The float-to-int64 conversions in the switch (the float32
and float64 cases) and the string->float->int path must validate overflow:
before casting a float (v or parsed f) to int64 in the function handling these
type switches, check for NaN/Inf and that f is within the inclusive int64 range
(math.MinInt64 <= f <= math.MaxInt64); if out of range or not finite return a
clear error instead of directly casting. For float32, promote to float64 for the
check; for the string path validate the parsed float64 similarly before
converting to int64. Ensure error messages reference the offending value and the
target type so callers can diagnose the overflow.
In `@pkg/utils/path.go`:
- Around line 13-15: The code currently returns absolute paths directly (uses
filepath.IsAbs(refPath) and returns filepath.Clean(refPath)), which bypasses
containment checks; update the logic in the function that handles refPath so
absolute paths are either rejected (return an error) or validated to ensure they
reside within the intended base directory before returning; if accepting
absolute paths is intentional, add a clear comment on the trust model and
document allowed directories and/or perform an explicit containment check (e.g.,
compare cleaned absolute path against a cleaned baseDir) to enforce containment.
- Around line 29-31: The prefix check using strings.HasPrefix on absPath vs
absBase in pkg/utils/path.go can be bypassed by sibling paths (e.g.
"/app/configs" vs "/app/configs-backup"); replace that logic in the function
containing absPath/absBase/refPath/baseDir with a filepath.Rel-based check:
compute rel, handle err, and reject when rel starts with ".." (or equals ".."
segments) to ensure absPath is truly inside absBase before returning the
resolved path or the existing escape error.
🧹 Nitpick comments (8)
pkg/utils/template.go (1)
15-91: Protect the exported FuncMap from accidental mutation.Because
TemplateFuncsis exported and mutable, external packages can modify it, which can lead to unexpected behavior or data races in concurrent use. Consider documenting it as read‑only (or providing a copy helper) to discourage mutation.📝 Suggested doc tweak
-// TemplateFuncs provides common functions for Go templates. +// TemplateFuncs provides common functions for Go templates. +// NOTE: Treat this map as read-only; copy it before adding/removing funcs.pkg/utils/map.go (1)
52-78: Add a warning when the type assertion fallback happens.If
copiedisn’t amap[string]interface{}, you silently return a shallow copy. Logging here would make unexpected type mismatches visible.♻️ Suggested change
result, ok := copied.(map[string]interface{}) if !ok { + log.Warnf(ctx, "Deep copy returned unexpected type %T. Falling back to shallow copy.", copied) result := make(map[string]interface{}) for k, v := range m { result[k] = v } return result }internal/config_loader/struct_validator.go (1)
104-122: Redundant condition check after early return.After the early return at lines 104-106 when
!param.Required, the check at line 121 (!param.Required || !strings.HasPrefix(param.Source, "env.")) has a dead first condition—param.Requiredis alwaystrueat that point.♻️ Suggested simplification
- if !param.Required || !strings.HasPrefix(param.Source, "env.") { + if !strings.HasPrefix(param.Source, "env.") { return }internal/manifest/render.go (1)
39-48: Consider adding index context to slice rendering errors.When rendering fails for a slice element, the error doesn't indicate which index failed. This makes debugging nested structures harder.
💡 Proposed improvement
case []interface{}: result := make([]interface{}, len(val)) for i, item := range val { rendered, err := renderManifestValue(item, renderFn, params) if err != nil { - return nil, err + return nil, fmt.Errorf("index [%d]: %w", i, err) } result[i] = rendered } return result, nilinternal/k8s_client/apply.go (1)
105-131: Consider making poll interval configurable or documenting the choice.The 100ms poll interval is hardcoded. While this is reasonable for most cases, it could be aggressive for high-latency environments or when dealing with finalizers that take time to complete.
Consider extracting this as a constant at package level with documentation explaining the rationale, or making it configurable via the client options if flexibility is needed in the future.
charts/examples/maestro/adapter-task-config.yaml (2)
31-35: Consider adding timeout units documentation.The
timeout: 10sformat is correct, but for example documentation clarity, consider adding a comment showing the supported duration formats (e.g.,10s,1m,500ms).
64-65: StatictargetClustervalue - consider using a template variable.The
targetClusteris hardcoded ascluster1. For a production-ready example, this should typically be templated (e.g.,{{ .clusterName }}or from a parameter) to demonstrate dynamic cluster targeting.💡 Suggested improvement
maestro: - targetCluster: cluster1 + targetCluster: "{{ .clusterName }}"test/integration/executor/executor_maestro_integration_test.go (1)
71-76: Consider usingio.ReadAllfor complete body reading.The current implementation reads up to 1MB with a fixed buffer, which may truncate larger request bodies. While unlikely in tests, using
io.ReadAllwould be more robust.💡 Suggested improvement
+ "io" ... var bodyStr string if r.Body != nil { - buf := make([]byte, 1024*1024) - n, _ := r.Body.Read(buf) - bodyStr = string(buf[:n]) + bodyBytes, _ := io.ReadAll(r.Body) + bodyStr = string(bodyBytes) }
| In this example is overly permissive since is creating deployments and jobs | ||
|
|
There was a problem hiding this comment.
Minor grammatical issue.
The sentence reads awkwardly. Consider rephrasing.
📝 Suggested fix
-In this example is overly permissive since is creating deployments and jobs
+This example is overly permissive since it creates deployments and jobs📝 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.
| In this example is overly permissive since is creating deployments and jobs | |
| This example is overly permissive since it creates deployments and jobs |
🤖 Prompt for AI Agents
In `@charts/examples/kubernetes/README.md` around lines 98 - 99, The sentence "In
this example is overly permissive since is creating deployments and jobs" is
grammatically awkward; update it (in charts/examples/kubernetes/README.md) to a
clearer phrasing such as "This example is overly permissive because it creates
deployments and jobs." Replace the existing sentence text wherever it appears
(search for that exact phrase) and ensure tense and subject are correct (use
"This example" + "because it creates deployments and jobs").
| --namespace <namespace> \ | ||
| --set image.registry=quay.io/<developer-registry> \ | ||
| --set broker.googlepubsub.projectId=<gcp-project> \ | ||
| --set broker.googlepubsub.subscriptionId=<gcp-subscription? \ |
There was a problem hiding this comment.
Typo in helm command.
Missing closing angle bracket in the subscription placeholder.
📝 Suggested fix
- --set broker.googlepubsub.subscriptionId=<gcp-subscription? \
+ --set broker.googlepubsub.subscriptionId=<gcp-subscription> \📝 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.
| --set broker.googlepubsub.subscriptionId=<gcp-subscription? \ | |
| --set broker.googlepubsub.subscriptionId=<gcp-subscription> \ |
🤖 Prompt for AI Agents
In `@charts/examples/kubernetes/README.md` at line 146, The helm example has a
malformed placeholder in the flag --set
broker.googlepubsub.subscriptionId=<gcp-subscription? \; update the placeholder
to close the angle bracket and remove the stray question mark so it reads --set
broker.googlepubsub.subscriptionId=<gcp-subscription> \ (edit the README.md
example line containing broker.googlepubsub.subscriptionId).
| rbac: | ||
| resources: | ||
| - namespaces | ||
| - serviceaccounts | ||
| - configmaps | ||
| - deployments | ||
| - roles | ||
| - rolebindings | ||
| - jobs | ||
| - jobs/status | ||
| - pods |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the actual file content including exact indentation
cat -n charts/examples/maestro/values.yaml | head -60Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1488
🏁 Script executed:
# Search for manifestworks references in the codebase
rg -i "manifestwork" --type yaml -A 2 -B 2Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 504
🏁 Script executed:
# Check for RBAC templates and other references to understand patterns
fd -e yaml -e yml | xargs rg -l "rbac" -i | head -20Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 394
🏁 Script executed:
# Read the RBAC template to understand how resources are used
cat -n charts/templates/rbac.yaml | head -100Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1310
🏁 Script executed:
# Check the main values.yaml for RBAC configuration
cat -n charts/values.yaml | head -100Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 3757
🏁 Script executed:
# Search for manifestworks in context with RBAC or ClusterRole
rg -i "manifestwork|clusterrole" --type yaml -B 5 -A 5Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 2443
🏁 Script executed:
# Check what the apiGroup helper function returns
rg -n "apiGroup" charts/templates/ -A 3 -B 3Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1793
🏁 Script executed:
# Check if there's any _helpers.tpl or similar that defines apiGroup
cat -n charts/templates/_helpers.tpl 2>/dev/null || echo "No _helpers.tpl found"Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 9250
🏁 Script executed:
# Look at the main values.yaml to see full rbac section
rg -n "rbac:" charts/values.yaml -A 20Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 636
🏁 Script executed:
# Check kubernetes example values.yaml for comparison
cat -n charts/examples/kubernetes/values.yaml | grep -A 20 "^rbac:"Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 65
🏁 Script executed:
# Final verification: check if there are any other example values files with manifestworks in RBAC
rg -i "manifestwork" charts/examples/ --type yaml | grep -i rbac || echo "No manifestwork references in RBAC contexts"Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 126
🏁 Script executed:
# Confirm the exact spacing in maestro values file one more time
sed -n '40,42p' charts/examples/maestro/values.yaml | cat -ARepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 123
Fix rbac section indentation to use 2 spaces instead of 3.
Line 41 uses 3-space indentation for resources: while all other top-level sections in this file use 2-space indentation. This should be standardized to 2 spaces for consistency with Helm chart conventions.
🤖 Prompt for AI Agents
In `@charts/examples/maestro/values.yaml` around lines 40 - 50, The rbac block has
inconsistent indentation: change the "resources:" key under the rbac mapping
(symbol "rbac" and its child "resources") to use 2-space indentation (and align
its list items) instead of the current 3-space indent so the "rbac:" section
matches the file's top-level 2-space indentation convention.
| // Metadata comes from task config | ||
| assert.Equal(t, "test-adapter", config.Metadata.Name) | ||
| assert.Equal(t, "deployment-config", config.Metadata.Name) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n internal/config_loader/loader_test.go | head -100Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 3208
🏁 Script executed:
grep -n "task config\|adapter config\|test-adapter\|deployment-config" internal/config_loader/loader_test.go | head -20Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 886
🏁 Script executed:
fd -t go loader.go | head -5Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 252
🏁 Script executed:
fd loader loader.go | head -5Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 179
🏁 Script executed:
find . -name "loader.go" -type f | head -5Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 117
🏁 Script executed:
rg "func Merge" -t goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 190
🏁 Script executed:
rg "func Merge" -A 50 internal/config_loader/types.goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1995
Remove or correct the misleading comment on line 97.
The comment "Metadata comes from task config" contradicts the actual behavior. The Merge function uses adapterCfg.Metadata (line in types.go), so metadata comes from the adapter config. Update the comment to "Metadata comes from adapter config" or remove it.
🤖 Prompt for AI Agents
In `@internal/config_loader/loader_test.go` around lines 97 - 98, The comment
above the assertion in loader_test.go is misleading; update or remove it so it
reflects that metadata is taken from the adapter config via Merge (it uses
adapterCfg.Metadata in types.go). Replace "Metadata comes from task config" with
"Metadata comes from adapter config" (or remove the comment) to match the actual
behaviour asserted by config.Metadata.Name.
| // If the URL is already absolute, return as-is | ||
| parsed, err := url.Parse(apiCallURL) | ||
| if err == nil && parsed.IsAbs() { | ||
| return apiCallURL | ||
| } | ||
|
|
||
| baseURL := strings.TrimRight(execCtx.Config.Spec.Clients.HyperfleetAPI.BaseURL, "/") | ||
| baseURL := execCtx.Config.Spec.Clients.HyperfleetAPI.BaseURL | ||
| if baseURL == "" { | ||
| return apiCallURL | ||
| } | ||
|
|
||
| // Parse base URL to extract scheme+host, then build the path via string | ||
| // concatenation to preserve Go template expressions like {{ .clusterId }}. | ||
| base, err := url.Parse(baseURL) | ||
| if err != nil { | ||
| return apiCallURL | ||
| } | ||
|
|
||
| // Reconstruct origin (scheme + host) without path encoding | ||
| origin := base.Scheme + "://" + base.Host | ||
| basePath := strings.TrimRight(base.Path, "/") | ||
|
|
||
| relative := strings.TrimLeft(apiCallURL, "/") | ||
| if strings.HasPrefix(relative, "api/") { | ||
| return baseURL + "/" + relative | ||
| return origin + basePath + "/" + relative | ||
| } | ||
|
|
||
| return fmt.Sprintf("%s/api/hyperfleet/%s/%s", baseURL, execCtx.Config.Spec.Clients.HyperfleetAPI.Version, relative) | ||
| return fmt.Sprintf("%s%s/api/hyperfleet/%s/%s", origin, basePath, execCtx.Config.Spec.Clients.HyperfleetAPI.Version, relative) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "HyperfleetAPI|BaseURL" -g '*.go' -C 3Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 41475
🏁 Script executed:
rg -n "BaseURL|baseUrl|template" internal/config_loader/ -g '*.go' -C 2Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 20600
🏁 Script executed:
rg -n "renderTemplate|template expression" internal/executor/utils.go -B 5 -A 5Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 4399
🏁 Script executed:
rg -n "validate|Validate" internal/config_loader/validator.go | head -30Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1969
🏁 Script executed:
rg -n "HyperfleetAPI|BaseURL" internal/config_loader/validator.goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 65
🏁 Script executed:
rg -n "BaseURL|baseUrl" internal/hyperfleet_api/types.go -B 2 -A 2Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 695
🏁 Script executed:
sed -n '1028,1233p' internal/executor/utils_test.goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 5709
🏁 Script executed:
rg -n "invalid.*url|malformed.*url" internal/executor/utils_test.goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 65
🏁 Script executed:
rg -n "url.Parse" internal/executor/utils.goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 161
🏁 Script executed:
sed -n '233,261p' internal/executor/utils.goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 978
Add BaseURL validation or explicit error handling to prevent silent fallback.
When url.Parse(baseURL) fails at line 246, the function silently returns the raw relative URL (line 248), which could cause requests to be sent without the configured base URL prefix. Since BaseURL validation is not enforced during config loading, a malformed BaseURL could slip through undetected. Either validate BaseURL format during config initialization or add logging/error handling here to prevent silent misrouting.
🤖 Prompt for AI Agents
In `@internal/executor/utils.go` around lines 233 - 260, The code silently falls
back to returning the raw apiCallURL when url.Parse(baseURL) fails; update the
function that builds the API URL (the block using
execCtx.Config.Spec.Clients.HyperfleetAPI.BaseURL and calling url.Parse) to
validate BaseURL and surface errors instead of returning silently—either (a)
return an error up the call chain when url.Parse(baseURL) fails, or (b) log a
clear error via the executor logger (include baseURL and parse error) and avoid
returning a potentially misrouted relative URL; ensure the change touches the
code paths that construct origin/basePath and the final fmt.Sprintf call so
callers receive a failure or see explicit logs when BaseURL is malformed.
| // Note: Tests for manifest.ValidateGeneration, manifest.ValidateGenerationFromUnstructured, | ||
| // and manifest.ValidateManifestWorkGeneration are in internal/generation/generation_test.go. |
There was a problem hiding this comment.
Clarify the referenced test file path.
The comment still points to internal/generation/generation_test.go; if those manifest tests moved under internal/manifest/generation_test.go, update the path to avoid confusion.
✏️ Suggested update
-// and manifest.ValidateManifestWorkGeneration are in internal/generation/generation_test.go.
+// and manifest.ValidateManifestWorkGeneration are in internal/manifest/generation_test.go.📝 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.
| // Note: Tests for manifest.ValidateGeneration, manifest.ValidateGenerationFromUnstructured, | |
| // and manifest.ValidateManifestWorkGeneration are in internal/generation/generation_test.go. | |
| // Note: Tests for manifest.ValidateGeneration, manifest.ValidateGenerationFromUnstructured, | |
| // and manifest.ValidateManifestWorkGeneration are in internal/manifest/generation_test.go. |
🤖 Prompt for AI Agents
In `@internal/maestro_client/operations_test.go` around lines 3 - 4, Update the
top comment that currently reads "Tests for manifest.ValidateGeneration,
manifest.ValidateGenerationFromUnstructured, and
manifest.ValidateManifestWorkGeneration are in
internal/generation/generation_test.go" to point to the new location
"internal/manifest/generation_test.go"; locate the comment containing the phrase
"Tests for manifest.ValidateGeneration" in operations_test.go and change the
referenced path accordingly so it no longer points to the old file.
| func ValidateManifest(obj *unstructured.Unstructured) error { | ||
| if obj.GetAPIVersion() == "" { | ||
| return fmt.Errorf("manifest missing apiVersion") | ||
| } | ||
| if obj.GetKind() == "" { | ||
| return fmt.Errorf("manifest missing kind") | ||
| } | ||
| if obj.GetName() == "" { | ||
| return fmt.Errorf("manifest missing metadata.name") | ||
| } | ||
|
|
||
| if GetGenerationFromUnstructured(obj) == 0 { | ||
| return fmt.Errorf("manifest missing required annotation %q", constants.AnnotationGeneration) |
There was a problem hiding this comment.
Guard against nil manifests to avoid a panic.
ValidateManifest dereferences obj without a nil check; a nil input will panic. Add an early guard and return a validation error.
🛡️ Suggested fix
func ValidateManifest(obj *unstructured.Unstructured) error {
+ if obj == nil {
+ return fmt.Errorf("manifest is nil")
+ }
if obj.GetAPIVersion() == "" {
return fmt.Errorf("manifest missing apiVersion")
}📝 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.
| func ValidateManifest(obj *unstructured.Unstructured) error { | |
| if obj.GetAPIVersion() == "" { | |
| return fmt.Errorf("manifest missing apiVersion") | |
| } | |
| if obj.GetKind() == "" { | |
| return fmt.Errorf("manifest missing kind") | |
| } | |
| if obj.GetName() == "" { | |
| return fmt.Errorf("manifest missing metadata.name") | |
| } | |
| if GetGenerationFromUnstructured(obj) == 0 { | |
| return fmt.Errorf("manifest missing required annotation %q", constants.AnnotationGeneration) | |
| func ValidateManifest(obj *unstructured.Unstructured) error { | |
| if obj == nil { | |
| return fmt.Errorf("manifest is nil") | |
| } | |
| if obj.GetAPIVersion() == "" { | |
| return fmt.Errorf("manifest missing apiVersion") | |
| } | |
| if obj.GetKind() == "" { | |
| return fmt.Errorf("manifest missing kind") | |
| } | |
| if obj.GetName() == "" { | |
| return fmt.Errorf("manifest missing metadata.name") | |
| } | |
| if GetGenerationFromUnstructured(obj) == 0 { | |
| return fmt.Errorf("manifest missing required annotation %q", constants.AnnotationGeneration) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@internal/manifest/manifest.go` around lines 11 - 23, ValidateManifest
currently dereferences obj without checking for nil; add an early nil guard at
the top of ValidateManifest that returns a descriptive error (e.g., "manifest is
nil" or similar) when obj == nil to avoid a panic, then proceed with existing
checks; reference ValidateManifest and ensure behavior remains same for
GetGenerationFromUnstructured and constants.AnnotationGeneration checks.
| case float32: | ||
| return int64(v), nil | ||
| case float64: | ||
| return int64(v), nil | ||
| case string: | ||
| if i, err := strconv.ParseInt(v, 10, 64); err == nil { | ||
| return i, nil | ||
| } | ||
| if f, err := strconv.ParseFloat(v, 64); err == nil { | ||
| return int64(f), nil | ||
| } |
There was a problem hiding this comment.
Missing overflow check for float-to-int64 conversion.
Float values outside the int64 range (e.g., 1e20, math.MaxFloat64, or very large negative values) will produce undefined/platform-dependent results when cast to int64. The same issue exists for the string-to-float-to-int path on line 87.
🛡️ Proposed fix to add overflow validation
case float32:
- return int64(v), nil
+ if float64(v) > float64(math.MaxInt64) || float64(v) < float64(math.MinInt64) {
+ return 0, fmt.Errorf("float32 value %v overflows int64", v)
+ }
+ return int64(v), nil
case float64:
- return int64(v), nil
+ if v > float64(math.MaxInt64) || v < float64(math.MinInt64) {
+ return 0, fmt.Errorf("float64 value %v overflows int64", v)
+ }
+ return int64(v), nil
case string:
if i, err := strconv.ParseInt(v, 10, 64); err == nil {
return i, nil
}
if f, err := strconv.ParseFloat(v, 64); err == nil {
+ if f > float64(math.MaxInt64) || f < float64(math.MinInt64) {
+ return 0, fmt.Errorf("string '%s' as float overflows int64", v)
+ }
return int64(f), nil
}📝 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.
| case float32: | |
| return int64(v), nil | |
| case float64: | |
| return int64(v), nil | |
| case string: | |
| if i, err := strconv.ParseInt(v, 10, 64); err == nil { | |
| return i, nil | |
| } | |
| if f, err := strconv.ParseFloat(v, 64); err == nil { | |
| return int64(f), nil | |
| } | |
| case float32: | |
| if float64(v) > float64(math.MaxInt64) || float64(v) < float64(math.MinInt64) { | |
| return 0, fmt.Errorf("float32 value %v overflows int64", v) | |
| } | |
| return int64(v), nil | |
| case float64: | |
| if v > float64(math.MaxInt64) || v < float64(math.MinInt64) { | |
| return 0, fmt.Errorf("float64 value %v overflows int64", v) | |
| } | |
| return int64(v), nil | |
| case string: | |
| if i, err := strconv.ParseInt(v, 10, 64); err == nil { | |
| return i, nil | |
| } | |
| if f, err := strconv.ParseFloat(v, 64); err == nil { | |
| if f > float64(math.MaxInt64) || f < float64(math.MinInt64) { | |
| return 0, fmt.Errorf("string '%s' as float overflows int64", v) | |
| } | |
| return int64(f), nil | |
| } |
🤖 Prompt for AI Agents
In `@pkg/utils/convert.go` around lines 78 - 88, The float-to-int64 conversions in
the switch (the float32 and float64 cases) and the string->float->int path must
validate overflow: before casting a float (v or parsed f) to int64 in the
function handling these type switches, check for NaN/Inf and that f is within
the inclusive int64 range (math.MinInt64 <= f <= math.MaxInt64); if out of range
or not finite return a clear error instead of directly casting. For float32,
promote to float64 for the check; for the string path validate the parsed
float64 similarly before converting to int64. Ensure error messages reference
the offending value and the target type so callers can diagnose the overflow.
| if filepath.IsAbs(refPath) { | ||
| return filepath.Clean(refPath), nil | ||
| } |
There was a problem hiding this comment.
Absolute paths bypass containment check.
Absolute paths are returned directly without validation against the base directory. If refPath can be influenced by untrusted input, this allows arbitrary file access outside the intended base directory.
If this is intentional for operator-controlled configs, consider adding a comment explaining the trust model. Otherwise, absolute paths should either be rejected or validated to be within an allowed set of directories.
🤖 Prompt for AI Agents
In `@pkg/utils/path.go` around lines 13 - 15, The code currently returns absolute
paths directly (uses filepath.IsAbs(refPath) and returns
filepath.Clean(refPath)), which bypasses containment checks; update the logic in
the function that handles refPath so absolute paths are either rejected (return
an error) or validated to ensure they reside within the intended base directory
before returning; if accepting absolute paths is intentional, add a clear
comment on the trust model and document allowed directories and/or perform an
explicit containment check (e.g., compare cleaned absolute path against a
cleaned baseDir) to enforce containment.
| if !strings.HasPrefix(absPath, absBase) { | ||
| return "", fmt.Errorf("path %q escapes base directory %q", refPath, baseDir) | ||
| } |
There was a problem hiding this comment.
strings.HasPrefix may allow directory boundary bypass.
The check strings.HasPrefix(absPath, absBase) can pass for sibling directories with similar prefixes. For example, if absBase = "/app/configs" and absPath = "/app/configs-backup/secret.yaml", the check passes.
🛡️ Proposed fix using path separator boundary
- if !strings.HasPrefix(absPath, absBase) {
+ // Ensure absBase ends with separator for proper boundary check
+ if !strings.HasPrefix(absPath, absBase+string(filepath.Separator)) && absPath != absBase {
return "", fmt.Errorf("path %q escapes base directory %q", refPath, baseDir)
}Alternatively, use filepath.Rel and check that the result doesn't start with ..:
rel, err := filepath.Rel(absBase, absPath)
if err != nil || strings.HasPrefix(rel, "..") {
return "", fmt.Errorf("path %q escapes base directory %q", refPath, baseDir)
}📝 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.
| if !strings.HasPrefix(absPath, absBase) { | |
| return "", fmt.Errorf("path %q escapes base directory %q", refPath, baseDir) | |
| } | |
| // Ensure absBase ends with separator for proper boundary check | |
| if !strings.HasPrefix(absPath, absBase+string(filepath.Separator)) && absPath != absBase { | |
| return "", fmt.Errorf("path %q escapes base directory %q", refPath, baseDir) | |
| } |
🤖 Prompt for AI Agents
In `@pkg/utils/path.go` around lines 29 - 31, The prefix check using
strings.HasPrefix on absPath vs absBase in pkg/utils/path.go can be bypassed by
sibling paths (e.g. "/app/configs" vs "/app/configs-backup"); replace that logic
in the function containing absPath/absBase/refPath/baseDir with a
filepath.Rel-based check: compute rel, handle err, and reject when rel starts
with ".." (or equals ".." segments) to ensure absPath is truly inside absBase
before returning the resolved path or the existing escape error.
4 participants
Summary
kubernetes/maestroclient directlytransportconfiguration block supporting bothkubernetes(direct) andmaestro(ManifestWork-based) client typesKey Changes
Transport Layer
transport.clientfield:kubernetes(default) ormaestrotargetClusterandmanifestWork.namefieldsCleanup
/configsdirectory containing outdated templates and documentation/charts/examplesSummary by CodeRabbit
New Features
Documentation