show how to use opid to trace logs #460
Conversation
skeeey
commented
Jan 9, 2026
skeeey
commented
- support setting context opid key (both maestro server and sdk-go) from source client
- support using get/list/create/delete/path context to tracing operation
- unify the manifestwork example
WalkthroughIntroduce a dynamic X-Operation-ID-Key header and propagate per-request operation IDs via configurable context keys; add context.Context parameters to multiple service/store APIs and their call sites; change error handlers to accept *http.Request for header-based opID retrieval; update request logging to use klog context and explicit log levels; extend OpenAPI/client for X-Operation-ID-Key; add/remove examples and bump deps. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP as HTTP Server / Middleware
participant Handler
participant Service
participant DAO as Database/DAO
participant Logger
Client->>HTTP: HTTP request (may include X-Operation-ID-Key and X-Operation-ID)
HTTP->>Logger: resolve opIDKey (header or default) and set opID in context under that key
HTTP->>Handler: call handler with request (context contains opID)
Handler->>Service: call List(ctx, ...)
Service->>DAO: FindByConsumerName(ctx, ...)
DAO-->>Service: return resources
Service-->>Handler: return resources
Handler->>HTTP: write response (errors read opID from r.Header)
Handler->>Logger: emit logs via klog.FromContext(req.Context()) using dynamic opID key
HTTP-->>Client: HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
pkg/handlers/framework.go (1)
44-59: Bug:handle()bypassescfg.ErrorHandlerfor body read/unmarshal errors.If a handler supplies a custom
cfg.ErrorHandler, it will not be used on the “Unable to read request body” and “Invalid request format” paths (Lines 51, 57). That breaks the new abstraction.Proposed fix
func handle(w http.ResponseWriter, r *http.Request, cfg *handlerConfig, httpStatus int) { if cfg.ErrorHandler == nil { cfg.ErrorHandler = handleError } bytes, err := io.ReadAll(r.Body) if err != nil { - handleError(w, r, errors.MalformedRequest("Unable to read request body: %s", err)) + cfg.ErrorHandler(w, r, errors.MalformedRequest("Unable to read request body: %s", err)) return } err = json.Unmarshal(bytes, &cfg.MarshalInto) if err != nil { - handleError(w, r, errors.MalformedRequest("Invalid request format: %s", err)) + cfg.ErrorHandler(w, r, errors.MalformedRequest("Invalid request format: %s", err)) return }pkg/client/cloudevents/grpcsource/util.go (1)
24-256: Add synchronization to guard concurrent access tocontextTracingOPIDKey(data race risk).
contextTracingOPIDKeyis read without synchronization ingetOperationID()(line 269) and directly inpager.goandwatcherstore.go, while being written unsafely inSetContextTracingOPIDKey()(line 254). Since these operations occur during request handling, concurrent access is possible and creates a data race. Additionally, the function silently ignores empty keys instead of returning an error.Protect reads and writes with a mutex, and return an error for invalid input:
Proposed fix
package grpcsource import ( "context" "encoding/json" "fmt" "strings" + "sync" jsonpatch "github.com/evanphx/json-patch" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/selection" workv1 "open-cluster-management.io/api/work/v1" "open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/payload" sdkgologging "open-cluster-management.io/sdk-go/pkg/logging" "github.com/openshift-online/maestro/pkg/api/openapi" ) const jsonbPrefix = `payload->'metadata'->'labels'` -var contextTracingOPIDKey = sdkgologging.ContextTracingOPIDKey +var ( + contextTracingOPIDKeyMu sync.RWMutex + contextTracingOPIDKey = sdkgologging.ContextTracingOPIDKey +) // SetContextTracingOPIDKey customize the context tracing operation ID key // The default operation ID key is sdkgologging.ContextTracingOPIDKey ("op-id") -func SetContextTracingOPIDKey(key sdkgologging.ContextTracingKey) { +func SetContextTracingOPIDKey(key sdkgologging.ContextTracingKey) error { if len(key) == 0 { - return + return fmt.Errorf("context tracing op-id key must be non-empty") } - + contextTracingOPIDKeyMu.Lock() contextTracingOPIDKey = key + contextTracingOPIDKeyMu.Unlock() sdkgologging.DefaultContextTracingKeys = []sdkgologging.ContextTracingKey{key} + return nil } func getOperationID(ctx context.Context) string { + contextTracingOPIDKeyMu.RLock() + defer contextTracingOPIDKeyMu.RUnlock() if opID, ok := ctx.Value(contextTracingOPIDKey).(string); ok { return opID } return "" }pkg/client/cloudevents/grpcsource/watcherstore.go (1)
48-70: Initializectxfield in constructor to fix nil-context usage inSync()andprocess().The
ctxfield inRESTFulAPIWatcherStoreis never initialized innewRESTFulAPIWatcherStore(), causing nil-context panics and broken cancellation/trace propagation inSync()(lines 226, 236, 248) andprocess()(lines 270, 273, 280).Proposed fix
func newRESTFulAPIWatcherStore(ctx context.Context, logger logging.Logger, apiClient *openapi.APIClient, sourceID string) *RESTFulAPIWatcherStore { s := &RESTFulAPIWatcherStore{ + ctx: ctx, logger: logger, sourceID: sourceID, apiClient: apiClient, watchers: make(map[string]*workWatcher), workQueue: cache.NewFIFO(func(obj interface{}) (string, error) {pkg/logger/operationid_middleware.go (1)
23-56: Critical: GeneratedopIDis shadowed, rendering span/sentry/logging with empty operation ID.In the
elsebranch at line 38, the redeclarationopID, _ := ctx.Value(contextOPIDKey).(string)shadows the outeropIDvariable. After the else block ends, lines 44, 49, and 54 reference the outeropID, which remains an empty string in the else case, so span, sentry, and logging all receive an empty value instead of the generated operation ID.Fix
func OperationIDMiddleware(handler http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() contextOPIDKey := sdkgologging.ContextTracingOPIDKey @@ // Get operation ID from request header if existed opID := r.Header.Get(OpIDHeader) if opID != "" { // Add operationID to context (override if existed) ctx = context.WithValue(ctx, contextOPIDKey, opID) } else { // If no operationID from header, get it from context or generate a new one - ctx = withOpID(r.Context(), contextOPIDKey) - opID, _ := ctx.Value(contextOPIDKey).(string) - w.Header().Set(OpIDHeader, opID) + ctx = withOpID(ctx, contextOPIDKey) + opID, _ = ctx.Value(contextOPIDKey).(string) } + + // Echo back operation id (whether client-provided or generated) + if opID != "" { + w.Header().Set(OpIDHeader, opID) + } @@ - logger := klog.FromContext(ctx).WithValues(contextOPIDKey, opID) + logger := klog.FromContext(ctx).WithValues(string(contextOPIDKey), opID) ctx = klog.NewContext(ctx, logger) handler.ServeHTTP(w, r.WithContext(ctx)) }) }pkg/api/openapi/docs/DefaultAPI.md (1)
360-415: Fix markdownlint failures: hard tabs + malformed parameter table row forxOperationIDKey.
The row forxOperationIDKeyneeds 4 columns like the others, and tabs should be spaces (or adjust linting for generated docs).Proposed fix (table row)
- **xOperationIDKey** | **string** | | + **xOperationIDKey** | **string** | |Also: once
X-Operation-ID-Keyhas adescriptioninopenapi.yaml, regen docs so this section isn’t blank.Also applies to: 500-553
🤖 Fix all issues with AI agents
In @examples/manifestwork/client.go:
- Around line 363-383: In watchWorks, the event handling unconditionally asserts
event.Object to *workv1.ManifestWork which can panic for watch.Error events
(they often contain *metav1.Status); change the loop to switch on event.Type:
handle watch.Added/Modified/Deleted by type-asserting event.Object to
*workv1.ManifestWork before calling printWatchEvent (return/log a clear error if
the assertion fails), handle watch.Error by type-asserting to *metav1.Status and
logging/returning the status error, and ensure watcher.Stop() is called before
any return (including ctx.Done and channel closure) to properly close the watch;
apply the same type-safe handling and watcher.Stop() fix to the other watcher
loop referenced (the code that also calls printWatchEvent).
- Around line 165-181: The example currently sets TLSClientConfig with
InsecureSkipVerify: true; change this to avoid insecure defaults by gating it
behind an explicit boolean flag (e.g., insecureSkipVerify or
--insecure-skip-verify) and only set InsecureSkipVerify when that flag is true,
and also set a safer MinVersion (e.g., tls.VersionTLS12) on the TLSClientConfig;
update the HTTPClient Transport construction that feeds openapi.NewAPIClient
(the TLSClientConfig in the &http.Transport used for maestroAPIClient) to use
the flagged value and include MinVersion to enforce stronger TLS.
- Around line 226-256: The token read from grpcClientTokenFile is used verbatim
(assigned to grpcOptions.Dialer.Token) and may include a trailing newline which
can break auth; after reading token (the variable token in the block that checks
grpcServerCAFile and grpcClientTokenFile) trim whitespace/newlines (e.g.
strings.TrimSpace or bytes.TrimSpace) before converting to string and assigning
to grpcOptions.Dialer.Token so the value used by the Dialer is clean.
- Around line 101-147: The opCtx created with context.WithValue(ctx,
sdkgologging.ContextTracingOPIDKey, opId) is only passed to getWork, so
listWorks, applyWork, deleteWork and watchWorks lose the opid; update the calls
to pass opCtx (not ctx) to listWorks, applyWork, deleteWork and watchWorks so
the tracing operation ID is propagated while keeping cancellation inherited from
the original ctx (e.g., call listWorks(opCtx, workClient), applyWork(opCtx,
workClient, manifestworkFile), deleteWork(opCtx, workClient, workName),
watchWorks(opCtx, workClient)).
- Around line 321-343: The GET error for existingWork must be discriminated:
call workClient.ManifestWorks(...).Get and if apierrors.IsNotFound(err) then
proceed to Create with workClient.ManifestWorks(...).Create; if err != nil (and
not NotFound) return that error immediately (e.g., "failed to get work: %w");
otherwise (err == nil) continue to create the merge patch via
grpcsource.ToWorkPatch and Patch as before. Use apierrors.IsNotFound to detect
missing resources and avoid trying Create on transport/auth errors.
In @examples/manifestwork/README.md:
- Line 52: Several Markdown lines contain hard tab characters (notably the
pagination comment "Third page: returns remaining 500 works, Continue will be
empty" and the "Commands" / "Common Flags" sections), causing rendering
inconsistencies; open the README.md, locate those paragraphs and replace each
hard tab with spaces (convert tabs to two or four spaces per project style),
ensure indentation for the "Commands" and "Common Flags" lists uses spaces only,
and re-run markdownlint or your editor's "convert tabs to spaces" to verify no
tabs remain.
- Around line 180-184: The ClusterRole YAML has misaligned indentation for the
rules block and verbs; adjust the indentation so that the top-level keys
'metadata', 'name', and 'rules' are at the same level, and ensure the
'nonResourceURLs' and 'verbs' entries are nested under the same 'rules' item
(i.e., 'nonResourceURLs:' and 'verbs:' should be indented equally under the
specific rule), so that 'verbs' aligns with 'nonResourceURLs' rather than being
outdented.
In @pkg/handlers/errors.go:
- Line 73: The NotImplemented error message currently uses the string "path" in
the call to errors.NotImplemented inside the handleError invocation; update that
literal to "patch" so the message correctly references the HTTP PATCH method
(change errors.NotImplemented("path") to errors.NotImplemented("patch") where
handleError(w, r, errors.NotImplemented(...)) is invoked).
In @test/store.go:
- Around line 129-131: The MemoryStore.List method currently ignores the ctx
parameter; update it to either accept and propagate context to ListByNamespace
(e.g., change or add a ListByNamespaceWithContext(ctx, clusterName) call and
invoke it from MemoryStore.List) or explicitly document why ctx is unused for
interface compliance; specifically modify the MemoryStore.List implementation
(the List method on type MemoryStore) to pass ctx into a context-aware variant
of ListByNamespace or remove the unused parameter only if the interface is
adjusted—prefer adding a ListByNamespace(ctx context.Context, clusterName
string) and calling that from MemoryStore.List so context handling mirrors other
implementations like sqlResourceService.List.
🧹 Nitpick comments (11)
pkg/client/cloudevents/grpcsource/pager.go (1)
29-50: Compute op-id key once (avoid inconsistency if key is mutable).If
contextTracingOPIDKeyis configurable at runtime, capture it once (or expose a getter) before building headers sogetOperationID(ctx)andX-Operation-ID-Keystay consistent.pkg/auth/helpers.go (1)
14-32: Avoid split-brain context: preferr.Context()(and nil-guardr).
opIDis derived fromr, but logging derives from the separatectxparam; if these ever diverge, logs and cancellation/deadlines won’t match the request that produced the opID. Consider droppingctxand usingr.Context()(and/or guardingr == nilto avoid panics in edge paths/tests).pkg/handlers/framework.go (1)
33-42: Consider logging op-id inhandleErrorfor correlation (response already has it).You already pass
X-Operation-IDintoAsOpenapiError(...)(Line 41), but logs don’t include it. Adding"op-id", r.Header.Get(maestrologger.OpIDHeader)to both branches would make log⇄response correlation easier.pkg/logger/operationid_middleware.go (1)
23-28: Validate/sanitizeX-Operation-ID-Keybefore using it as a context key / log key / OTEL attribute key.
Allowing arbitrary header values can create high-cardinality attribute keys and may violatelogr/klogexpectations forWithValueskeys (should bestring).Also applies to: 44-45, 53-55
cmd/maestro/server/grpc_broker.go (1)
61-66: Usingcontext.Background()is acceptable here, but it drops cancellation/deadlines for List.
If the upstream interface ever exposes actx, prefer threading it through toresourceService.List(...).openapi/openapi.yaml (1)
50-57: Add a description (and optionally reuse viacomponents/parameters) forX-Operation-ID-Key.
This will make generated docs clearer about expected values/format and intended behavior.Also applies to: 96-103
pkg/api/openapi/api/openapi.yaml (1)
96-103: Adddescription(and optionally centralizeX-Operation-ID-Keyas a reusable component parameter).
Right now the parameter is undocumented (the generated docs show an empty description), and it’s duplicated across operations.Proposed fix (adds description)
- explode: false in: header name: X-Operation-ID-Key required: false + description: Context key name used for operation-id tracing/correlation. schema: type: string style: simpleAlso applies to: 195-201
cmd/maestro/server/logging/request_logging_middleware.go (1)
17-47: Middleware flow looks solid; consider avoiding magic verbosity numbers.
ThelogLevel := 2/logLevel = 4approach is fine, but constants would make intent clearer and reduce drift.examples/manifestwork/client.go (1)
287-305:listWorksprints the header for every row.
Move the header print outside the loop.examples/manifestwork/README.md (2)
77-77: Format bare URL properly.The URL on line 77 (in the
--maestro-serverflag description) should be formatted as code or enclosed in angle brackets for proper Markdown rendering.📝 Proposed fix
- --maestro-server string The maestro server address (default "http://127.0.0.1:30080") + --maestro-server string The maestro server address (default `http://127.0.0.1:30080`)Based on static analysis hints from markdownlint.
93-101: Fix list indentation.The nested list items in the "Important notes" section have inconsistent indentation (1 space instead of expected 0).
📝 Proposed fix
**Important notes about supported features:** -- **Deletion options**: Only `Foreground` and `Orphan` propagation policies are verified. Others may not work. -- **Update strategy**: `ServerSideApply`, `Update`, `CreateOnly`, and `ReadOnly` are verified. Others are not guaranteed. +- **Deletion options**: Only `Foreground` and `Orphan` propagation policies are verified. Others may not work. +- **Update strategy**: `ServerSideApply`, `Update`, `CreateOnly`, and `ReadOnly` are verified. Others are not guaranteed.Based on static analysis hints from markdownlint.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (2)
data/generated/openapi/openapi.gois excluded by!**/generated/**go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
cmd/maestro/main.gocmd/maestro/server/grpc_broker.gocmd/maestro/server/logging/request_logging_middleware.gocmd/maestro/server/logging/writer.goexamples/cloudevents/grpcclient.goexamples/manifestwork/README.mdexamples/manifestwork/client-a/main.goexamples/manifestwork/client-b/main.goexamples/manifestwork/client.goexamples/manifestwork/client/README.mdexamples/manifestwork/client/main.goexamples/manifestwork/manifestwork.jsongo.modopenapi/openapi.yamlpkg/api/openapi/.openapi-generator/VERSIONpkg/api/openapi/README.mdpkg/api/openapi/api/openapi.yamlpkg/api/openapi/api_default.gopkg/api/openapi/docs/DefaultAPI.mdpkg/auth/auth_middleware.gopkg/auth/helpers.gopkg/client/cloudevents/grpcsource/client.gopkg/client/cloudevents/grpcsource/pager.gopkg/client/cloudevents/grpcsource/util.gopkg/client/cloudevents/grpcsource/watcherstore.gopkg/db/transaction_middleware.gopkg/handlers/errors.gopkg/handlers/framework.gopkg/logger/operationid_middleware.gopkg/services/resource.gopkg/services/resource_test.gotest/e2e/pkg/suite_test.gotest/performance/pkg/hub/store/createonly.gotest/store.go
💤 Files with no reviewable changes (8)
- examples/manifestwork/client/main.go
- examples/manifestwork/client/README.md
- examples/cloudevents/grpcclient.go
- examples/manifestwork/client-b/main.go
- pkg/client/cloudevents/grpcsource/client.go
- cmd/maestro/main.go
- examples/manifestwork/client-a/main.go
- test/e2e/pkg/suite_test.go
🧰 Additional context used
🧬 Code graph analysis (7)
cmd/maestro/server/logging/request_logging_middleware.go (2)
cmd/maestro/server/logging/writer.go (1)
NewLoggingWriter(9-11)cmd/maestro/server/logging/formatter_json.go (1)
NewJSONLogFormatter(11-13)
pkg/db/transaction_middleware.go (1)
pkg/logger/operationid_middleware.go (1)
OpIDHeader(17-17)
test/performance/pkg/hub/store/createonly.go (2)
pkg/api/openapi/model_list.go (1)
List(23-28)pkg/api/resource_types.go (1)
ResourceList(29-29)
examples/manifestwork/client.go (2)
pkg/client/cloudevents/grpcsource/client.go (1)
NewMaestroGRPCSourceWorkClient(19-63)pkg/client/cloudevents/grpcsource/util.go (1)
ToWorkPatch(187-231)
pkg/auth/helpers.go (2)
pkg/errors/errors.go (2)
ServiceErrorCode(61-61)New(102-116)pkg/logger/operationid_middleware.go (1)
OpIDHeader(17-17)
pkg/client/cloudevents/grpcsource/watcherstore.go (3)
pkg/client/cloudevents/grpcsource/pager.go (1)
PageList(21-84)pkg/api/openapi/model_list.go (1)
List(23-28)pkg/api/resource_types.go (1)
ResourceList(29-29)
pkg/handlers/errors.go (1)
pkg/errors/errors.go (1)
NotImplemented(177-179)
🪛 ast-grep (0.40.4)
examples/manifestwork/client.go
[warning] 176-178: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 markdownlint-cli2 (0.18.1)
examples/manifestwork/README.md
52-52: Hard tabs
Column: 1
(MD010, no-hard-tabs)
66-66: Hard tabs
Column: 1
(MD010, no-hard-tabs)
69-69: Hard tabs
Column: 1
(MD010, no-hard-tabs)
70-70: Hard tabs
Column: 1
(MD010, no-hard-tabs)
71-71: Hard tabs
Column: 1
(MD010, no-hard-tabs)
77-77: Bare URL used
(MD034, no-bare-urls)
93-93: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
94-94: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
95-95: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
96-96: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
97-97: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
98-98: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
99-99: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
100-100: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
101-101: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
pkg/api/openapi/docs/DefaultAPI.md
382-382: Hard tabs
Column: 1
(MD010, no-hard-tabs)
383-383: Hard tabs
Column: 1
(MD010, no-hard-tabs)
385-385: Hard tabs
Column: 1
(MD010, no-hard-tabs)
386-386: Hard tabs
Column: 1
(MD010, no-hard-tabs)
387-387: Hard tabs
Column: 1
(MD010, no-hard-tabs)
413-413: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
413-413: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
520-520: Hard tabs
Column: 1
(MD010, no-hard-tabs)
521-521: Hard tabs
Column: 1
(MD010, no-hard-tabs)
523-523: Hard tabs
Column: 1
(MD010, no-hard-tabs)
524-524: Hard tabs
Column: 1
(MD010, no-hard-tabs)
525-525: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: upgrade
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e
- GitHub Check: e2e-with-istio
- GitHub Check: e2e-broadcast-subscription
- GitHub Check: verify
🔇 Additional comments (25)
go.mod (4)
51-55: Approve Kubernetes API consistency patch updates.The Kubernetes ecosystem dependencies (k8s.io/api, k8s.io/apimachinery, k8s.io/apiserver, k8s.io/client-go, k8s.io/component-base) are all consistently updated from v0.34.2 → v0.34.3. This consistent patch-level upgrade across the board indicates a coordinated maintenance release.
Also applies to: 51-55
58-60: The open-cluster-management.io package versions are correctly aligned with the opid/tracing context propagation objectives.The codebase actively uses these packages for operation ID and context tracing: the middleware in
pkg/logger/operationid_middleware.goextracts operation IDs from request headers, propagates them through context usingsdkgologging.ContextTracingOPIDKey, and integrates with tracing spans, Sentry, and logging. The example client inexamples/manifestwork/client.godemonstrates the same pattern. The recent pseudo-version timestamps indicate intentional, targeted updates to support this functionality.
45-45: gRPC v1.78.0 upgrade is safe; no breaking changes affect maestro.The minor version bump from v1.77.0 to v1.78.0 introduces no breaking changes. The only behavior change—rejecting unbracketed colons in hostnames on Go ≥1.26—is unlikely to impact maestro given its straightforward gRPC usage patterns (standard dial options and target address handling). The new features (OpenTelemetry metrics, experimental AcceptCompressors API) are additive and do not require code changes.
127-127: No action needed. Based on release notes and changelog data, mailru/easyjson v0.9.0 contains no documented breaking changes compared to v0.7.7. The changes include a Go version compatibility update and bug fixes in v0.9.0, and only an additive API change (making Lexer.CurrentToken public) in v0.8.0, which is backward compatible. The concern about breaking changes to JSON serialization behavior or code generation is not substantiated.pkg/api/openapi/.openapi-generator/VERSION (1)
1-1: Version bump is consistent.The version update aligns with the README.md change and reflects the OpenAPI generator upgrade.
pkg/handlers/errors.go (2)
69-69: LGTM - Error handler signature updated correctly.The handleError signature change from
(ctx, w, err)to(w, r, err)is appropriate, as the request already contains the context.
77-77: LGTM - Error handler signature updated correctly.The handleError signature change is consistent with the refactoring.
test/store.go (1)
4-4: Context import added for List signature update.The context package import supports the updated List method signature for interface compliance with the SDK.
pkg/auth/auth_middleware.go (1)
31-31: Line 31handleErrorcall is correctly consistent with its function signature.The call at line 31 correctly uses the
handleErrorfunction signature defined inpkg/auth/helpers.gowith parameters(ctx, w, r, code, reason). All other call sites across the codebase consistently match their respective function definitions. No inconsistency issues found.pkg/api/openapi/README.md (1)
10-10: OpenAPI Generator version 7.18.0 is valid and has been officially released.Version 7.18.0 was released on December 22, 2025 and is available in Maven Central repositories. The version bump is legitimate.
pkg/client/cloudevents/grpcsource/util.go (1)
268-274:getOperationIDlooks fine; avoid reading a concurrently-mutated key.Logic is straightforward, but it should read
contextTracingOPIDKeyvia the same synchronization/“init-only” policy as the setter to avoid races.test/performance/pkg/hub/store/createonly.go (1)
21-53: ConfirmGetshould not return “unsupported” (inconsistent with the other methods).
GetWatcher/List/ListAllreturnunsupported, butGetreturns(nil, false, nil)(Line 52). If callers interpret “not found” (found=false, err=nil) differently from “unsupported”, this could mask an unimplemented path.pkg/logger/operationid_middleware.go (2)
12-17: Header constants/exported names are clear and consistent.
Keeps the API surface straightforward for both server and clients.
60-72: Helper functions look fine; keying bysdkgologging.ContextTracingKeykeeps collisions manageable.
withOpIDis minimal and does the right “generate-once” behavior.pkg/client/cloudevents/grpcsource/watcherstore.go (4)
74-112: Ctx plumbing inGetWatcherlooks good (PageList + logging now cancel/trace-aware).
Nice improvement over relying on receiver state for request-scoped calls.
126-151: Good: propagate operation id headers on REST calls when present in ctx.
This aligns with the new server middleware behavior and should make client-server tracing much easier.Also applies to: 155-191
193-196:ListAll(ctx)explicitly “does nothing” is fine given the store’s intended usage.
Just ensure callers don’t rely on it for completeness.
288-302:registerWatcher(ctx, ...)change is a solid move—watchers now inherit the caller’s context.
This should help with structured logs and cancellations during watcher setup.pkg/db/transaction_middleware.go (1)
13-14: Good: error responses now include the request’sX-Operation-IDvia the shared constant.
This should make “fail-fast before middleware attaches ctx” cases traceable.Also applies to: 27-29
pkg/services/resource_test.go (1)
89-107: Tests correctly updated to the newList(context.Context, ListOptions)signature.
Keeps call sites consistent with the service contract.cmd/maestro/server/logging/writer.go (1)
32-39: Nice: parameterized verbosity for request/response logs.
Just ensure call sites don’t accidentally log noisy payloads at low verbosity levels.pkg/services/resource.go (1)
25-38: Context properly threaded through service and DAO layers. TheListmethod signature on the interface and all implementations (sqlResourceService, DAO, and mocks) correctly accept and passcontext.Context. DAO calls receive the context and use it for session creation, enabling proper cancellation and deadline propagation. All identified call sites pass context appropriately.examples/manifestwork/README.md (1)
1-294: Documentation updates look comprehensive.The README now provides clear guidance for both programmatic usage and CLI interactions, with excellent coverage of local development and production TLS scenarios. The pagination example and detailed command-line reference significantly improve usability.
pkg/api/openapi/api_default.go (2)
845-890: Generated code for X-Operation-ID-Key header support looks correct.The additions to
ApiApiMaestroV1ResourceBundlesGetRequestconsistently implement the newX-Operation-ID-Keyheader parameter:
- Field added to struct (line 853)
- Builder method follows existing patterns (lines 887-890)
- Header correctly added before X-Operation-ID in Execute (lines 976-978)
Also applies to: 976-978
1195-1206: Generated code for X-Operation-ID-Key header support looks correct.The additions to
ApiApiMaestroV1ResourceBundlesIdGetRequestmirror the implementation in the list endpoint, maintaining consistency:
- Field added to struct (line 1199)
- Builder method follows same pattern (lines 1203-1206)
- Header correctly added before X-Operation-ID in Execute (lines 1272-1274)
Also applies to: 1272-1274
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @examples/manifestwork/client.go:
- Around line 177-181: The TLS config in the HTTP client creation (the
tls.Config inside the http.Transport in the HTTPClient initialization) should
set a MinVersion to enforce modern TLS; update the tls.Config used in that block
to include MinVersion: tls.VersionTLS12 (or tls.VersionTLS13 if you can drop
legacy support) alongside the existing InsecureSkipVerify setting so the client
refuses older protocol versions.
- Around line 299-306: The header printf is inside the loop causing it to print
once per item; move the two header Printf calls so they run once before
iterating over workList.Items, then inside the for _, work := range
workList.Items loop only print the per-item line using consumerName, work.Name,
work.UID, and work.CreationTimestamp.Format(time.RFC3339); ensure the header
ordering and formatting exactly match the item row.
In @pkg/api/openapi/api/openapi.yaml:
- Around line 96-102: Add a descriptive "description" field for the header
parameter X-Operation-ID-Key (and also for X-Operation-ID where present) in the
OpenAPI spec so the GET /api/maestro/v1/resource-bundles and GET
/api/maestro/v1/resource-bundles/{id} parameter definitions include
documentation; update the parameter objects in pkg/api/openapi/api/openapi.yaml
(the header parameter block with name: X-Operation-ID-Key and the similar
X-Operation-ID block) to include a brief description such as "Client-supplied
operation identifier for request tracing and idempotency; opaque string used for
logging and debugging" so consumers understand its purpose and scope (note: keep
the header optional and only on the GET resource-bundles endpoints as currently
implemented).
In @pkg/api/openapi/docs/DefaultAPI.md:
- Line 413: The OpenAPI-generated docs show an empty description for
xOperationIDKey; update the OpenAPI source by adding a description to the
X-Operation-ID-Key parameter in openapi.yaml (the parameter definition named
X-Operation-ID-Key / xOperationIDKey) so the generator emits a non-empty
description for xOperationIDKey in DefaultAPI.md.
- Line 551: The docs row for the OpenAPI parameter named xOperationIDKey is
missing a description; open the OpenAPI spec where the parameter object for
xOperationIDKey is defined and add a descriptive "description" property
explaining its purpose/usage (mirror the wording used for the list endpoint's
parameter). After adding the description to the xOperationIDKey parameter in the
spec, regenerate the OpenAPI docs so DefaultAPI.md reflects the updated
description.
In @pkg/client/cloudevents/grpcsource/util.go:
- Line 24: The package-level variable contextTracingOPIDKey is mutated by
SetContextTracingOPIDKey and read by getOperationID concurrently, causing a data
race; replace the plain variable with an atomic.Value (e.g.,
contextTracingOPIDKeyVal) and use contextTracingOPIDKeyVal.Load() in
getOperationID and contextTracingOPIDKeyVal.Store(newKey) in
SetContextTracingOPIDKey, and update the other read/write sites referenced
(around the other occurrences) to use Load/Store so access is lock-free and
race-free.
- Around line 254-255: The code assigns contextTracingOPIDKey and then replaces
sdkgologging.DefaultContextTracingKeys with a single-element slice, which
mutates an external package global and can drop existing keys; change this to
avoid clobbering external state by either merging/appending the new key into the
existing sdkgologging.DefaultContextTracingKeys slice (preserving prior entries)
or stop mutating that global entirely and keep a local slice/variable for your
component; update the function/method documentation to explicitly state any side
effect if mutation is required and add a comment referencing
contextTracingOPIDKey and sdkgologging.DefaultContextTracingKeys so reviewers
understand the intent, and add a unit test or assertion to verify behavior
(merge vs no-mutation) depending on the chosen approach.
🧹 Nitpick comments (2)
pkg/client/cloudevents/grpcsource/watcherstore.go (1)
227-256: Consider using the provided context consistently.The
Syncmethod usesm.ctx(struct field) for logging and API calls (lines 227, 237), while other methods in this file use the provided context parameter. For consistency and to enable proper operation ID tracing in sync operations, consider accepting a context parameter and using it throughout.♻️ Suggested refactor
Update the method signature to accept context:
-func (m *RESTFulAPIWatcherStore) Sync() error { +func (m *RESTFulAPIWatcherStore) Sync(ctx context.Context) error { m.RLock() defer m.RUnlock() if len(m.watchers) == 0 { // there are no watchers, do nothing return nil } - m.logger.Info(m.ctx, "sync the works for current watchers (source=%s)", m.sourceID) + m.logger.Info(ctx, "sync the works for current watchers (source=%s)", m.sourceID) namespaces := []string{} for namespace := range m.watchers { namespaces = append(namespaces, namespace) } search := ToSyncSearch(m.sourceID, namespaces) // for sync, we need list all works with the search condition from maestro server - rbs, _, err := PageList(m.ctx, m.logger, m.apiClient, search, metav1.ListOptions{}) + rbs, _, err := PageList(ctx, m.logger, m.apiClient, search, metav1.ListOptions{}) if err != nil { return err } // save the works to a queue for _, rb := range rbs.Items { work, err := ToManifestWork(&rb) if err != nil { return err } - m.logger.Debug(m.ctx, "enqueue the work %s/%s (source=%s)", work.Namespace, work.Name, m.sourceID) + m.logger.Debug(ctx, "enqueue the work %s/%s (source=%s)", work.Namespace, work.Name, m.sourceID) if err := m.workQueue.Add(work); err != nil { return err } } return nil }examples/manifestwork/README.md (1)
217-301: Add language specifiers to fenced code blocks.Several output example code blocks are missing language specifiers, which reduces readability and prevents proper syntax highlighting.
📝 Suggested fix
Add
textorconsoleas the language identifier for output examples:-``` +```text Apply manifestwork (opid=3bd7d...) Work 77112687-f4ac-42d1-b8b8-6f999428c19d/nginx-work (uid=abc123...) created successfullyApply similar fixes to the code blocks at lines 223, 229, 262, and 295. </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Cache: Disabled due to data retention organization setting** **Knowledge base: Disabled due to `Reviews -> Disable Knowledge Base` setting** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 2b721c5dae6525a6ddbf5e6f82d82592606b15e0 and 31b3f62e788d8320d1c09c8da567043514214f60. </details> <details> <summary>⛔ Files ignored due to path filters (2)</summary> * `data/generated/openapi/openapi.go` is excluded by `!**/generated/**` * `go.sum` is excluded by `!**/*.sum` </details> <details> <summary>📒 Files selected for processing (34)</summary> * `cmd/maestro/main.go` * `cmd/maestro/server/grpc_broker.go` * `cmd/maestro/server/logging/request_logging_middleware.go` * `cmd/maestro/server/logging/writer.go` * `examples/cloudevents/grpcclient.go` * `examples/manifestwork/README.md` * `examples/manifestwork/client-a/main.go` * `examples/manifestwork/client-b/main.go` * `examples/manifestwork/client.go` * `examples/manifestwork/client/README.md` * `examples/manifestwork/client/main.go` * `examples/manifestwork/manifestwork.json` * `go.mod` * `openapi/openapi.yaml` * `pkg/api/openapi/.openapi-generator/VERSION` * `pkg/api/openapi/README.md` * `pkg/api/openapi/api/openapi.yaml` * `pkg/api/openapi/api_default.go` * `pkg/api/openapi/docs/DefaultAPI.md` * `pkg/auth/auth_middleware.go` * `pkg/auth/helpers.go` * `pkg/client/cloudevents/grpcsource/client.go` * `pkg/client/cloudevents/grpcsource/pager.go` * `pkg/client/cloudevents/grpcsource/util.go` * `pkg/client/cloudevents/grpcsource/watcherstore.go` * `pkg/db/transaction_middleware.go` * `pkg/handlers/errors.go` * `pkg/handlers/framework.go` * `pkg/logger/operationid_middleware.go` * `pkg/services/resource.go` * `pkg/services/resource_test.go` * `test/e2e/pkg/suite_test.go` * `test/performance/pkg/hub/store/createonly.go` * `test/store.go` </details> <details> <summary>💤 Files with no reviewable changes (8)</summary> * examples/manifestwork/client/README.md * pkg/client/cloudevents/grpcsource/client.go * examples/manifestwork/client-b/main.go * cmd/maestro/main.go * test/e2e/pkg/suite_test.go * examples/manifestwork/client/main.go * examples/manifestwork/client-a/main.go * examples/cloudevents/grpcclient.go </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (8)</summary> * pkg/api/openapi/README.md * pkg/auth/helpers.go * cmd/maestro/server/logging/writer.go * pkg/client/cloudevents/grpcsource/pager.go * openapi/openapi.yaml * pkg/logger/operationid_middleware.go * pkg/api/openapi/api_default.go * pkg/api/openapi/.openapi-generator/VERSION </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (6)</summary> <details> <summary>pkg/handlers/framework.go (2)</summary><blockquote> <details> <summary>pkg/errors/errors.go (2)</summary> * `ServiceError` (92-99) * `MalformedRequest` (189-191) </details> <details> <summary>pkg/logger/operationid_middleware.go (1)</summary> * `OpIDHeader` (17-17) </details> </blockquote></details> <details> <summary>pkg/services/resource.go (4)</summary><blockquote> <details> <summary>pkg/api/openapi/model_list.go (1)</summary> * `List` (23-28) </details> <details> <summary>pkg/db/migrations/202311151850_add_resources.go (1)</summary> * `Resource` (10-24) </details> <details> <summary>pkg/api/resource_types.go (1)</summary> * `Resource` (14-27) </details> <details> <summary>test/performance/pkg/util/util.go (1)</summary> * `ClusterName` (22-24) </details> </blockquote></details> <details> <summary>test/performance/pkg/hub/store/createonly.go (2)</summary><blockquote> <details> <summary>pkg/api/openapi/model_list.go (1)</summary> * `List` (23-28) </details> <details> <summary>pkg/api/resource_types.go (1)</summary> * `ResourceList` (29-29) </details> </blockquote></details> <details> <summary>pkg/services/resource_test.go (2)</summary><blockquote> <details> <summary>pkg/api/openapi/model_list.go (1)</summary> * `List` (23-28) </details> <details> <summary>test/performance/pkg/util/util.go (1)</summary> * `ClusterName` (22-24) </details> </blockquote></details> <details> <summary>pkg/client/cloudevents/grpcsource/watcherstore.go (1)</summary><blockquote> <details> <summary>pkg/client/cloudevents/grpcsource/pager.go (1)</summary> * `PageList` (21-84) </details> </blockquote></details> <details> <summary>pkg/handlers/errors.go (1)</summary><blockquote> <details> <summary>pkg/errors/errors.go (1)</summary> * `NotImplemented` (177-179) </details> </blockquote></details> </details><details> <summary>🪛 ast-grep (0.40.4)</summary> <details> <summary>examples/manifestwork/client.go</summary> [warning] 177-179: MinVersion` is missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. Add `MinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3. Context: tls.Config{ InsecureSkipVerify: *insecureSkipVerify, } Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES] https://owasp.org/Top10/A02_2021-Cryptographic_Failures (missing-ssl-minversion-go) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>examples/manifestwork/README.md</summary> 217-217: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 223-223: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 229-229: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 262-262: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 295-295: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>pkg/api/openapi/docs/DefaultAPI.md</summary> 382-382: Hard tabs Column: 1 (MD010, no-hard-tabs) --- 383-383: Hard tabs Column: 1 (MD010, no-hard-tabs) --- 385-385: Hard tabs Column: 1 (MD010, no-hard-tabs) --- 386-386: Hard tabs Column: 1 (MD010, no-hard-tabs) --- 387-387: Hard tabs Column: 1 (MD010, no-hard-tabs) --- 413-413: Table pipe style Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe (MD055, table-pipe-style) --- 413-413: Table column count Expected: 4; Actual: 3; Too few cells, row will be missing data (MD056, table-column-count) --- 520-520: Hard tabs Column: 1 (MD010, no-hard-tabs) --- 521-521: Hard tabs Column: 1 (MD010, no-hard-tabs) --- 523-523: Hard tabs Column: 1 (MD010, no-hard-tabs) --- 524-524: Hard tabs Column: 1 (MD010, no-hard-tabs) --- 525-525: Hard tabs Column: 1 (MD010, no-hard-tabs) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)</summary> * GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request * GitHub Check: Red Hat Konflux / maestro-on-pull-request * GitHub Check: e2e-grpc-broker * GitHub Check: e2e-broadcast-subscription * GitHub Check: e2e * GitHub Check: e2e-with-istio * GitHub Check: upgrade </details> <details> <summary>🔇 Additional comments (31)</summary><blockquote> <details> <summary>test/store.go (2)</summary><blockquote> `4-4`: **LGTM!** The context package import supports the updated `List` method signature. --- `129-131`: **LGTM!** The method signature is correctly updated to accept `context.Context` as the first parameter, aligning with the broader context propagation effort across the codebase. Using the blank identifier `_` appropriately signals that the context is intentionally unused in this test implementation. </blockquote></details> <details> <summary>pkg/services/resource.go (2)</summary><blockquote> `36-36`: **LGTM!** The interface signature correctly adds context propagation to the `List` method, enabling operation ID tracing as intended by this PR. --- `327-333`: **LGTM!** The implementation correctly propagates the provided context to `FindByConsumerName`, replacing the previous `context.TODO()` call. This enables proper operation ID tracing throughout the call chain. </blockquote></details> <details> <summary>pkg/client/cloudevents/grpcsource/watcherstore.go (5)</summary><blockquote> `75-113`: **LGTM!** The `GetWatcher` method signature is correctly updated to accept and propagate context throughout its implementation, including to `PageList`, logger calls, and `registerWatcher`. --- `127-152`: **LGTM!** The `Get` method correctly propagates context to the API client and logger. The operation ID handling at lines 131-134 properly uses `getOperationID(ctx)` and sets both `XOperationIDKey` and `XOperationID` headers when an operation ID is present. --- `156-192`: **LGTM!** The `List` method signature and implementation correctly propagate context to `PageList` and logger calls, consistent with the broader context propagation pattern. --- `194-196`: **LGTM!** The `ListAll` signature is correctly updated to accept context for interface compliance. --- `289-303`: **LGTM!** The `registerWatcher` method correctly accepts and propagates context to `newWorkWatcher` and logger calls. </blockquote></details> <details> <summary>pkg/services/resource_test.go (3)</summary><blockquote> `89-94`: **LGTM!** The test correctly updates the `List` call to pass `context.Background()` as the first argument, aligning with the updated interface signature. --- `96-100`: **LGTM!** The test call is correctly updated with the context parameter. --- `102-107`: **LGTM!** The test call is correctly updated with the context parameter. </blockquote></details> <details> <summary>cmd/maestro/server/grpc_broker.go (1)</summary><blockquote> `62-78`: **LGTM!** The `List` method correctly updates the call to `resourceService.List` by passing `context.Background()` as the first argument, aligning with the updated interface signature. </blockquote></details> <details> <summary>test/performance/pkg/hub/store/createonly.go (1)</summary><blockquote> `21-21`: **LGTM! Context propagation aligns with PR objectives.** The addition of `context.Context` parameters to `GetWatcher`, `List`, `ListAll`, and `Get` methods is consistent with the broader refactoring effort to support operation ID tracing across the codebase. Since this is a test stub implementation that returns unsupported errors or nil values, the unused context parameters are appropriate. Also applies to: 43-43, 47-47, 51-51 </blockquote></details> <details> <summary>pkg/client/cloudevents/grpcsource/util.go (2)</summary><blockquote> `4-4`: **LGTM!** The import additions are appropriate for the new tracing functionality. Also applies to: 17-17 --- `268-274`: **Function logic is sound.** The implementation correctly extracts the operation ID from context with safe type assertion. The empty string return for missing values is an appropriate sentinel value for this use case. Note: The concurrency concern regarding `contextTracingOPIDKey` access was addressed in a separate comment. </blockquote></details> <details> <summary>go.mod (3)</summary><blockquote> `51-60`: **Kubernetes and Open Cluster Management dependencies properly aligned for PR objectives.** All Kubernetes modules are consistently pinned to v0.34.3 (aligned patch bump from v0.34.2), and open-cluster-management.io modules are updated with newer pre-release snapshots. The go-openapi library updates (jsonpointer, jsonreference, swag) align with the PR objective to add X-Operation-ID-Key support in OpenAPI specs. --- `45-45`: **gRPC v1.78.0 upgrade is compatible; no API changes impact this codebase.** The v1.78.0 release introduces no breaking changes to streaming, interceptor, or connection handling APIs. The only behavior change is hostname validation with unbracketed colons on Go 1.26+, which only applies if the codebase runs on Go 1.26+ and uses malformed hostnames (not evident in current code). Standard gRPC patterns (Dial/NewClient, unary interceptors, MaxConcurrentStreams) remain fully compatible. OTEL/stats additions are backward-compatible. --- `127-127`: **mailru/easyjson v0.9.0 update is safe (no breaking API changes).** The v0.9.0 release contains only Go toolchain updates and bug fixes with no documented breaking changes. Regenerate code with the new version and run tests as standard practice. > Likely an incorrect or invalid review comment. </blockquote></details> <details> <summary>pkg/auth/auth_middleware.go (1)</summary><blockquote> `31-31`: **LGTM - Operation ID tracing enhancement** The addition of the HTTP request parameter to `handleError` enables request-context-aware error handling, allowing operation IDs to be extracted from request headers for tracing. </blockquote></details> <details> <summary>examples/manifestwork/client.go (2)</summary><blockquote> `54-70`: **Custom argument parsing is acceptable for this example.** The manual flag/command parsing is non-standard but works for this demonstration CLI. It allows flexible flag placement (flags can appear anywhere on the command line). --- `323-343`: **Get-then-patch pattern is acceptable for CLI usage.** While the get-then-patch/create pattern has a potential TOCTOU (time-of-check-to-time-of-use) race condition, this is acceptable for a command-line tool where concurrent modifications are unlikely. </blockquote></details> <details> <summary>pkg/db/transaction_middleware.go (1)</summary><blockquote> `27-27`: **LGTM - Aligned with header-based operation ID strategy** The change to retrieve the operation ID from the request header (`maestrologger.OpIDHeader`) instead of from logger context aligns with the PR's objective to support dynamic operation ID tracing via headers. </blockquote></details> <details> <summary>cmd/maestro/server/logging/request_logging_middleware.go (1)</summary><blockquote> `19-46`: **LGTM - Improved logging with explicit levels** The updates introduce explicit log levels (default level 2, healthcheck level 4) and align with the refactored logging writer API. Using `klog.FromContext` is the standard pattern for extracting loggers from request context, and the healthcheck path appropriately uses a higher (less verbose) log level to reduce log noise. </blockquote></details> <details> <summary>pkg/handlers/errors.go (1)</summary><blockquote> `68-78`: **LGTM! Error handler calls correctly updated to new signature.** The `handleError` calls are consistently updated to pass `(w, r, err)` matching the new signature in `framework.go`. The fix from `"path"` to `"patch"` on line 73 also corrects the typo in the NotImplemented error message. </blockquote></details> <details> <summary>pkg/api/openapi/api/openapi.yaml (1)</summary><blockquote> `195-201`: **Consistent with the list endpoint.** The `X-Operation-ID-Key` header addition here mirrors the list endpoint definition, maintaining consistency. </blockquote></details> <details> <summary>pkg/handlers/framework.go (3)</summary><blockquote> `30-42`: **LGTM! Clean refactoring to request-centric error handling.** The signature change to accept `*http.Request` instead of `context.Context` provides direct access to both the context (via `r.Context()`) and the request headers. The operation ID is now extracted from the header using `maestrologger.OpIDHeader`, aligning with the new header-based tracing approach. --- `44-77`: **LGTM! All error handler invocations consistently updated.** The `handle()` function correctly passes `(w, r, err)` to both `handleError` and `cfg.ErrorHandler` calls, maintaining consistency with the new signature. --- `80-127`: **LGTM! Remaining handler functions updated consistently.** `handleDelete`, `handleGet`, and `handleList` all correctly pass `(w, r, err)` to the error handlers. </blockquote></details> <details> <summary>pkg/api/openapi/docs/DefaultAPI.md (2)</summary><blockquote> `360-394`: **Documentation correctly reflects the new API parameter.** The example code and parameter documentation for `ApiMaestroV1ResourceBundlesGet` properly show the new `xOperationIDKey` parameter usage in the builder chain. --- `500-533`: **Documentation correctly reflects the new API parameter.** The example code and parameter documentation for `ApiMaestroV1ResourceBundlesIdGet` properly show the new `xOperationIDKey` parameter usage. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
examples/manifestwork/README.md (1)
217-301: Add languages to fenced “Example Output” blocks to satisfy markdownlint (MD040).Proposed markdown tweaks
-``` +```text Apply manifestwork (opid=3bd7d...) Work 77112687-f4ac-42d1-b8b8-6f999428c19d/nginx-work (uid=abc123...) created successfully-
+text
Apply manifestwork (opid=5ad7d...)
Work 77112687-f4ac-42d1-b8b8-6f999428c19d/nginx-work (uid=abc123...) updated successfully-``` +```text Watch manifestwork (opid=9ed7d...) [MODIFIED] Work: 77112687-f4ac-42d1-b8b8-6f999428c19d/nginx-work (uid=abc123...) { ... }-
+text
Get manifestwork ... (opid=7cd7d...)
{
...
}-``` +```text List manifestworks (opid=9dd7d...): Consumer Name UID Created ...</details> </blockquote></details> <details> <summary>pkg/handlers/framework.go (1)</summary><blockquote> `44-59`: **Major: `json.Unmarshal(bytes, &cfg.MarshalInto)` likely unmarshals into `*interface{}` instead of the intended target object.** Given the comment that `MarshalInto` is “a pointer to the object”, this should almost certainly be `json.Unmarshal(bytes, cfg.MarshalInto)`; also use `cfg.ErrorHandler` consistently (don’t bypass it on early errors). <details> <summary>Proposed fix</summary> ```diff func handle(w http.ResponseWriter, r *http.Request, cfg *handlerConfig, httpStatus int) { if cfg.ErrorHandler == nil { cfg.ErrorHandler = handleError } bytes, err := io.ReadAll(r.Body) if err != nil { - handleError(w, r, errors.MalformedRequest("Unable to read request body: %s", err)) + cfg.ErrorHandler(w, r, errors.MalformedRequest("Unable to read request body: %s", err)) return } - err = json.Unmarshal(bytes, &cfg.MarshalInto) + err = json.Unmarshal(bytes, cfg.MarshalInto) if err != nil { - handleError(w, r, errors.MalformedRequest("Invalid request format: %s", err)) + cfg.ErrorHandler(w, r, errors.MalformedRequest("Invalid request format: %s", err)) return }
🤖 Fix all issues with AI agents
In @examples/manifestwork/client.go:
- Around line 372-398: The watcher can send watch.Error events whose
event.Object is a *metav1.Status, so update watchWorks to check event.Type (or
attempt a type switch) before asserting to *workv1.ManifestWork: if event.Type
== watch.Error handle/log the *metav1.Status (or convert to error) instead of
treating it as a ManifestWork; otherwise proceed to assert to
*workv1.ManifestWork and call printWatchEvent. Ensure you reference the
watcher.ResultChan() loop and the printWatchEvent(event.Type, work) call when
making the change.
- Around line 313-323: In applyWork, after json.Unmarshal populates
manifestwork, validate that manifestwork.Name (metadata.name) is non-empty and
return a clear error if missing; locate the json.Unmarshal call in applyWork and
add a check like if strings.TrimSpace(manifestwork.Name) == "" { return
fmt.Errorf("manifestwork missing metadata.name: please set metadata.name in the
manifestwork file") } so callers get an actionable error before sending to the
server.
- Around line 48-75: The custom arg parsing in main incorrectly assumes any
non-`--flag=value` flag without "=" consumes the next token as its value,
causing boolean flags to swallow the following positional (e.g., `watch`);
update the parsing loop to check the flag definition via
flag.CommandLine.Lookup(flagName).IsBoolFlag (extract flagName from arg before
any "=" or leading dashes) and only consume the next arg when the flag exists
and IsBoolFlag is false, otherwise treat the next token as positional and keep
the current logic for non-flag tokens; ensure you use
flag.CommandLine.Parse(otherArgs) as before and preserve handling of `--flag
value` and `--flag=value` forms.
In @examples/manifestwork/README.md:
- Around line 9-24: The README snippet calls
grpcsource.NewMaestroGRPCSourceWorkClient without the required logger argument;
update the example to create or obtain a logger (e.g., named logger or log) and
pass it as the second parameter so the call matches
NewMaestroGRPCSourceWorkClient(ctx, logger, maestroAPIClient,
maestroGRPCOptions, sourceID); ensure the logger variable is declared earlier in
the snippet and used in that function invocation.
In @pkg/api/openapi/docs/DefaultAPI.md:
- Line 413: The OpenAPI docs show the parameter xOperationIDKey with no
description; update the OpenAPI source (e.g., openapi/openapi.yaml) to add a
clear description for the xOperationIDKey parameter such as: "The context key
name used to store the operation ID for distributed tracing. Must be provided
when using a custom operation ID key." Locate the parameter definition for
xOperationIDKey in the OpenAPI spec (the same key that generated
pkg/api/openapi/docs/DefaultAPI.md) and add the description field; apply the
same change for the other occurrence that generated the entry at line 551 so
both generated docs include the description.
In @pkg/auth/helpers.go:
- Around line 14-15: handleError is reading the operation ID from the HTTP
header but the OperationIDMiddleware injects it into the request context; change
handleError to extract the opID from ctx using the ContextTracingOPIDKey
(sdkgologging.ContextTracingOPIDKey) rather than r.Header.Get so the generated
ID from the middleware is always used for logging and error responses. Ensure
you perform a type assertion to string (e.g., opID, _ :=
ctx.Value(sdkgologging.ContextTracingOPIDKey).(string)) and fall back to an
empty string only if missing, leaving the rest of handleError logic unchanged.
🧹 Nitpick comments (6)
pkg/services/resource_test.go (1)
89-107: LGTM! Context parameter correctly added.The three
List()calls have been properly updated to passcontext.Background()as the first parameter, aligning with the new API signature for context-based operation tracing.Optional: Consider adding test coverage for operation ID tracing.
Since the PR introduces operation ID tracing through context, you might consider adding a test case that demonstrates this feature by:
- Creating a context with an operation ID
- Passing it to
List()- Verifying the operation ID is properly propagated (if testable at this layer)
This would serve as both documentation and validation of the new tracing capability, though it's not strictly necessary if covered by integration tests.
pkg/api/openapi/api/openapi.yaml (1)
96-109: Add a description (and ideally ref viacomponents.parameters) forX-Operation-ID-Key.
Right now the new header is duplicated and undocumented, which makes it harder for client users to understand when/why to set it.Proposed spec refactor
paths: /api/maestro/v1/resource-bundles: get: parameters: + - $ref: "#/components/parameters/XOperationIDKey" - explode: false in: header name: X-Operation-ID required: false schema: type: string style: simple /api/maestro/v1/resource-bundles/{id}: get: parameters: - description: The id of record explode: false in: path name: id required: true schema: type: string style: simple + - $ref: "#/components/parameters/XOperationIDKey" - explode: false in: header name: X-Operation-ID required: false schema: type: string style: simple components: parameters: + XOperationIDKey: + description: Overrides the request header name used to carry the operation ID for log tracing. + explode: false + in: header + name: X-Operation-ID-Key + required: false + schema: + type: string + style: simpleAlso applies to: 195-209
test/performance/pkg/hub/store/createonly.go (1)
21-23: Consider_ context.Contextfor the intentionally-unused ctx params in this stub store.
This avoids implying ctx is honored when it’s not (and matches the pattern used intest/store.go).Proposed tweak
-func (s *CreateOnlyWatcherStore) GetWatcher(ctx context.Context, namespace string, opts metav1.ListOptions) (watch.Interface, error) { +func (s *CreateOnlyWatcherStore) GetWatcher(_ context.Context, namespace string, opts metav1.ListOptions) (watch.Interface, error) { return nil, fmt.Errorf("unsupported") } -func (s *CreateOnlyWatcherStore) List(ctx context.Context, namespace string, opts metav1.ListOptions) (*store.ResourceList[*workv1.ManifestWork], error) { +func (s *CreateOnlyWatcherStore) List(_ context.Context, namespace string, opts metav1.ListOptions) (*store.ResourceList[*workv1.ManifestWork], error) { return nil, fmt.Errorf("unsupported") } -func (s *CreateOnlyWatcherStore) ListAll(ctx context.Context) ([]*workv1.ManifestWork, error) { +func (s *CreateOnlyWatcherStore) ListAll(_ context.Context) ([]*workv1.ManifestWork, error) { return nil, fmt.Errorf("unsupported") } -func (s *CreateOnlyWatcherStore) Get(ctx context.Context, namespace, name string) (*workv1.ManifestWork, bool, error) { +func (s *CreateOnlyWatcherStore) Get(_ context.Context, namespace, name string) (*workv1.ManifestWork, bool, error) { return nil, false, nil }Also applies to: 43-53
pkg/client/cloudevents/grpcsource/util.go (2)
253-256: Consider adding key format validation.While the empty check is good, consider validating that the key doesn't contain whitespace or other problematic characters that could cause issues when used as a context key or header value.
🔧 Optional enhancement
func SetContextTracingOPIDKey(key sdkgologging.ContextTracingKey) { - if len(key) == 0 { + if len(key) == 0 || strings.TrimSpace(string(key)) != string(key) { return }
251-265: Document the intent and impact of replacingDefaultContextTracingKeysat line 264.The comment "It should be fine to overwrite the ContextTracingOPIDKey here" reflects informal confidence. While the replacement appears intentional (the function name uses "Set"), this operation modifies global state in an external package without test coverage.
Consider clarifying the rationale in comments or documentation:
- Why is complete replacement preferred over appending to existing keys?
- Are there any dependencies in sdk-go on the original default keys that should be documented?
Adding a test case for this function would strengthen confidence in the intended behavior and prevent regressions.
pkg/api/openapi/docs/DefaultAPI.md (1)
382-383: Fix markdown formatting issues in the generator.The static analysis tool detected formatting issues including hard tabs and table structure problems. Since this is generated documentation, these issues should be addressed in the OpenAPI generator configuration or templates rather than in this file directly.
Based on static analysis hints.
Also applies to: 385-387, 413-413, 520-521, 523-525
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (2)
data/generated/openapi/openapi.gois excluded by!**/generated/**go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
cmd/maestro/main.gocmd/maestro/server/grpc_broker.gocmd/maestro/server/logging/request_logging_middleware.gocmd/maestro/server/logging/writer.goexamples/cloudevents/grpcclient.goexamples/manifestwork/README.mdexamples/manifestwork/client-a/main.goexamples/manifestwork/client-b/main.goexamples/manifestwork/client.goexamples/manifestwork/client/README.mdexamples/manifestwork/client/main.goexamples/manifestwork/manifestwork.jsongo.modopenapi/openapi.yamlpkg/api/openapi/.openapi-generator/VERSIONpkg/api/openapi/README.mdpkg/api/openapi/api/openapi.yamlpkg/api/openapi/api_default.gopkg/api/openapi/docs/DefaultAPI.mdpkg/auth/auth_middleware.gopkg/auth/helpers.gopkg/client/cloudevents/grpcsource/client.gopkg/client/cloudevents/grpcsource/pager.gopkg/client/cloudevents/grpcsource/util.gopkg/client/cloudevents/grpcsource/watcherstore.gopkg/db/transaction_middleware.gopkg/handlers/errors.gopkg/handlers/framework.gopkg/logger/operationid_middleware.gopkg/services/resource.gopkg/services/resource_test.gotest/e2e/pkg/suite_test.gotest/performance/pkg/hub/store/createonly.gotest/store.go
💤 Files with no reviewable changes (8)
- examples/manifestwork/client/main.go
- pkg/client/cloudevents/grpcsource/client.go
- examples/manifestwork/client/README.md
- test/e2e/pkg/suite_test.go
- examples/manifestwork/client-b/main.go
- cmd/maestro/main.go
- examples/manifestwork/client-a/main.go
- examples/cloudevents/grpcclient.go
✅ Files skipped from review due to trivial changes (2)
- pkg/api/openapi/.openapi-generator/VERSION
- pkg/api/openapi/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/maestro/server/grpc_broker.go
- pkg/db/transaction_middleware.go
- openapi/openapi.yaml
- pkg/services/resource.go
🧰 Additional context used
🧬 Code graph analysis (4)
pkg/auth/helpers.go (2)
pkg/errors/errors.go (2)
ServiceErrorCode(61-61)New(102-116)pkg/logger/operationid_middleware.go (1)
OpIDHeader(17-17)
examples/manifestwork/client.go (4)
pkg/api/openapi/client.go (1)
NewAPIClient(60-73)pkg/api/openapi/configuration.go (2)
Configuration(77-86)ServerConfigurations(74-74)pkg/client/cloudevents/grpcsource/client.go (1)
NewMaestroGRPCSourceWorkClient(19-63)pkg/client/cloudevents/grpcsource/util.go (1)
ToWorkPatch(191-235)
cmd/maestro/server/logging/request_logging_middleware.go (2)
cmd/maestro/server/logging/writer.go (1)
NewLoggingWriter(9-11)cmd/maestro/server/logging/formatter_json.go (1)
NewJSONLogFormatter(11-13)
pkg/client/cloudevents/grpcsource/watcherstore.go (1)
pkg/client/cloudevents/grpcsource/pager.go (1)
PageList(21-84)
🪛 markdownlint-cli2 (0.18.1)
examples/manifestwork/README.md
217-217: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
223-223: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
229-229: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
262-262: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
295-295: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
pkg/api/openapi/docs/DefaultAPI.md
382-382: Hard tabs
Column: 1
(MD010, no-hard-tabs)
383-383: Hard tabs
Column: 1
(MD010, no-hard-tabs)
385-385: Hard tabs
Column: 1
(MD010, no-hard-tabs)
386-386: Hard tabs
Column: 1
(MD010, no-hard-tabs)
387-387: Hard tabs
Column: 1
(MD010, no-hard-tabs)
413-413: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
413-413: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
520-520: Hard tabs
Column: 1
(MD010, no-hard-tabs)
521-521: Hard tabs
Column: 1
(MD010, no-hard-tabs)
523-523: Hard tabs
Column: 1
(MD010, no-hard-tabs)
524-524: Hard tabs
Column: 1
(MD010, no-hard-tabs)
525-525: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e
- GitHub Check: upgrade
- GitHub Check: e2e-broadcast-subscription
- GitHub Check: e2e-with-istio
🔇 Additional comments (24)
pkg/auth/auth_middleware.go (1)
31-31: LGTM! Consistent with updated error handler signature.The addition of the
rparameter aligns with the updatedhandleErrorsignature that now requires the HTTP request for operation ID extraction from headers.cmd/maestro/server/logging/writer.go (1)
32-39: LGTM! Dynamic log level support improves flexibility.The updated signature enables per-path log level control, allowing different verbosity for different endpoints (e.g., higher verbosity for healthcheck paths).
pkg/handlers/errors.go (1)
69-77: LGTM! Error handler calls updated consistently.All three stub methods now use the updated
handleError(w, r, ...)signature. Additionally, line 73 correctly fixes the typo from "path" to "patch" for the Patch method.cmd/maestro/server/logging/request_logging_middleware.go (2)
19-29: LGTM! Clear and effective log level control.The default log level of 2 with special handling for healthcheck (level 4) provides appropriate verbosity for different endpoints.
32-46: Simplified logging flow looks good.The removal of context augmentation and direct use of
klog.FromContextsimplifies the logging path. The explicitlogLevelparameter enables flexible verbosity control per endpoint.Operation ID propagation is maintained through the middleware chain:
OperationIDMiddleware(registered at line 38 ofcmd/maestro/server/routes.go) runs beforeRegisterLoggerMiddleware(line 41) and stores the operation ID in the request context, which the logging middleware retrieves viaklog.FromContext().test/store.go (1)
3-5: Context-accepting List signature looks fine;_ context.Contextclearly signals it’s intentionally unused.
No functional concerns here; this aligns with the repo-wide context propagation.Also applies to: 129-131
pkg/handlers/framework.go (1)
30-47: Request-based error handling + logging context propagation looks good.
Usingr.Context()for logging and pulling opid from the request header aligns well with the tracing goal.go.mod (1)
29-60: Dependency bumps are safe as-is; no breaking changes found in the specified versions.Verification results:
easyjson v0.9.0: No breaking changes—only Go version bump and bugfixes since v0.7.7.grpc v1.78.0: Contains a behavior change for Go 1.26+ (unbracketed colon rejection in hostnames), but this codebase targets Go 1.25.0, so it does not apply.k8s v0.34.3(client-go, apimachinery, api, etc.): Routine patch-level maintenance with no special compatibility concerns.General best practice remains: run integration tests and any codegen steps (e.g., protobuf generation if affected by grpc changes), though no new issues are anticipated from these bumps.
pkg/client/cloudevents/grpcsource/pager.go (2)
29-29: LGTM!The refactoring to use the local
getOperationID(ctx)helper function aligns with the new context-based operation ID retrieval pattern.
47-50: LGTM!The header ordering is correct—setting
XOperationIDKeybeforeXOperationIDensures the server can interpret the operation ID with the correct context key.pkg/client/cloudevents/grpcsource/util.go (2)
25-28: LGTM!The use of
sync.RWMutexproperly protects concurrent access to the package-levelcontextTracingOPIDKeyvariable.
277-287: LGTM!The read lock pattern is correct—the key is safely copied under lock, then used to retrieve the value from context. The type assertion and empty string fallback are appropriate.
pkg/logger/operationid_middleware.go (4)
16-17: LGTM!The header name constants follow HTTP header conventions and clearly distinguish between the operation ID key selector and the operation ID value.
23-28: LGTM!The fallback to the default
ContextTracingOPIDKeywhen no custom key header is provided ensures backward compatibility while supporting dynamic key selection.
30-40: LGTM!The conditional logic correctly handles both scenarios:
- When an operation ID is provided via header, it's stored in context under the dynamic key
- When not provided, a new KSUID is generated and the response header is set
42-44: Verify that lowercasing the operation ID key for OTEL attributes is intentional.Line 44 converts the operation ID key to lowercase before using it as an OTEL span attribute key:
strings.ToLower(string(contextOPIDKey)).While this can improve consistency in observability tooling, verify that:
- This aligns with your OTEL attribute naming conventions
- The lowercase key doesn't conflict with other attributes
- This transformation is documented so users know the attribute name won't match the header value exactly
pkg/api/openapi/api_default.go (3)
845-890: LGTM!The generated code correctly adds the
xOperationIDKeyfield and builder method to the request struct, following the same pattern as existing optional parameters.
976-981: LGTM!The header emission order is correct—
X-Operation-ID-Keyis set beforeX-Operation-ID, ensuring the server can interpret the operation ID using the correct context key.
1195-1277: LGTM!The IdGet endpoint changes mirror the Get endpoint implementation—struct updated, builder method added, and headers set in the correct order.
pkg/client/cloudevents/grpcsource/watcherstore.go (5)
75-113: LGTM! Context propagation implemented correctly.The method now accepts and properly propagates the context parameter to
PageList,registerWatcher, and logging calls, enabling operation ID tracing throughout the watch registration flow.
127-152: LGTM! Operation ID tracing correctly implemented.The method properly:
- Accepts context as the first parameter
- Extracts operation ID via
getOperationID(ctx)and sets bothX-Operation-ID-KeyandX-Operation-IDheaders when present- Propagates context to the API client and logging
This aligns with the PR's objective to support operation ID tracing.
156-192: LGTM! Context propagation is consistent.The method correctly threads the context parameter through
PageListand logging calls, maintaining consistency with other updated methods in this file.
218-256: Verify: Context usage in background sync operations.The
Sync()method continues to usem.ctxfor logging and API calls (lines 227, 237), while other methods now accept acontext.Contextparameter. This means background sync operations won't benefit from operation ID tracing.If
Syncis always called from background goroutines without an external request context (as suggested by line 68), this usage is likely intentional. However, please confirm whether operation ID tracing is desired for sync operations.
289-303: LGTM! Watcher registration now supports context tracing.The method correctly accepts and propagates the context parameter to
newWorkWatcherand logging, enabling operation ID tracing during watcher registration.
skeeey
force-pushed
the
ex
branch
2 times, most recently
from
c244218 to
424cbaa
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cmd/maestro/server/grpc_broker.go (1)
62-78: Update method signature to accept and propagate context.The
Listmethod creates a newcontext.Background()instead of accepting a context parameter from the caller. This breaks the context propagation chain and defeats the PR's objective of supporting operation ID tracing through context. Any operation ID or other context values from the caller are lost at this layer.🔧 Proposed fix to accept and propagate context
-func (s *GRPCBrokerService) List(listOpts types.ListOptions) ([]*ce.Event, error) { - resources, err := s.resourceService.List(context.Background(), listOpts) +func (s *GRPCBrokerService) List(ctx context.Context, listOpts types.ListOptions) ([]*ce.Event, error) { + resources, err := s.resourceService.List(ctx, listOpts) if err != nil { return nil, err }Note: You'll also need to update all callers of this method to pass a context parameter.
examples/manifestwork/README.md (1)
215-261: Fix markdownlint MD040 by adding languages to fenced code blocks.The “Example Output” blocks should specify a language (e.g.
textorconsole) to satisfy MD040.Also applies to: 262-293, 295-303
pkg/client/cloudevents/grpcsource/watcherstore.go (1)
289-303: Use store-lifetime context for watcher lifecycle instead of request context.The
ctxpassed toregisterWatcher()is stored directly innewWorkWatcher()and then reused for all logging calls throughout the watcher's lifetime (inStop(),Receive(), etc.). If callers pass a short-lived request context (from operations like Get/List), that context will be cancelled after the request completes, and subsequent logging calls will use a cancelled context. More problematically, any operation ID or trace ID from the initial request will be permanently bound to all future logs from this long-lived watcher. Usem.ctx(the store-lifetime context) for the watcher's lifecycle instead.
🤖 Fix all issues with AI agents
In @examples/manifestwork/client.go:
- Around line 405-423: The function printWatchEvent has a syntax error: the
fmt.Printf call inside the watch.Deleted case is missing its closing parenthesis
and trailing newline; update the Deleted case in printWatchEvent to close the
fmt.Printf call (add the missing ")") and include the "\n" at the end of the
format string so the line reads similarly to the Added/Modified cases, ensuring
the switch block compiles correctly.
- Around line 114-156: The default branch currently calls log.Fatalf which exits
immediately, so printUsage() is never reached; change the branch to first call
printUsage() and then exit (e.g., call printUsage() then log.Fatalf or
log.Printf followed by os.Exit(1)), or replace log.Fatalf with log.Printf then
call printUsage() and os.Exit(1) so the usage is printed for unknown commands;
modify the default branch around log.Fatalf, printUsage(), and os.Exit(1)
accordingly.
In @pkg/api/openapi/docs/DefaultAPI.md:
- Line 360: Add descriptive OpenAPI parameter descriptions for
X-Operation-ID-Key and X-Operation-ID so generated docs explain their purpose
and usage: update the OpenAPI spec entries (the parameters used by
ApiMaestroV1ResourceBundlesGet / ResourceBundleList and similar endpoints) to
include a "description" for X-Operation-ID-Key explaining it is the header name
used to group/route tracing or idempotency keys, when to provide it and allowed
formats, and a "description" for X-Operation-ID explaining it is the actual
operation identifier (UUID/string) value tied to that key, valid formats (e.g.,
UUID or custom string), and the relationship (X-Operation-ID is scoped by
X-Operation-ID-Key), and apply the same descriptions to the other occurrences
referenced in the docs so generated docs show helpful guidance.
In @pkg/auth/helpers.go:
- Around line 14-16: The handleError function accesses r.Header without a nil
check which can panic if a nil *http.Request is passed; update handleError to
first test if r == nil and, if so, set opID to a safe fallback (e.g., an empty
string or obtain opID from ctx if available) before calling r.Header.Get, and
apply the same nil-safe pattern for any other r.Header or r usages in the nearby
block (the code around lines 18-32) so all error-paths are defensive against a
nil request.
In @pkg/client/cloudevents/grpcsource/pager.go:
- Around line 47-50: The code reads the package-level contextTracingOPIDKey
without holding the mutex, causing a data race; change getOperationID to return
both the key and the operationID (e.g., func getOperationID(...) (string,
string, error) or similar) or add a small getter that acquires
contextTracingOPIDKeyMu and returns the key safely, then update pager.go to use
the returned key instead of referencing contextTracingOPIDKey directly in the
req.XOperationIDKey call (adjust call sites that currently call getOperationID
to accept the extra key return value and pass that to req.XOperationIDKey,
leaving SetContextTracingOPIDKey unchanged).
In @pkg/logger/operationid_middleware.go:
- Around line 22-28: The middleware currently accepts any X-Operation-ID-Key
header value and uses it to create a context key via
sdkgologging.ContextTracingKey(opIDKey), which risks context pollution and
unbounded key creation; validate the header before using it: add a helper (e.g.,
isValidContextKey) and only set contextOPIDKey from opIDKey when opIDKey passes
validation (whitelist allowed chars, enforce max length like 128, reject empty
or special chars); keep the default sdkgologging.ContextTracingOPIDKey when
validation fails and log or ignore invalid values to avoid creating arbitrary
context keys (references: OpIDKeyHeader, opIDKey variable,
sdkgologging.ContextTracingKey and sdkgologging.ContextTracingOPIDKey).
🧹 Nitpick comments (7)
pkg/handlers/framework.go (1)
44-78: Consider usingcfg.ErrorHandlerfor consistency.Lines 51 and 57 call
handleErrordirectly, while lines 64 and 73 usecfg.ErrorHandler. This means custom error handlers won't be invoked for request body read or JSON unmarshal errors, only for validation and action errors.If this is intentional (framework-level errors always use the default handler), consider adding a comment to clarify. Otherwise, consider using
cfg.ErrorHandlerconsistently:♻️ Proposed fix for consistency
bytes, err := io.ReadAll(r.Body) if err != nil { - handleError(w, r, errors.MalformedRequest("Unable to read request body: %s", err)) + cfg.ErrorHandler(w, r, errors.MalformedRequest("Unable to read request body: %s", err)) return } err = json.Unmarshal(bytes, &cfg.MarshalInto) if err != nil { - handleError(w, r, errors.MalformedRequest("Invalid request format: %s", err)) + cfg.ErrorHandler(w, r, errors.MalformedRequest("Invalid request format: %s", err)) return }pkg/client/cloudevents/grpcsource/util.go (1)
251-267: Consider usingsync.Onceto enforce single-initialization semantics.The function correctly protects
contextTracingOPIDKeywith a mutex, butsdkgologging.DefaultContextTracingKeysis modified without synchronization. While the comment appropriately documents the assumption that this function is called once at startup, the code currently relies on user discipline rather than enforcement. Adding async.Oncewrapper would make this intention explicit and prevent issues if the function is accidentally called multiple times from different goroutines during application initialization.pkg/logger/operationid_middleware.go (2)
30-40: Refactor to eliminate variable shadowing.Line 38 shadows the
opIDvariable declared on line 31, which reduces code clarity. Consider renaming one of the variables or restructuring the logic.♻️ Proposed refactoring
// Get operation ID from request header if existed opID := r.Header.Get(OpIDHeader) if opID != "" { // Add operationID to context (override if existed) ctx = context.WithValue(ctx, contextOPIDKey, opID) } else { // If no operationID from header, get it from context or generate a new one ctx = withOpID(r.Context(), contextOPIDKey) - opID, _ := ctx.Value(contextOPIDKey).(string) + opID = ctx.Value(contextOPIDKey).(string) w.Header().Set(OpIDHeader, opID) }This eliminates the shadowing by reusing the existing
opIDvariable instead of declaring a new one.
42-51: Consider consistency in operation ID key representation across observability systems.The operation ID key is lowercased for OTEL attributes (line 44) but uses original casing in context storage and logger. Additionally, Sentry uses the hardcoded string "operation_id". This inconsistency may complicate correlation across observability systems.
Is the lowercasing for OTEL intentional? If not, consider using consistent casing across all systems for easier debugging and correlation.
test/performance/pkg/hub/store/createonly.go (1)
21-23: Centralize “unsupported” + consider intentionally-ignored ctx to avoid lint churn.These methods all ignore
ctxand return the same generic error. Consider a sharedvar errUnsupported = errors.New("...")(or a package-level sentinel if you have one) and optionally name the param_ context.Contextif your linters flag unused parameters.Also applies to: 43-45, 47-49, 51-53
pkg/client/cloudevents/grpcsource/watcherstore.go (1)
218-256: Clarify/standardize when to usectxvsm.ctxin this type.
Sync()(andprocess()) still usem.ctx, while other methods now use the passed-inctx. That’s probably intentional (background vs per-operation), but it would help to document this contract (or renamem.ctxto something likebaseCtx) to avoid future reintroductions of mixed-context behavior in request paths.pkg/api/openapi/docs/DefaultAPI.md (1)
382-383: Fix markdown formatting in generator templates.Static analysis detected hard tabs in the code examples, which can cause inconsistent rendering across markdown viewers. Since this is generated documentation, the fix should be applied to the OpenAPI Generator templates or configuration to emit spaces instead of tabs.
Also applies to: 385-387, 520-521, 523-525
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (2)
data/generated/openapi/openapi.gois excluded by!**/generated/**go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
cmd/maestro/main.gocmd/maestro/server/grpc_broker.gocmd/maestro/server/logging/request_logging_middleware.gocmd/maestro/server/logging/writer.goexamples/cloudevents/grpcclient.goexamples/manifestwork/README.mdexamples/manifestwork/client-a/main.goexamples/manifestwork/client-b/main.goexamples/manifestwork/client.goexamples/manifestwork/client/README.mdexamples/manifestwork/client/main.goexamples/manifestwork/manifestwork.jsongo.modopenapi/openapi.yamlpkg/api/openapi/.openapi-generator/VERSIONpkg/api/openapi/README.mdpkg/api/openapi/api/openapi.yamlpkg/api/openapi/api_default.gopkg/api/openapi/docs/DefaultAPI.mdpkg/auth/auth_middleware.gopkg/auth/helpers.gopkg/client/cloudevents/grpcsource/client.gopkg/client/cloudevents/grpcsource/pager.gopkg/client/cloudevents/grpcsource/util.gopkg/client/cloudevents/grpcsource/watcherstore.gopkg/db/transaction_middleware.gopkg/handlers/errors.gopkg/handlers/framework.gopkg/logger/operationid_middleware.gopkg/services/resource.gopkg/services/resource_test.gotest/e2e/pkg/suite_test.gotest/performance/pkg/hub/store/createonly.gotest/store.go
💤 Files with no reviewable changes (8)
- examples/manifestwork/client/README.md
- examples/manifestwork/client-a/main.go
- examples/cloudevents/grpcclient.go
- test/e2e/pkg/suite_test.go
- examples/manifestwork/client/main.go
- examples/manifestwork/client-b/main.go
- pkg/client/cloudevents/grpcsource/client.go
- cmd/maestro/main.go
✅ Files skipped from review due to trivial changes (1)
- pkg/api/openapi/README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- openapi/openapi.yaml
- pkg/api/openapi/api/openapi.yaml
- pkg/handlers/errors.go
- pkg/services/resource_test.go
- pkg/api/openapi/.openapi-generator/VERSION
🧰 Additional context used
🧬 Code graph analysis (6)
cmd/maestro/server/logging/request_logging_middleware.go (2)
cmd/maestro/server/logging/writer.go (1)
NewLoggingWriter(9-11)cmd/maestro/server/logging/formatter_json.go (1)
NewJSONLogFormatter(11-13)
pkg/handlers/framework.go (2)
pkg/errors/errors.go (2)
ServiceError(92-99)MalformedRequest(189-191)pkg/logger/operationid_middleware.go (1)
OpIDHeader(17-17)
pkg/auth/helpers.go (2)
pkg/errors/errors.go (2)
ServiceErrorCode(61-61)New(102-116)pkg/logger/operationid_middleware.go (1)
OpIDHeader(17-17)
examples/manifestwork/client.go (1)
pkg/client/cloudevents/grpcsource/util.go (1)
ToWorkPatch(191-235)
pkg/client/cloudevents/grpcsource/watcherstore.go (1)
pkg/client/cloudevents/grpcsource/pager.go (1)
PageList(21-84)
pkg/auth/auth_middleware.go (1)
pkg/errors/errors.go (1)
ErrorUnauthorized(43-43)
🪛 markdownlint-cli2 (0.18.1)
pkg/api/openapi/docs/DefaultAPI.md
382-382: Hard tabs
Column: 1
(MD010, no-hard-tabs)
383-383: Hard tabs
Column: 1
(MD010, no-hard-tabs)
385-385: Hard tabs
Column: 1
(MD010, no-hard-tabs)
386-386: Hard tabs
Column: 1
(MD010, no-hard-tabs)
387-387: Hard tabs
Column: 1
(MD010, no-hard-tabs)
413-413: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
413-413: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
520-520: Hard tabs
Column: 1
(MD010, no-hard-tabs)
521-521: Hard tabs
Column: 1
(MD010, no-hard-tabs)
523-523: Hard tabs
Column: 1
(MD010, no-hard-tabs)
524-524: Hard tabs
Column: 1
(MD010, no-hard-tabs)
525-525: Hard tabs
Column: 1
(MD010, no-hard-tabs)
examples/manifestwork/README.md
218-218: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
224-224: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
230-230: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
263-263: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
296-296: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: upgrade
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e-broadcast-subscription
- GitHub Check: e2e-with-istio
- GitHub Check: e2e
🔇 Additional comments (26)
test/store.go (1)
129-131: LGTM!The context parameter is properly ignored with the blank identifier for this test helper. The implementation correctly delegates to
ListByNamespacewhile maintaining the updated interface signature.pkg/services/resource.go (2)
36-36: LGTM!The interface signature update to include
context.Contextis correct and follows Go best practices. This enables proper context propagation for operation ID tracing as mentioned in the PR objectives.
327-333: LGTM!The implementation correctly propagates the context to the DAO layer. This improvement replaces the previous
context.TODO()usage and enables proper context propagation for operation ID tracing.pkg/handlers/framework.go (4)
11-11: LGTM!The import is correctly added to support retrieving the operation ID header constant.
30-30: Improved signature provides access to both context and headers.The signature change from
context.Contextto*http.Requestis a breaking change for custom error handlers, but provides better access to both context and headers for operation ID tracing, aligning with the PR objectives.
33-42: LGTM! Operation ID correctly sourced from request header.The function now derives the logger from the request context and retrieves the operation ID from the request header, enabling client-driven operation tracing as intended by the PR objectives.
80-128: LGTM! Error handler calls correctly updated.All error handler invocations in
handleDelete,handleGet, andhandleListconsistently usecfg.ErrorHandlerwith the updated signature, properly passing the request object for operation ID tracing.cmd/maestro/server/logging/writer.go (1)
32-39: LGTM! Dynamic log level control implemented correctly.The signature change allows callers to specify the log level dynamically, which aligns with the PR objectives for context-aware logging. The implementation correctly applies the provided
logLevelparameter for info logs while preserving error logging behavior.cmd/maestro/server/logging/request_logging_middleware.go (2)
19-29: LGTM! Appropriate log level differentiation for healthcheck.The default log level of 2 for normal requests and elevated level of 4 for healthcheck endpoints provides good control over log verbosity, helping reduce log spam from frequent healthcheck calls while maintaining visibility for regular operations.
36-37: LGTM! Logging calls correctly updated to use explicit log levels.The request and response logging calls properly utilize the new
logsignature with the explicitlogLevelparameter. The middleware correctly passes the original request to the next handler, preserving the request context throughout the chain.Also applies to: 41-41, 45-46
pkg/client/cloudevents/grpcsource/util.go (2)
25-28: LGTM! Proper mutex protection for package-level state.The RWMutex is correctly used to protect concurrent access to
contextTracingOPIDKey, allowing multiple readers while serializing writes.
279-289: LGTM! Thread-safe read with proper lock semantics.The function correctly:
- Acquires RLock before reading the shared key
- Copies the key to a local variable before releasing the lock
- Uses the local copy for the context lookup
pkg/client/cloudevents/grpcsource/pager.go (1)
29-29: Good use of the centralized helper function.Using
getOperationID(ctx)instead of directly accessing the logger ensures consistent operation ID extraction with proper synchronization.pkg/db/transaction_middleware.go (1)
13-14: Header-based opID propagation looks consistent (but consider a safe fallback when missing).Reading the operation ID from
r.Header.Get(maestrologger.OpIDHeader)aligns with the rest of the PR’s “header is source of truth” approach; just confirmAsOpenapiError("")behaves acceptably when the header is absent (e.g., omits opid vs. emitting empty string).Also applies to: 27-29
pkg/auth/auth_middleware.go (1)
27-33: UpdatedhandleError(..., r, ...)callsite is the right wiring.This matches the new helper signature and enables opID extraction from request headers during auth failures.
pkg/logger/operationid_middleware.go (5)
12-13: LGTM: Imports support dynamic operation ID key functionality.The additions of klog/v2 and sdk-go logging are appropriate for the enhanced tracing capabilities.
16-17: LGTM: Header constants are well-defined.The constants follow Go naming conventions and provide clear semantic meaning.
53-56: LGTM: klog integration follows best practices.The structured logging approach correctly propagates the operation ID to all downstream log messages.
60-67: LGTM: withOpID correctly handles dynamic key.The function properly checks for existing values and generates unique KSUIDs when needed. The signature change supports the dynamic key functionality well.
69-72: LGTM: operationIDAttribute correctly accepts dynamic key.The updated signature properly supports the dynamic key functionality introduced in this PR.
pkg/client/cloudevents/grpcsource/watcherstore.go (1)
75-113: Good: ctx is now consistently threaded through REST calls + per-op logging.Passing
ctxintoPageList(...),ApiMaestroV1ResourceBundlesIdGet(ctx, ...), andm.logger.*(ctx, ...)is the right direction for opid correlation and cancellation behavior.Also applies to: 126-152, 154-192
go.mod (1)
1-62: Dependency versions verified as valid and secure.All listed dependency versions exist and are free from critical security vulnerabilities. The Kubernetes library versions at v0.34.3 are valid (corresponding to Kubernetes 1.34.3), the grpc v1.78.0 bump includes no active CVEs, and the open-cluster-management.io pseudo-versions correctly reference specific commits. No action required.
pkg/api/openapi/api_default.go (4)
845-855: LGTM: Struct field addition follows correct pattern.The
xOperationIDKeyfield is properly added to the request struct using the same pattern as the existingxOperationIDfield (pointer type for optional parameter).
887-890: LGTM: Builder methods correctly implemented.Both
XOperationIDKeybuilder methods follow the standard fluent interface pattern used throughout this generated client.Also applies to: 1203-1206
976-981: LGTM: Header emission order is logical.Setting
X-Operation-ID-KeybeforeX-Operation-IDmakes sense, as the key defines the semantic context for the operation ID value.Also applies to: 1272-1277
845-1356: OpenAPI specification only defines X-Operation-ID-Key for two endpoints; implementation is correct.The review comment is accurate: the generated code implements
xOperationIDKeysupport only for two endpoints because the OpenAPI specification (openapi/openapi.yaml) only defines theX-Operation-ID-Keyparameter for those two operations:
- GET
/api/maestro/v1/resource-bundles- GET
/api/maestro/v1/resource-bundles/{id}The other endpoints listed (Consumers GET, POST, PATCH, DELETE and ResourceBundles DELETE) do not have
X-Operation-ID-Keydefined in the OpenAPI spec, so the code correctly excludes them.The gap identified is legitimate: if the PR intent is to "support using get/list/create/delete/patch context for tracing operations" as mentioned in the PR description, then the OpenAPI specification itself is incomplete and needs to be updated to include
X-Operation-ID-Keyparameters on the remaining endpoints before code generation.
skeeey
force-pushed
the
ex
branch
2 times, most recently
from
8244c46 to
be843e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @examples/manifestwork/client.go:
- Around line 321-362: The manifestwork's Namespace may differ from the
--consumer-name used when calling workClient.ManifestWorks(*consumerName),
causing server validation errors; in applyWork, after unmarshalling into
manifestwork (variable manifestwork in function applyWork) explicitly set
manifestwork.Namespace = *consumerName (or clear it if you prefer empty) before
any Create or Patch calls (workClient.ManifestWorks(*consumerName).Create /
.Patch) and before generating the patch (grpcsource.ToWorkPatch) so the object
and patch target the same namespace.
In @examples/manifestwork/README.md:
- Around line 218-222: The README contains several fenced output blocks missing
a language tag (e.g., the blocks starting with "Apply manifestwork (opid=...)",
"Watch manifestwork (opid=...)", "Get manifestwork ...", and "List manifestworks
(opid=...)"); to fix markdownlint MD040, change each opening fence from ``` to
```text so all shown examples are marked as plain text—update every occurrence
mentioned (around the snippets beginning with those lines) accordingly.
In @pkg/client/cloudevents/grpcsource/util.go:
- Around line 251-267: SetContextTracingOPIDKey is unsafely overwriting external
global sdkgologging.DefaultContextTracingKeys; instead avoid replacing global
state: use a sync.Once guard to ensure the operation runs only once and append
the new key (while checking for duplicates) to
sdkgologging.DefaultContextTracingKeys rather than assigning a single-element
slice, or remove the external modification entirely and keep the custom key
local to this package; update SetContextTracingOPIDKey to perform one-time,
thread-safe append (or stop touching sdkgologging.DefaultContextTracingKeys) and
preserve any existing keys.
In @pkg/logger/operationid_middleware.go:
- Around line 23-28: The code uses sdkgologging.ContextTracingKey from the
X-Operation-ID-Key header without validation and passes it directly to
klog.FromContext().WithValues(), which expects string keys; update the handler
to validate the header (OpIDKeyHeader) against a safe pattern (e.g.
[a-zA-Z0-9_.-]{1,64}) using a helper like isSafeOpIDKey and only set
contextOPIDKey = sdkgologging.ContextTracingKey(opIDKey) when valid, otherwise
keep the default; when calling klog.FromContext().WithValues() cast the key to
string (string(contextOPIDKey)) so the log key is a string and avoid unbounded
cardinality by rejecting or falling back on malformed/too-long header values.
🧹 Nitpick comments (9)
pkg/client/cloudevents/grpcsource/watcherstore.go (1)
127-153: LGTM: Context propagation in Get method.The method correctly accepts and propagates the context parameter. Operation ID handling is functional.
Minor style inconsistency: Line 132 uses
operationID != ""while pager.go:47 useslen(operationID) > 0. Consider standardizing to one approach across the codebase for consistency.♻️ Optional: Standardize empty string check
- if operationID != "" { + if len(operationID) > 0 {cmd/maestro/server/logging/request_logging_middleware.go (2)
13-34: Consider marking the unusedctxparameter explicitly (or remove if API allows).
Right nowRegisterLoggerMiddleware(ctx, router)doesn’t usectx; if the signature must stay, preferfunc RegisterLoggerMiddleware(_ context.Context, router *mux.Router)to make intent obvious.
19-47: Healthcheck V(4) logging looks intentional; verify verbosity expectations.
Setting/healthchecktologLevel = 4will typically hide these entries unless the process runs with-v=4(good for spam control, but easy to misread as “more important”). Consider a short comment clarifying that higher V means “more verbose / usually suppressed.”examples/manifestwork/client.go (3)
48-90: Custom flag parsing: watch for “negative value” edge cases.
Your “next token starts with-=> it’s a flag” rule breaks negative numeric/duration values (e.g.,--server-healthiness-timeout -1s). If you want to keep this parsing, document the limitation; otherwise consider per-commandflag.FlagSetparsing.
155-159: Unreachable code afterlog.Fatalf.
log.Fatalfalready exits;printUsage()andos.Exit(1)will never run.Proposed diff
default: - log.Fatalf("unknown command: %s", command) - printUsage() - os.Exit(1) + printUsage() + log.Fatalf("unknown command: %s", command) }
176-195: TLS config in OpenAPI client: MinVersion TLS1.3 may be unexpectedly strict.
As an example this is fine, but if you expect users to point at older endpoints, consider relaxing totls.VersionTLS12or making it a flag.pkg/logger/operationid_middleware.go (1)
60-67:withOpIDlooks good; consider also setting response header when request provided opid (optional).
Today you only setX-Operation-IDresponse header when generating one; echoing it even when provided can simplify debugging/proxies.pkg/api/openapi/docs/DefaultAPI.md (2)
360-360: Address markdown formatting issues.The functional changes correctly add
X-Operation-ID-Keysupport to the API documentation. However, markdownlint reports formatting issues:
- Hard tabs should be replaced with spaces (lines 382-387)
- Table formatting issue on line 413 (trailing pipe and column count mismatch)
Since this is a generated file, consider whether the OpenAPI generator configuration needs adjustment to produce properly formatted markdown, or if a post-generation formatting step should be added.
Also applies to: 382-387, 413-413
502-502: Address markdown formatting issues.The functional changes correctly add
X-Operation-ID-Keysupport. However, markdownlint reports hard tabs on lines 520-525 that should be replaced with spaces for consistency.Since this is a generated file, consider whether the OpenAPI generator configuration needs adjustment to produce properly formatted markdown.
Also applies to: 520-525, 551-551
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (2)
data/generated/openapi/openapi.gois excluded by!**/generated/**go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
cmd/maestro/main.gocmd/maestro/server/grpc_broker.gocmd/maestro/server/logging/request_logging_middleware.gocmd/maestro/server/logging/writer.goexamples/cloudevents/grpcclient.goexamples/manifestwork/README.mdexamples/manifestwork/client-a/main.goexamples/manifestwork/client-b/main.goexamples/manifestwork/client.goexamples/manifestwork/client/README.mdexamples/manifestwork/client/main.goexamples/manifestwork/manifestwork.jsongo.modopenapi/openapi.yamlpkg/api/openapi/.openapi-generator/VERSIONpkg/api/openapi/README.mdpkg/api/openapi/api/openapi.yamlpkg/api/openapi/api_default.gopkg/api/openapi/docs/DefaultAPI.mdpkg/auth/auth_middleware.gopkg/auth/helpers.gopkg/client/cloudevents/grpcsource/client.gopkg/client/cloudevents/grpcsource/pager.gopkg/client/cloudevents/grpcsource/util.gopkg/client/cloudevents/grpcsource/watcherstore.gopkg/db/transaction_middleware.gopkg/handlers/errors.gopkg/handlers/framework.gopkg/logger/operationid_middleware.gopkg/services/resource.gopkg/services/resource_test.gotest/e2e/pkg/suite_test.gotest/performance/pkg/hub/store/createonly.gotest/store.go
💤 Files with no reviewable changes (8)
- examples/cloudevents/grpcclient.go
- pkg/client/cloudevents/grpcsource/client.go
- examples/manifestwork/client/main.go
- examples/manifestwork/client-b/main.go
- cmd/maestro/main.go
- examples/manifestwork/client/README.md
- test/e2e/pkg/suite_test.go
- examples/manifestwork/client-a/main.go
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/maestro/server/grpc_broker.go
- pkg/auth/helpers.go
- cmd/maestro/server/logging/writer.go
- openapi/openapi.yaml
- go.mod
- pkg/api/openapi/api/openapi.yaml
🧰 Additional context used
🧬 Code graph analysis (6)
pkg/db/transaction_middleware.go (1)
pkg/logger/operationid_middleware.go (1)
OpIDHeader(17-17)
pkg/auth/auth_middleware.go (1)
pkg/errors/errors.go (1)
ErrorUnauthorized(43-43)
pkg/handlers/errors.go (1)
pkg/errors/errors.go (1)
NotImplemented(177-179)
pkg/client/cloudevents/grpcsource/watcherstore.go (3)
pkg/client/cloudevents/grpcsource/pager.go (1)
PageList(21-84)pkg/api/openapi/model_list.go (1)
List(23-28)pkg/api/resource_types.go (1)
ResourceList(29-29)
pkg/services/resource_test.go (2)
pkg/api/openapi/model_list.go (1)
List(23-28)test/performance/pkg/util/util.go (1)
ClusterName(22-24)
examples/manifestwork/client.go (2)
pkg/client/cloudevents/grpcsource/client.go (1)
NewMaestroGRPCSourceWorkClient(19-63)pkg/client/cloudevents/grpcsource/util.go (1)
ToWorkPatch(191-235)
🪛 markdownlint-cli2 (0.18.1)
examples/manifestwork/README.md
218-218: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
224-224: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
230-230: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
263-263: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
296-296: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
pkg/api/openapi/docs/DefaultAPI.md
382-382: Hard tabs
Column: 1
(MD010, no-hard-tabs)
383-383: Hard tabs
Column: 1
(MD010, no-hard-tabs)
385-385: Hard tabs
Column: 1
(MD010, no-hard-tabs)
386-386: Hard tabs
Column: 1
(MD010, no-hard-tabs)
387-387: Hard tabs
Column: 1
(MD010, no-hard-tabs)
413-413: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
413-413: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
520-520: Hard tabs
Column: 1
(MD010, no-hard-tabs)
521-521: Hard tabs
Column: 1
(MD010, no-hard-tabs)
523-523: Hard tabs
Column: 1
(MD010, no-hard-tabs)
524-524: Hard tabs
Column: 1
(MD010, no-hard-tabs)
525-525: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e
- GitHub Check: e2e-with-istio
- GitHub Check: upgrade
- GitHub Check: e2e-broadcast-subscription
🔇 Additional comments (34)
test/performance/pkg/hub/store/createonly.go (4)
21-23: LGTM: Context parameter added to match interface.The signature correctly adds
ctx context.Contextto match the updated interface. The unused context is acceptable for this stub implementation.
43-45: LGTM: Context parameter added to match interface.The signature correctly adds
ctx context.Context. Consistent with the other updated methods in this stub implementation.
47-49: LGTM: Context parameter added to match interface.The signature correctly adds
ctx context.Context. The stub behavior is appropriate for this performance testing utility.
51-53: LGTM: Context parameter added to match interface.The signature correctly adds
ctx context.Context. The return values(nil, false, nil)appropriately indicate "not found" with no error.test/store.go (1)
4-4: LGTM! Context parameter added for interface compatibility.The
Listsignature now accepts acontext.Contextparameter to align with the updatedcegeneric.Listerinterface. Using the blank identifier_is appropriate here since this test/mock store doesn't need context for its in-memory operations.Also applies to: 129-131
pkg/services/resource.go (1)
36-36: LGTM! Context properly propagated for tracing support.The
Listmethod signature and implementation have been correctly updated to accept and propagate thecontext.Contextparameter. This enables operation ID tracing throughout the request lifecycle, which aligns with the PR's objective of supporting distributed tracing via context.Also applies to: 327-332
pkg/services/resource_test.go (1)
89-94: LGTM! Test call sites updated consistently.All three
Listinvocations have been correctly updated to passcontext.Background()as the first argument, which is the appropriate context to use in test scenarios.Also applies to: 96-100, 102-107
pkg/client/cloudevents/grpcsource/pager.go (2)
29-29: LGTM: Context-based operation ID retrieval.The switch to
getOperationID(ctx)enables dynamic operation ID key configuration and aligns with the broader context-aware tracing approach introduced in this PR.
47-50: LGTM: Correct header propagation.The conditional block properly sets both the operation ID key and value headers when an operation ID is present in the context. The ordering (key before value) is appropriate.
pkg/client/cloudevents/grpcsource/util.go (2)
25-28: LGTM: Thread-safe package state.The RWMutex properly protects the package-level
contextTracingOPIDKeyvariable, allowing concurrent reads while serializing writes. The default initialization tosdkgologging.ContextTracingOPIDKeyis appropriate.
279-289: LGTM: Thread-safe operation ID extraction.The function correctly uses RLock for concurrent read access, performs a safe type assertion, and returns both the key name and operation ID value together (or empty strings if absent). This ensures consistency when propagating operation IDs.
pkg/client/cloudevents/grpcsource/watcherstore.go (4)
75-113: LGTM: Proper context propagation in GetWatcher.The method now correctly accepts a context parameter and propagates it to
PageList,registerWatcher, and logging calls. This enables request-scoped operation ID tracing.
157-193: LGTM: Context propagation in List method.The method correctly accepts a context parameter and propagates it consistently to
PageListand logging calls.
219-257: Verify: Inconsistent context usage in Sync method.Unlike
GetWatcher,Get, andListmethods which now accept acontext.Contextparameter, theSyncmethod continues to usem.ctx(lines 228, 238). This meansSyncoperations cannot use request-scoped operation IDs.This appears intentional since
Syncis called from a background goroutine (line 68), but it creates an inconsistency in the API surface and tracing behavior.Question: Is this intentional? If
Syncshould participate in operation ID tracing, consider:
- Accepting a
context.Contextparameter (though this requires changes to how it's invoked)- Generating a new operation ID within
Syncfor its background workIf the current behavior is intentional (background sync operations use store-level context), consider adding a comment explaining why
Syncdoesn't accept a context parameter.
290-304: LGTM: Context propagation in registerWatcher.The method correctly accepts and propagates the context parameter to
newWorkWatcherand logging calls.pkg/api/openapi/.openapi-generator/VERSION (1)
1-1: Generator version bump is fine—ensure repo/CI regeneration is consistent.
If generated code is checked in, make sure it was regenerated with7.18.0and CI enforces the same version to avoid churn.pkg/logger/operationid_middleware.go (1)
16-18: No downstream compile impact from removed APIs. Verification confirms no remaining references toWithOpID,GetOperationID, or the previously named constants in other packages. The constantsOpIDKeyHeaderandOpIDHeaderare used only within this middleware file (lines 25, 31, 39), and the internalwithOpIDfunction remains available for local use (line 37). The API removals do not break any external code.pkg/db/transaction_middleware.go (1)
13-13: LGTM: Header-based operation ID retrieval.The changes correctly shift from context-based to header-based operation ID retrieval, aligning with the PR's tracing objectives. The import alias and header extraction are properly implemented.
Also applies to: 27-27
pkg/api/openapi/README.md (1)
10-10: LGTM: Generator version bump.The OpenAPI generator version update is consistent with the regeneration of client code to support X-Operation-ID-Key.
pkg/auth/auth_middleware.go (1)
31-31: LGTM: Updated error handler signature.The addition of the request parameter to
handleErrorenables extraction of the operation ID from request headers for error responses, consistent with the PR's tracing objectives.pkg/handlers/errors.go (2)
69-69: LGTM: Updated error handler signature.The signature change from
handleError(r.Context(), w, ...)tohandleError(w, r, ...)correctly aligns with the new pattern of extracting operation IDs from request headers.Also applies to: 77-77
73-73: Good catch: Fixed NotImplemented reason.The correction from
"path"to"patch"properly identifies the unimplemented operation, improving error message accuracy.pkg/handlers/framework.go (6)
11-11: LGTM on the new import.The import for
maestrologgeris added to access theOpIDHeaderconstant used in the error handler.
30-30: Signature change is appropriate for the new tracing approach.Changing from
context.Contextto*http.Requestprovides direct access to headers for operation ID retrieval while still allowing context access viar.Context().
44-77: LGTM onhandlefunction updates.All error handler invocations consistently pass
(w, r, err)matching the updated signature. The default handler assignment and validation loop error handling are correct.
80-101: LGTM onhandleDeletefunction updates.Error handler calls at lines 87 and 96 correctly pass the request object.
103-128: LGTM onhandleGetandhandleListfunction updates.Both functions correctly invoke the error handler with the updated signature.
33-42: OpIDHeader constant is properly defined and used.The
OpIDHeaderconstant is defined in themaestrologgerpackage as"X-Operation-ID"and the usage in line 41 offramework.gois correct.pkg/api/openapi/api_default.go (6)
845-855: LGTM on the newxOperationIDKeyfield addition.The field is correctly positioned alongside
xOperationIDin the request struct for the ResourceBundles list endpoint.
887-895: LGTM on theXOperationIDKeybuilder method.The builder pattern is consistent with the existing
XOperationIDmethod.
976-981: LGTM on header emission order.
X-Operation-ID-Keyis correctly set beforeX-Operation-ID, which aligns with the documented API behavior where the key specifies how to interpret the ID.
1195-1206: LGTM on thexOperationIDKeyfield for the single resource endpoint.The struct fields for
ApiApiMaestroV1ResourceBundlesIdGetRequestare consistent with the list endpoint pattern.
1272-1277: LGTM on header emission for single resource endpoint.Header handling is consistent with the list endpoint implementation.
9-9: This is auto-generated code — verify changes come from OpenAPI spec regeneration.The file header confirms this is generated by OpenAPI Generator and should not be manually edited. The changes to add
xOperationIDKeyfield and builder methods are consistent with OpenAPI Generator output and match theX-Operation-ID-Keyparameter now defined inopenapi/openapi.yaml.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/api/openapi/docs/DefaultAPI.md (1)
360-415: Fix markdownlint failures (tabs + malformed table row).Static analysis flags hard tabs (MD010) and a broken parameter table row (MD055/MD056) around the added
xOperationIDKeydocs.What to fix
- Replace hard tabs with spaces in the affected code blocks.
- Ensure the
xOperationIDKeyparameter row has 4 cells and consistent pipes, e.g.| **xOperationIDKey** | **string** | ... | ... |.Also applies to: 500-553
pkg/client/cloudevents/grpcsource/watcherstore.go (2)
290-304: Use a background or long-lived context for the watcher, not the request-scoped context.The
workWatcherstorescontext.Context(watch.go:22) and uses it inStop()andReceive()methods, but receives a request-scopedctxfromGetWatcher(). When the request completes and cancels its context, the watcher—stored long-term inm.watchers—continues using a canceled context, causing context cancellation leaks. Consider usingcontext.Background()or deriving fromm.ctx(the store's initialization context) instead.
228-239: Sync usesm.ctxinstead of a per-request context, so logging and API calls won't carry operation IDs.The
Sync()method uses the initialization-timem.ctx(line 230, 238) rather than accepting a context parameter. When called from the reconnect handler inclient.go(line 53), this means thePageList()call and logging won't include per-request operation IDs, unlike other methods likeGet()andGetWatcher()which correctly accept and propagate the context.If the
ClientWatcherStoreinterface permits, consider adding a context parameter toSync()to maintain consistent tracing across all API calls. Otherwise, document why this design differs from other store methods.
🤖 Fix all issues with AI agents
In @examples/manifestwork/README.md:
- Line 218: Add explicit language identifiers to the fenced code blocks in the
example output sections (e.g., change ``` to ```text for plain output and
```json for JSON output) so markdownlint stops warning; update the blocks
surrounding the "Apply manifestwork (opid=...)", "Updating an existing work:",
"Watching changes with `--print-work-details`:", "Getting work:", and "Listing
works:" examples to use ```text or ```json as appropriate.
In @pkg/handlers/framework.go:
- Around line 49-59: The request body read/unmarshal error paths use handleError
directly instead of delegating to the configured cfg.ErrorHandler, so custom
handlers set on cfg are bypassed; change the error handling in the io.ReadAll
and json.Unmarshal failure branches to call cfg.ErrorHandler(w, r, err)
(wrapping or converting the error into the same error type you currently pass to
handleError, e.g., errors.MalformedRequest(...)) or, if you intentionally want
to bypass custom handlers, add a clarifying comment above the read/unmarshal
block explaining why handleError is used instead of cfg.ErrorHandler; ensure you
reference cfg.MarshalInto for the Unmarshal target and preserve the same error
messages when invoking cfg.ErrorHandler.
In @pkg/logger/operationid_middleware.go:
- Around line 23-40: The middleware generates an OpID when missing but only
writes it to the response header and context, so downstream code reading
r.Header (e.g., transaction_middleware) still sees empty; fix the handler in
operationid_middleware (where withOpID is called and opID is obtained) to also
set the request header and attach the updated context to the request: call r =
r.WithContext(ctx) and r.Header.Set(OpIDHeader, opID) after ctx is populated;
apply the same change to the second occurrence noted in the file (the other
generation branch at the later location).
- Around line 60-67: withOpID currently only checks for a non-nil
ctx.Value(opIDKey), which lets wrong types or empty strings bypass generation;
change the guard to fetch val := ctx.Value(opIDKey) and only return the existing
context when val is a non-empty string (e.g., if s, ok := val.(string); ok && s
!= "" { return ctx }), otherwise generate a new ksuid and store it via
context.WithValue(ctx, opIDKey, opID); keep references to the same function name
withOpID and the opIDKey of type sdkgologging.ContextTracingKey.
🧹 Nitpick comments (2)
pkg/logger/operationid_middleware.go (2)
23-28: ConstrainX-Operation-ID-Keybefore using it as an OTEL attribute/log field key.Because it’s client-controlled, a long/high-cardinality/odd value can explode attribute key space and log field keys.
Sketch: minimal validation
opIDKey := r.Header.Get(OpIDKeyHeader) if opIDKey != "" { + // Keep this conservative: avoid unbounded cardinality / weird keys. + if len(opIDKey) > 64 { + opIDKey = opIDKey[:64] + } contextOPIDKey = sdkgologging.ContextTracingKey(opIDKey) }Also applies to: 44-45
54-55: Consider using a string literal for consistency: castcontextOPIDKeytostringwhen passing toWithValues.The logr/klog API is designed to accept string constants for keys. While
sdkgologging.ContextTracingKeyis a string type alias and will work withWithValues, all other logging calls in the codebase use string literals (e.g.,"resourceID","owner","key"). For API consistency and clarity, cast tostring:Suggested improvement
- logger := klog.FromContext(ctx).WithValues(contextOPIDKey, opID) + logger := klog.FromContext(ctx).WithValues(string(contextOPIDKey), opID) ctx = klog.NewContext(ctx, logger)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (2)
data/generated/openapi/openapi.gois excluded by!**/generated/**go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
cmd/maestro/main.gocmd/maestro/server/grpc_broker.gocmd/maestro/server/logging/request_logging_middleware.gocmd/maestro/server/logging/writer.goexamples/cloudevents/grpcclient.goexamples/manifestwork/README.mdexamples/manifestwork/client-a/main.goexamples/manifestwork/client-b/main.goexamples/manifestwork/client.goexamples/manifestwork/client/README.mdexamples/manifestwork/client/main.goexamples/manifestwork/manifestwork.jsongo.modopenapi/openapi.yamlpkg/api/openapi/.openapi-generator/VERSIONpkg/api/openapi/README.mdpkg/api/openapi/api/openapi.yamlpkg/api/openapi/api_default.gopkg/api/openapi/docs/DefaultAPI.mdpkg/auth/auth_middleware.gopkg/auth/helpers.gopkg/client/cloudevents/grpcsource/client.gopkg/client/cloudevents/grpcsource/pager.gopkg/client/cloudevents/grpcsource/util.gopkg/client/cloudevents/grpcsource/watcherstore.gopkg/db/transaction_middleware.gopkg/handlers/errors.gopkg/handlers/framework.gopkg/logger/operationid_middleware.gopkg/services/resource.gopkg/services/resource_test.gotest/e2e/pkg/suite_test.gotest/performance/pkg/hub/store/createonly.gotest/store.go
💤 Files with no reviewable changes (8)
- examples/manifestwork/client/README.md
- examples/manifestwork/client-b/main.go
- pkg/client/cloudevents/grpcsource/client.go
- test/e2e/pkg/suite_test.go
- examples/manifestwork/client-a/main.go
- cmd/maestro/main.go
- examples/cloudevents/grpcclient.go
- examples/manifestwork/client/main.go
✅ Files skipped from review due to trivial changes (1)
- pkg/api/openapi/.openapi-generator/VERSION
🚧 Files skipped from review as they are similar to previous changes (10)
- cmd/maestro/server/grpc_broker.go
- cmd/maestro/server/logging/request_logging_middleware.go
- pkg/auth/auth_middleware.go
- test/store.go
- pkg/handlers/errors.go
- pkg/api/openapi/api/openapi.yaml
- pkg/services/resource.go
- examples/manifestwork/client.go
- openapi/openapi.yaml
- go.mod
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/auth/helpers.go (2)
pkg/errors/errors.go (2)
ServiceErrorCode(61-61)New(102-116)pkg/logger/operationid_middleware.go (1)
OpIDHeader(17-17)
test/performance/pkg/hub/store/createonly.go (2)
pkg/api/openapi/model_list.go (1)
List(23-28)pkg/api/resource_types.go (1)
ResourceList(29-29)
pkg/client/cloudevents/grpcsource/watcherstore.go (3)
pkg/client/cloudevents/grpcsource/pager.go (1)
PageList(21-84)pkg/api/openapi/model_list.go (1)
List(23-28)pkg/api/resource_types.go (1)
ResourceList(29-29)
🪛 markdownlint-cli2 (0.18.1)
examples/manifestwork/README.md
218-218: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
224-224: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
230-230: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
263-263: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
296-296: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
pkg/api/openapi/docs/DefaultAPI.md
382-382: Hard tabs
Column: 1
(MD010, no-hard-tabs)
383-383: Hard tabs
Column: 1
(MD010, no-hard-tabs)
385-385: Hard tabs
Column: 1
(MD010, no-hard-tabs)
386-386: Hard tabs
Column: 1
(MD010, no-hard-tabs)
387-387: Hard tabs
Column: 1
(MD010, no-hard-tabs)
413-413: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
413-413: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
520-520: Hard tabs
Column: 1
(MD010, no-hard-tabs)
521-521: Hard tabs
Column: 1
(MD010, no-hard-tabs)
523-523: Hard tabs
Column: 1
(MD010, no-hard-tabs)
524-524: Hard tabs
Column: 1
(MD010, no-hard-tabs)
525-525: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Red Hat Konflux / integration-tests / maestro
- GitHub Check: Red Hat Konflux / unit-tests / maestro
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e
- GitHub Check: upgrade
- GitHub Check: e2e-with-istio
- GitHub Check: e2e-broadcast-subscription
🔇 Additional comments (13)
pkg/services/resource_test.go (1)
89-107: LGTM!The addition of
context.Background()correctly aligns with the updatedResourceService.ListAPI signature. The variable handling is appropriate, using:=for the first declaration (line 89) and=for subsequent reassignments (lines 96, 102) to avoid shadowing. All test assertions remain intact.cmd/maestro/server/logging/writer.go (1)
32-39: LGTM! Dynamic log level improves logging flexibility.The refactoring to accept an explicit
logLevelparameter is well-implemented and enables the flexibility needed for context-aware logging (e.g., using higher log levels for healthcheck endpoints). The change maintains the existing error handling path appropriately.test/performance/pkg/hub/store/createonly.go (1)
21-52: LGTM! Context parameters added to align with interface changes.The addition of
context.Contextparameters toGetWatcher,List,ListAll, andGetmethods correctly updates the stub implementation to match the broader context-propagation pattern introduced in this PR. Since this is a test stub that returns unsupported errors, not using the context is appropriate.pkg/client/cloudevents/grpcsource/util.go (2)
25-28: LGTM! Thread-safe package-level state.The mutex and default operation ID key initialization provide thread-safe access to the customizable operation ID tracing key.
279-289: LGTM! Thread-safe operation ID extraction.The function correctly uses a read lock to access the package-level key and safely extracts the operation ID from the context.
pkg/client/cloudevents/grpcsource/pager.go (1)
29-50: LGTM! Correctly propagates operation ID headers.The code properly extracts both the operation ID key and value using the new
getOperationID(ctx)helper, and sets theX-Operation-ID-Keyheader beforeX-Operation-IDwhen an operation ID is present.pkg/auth/helpers.go (1)
14-32: All call sites have been properly updated.The signature change from
handleError(ctx context.Context, w http.ResponseWriter, code errors.ServiceErrorCode, reason string)to includer *http.Requestis correct and aligns with the shift to header-based operation ID tracing. The implementation correctly reads the operation ID from the request header and propagates it through logging and error responses. The single call site inpkg/auth/auth_middleware.go:31has been updated to pass the*http.Requestparameter correctly.pkg/client/cloudevents/grpcsource/watcherstore.go (1)
75-113: Context propagation into PageList + logging looks good.Threading
ctxthroughPageList(...)andm.logger.*(ctx, ...)should make opID/cancellation/trace propagation much cleaner.Also applies to: 127-153, 157-193
pkg/api/openapi/api_default.go (1)
845-855: Generated client wiring forX-Operation-ID-Keyis correctly implemented.The struct fields, builder method, and header emission are properly aligned across both list and get-by-id endpoints. Header emission order is non-deterministic (due to Go map iteration), but this does not affect HTTP semantics—RFC 7230 and OpenAPI specifications impose no ordering requirement for differently-named header fields. The implementation follows standard OpenAPI Generator patterns and is ready for use.
pkg/handlers/framework.go (4)
30-31: LGTM!The signature change to accept
*http.Requestis a clean approach - it provides access to both the context viar.Context()and the request headers for operation ID extraction.
33-42: LGTM!The implementation correctly derives the logger from the request context and extracts the operation ID from the header for error response correlation. The log-level distinction between client errors (4xx) and server errors (5xx) is appropriate.
80-101: LGTM!The
handleDeletefunction consistently usescfg.ErrorHandlerfor all error paths after setting the default handler.
103-128: LGTM!Both
handleGetandhandleListfunctions consistently usecfg.ErrorHandlerfor error handling after setting the default.
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 (2)
examples/manifestwork/README.md (1)
218-302: Add language specifiers to fenced code blocks.The markdown linter flagged several code blocks (lines 218, 224, 230, 263, 296) that lack language specifiers. Adding identifiers improves syntax highlighting and linting compliance.
Based on static analysis hints from markdownlint-cli2.
📝 Suggested fix
For terminal output examples, use
textorconsoleas the language identifier:**Creating a work:** -``` +```text Apply manifestwork (opid=3bd7d...) Work 77112687-f4ac-42d1-b8b8-6f999428c19d/nginx-work (uid=abc123...) created successfullyUpdating an existing work:
-+text
Apply manifestwork (opid=5ad7d...)
Work 77112687-f4ac-42d1-b8b8-6f999428c19d/nginx-work (uid=abc123...) updated successfully**Watching changes with `--print-work-details`:** -``` +```text Watch manifestwork (opid=9ed7d...) [MODIFIED] Work: 77112687-f4ac-42d1-b8b8-6f999428c19d/nginx-work (uid=abc123...) { ... }Getting work:
-+text
Get manifestwork 114916d9-f470-4905-8793-785d58ee0f8d/nginx-work (opid=7cd7d...)
{
...
}**Listing works:** -``` +```text List manifestworks (opid=9dd7d...): Consumer Name UID Created 77112687... nginx-work 941681df... 2026-01-08T17:04:58+08:00 ...pkg/client/cloudevents/grpcsource/watcherstore.go (1)
219-257: UpdateSync()to accept a context parameter.The method uses
m.ctxfor API calls and logging operations, while all other methods inRESTFulAPIWatcherStore(GetWatcher,Get,List,ListAll) accept acontext.Contextparameter. SinceSync()is called from a location with an available context (the reconnect handler atpkg/client/cloudevents/grpcsource/client.go:53), it should accept and use the provided context to maintain consistent operation ID tracing throughout the call chain.
🧹 Nitpick comments (7)
examples/manifestwork/client.go (2)
114-116: Clarify when to useSetContextTracingOPIDKey.The comment mentions
grpcsource.SetContextTracingOPIDKeybut doesn't explain when a developer would need to call it. Consider adding context about the default key and when customization is necessary.📝 Suggested clarification
- opId := uuid.NewString() - // use grpcsource.SetContextTracingOPIDKey to customize the operation ID key - // e.g. grpcsource.SetContextTracingOPIDKey(sdkgologging.ContextTracingKey("my-op-id")) - opCtx := context.WithValue(ctx, sdkgologging.ContextTracingOPIDKey, opId) + opId := uuid.NewString() + // Note: sdkgologging.ContextTracingOPIDKey is the default operation ID key. + // Call grpcsource.SetContextTracingOPIDKey(customKey) before this line if you need + // a different key (e.g., to align with existing tracing infrastructure). + opCtx := context.WithValue(ctx, sdkgologging.ContextTracingOPIDKey, opId)
207-266: Consider extracting common TLS configuration logic.The cert-based and token-based authentication paths share substantial TLS setup code (embedding certs, calling
AutoLoadTLSConfig). Extracting a helper function would reduce duplication and make the code easier to maintain.♻️ Suggested refactor
// Helper function to set up TLS config func setupTLSConfig(grpcOptions *grpc.GRPCOptions, caFile, certFile, keyFile string) error { certConfig := cert.CertConfig{ CAFile: caFile, ClientCertFile: certFile, ClientKeyFile: keyFile, } if err := certConfig.EmbedCerts(); err != nil { return err } tlsConfig, err := cert.AutoLoadTLSConfig( certConfig, func() (*cert.CertConfig, error) { certConfig := cert.CertConfig{ CAFile: caFile, ClientCertFile: certFile, ClientKeyFile: keyFile, } if err := certConfig.EmbedCerts(); err != nil { return nil, err } return &certConfig, nil }, grpcOptions.Dialer, ) if err != nil { return err } grpcOptions.Dialer.TLSConfig = tlsConfig return nil }Then use it in both branches:
if *grpcServerCAFile != "" && *grpcClientCertFile != "" && *grpcClientKeyFile != "" { if err := setupTLSConfig(grpcOptions, *grpcServerCAFile, *grpcClientCertFile, *grpcClientKeyFile); err != nil { return nil, err } } else if *grpcServerCAFile != "" && *grpcClientTokenFile != "" { token, err := os.ReadFile(*grpcClientTokenFile) if err != nil { return nil, err } grpcOptions.Dialer.Token = strings.TrimSpace(string(token)) if err := setupTLSConfig(grpcOptions, *grpcServerCAFile, "", ""); err != nil { return nil, err } }pkg/logger/operationid_middleware.go (1)
37-41: Add safety check for the operation ID type assertion.Line 38 performs a type assertion on
ctx.Value(contextOPIDKey)without checking if the assertion succeeds. WhilewithOpIDensures the value is a string, defensive programming suggests adding an explicit check or comment to document the invariant.🔍 Proposed defensive assertion
} else { // If no operationID from header, get it from context or generate a new one ctx = withOpID(r.Context(), contextOPIDKey) - opID, _ := ctx.Value(contextOPIDKey).(string) + var ok bool + opID, ok = ctx.Value(contextOPIDKey).(string) + if !ok { + opID = "" // Fallback to empty if assertion fails + } // Set the generated operation ID in the request header for consistency r.Header.Set(OpIDHeader, opID) }pkg/api/openapi/docs/DefaultAPI.md (2)
382-383: Fix hard tabs flagged by markdownlint.The markdown linter reports hard tabs in these lines. Consider regenerating the documentation with proper space indentation to maintain consistent formatting.
Also applies to: 520-521
413-413: Fix table formatting issue.The markdown linter reports a table column count mismatch on Line 413. The table row has only 3 cells but 4 are expected, and there's an unexpected trailing pipe.
pkg/client/cloudevents/grpcsource/util.go (1)
289-298: Add safety check for type assertion.Line 291 performs a type assertion on
contextTracingOPIDKey.Load()without checking if it succeeds. While the init() function and SetContextTracingOPIDKey ensure the value is always aContextTracingKey, a defensive check or explicit comment would improve robustness.🔍 Proposed defensive assertion
func getOperationID(ctx context.Context) (string, string) { // Load atomically - provides memory barrier to see the value stored in SetContextTracingOPIDKey - key := contextTracingOPIDKey.Load().(sdkgologging.ContextTracingKey) + key, ok := contextTracingOPIDKey.Load().(sdkgologging.ContextTracingKey) + if !ok { + // This should never happen due to init() and SetContextTracingOPIDKey guards + return "", "" + } if opID, ok := ctx.Value(key).(string); ok { return string(key), opID } return "", "" }pkg/handlers/framework.go (1)
44-101: Consider extracting the repeated default error handler assignment.The pattern of checking
cfg.ErrorHandler == niland assigninghandleErroris repeated acrosshandle,handleDelete,handleGet, andhandleListfunctions (lines 45-47, 81-83, 104-106, 118-120).♻️ Proposed refactor to reduce duplication
You could create a helper function or ensure the default is set once when the config is created:
+func ensureDefaultErrorHandler(cfg *handlerConfig) { + if cfg.ErrorHandler == nil { + cfg.ErrorHandler = handleError + } +} + func handle(w http.ResponseWriter, r *http.Request, cfg *handlerConfig, httpStatus int) { - if cfg.ErrorHandler == nil { - cfg.ErrorHandler = handleError - } + ensureDefaultErrorHandler(cfg) bytes, err := io.ReadAll(r.Body)Apply the same pattern to
handleDelete,handleGet, andhandleList.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (2)
data/generated/openapi/openapi.gois excluded by!**/generated/**go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
cmd/maestro/main.gocmd/maestro/server/grpc_broker.gocmd/maestro/server/logging/request_logging_middleware.gocmd/maestro/server/logging/writer.goexamples/cloudevents/grpcclient.goexamples/manifestwork/README.mdexamples/manifestwork/client-a/main.goexamples/manifestwork/client-b/main.goexamples/manifestwork/client.goexamples/manifestwork/client/README.mdexamples/manifestwork/client/main.goexamples/manifestwork/manifestwork.jsongo.modopenapi/openapi.yamlpkg/api/openapi/.openapi-generator/VERSIONpkg/api/openapi/README.mdpkg/api/openapi/api/openapi.yamlpkg/api/openapi/api_default.gopkg/api/openapi/docs/DefaultAPI.mdpkg/auth/auth_middleware.gopkg/auth/helpers.gopkg/client/cloudevents/grpcsource/client.gopkg/client/cloudevents/grpcsource/pager.gopkg/client/cloudevents/grpcsource/util.gopkg/client/cloudevents/grpcsource/watcherstore.gopkg/db/transaction_middleware.gopkg/handlers/errors.gopkg/handlers/framework.gopkg/logger/operationid_middleware.gopkg/services/resource.gopkg/services/resource_test.gotest/e2e/pkg/suite_test.gotest/performance/pkg/hub/store/createonly.gotest/store.go
💤 Files with no reviewable changes (8)
- cmd/maestro/main.go
- examples/manifestwork/client/README.md
- examples/manifestwork/client/main.go
- pkg/client/cloudevents/grpcsource/client.go
- examples/manifestwork/client-b/main.go
- examples/cloudevents/grpcclient.go
- test/e2e/pkg/suite_test.go
- examples/manifestwork/client-a/main.go
🚧 Files skipped from review as they are similar to previous changes (12)
- pkg/db/transaction_middleware.go
- cmd/maestro/server/grpc_broker.go
- cmd/maestro/server/logging/request_logging_middleware.go
- pkg/services/resource_test.go
- pkg/handlers/errors.go
- pkg/auth/auth_middleware.go
- test/store.go
- pkg/api/openapi/.openapi-generator/VERSION
- openapi/openapi.yaml
- pkg/client/cloudevents/grpcsource/pager.go
- pkg/api/openapi/api/openapi.yaml
- pkg/api/openapi/README.md
🧰 Additional context used
🧬 Code graph analysis (5)
pkg/client/cloudevents/grpcsource/watcherstore.go (1)
pkg/client/cloudevents/grpcsource/pager.go (1)
PageList(21-84)
pkg/logger/operationid_middleware.go (2)
pkg/db/context.go (1)
NewContext(14-23)pkg/errors/errors.go (1)
New(102-116)
pkg/services/resource.go (4)
pkg/api/openapi/model_list.go (1)
List(23-28)pkg/db/migrations/202311151850_add_resources.go (1)
Resource(10-24)pkg/api/resource_types.go (1)
Resource(14-27)test/performance/pkg/util/util.go (1)
ClusterName(22-24)
test/performance/pkg/hub/store/createonly.go (2)
pkg/api/openapi/model_list.go (1)
List(23-28)pkg/api/resource_types.go (1)
ResourceList(29-29)
pkg/auth/helpers.go (2)
pkg/errors/errors.go (2)
ServiceErrorCode(61-61)New(102-116)pkg/logger/operationid_middleware.go (1)
OpIDHeader(17-17)
🪛 markdownlint-cli2 (0.18.1)
pkg/api/openapi/docs/DefaultAPI.md
382-382: Hard tabs
Column: 1
(MD010, no-hard-tabs)
383-383: Hard tabs
Column: 1
(MD010, no-hard-tabs)
385-385: Hard tabs
Column: 1
(MD010, no-hard-tabs)
386-386: Hard tabs
Column: 1
(MD010, no-hard-tabs)
387-387: Hard tabs
Column: 1
(MD010, no-hard-tabs)
413-413: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
413-413: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
520-520: Hard tabs
Column: 1
(MD010, no-hard-tabs)
521-521: Hard tabs
Column: 1
(MD010, no-hard-tabs)
523-523: Hard tabs
Column: 1
(MD010, no-hard-tabs)
524-524: Hard tabs
Column: 1
(MD010, no-hard-tabs)
525-525: Hard tabs
Column: 1
(MD010, no-hard-tabs)
examples/manifestwork/README.md
218-218: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
224-224: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
230-230: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
263-263: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
296-296: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: e2e-with-istio
- GitHub Check: e2e
- GitHub Check: upgrade
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e-broadcast-subscription
🔇 Additional comments (25)
cmd/maestro/server/logging/writer.go (1)
32-39: LGTM! Parameterized log level improves verbosity control.The addition of the
logLevelparameter and the use ofwriter.logger.V(logLevel).Info()correctly implements klog's verbosity API, allowing different log statements to be controlled independently. The error path appropriately continues to always log errors regardless of verbosity level.go.mod (4)
51-55: Consistent Kubernetes component patch update.All Kubernetes components are uniformly updated from v0.34.2 to v0.34.3 (patch-level). This is a safe, consistent update across the Kubernetes client/server ecosystem.
Also applies to: 197-199
58-60: open-cluster-management.io updates align with PR objectives.The pseudo-version updates to the open-cluster-management modules (api, ocm, sdk-go with 2026-01-08 timestamps) appear to include the necessary changes for operation-ID propagation and header handling as described in the PR objectives. The updates are fresh and timely relative to the PR creation date.
98-100: go-openapi module updates support X-Operation-ID-Key feature.The updates to jsonpointer, jsonreference, and swag (indirect dependencies) are consistent with the PR's addition of X-Operation-ID-Key header support and OpenAPI client/server boilerplate changes.
127-127: No action needed for mailru/easyjson v0.9.0.The v0.9.0 release is a backwards-compatible update focused on Go version updates and bugfixes, with no breaking changes. Since easyjson is an indirect dependency (not used for code generation in this project), the upgrade is low-risk and safe to proceed.
examples/manifestwork/client.go (6)
282-295: LGTM!Clean implementation with proper error handling and JSON formatting.
297-318: LGTM!Proper error handling and user-friendly tabular output for the list command.
320-361: LGTM!Correctly implements the apply pattern with proper create-or-update logic using
ToWorkPatchfor merge patches.
363-377: LGTM! Good practice for auditability.Retrieving the UID before deletion provides a complete audit trail, which is helpful for troubleshooting and logging.
379-407: LGTM!Properly implements watch pattern with graceful shutdown support and correct channel handling.
409-429: LGTM!Clean event formatting with appropriate verbosity control.
pkg/logger/operationid_middleware.go (3)
23-28: Verify dynamic operation ID key resolution.The dynamic key resolution from the
X-Operation-ID-Keyheader is well-designed. However, ensure that clients sending this header understand that the key must be a valid context key string.
44-45: Verify lowercase conversion for OTEL attribute keys.The use of
strings.ToLower(string(contextOPIDKey))on Line 45 converts the context key to lowercase for the OTEL span attribute. Confirm that this lowercase transformation aligns with your observability conventions and doesn't conflict with other attribute naming schemes in your tracing infrastructure.
54-57: LGTM: klog integration for operation ID propagation.The addition of operation ID to the klog logger ensures consistent tracing across all log messages in the request context. This is a solid improvement for observability.
pkg/api/openapi/api_default.go (2)
845-854: LGTM: Consistent pattern for operation ID key parameter.The addition of
xOperationIDKeyfollows the same builder pattern as the existingxOperationIDparameter. The implementation is consistent across both resource bundle GET endpoints.Also applies to: 887-890
976-978: LGTM: Header emission ordering is correct.The X-Operation-ID-Key header is correctly emitted before X-Operation-ID, allowing the server middleware to determine the dynamic key before reading the operation ID.
Also applies to: 1272-1274
pkg/client/cloudevents/grpcsource/util.go (1)
26-36: LGTM: Thread-safe initialization pattern.The use of
atomic.Valuefor lock-free reads andsync.Oncefor single initialization is an appropriate pattern for managing the global operation ID key configuration.pkg/services/resource.go (1)
36-36: LGTM: Proper context propagation in List method.The addition of
ctx context.Contextas the first parameter toListfollows Go best practices and enables proper propagation of tracing, cancellation, and timeout information through the call chain. The replacement ofcontext.TODO()with the passed context on Line 328 is the correct approach.Also applies to: 327-333
pkg/auth/helpers.go (1)
14-32: LGTM! Operation ID tracing via headers implemented correctly.The refactored error handling correctly:
- Accepts the HTTP request to extract the operation ID from headers
- Includes the operation ID in both info and error log statements
- Passes the operation ID to the OpenAPI error payload for end-to-end traceability
test/performance/pkg/hub/store/createonly.go (1)
21-53: LGTM! Stub implementation correctly updated.The context parameter additions align the stub with the updated interface signatures while maintaining the original stub behavior. Not using the context parameter is acceptable for test stubs.
pkg/handlers/framework.go (1)
30-42: LGTM! Error handling refactored correctly.The signature changes and operation ID retrieval from request headers align with the PR objectives. The logger is correctly derived from the request context, and the operation ID is properly passed to the OpenAPI error payload.
pkg/client/cloudevents/grpcsource/watcherstore.go (4)
75-113: LGTM! Context propagation implemented correctly in GetWatcher.The method now accepts a context parameter and correctly:
- Propagates it to PageList for API calls
- Uses it for watcher registration
- Passes it to logging calls
This enables proper operation ID tracing for request-scoped operations.
127-153: LGTM! Context propagation and operation ID handling implemented correctly.The Get method properly:
- Accepts and uses the provided context for API calls
- Retrieves and applies operation ID headers when present
- Uses context-aware logging
157-193: LGTM! List method correctly updated for context propagation.The context is properly threaded through PageList calls and logging, enabling operation ID tracing for list operations.
290-304: LGTM! Watcher registration correctly updated.The
registerWatchermethod now accepts and uses the provided context for logging and watcher initialization, ensuring proper operation ID propagation.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/logger/operationid_middleware.go (1)
45-56: Inconsistent operation ID key usage between observability backends.The middleware uses different strategies for OTEL vs. Sentry:
- Line 45 (OTEL): Attribute key is dynamic, derived from
contextOPIDKeyand lowercased:strings.ToLower(string(contextOPIDKey))- Line 50 (Sentry): Tag key is hardcoded to
"operation_id"Impact:
- OTEL spans will have different attribute names depending on the
X-Operation-ID-Keyheader, fragmenting observability queries (e.g., one request usesopid, another usesrequest_id).- Sentry consistently uses
operation_id, but won't align with custom OTEL keys.Recommendation: Decide on one approach:
- Option A (consistency): Use the same hardcoded key for both (e.g.,
"operation_id"or lowercased default).- Option B (full dynamic): Apply the dynamic key to Sentry as well (line 50).
Option A is typically preferred for observability tooling to ensure queries remain consistent.
♻️ Option A: Use consistent hardcoded key for OTEL
// Add operationID attribute to span span := trace.SpanFromContext(ctx) - span.SetAttributes(operationIDAttribute(strings.ToLower(string(contextOPIDKey)), opID)) + span.SetAttributes(operationIDAttribute("operation_id", opID))♻️ Option B: Use dynamic key for Sentry as well
// Add operationID to sentry context if hub := sentry.GetHubFromContext(ctx); hub != nil { hub.ConfigureScope(func(scope *sentry.Scope) { - scope.SetTag("operation_id", opID) + scope.SetTag(strings.ToLower(string(contextOPIDKey)), opID) }) }
🤖 Fix all issues with AI agents
In @openapi/openapi.yaml:
- Around line 96-99: The X-Operation-ID-Key header parameter must be added to
every operation that currently lacks it to enable consistent tracing: add a
header parameter named X-Operation-ID-Key with schema type string to DELETE
/api/maestro/v1/resource-bundles/{id} and to all consumer endpoints (GET list,
POST, GET by ID, PATCH, DELETE under the consumer paths). For each operation
(e.g., the DELETE resource-bundles/{id} operation and the consumer operations
for list, create, retrieve, patch, delete), insert the same parameter block
under parameters so it is treated as an operation-level header parameter (name:
X-Operation-ID-Key, in: header, schema: type: string). Ensure parameter names
match exactly and there are no duplicates if the header is already declared at
the path level.
In @pkg/api/openapi/docs/DefaultAPI.md:
- Around line 551-552: The OpenAPI docs table for the IdGet endpoint is missing
the Notes column for the xOperationIDKey (and xOperationID) entries; update the
OpenAPI source (where the IdGet operation parameters/headers are defined) to
include the Notes cell for xOperationIDKey so the generated docs show the full
four-column table (Name | Type | Notes | Default/Value) — locate the IdGet
operation and the definitions for xOperationIDKey/xOperationID and add the
missing Notes text (or an explicit empty placeholder) so the markdown table
renders correctly.
🧹 Nitpick comments (6)
examples/manifestwork/client.go (3)
48-84: Consider using a proper CLI library for argument parsing.The custom argument parsing logic is complex and handles flag positioning manually. For a more maintainable solution, consider using established CLI libraries like:
github.com/spf13/cobra- comprehensive CLI frameworkgithub.com/urfave/cli- simpler alternativeThese would handle flag parsing, help text, and command routing more robustly.
188-191: Document the security implications of InsecureSkipVerify.While the flag description marks this as "INSECURE", the TLS configuration also sets MinVersion to TLS 1.3, which provides a false sense of security. Consider adding a more prominent warning in the code comments:
💡 Suggested documentation improvement
HTTPClient: &http.Client{ Transport: &http.Transport{TLSClientConfig: &tls.Config{ + // WARNING: InsecureSkipVerify bypasses certificate verification and should + // NEVER be used in production. This is only for local development/testing. MinVersion: tls.VersionTLS13, InsecureSkipVerify: *insecureSkipVerify, }},
207-266: Consider extracting common TLS configuration logic.The TLS setup code for certificate-based (lines 207-235) and token-based (lines 236-266) authentication contains significant duplication. A helper function could reduce this:
♻️ Suggested refactor
func setupTLSConfig(caFile string, reloader func() (*cert.CertConfig, error), dialer *grpc.GRPCDialer) error { tlsConfig, err := cert.AutoLoadTLSConfig(*initialCertConfig*, reloader, dialer) if err != nil { return err } dialer.TLSConfig = tlsConfig return nil }Then use it in both branches to eliminate duplication while keeping the cert/token-specific setup separate.
examples/manifestwork/README.md (1)
218-218: Optional: Add language specifiers to fenced code blocks.Markdownlint reports missing language specifiers in several output example blocks. Consider adding language hints for better rendering:
-``` +```text Apply manifestwork (opid=3bd7d...)Apply this pattern to output blocks at lines 218, 224, 230, 263, and 296.
Also applies to: 224-224, 230-230, 263-263, 296-296
pkg/client/cloudevents/grpcsource/watcherstore.go (1)
219-257: Consider accepting context parameter for consistency.The
Syncmethod uses the receiver'sm.ctxforPageListcalls (line 238) and logging (lines 228, 250), while all other public methods (GetWatcher,Get,List,ListAll,registerWatcher) now accept acontext.Contextparameter. This inconsistency makes the API less uniform.If
Syncis a background operation initiated by the receiver's lifecycle, usingm.ctxmay be intentional. However, accepting a context parameter would allow callers to propagate operation IDs and cancellation signals, improving observability and control.♻️ Proposed refactor to accept context parameter
-func (m *RESTFulAPIWatcherStore) Sync() error { +func (m *RESTFulAPIWatcherStore) Sync(ctx context.Context) error { m.RLock() defer m.RUnlock() if len(m.watchers) == 0 { // there are no watchers, do nothing return nil } - m.logger.Info(m.ctx, "sync the works for current watchers (source=%s)", m.sourceID) + m.logger.Info(ctx, "sync the works for current watchers (source=%s)", m.sourceID) namespaces := []string{} for namespace := range m.watchers { namespaces = append(namespaces, namespace) } search := ToSyncSearch(m.sourceID, namespaces) // for sync, we need list all works with the search condition from maestro server - rbs, _, err := PageList(m.ctx, m.logger, m.apiClient, search, metav1.ListOptions{}) + rbs, _, err := PageList(ctx, m.logger, m.apiClient, search, metav1.ListOptions{}) if err != nil { return err } // save the works to a queue for _, rb := range rbs.Items { work, err := ToManifestWork(&rb) if err != nil { return err } - m.logger.Debug(m.ctx, "enqueue the work %s/%s (source=%s)", work.Namespace, work.Name, m.sourceID) + m.logger.Debug(ctx, "enqueue the work %s/%s (source=%s)", work.Namespace, work.Name, m.sourceID) if err := m.workQueue.Add(work); err != nil { return err } } return nil }pkg/logger/operationid_middleware.go (1)
23-41: Handle type assertion failure explicitly.Line 38 discards the
okreturn from the type assertion:opID, _ := ctx.Value(contextOPIDKey).(string). If the context value is not a string,opIDsilently becomes an empty string, and line 40 sets an emptyX-Operation-IDheader.While this may not break functionality (since
withOpIDgenerated a valid string immediately before), defensive programming suggests checking the assertion or adding a comment explaining why it's safe to ignore.♻️ Proposed fix for explicit handling
} else { // If no operationID from header, get it from context or generate a new one ctx = withOpID(r.Context(), contextOPIDKey) - opID, _ := ctx.Value(contextOPIDKey).(string) + opID, ok := ctx.Value(contextOPIDKey).(string) + if !ok { + // This should never happen since withOpID just set it, but be defensive + opID = ksuid.New().String() + } // Set the generated operation ID in the request header for consistency r.Header.Set(OpIDHeader, opID) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (2)
data/generated/openapi/openapi.gois excluded by!**/generated/**go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
cmd/maestro/main.gocmd/maestro/server/grpc_broker.gocmd/maestro/server/logging/request_logging_middleware.gocmd/maestro/server/logging/writer.goexamples/cloudevents/grpcclient.goexamples/manifestwork/README.mdexamples/manifestwork/client-a/main.goexamples/manifestwork/client-b/main.goexamples/manifestwork/client.goexamples/manifestwork/client/README.mdexamples/manifestwork/client/main.goexamples/manifestwork/manifestwork.jsongo.modopenapi/openapi.yamlpkg/api/openapi/.openapi-generator/VERSIONpkg/api/openapi/README.mdpkg/api/openapi/api/openapi.yamlpkg/api/openapi/api_default.gopkg/api/openapi/docs/DefaultAPI.mdpkg/auth/auth_middleware.gopkg/auth/helpers.gopkg/client/cloudevents/grpcsource/client.gopkg/client/cloudevents/grpcsource/pager.gopkg/client/cloudevents/grpcsource/util.gopkg/client/cloudevents/grpcsource/watcherstore.gopkg/db/transaction_middleware.gopkg/handlers/errors.gopkg/handlers/framework.gopkg/logger/operationid_middleware.gopkg/services/resource.gopkg/services/resource_test.gotest/e2e/pkg/suite_test.gotest/performance/pkg/hub/store/createonly.gotest/store.go
💤 Files with no reviewable changes (8)
- examples/manifestwork/client-b/main.go
- examples/cloudevents/grpcclient.go
- cmd/maestro/main.go
- examples/manifestwork/client/README.md
- examples/manifestwork/client-a/main.go
- test/e2e/pkg/suite_test.go
- examples/manifestwork/client/main.go
- pkg/client/cloudevents/grpcsource/client.go
🚧 Files skipped from review as they are similar to previous changes (8)
- cmd/maestro/server/grpc_broker.go
- cmd/maestro/server/logging/request_logging_middleware.go
- pkg/api/openapi/api/openapi.yaml
- pkg/handlers/framework.go
- pkg/db/transaction_middleware.go
- pkg/handlers/errors.go
- pkg/client/cloudevents/grpcsource/pager.go
- pkg/api/openapi/.openapi-generator/VERSION
🧰 Additional context used
🧬 Code graph analysis (6)
pkg/auth/helpers.go (2)
pkg/errors/errors.go (2)
ServiceErrorCode(61-61)New(102-116)pkg/logger/operationid_middleware.go (1)
OpIDHeader(17-17)
pkg/client/cloudevents/grpcsource/watcherstore.go (1)
pkg/client/cloudevents/grpcsource/pager.go (1)
PageList(21-84)
pkg/auth/auth_middleware.go (1)
pkg/errors/errors.go (1)
ErrorUnauthorized(43-43)
examples/manifestwork/client.go (2)
pkg/client/cloudevents/grpcsource/client.go (1)
NewMaestroGRPCSourceWorkClient(19-63)pkg/client/cloudevents/grpcsource/util.go (1)
ToWorkPatch(199-243)
pkg/services/resource_test.go (2)
pkg/api/openapi/model_list.go (1)
List(23-28)test/performance/pkg/util/util.go (1)
ClusterName(22-24)
test/performance/pkg/hub/store/createonly.go (2)
pkg/api/openapi/model_list.go (1)
List(23-28)pkg/api/resource_types.go (1)
ResourceList(29-29)
🪛 markdownlint-cli2 (0.18.1)
examples/manifestwork/README.md
218-218: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
224-224: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
230-230: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
263-263: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
296-296: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
pkg/api/openapi/docs/DefaultAPI.md
382-382: Hard tabs
Column: 1
(MD010, no-hard-tabs)
383-383: Hard tabs
Column: 1
(MD010, no-hard-tabs)
385-385: Hard tabs
Column: 1
(MD010, no-hard-tabs)
386-386: Hard tabs
Column: 1
(MD010, no-hard-tabs)
387-387: Hard tabs
Column: 1
(MD010, no-hard-tabs)
413-413: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
413-413: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
520-520: Hard tabs
Column: 1
(MD010, no-hard-tabs)
521-521: Hard tabs
Column: 1
(MD010, no-hard-tabs)
523-523: Hard tabs
Column: 1
(MD010, no-hard-tabs)
524-524: Hard tabs
Column: 1
(MD010, no-hard-tabs)
525-525: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e-broadcast-subscription
- GitHub Check: e2e-with-istio
- GitHub Check: upgrade
- GitHub Check: e2e
🔇 Additional comments (28)
cmd/maestro/server/logging/writer.go (1)
32-39: LGTM! Parameterizing log level improves flexibility.The refactor correctly replaces the hardcoded
V(4)with a configurablelogLevelparameter, enabling dynamic log level control for different request types (e.g., healthcheck vs. regular requests). The error path appropriately continues usingError()regardless of the requested log level.test/performance/pkg/hub/store/createonly.go (1)
21-23: LGTM! Context parameters correctly added to align with interface changes.The addition of
context.Contextparameters toGetWatcher,List,ListAll, andGetmethods correctly aligns these stub implementations with the broader repository changes propagating context through watcher/resource-store APIs. Since these are stub implementations that return errors or nil values, not using the context is acceptable.Also applies to: 43-45, 47-49, 51-53
go.mod (1)
58-60: Verify open-cluster-management.io dependencies are compatible with maestro.The three open-cluster-management.io packages (api, ocm, sdk-go) are properly imported across 30+ files in critical components including context propagation, operation ID tracing (operationid_middleware.go), and GRPC communication. Basic import resolution succeeds, but full compatibility verification requires running build/test suite or reviewing version changelogs for potential breaking API changes.
pkg/api/openapi/README.md (1)
10-10: LGTM - Generator version updated.The version bump to 7.18.0 is consistent with the OpenAPI generator upgrade across the PR.
openapi/openapi.yaml (1)
50-53: LGTM - Operation ID key header added for tracing.The X-Operation-ID-Key header enables clients to specify a custom operation ID key for request tracing, aligning with the PR's objective to support operation ID propagation from source clients.
test/store.go (1)
4-4: LGTM - Test mock updated to match interface.The List method signature correctly accepts context.Context to match the updated interface, using a blank identifier since the mock implementation doesn't require context handling. This is standard practice for test mocks.
Also applies to: 129-129
examples/manifestwork/client.go (3)
113-116: LGTM - Operation ID tracing well implemented.The operation ID generation and context propagation aligns perfectly with the PR's objective to enable tracing. The comment about customization via
SetContextTracingOPIDKeyis helpful for users.
320-361: LGTM - Create-or-update logic well implemented.The apply function correctly implements create-or-update semantics:
- Attempts to get existing work
- Creates if not found
- Uses
ToWorkPatchfor proper merge-patch updates on existing worksThis aligns with common kubectl-style apply behavior.
379-407: LGTM - Watch implementation correctly handles lifecycle.The watch function properly:
- Creates and cleans up the watcher with
defer watcher.Stop()- Respects context cancellation
- Handles channel closure
- Type-checks event objects before processing
examples/manifestwork/README.md (2)
1-26: LGTM - Documentation correctly reflects API signature change.The updated constructor signature with the mandatory
loggerparameter is accurately documented, matching the code changes in the PR.
63-213: LGTM - Comprehensive CLI documentation.The documentation thoroughly covers:
- All available commands with clear usage examples
- Both local development and production scenarios
- TLS configuration options
- Important security considerations (cluster role for gRPC pub/sub)
This provides excellent guidance for users of the new CLI client.
pkg/api/openapi/api_default.go (2)
845-981: LGTM - Generated OpenAPI client correctly implements X-Operation-ID-Key header support.The generated code properly adds the
xOperationIDKeyfield, builder method, and conditional header emission for the resource bundles list endpoint. The implementation follows the established pattern for the existingxOperationIDheader.
1195-1277: LGTM - Consistent implementation for the get-by-id endpoint.The same pattern is correctly applied to
ApiApiMaestroV1ResourceBundlesIdGetRequest, maintaining consistency across both endpoints.pkg/auth/auth_middleware.go (1)
31-32: LGTM - Correctly passes the HTTP request to handleError for operation ID extraction.The change aligns with the updated
handleErrorsignature inpkg/auth/helpers.go, enabling operation ID propagation from the request header for improved log tracing.pkg/services/resource_test.go (1)
89-107: LGTM - Test correctly updated to use the new List signature with context.The test properly passes
context.Background()as the first argument to match the updatedResourceService.Listinterface, which now requires a context for operation ID propagation and tracing support.pkg/client/cloudevents/grpcsource/util.go (3)
26-36: LGTM - Thread-safe initialization of operation ID tracing key.Using
atomic.Valuefor lock-free reads andsync.Oncefor single initialization is the correct approach for this package-level state.
289-298: LGTM - Safe context value retrieval with proper type assertion.The function correctly uses the comma-ok idiom for type assertion, returning empty strings when the operation ID is not present in the context.
259-277: Acknowledged race condition with sdk-go's global state.The TODO on line 274 correctly identifies that modifying
sdkgologging.DefaultContextTracingKeysdirectly is only safe if called once at startup before any concurrent readers. Thesync.Onceguard ensures this function executes at most once, and the atomic Store/Load pattern forcontextTracingOPIDKeyprovides the necessary memory barriers for thread-safe visibility. The implementation is safe under the documented constraint.Consider tracking coordination with sdk-go maintainers to provide a thread-safe API for setting default tracing keys, eliminating the need for this startup-only pattern.
pkg/auth/helpers.go (1)
14-31: LGTM - Operation ID correctly extracted from request header and propagated to logs/responses.The implementation properly:
- Extracts the operation ID from the
X-Operation-IDheader- Includes it in structured log fields for both client and server errors
- Passes it to
AsOpenapiErrorfor inclusion in the error response payloadThis enables end-to-end request tracing as intended by the PR.
pkg/services/resource.go (2)
36-36: LGTM - Interface correctly updated to accept context.Adding
context.Contextas the first parameter toListenables proper context propagation for cancellation, deadlines, and operation ID tracing.
327-333: LGTM - Context properly propagated to DAO layer.The implementation now correctly uses the provided context when calling
FindByConsumerName, replacing the previouscontext.TODO()usage. This ensures operation ID tracing and cancellation signals flow through the entire call chain.pkg/api/openapi/docs/DefaultAPI.md (1)
413-414: Table formatting at line 413 is correct and requires no changes.The table row for
xOperationIDKeyis properly formatted with all four columns (Name, Type, Description, Notes). Empty cells in Markdown tables are valid syntax and will render correctly. The row includes the trailing|delimiter creating the fourth column, so the table is not malformed.Likely an incorrect or invalid review comment.
pkg/client/cloudevents/grpcsource/watcherstore.go (4)
75-113: LGTM! Context properly threaded through GetWatcher.The context parameter is correctly propagated to
PageList,registerWatcher, and logging calls, enabling proper operation ID tracing for watch initialization.
127-153: LGTM! Operation ID extraction and propagation implemented correctly.The Get method properly:
- Uses the provided context for API calls
- Extracts operation ID via
getOperationID(ctx)- Conditionally sets operation ID headers when present
- Threads context through logging
157-193: LGTM! List method context handling is correct.Context is properly threaded through
PageListand logging calls.
290-304: LGTM! Watcher registration properly uses context.The context is correctly passed to watcher creation and logging, enabling proper operation tracing.
pkg/logger/operationid_middleware.go (2)
61-68: LGTM! Dynamic key parameter enables flexible operation ID management.The refactored
withOpIDcorrectly accepts a configurable key and checks for existing values before generating a new KSUID.
71-73: LGTM! Parameterized attribute key supports dynamic operation ID naming.The updated signature allows callers to specify the attribute key, aligning with the middleware's dynamic key resolution strategy.
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)
pkg/logger/operationid_middleware.go (1)
20-59: ValidateopIDfrom headers and castklog.WithValues()key tostring.Two issues:
klog.WithValues key type mismatch (line 56): Passing
contextOPIDKey(a customsdkgologging.ContextTracingKeytype) as a key is inconsistent with the rest of the codebase, which uses string literals. Cast it:WithValues(string(contextOPIDKey), opID).Missing opID validation (line 33): The
opIDheader value is accepted without validation, unlikeopIDKeywhich is checked withisSafeOpIDKey(). This allows arbitrary header content to flow into context, logs, OTEL spans, and Sentry—creating log injection and oversized-value risks. Validate it before storing.Proposed fix
// Get operation ID from request header if existed opID := r.Header.Get(OpIDHeader) - if opID != "" { + if opID != "" && isSafeOpIDKey(opID) { // Add operationID to context (override if existed) ctx = context.WithValue(ctx, contextOPIDKey, opID) } else { @@ - logger := klog.FromContext(ctx).WithValues(contextOPIDKey, opID) + logger := klog.FromContext(ctx).WithValues(string(contextOPIDKey), opID) ctx = klog.NewContext(ctx, logger) handler.ServeHTTP(w, r.WithContext(ctx))
🤖 Fix all issues with AI agents
In @pkg/logger/operationid_middleware.go:
- Around line 62-69: withOpID currently treats any non-nil value as present
which lets wrong types or empty strings bypass ID generation; change the
presence check to assert a non-empty string: retrieve ctx.Value(opIDKey), do a
type assertion to string (e.g., opIDStr, ok := ctx.Value(opIDKey).(string)) and
only return the original ctx if ok && opIDStr != ""; otherwise generate a new
ksuid and store it with context.WithValue as before.
🧹 Nitpick comments (13)
examples/manifestwork/client.go (6)
48-84: Consider using a standard flag parsing library.The custom argument parsing logic is complex and may have edge cases (e.g., negative numbers like
--timeout -1would be misinterpreted as flags because of thestrings.HasPrefix(arg, "-")check on Line 57). Consider using a well-established CLI library likecobraorurfave/clithat handles these cases correctly and provides better user experience with subcommands, help text, and validation.
99-106: Signal handling only for watch command.Signal handling for graceful shutdown is only set up for the
watchcommand. While watch is likely the longest-running operation, consider whether other commands might also benefit from interrupt handling, especially if network operations or file I/O could be slow.
207-266: Reduce TLS configuration duplication.The TLS setup logic is duplicated between certificate-based auth (lines 207-235) and token-based auth (lines 236-266). The
AutoLoadTLSConfigcalls and cert embedding logic are nearly identical.♻️ Proposed refactoring to extract common TLS setup
+func setupTLSConfig(grpcServerCAFile, grpcClientCertFile, grpcClientKeyFile string, dialer *grpc.GRPCDialer) (*tls.Config, error) { + certConfig := cert.CertConfig{ + CAFile: grpcServerCAFile, + ClientCertFile: grpcClientCertFile, + ClientKeyFile: grpcClientKeyFile, + } + if err := certConfig.EmbedCerts(); err != nil { + return nil, err + } + return cert.AutoLoadTLSConfig( + certConfig, + func() (*cert.CertConfig, error) { + certConfig := cert.CertConfig{ + CAFile: grpcServerCAFile, + ClientCertFile: grpcClientCertFile, + ClientKeyFile: grpcClientKeyFile, + } + if err := certConfig.EmbedCerts(); err != nil { + return nil, err + } + return &certConfig, nil + }, + dialer, + ) +} if *grpcServerCAFile != "" && *grpcClientCertFile != "" && *grpcClientKeyFile != "" { - // Setup TLS if certificates are provided - certConfig := cert.CertConfig{ - CAFile: *grpcServerCAFile, - ClientCertFile: *grpcClientCertFile, - ClientKeyFile: *grpcClientKeyFile, - } - if err := certConfig.EmbedCerts(); err != nil { - return nil, err - } - tlsConfig, err := cert.AutoLoadTLSConfig( - certConfig, - func() (*cert.CertConfig, error) { - certConfig := cert.CertConfig{ - CAFile: *grpcServerCAFile, - ClientCertFile: *grpcClientCertFile, - ClientKeyFile: *grpcClientKeyFile, - } - if err := certConfig.EmbedCerts(); err != nil { - return nil, err - } - return &certConfig, nil - }, - grpcOptions.Dialer, - ) + tlsConfig, err := setupTLSConfig(*grpcServerCAFile, *grpcClientCertFile, *grpcClientKeyFile, grpcOptions.Dialer) if err != nil { return nil, err } grpcOptions.Dialer.TLSConfig = tlsConfig } else if *grpcServerCAFile != "" && *grpcClientTokenFile != "" { - // Setup token if provided token, err := os.ReadFile(*grpcClientTokenFile) if err != nil { return nil, err } grpcOptions.Dialer.Token = strings.TrimSpace(string(token)) - certConfig := cert.CertConfig{ - CAFile: *grpcServerCAFile, - } - if err := certConfig.EmbedCerts(); err != nil { - return nil, err - } - tlsConfig, err := cert.AutoLoadTLSConfig( - certConfig, - func() (*cert.CertConfig, error) { - certConfig := cert.CertConfig{ - CAFile: *grpcServerCAFile, - } - if err := certConfig.EmbedCerts(); err != nil { - return nil, err - } - return &certConfig, nil - }, - grpcOptions.Dialer, - ) + tlsConfig, err := setupTLSConfig(*grpcServerCAFile, "", "", grpcOptions.Dialer) if err != nil { return nil, err } grpcOptions.Dialer.TLSConfig = tlsConfig }
308-315: Consider dynamic column widths for better formatting.The fixed-width format on Line 308 uses
%-15sfor the Name column, which may truncate longer names and misalign the table. Consider either:
- Using dynamic column widths based on the actual data
- Increasing the Name column width (e.g., to 30 or 40)
- Using a table formatting library like
tablewriterorgo-pretty/table
398-404: Consider logging and continuing on unexpected object type.Line 400 returns an error when an unexpected object type is encountered, which terminates the watch. Depending on your requirements, you might want to log the error and continue watching for other events instead of exiting completely.
Alternative: log and continue
work, ok := event.Object.(*workv1.ManifestWork) if !ok { - return fmt.Errorf("unexpected object type: %T\n", event.Object) + log.Printf("Warning: unexpected object type: %T, skipping event\n", event.Object) + continue } if err := printWatchEvent(event.Type, work); err != nil { return err }
409-419: Handle additional watch event types.The function only handles
Added,Modified, andDeletedevents. Kubernetes watches can also emitErrorandBookmarkevent types. Consider handling these to avoid unexpected termination of the watch:Handle additional event types
case watch.Deleted: fmt.Printf("[DELETED] Work: %s/%s (uid=%s)\n", work.Namespace, work.Name, work.UID) + case watch.Error: + fmt.Printf("[ERROR] %v\n", work) + return nil + case watch.Bookmark: + // Bookmark events can be ignored for display + return nil default: - return fmt.Errorf("unsupported event type") + fmt.Printf("[UNKNOWN:%s] Work: %s/%s (uid=%s)\n", eventType, work.Namespace, work.Name, work.UID) }examples/manifestwork/README.md (3)
52-52: Replace hard tabs with spaces.Markdown files should use spaces instead of hard tabs for consistent rendering across different editors and platforms.
Also applies to: 66-66, 69-71
100-100: Wrap bare URL in markdown link or angle brackets.The bare URL on this line should be formatted as a proper markdown link for better rendering and accessibility.
For example:
See the full [ManifestWork API](https://github.com/open-cluster-management-io/api/blob/main/work/v1/types.go) for all available options.Or with angle brackets:
See the full ManifestWork API at <https://github.com/open-cluster-management-io/api/blob/main/work/v1/types.go> for all available options.
96-99: Fix list item indentation.The list items have incorrect indentation (1 space instead of 0). Remove the leading space from these lines for proper markdown formatting.
pkg/api/openapi/docs/DefaultAPI.md (2)
413-414: Minor: Generated table has formatting inconsistency.The
xOperationIDKeyrow appears to have a trailing pipe without content in the Notes column, causing the static analysis to flag table column count mismatch. Since this is generated documentation, this would need to be addressed in the OpenAPI spec or generator template rather than manually editing here.
551-552: Same table formatting applies to the IdGet endpoint.Similar observation as above - the generated docs have the same pattern.
cmd/maestro/server/grpc_broker.go (1)
62-66: GRPCBrokerService.List lacks context propagation while other methods in the same service accept context.The
Get(),HandleStatusUpdate(), andRegisterHandler()methods all acceptcontext.Context, butList()does not. This means operation tracing and cancellation signals from gRPC handlers won't flow through the resource listing path. Currently usingcontext.Background()works but loses context propagation.Consider whether
GRPCBrokerService.Listcan accept acontext.Contextparameter. This would require verifying that the interface definition in the sdk-go package (which defines the expected method signatures) allows this change, as the service is registered viaeventServer.RegisterService()fromopen-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc.pkg/handlers/framework.go (1)
51-58: Consider usingcfg.ErrorHandlerfor consistency.Lines 51 and 57 call
handleErrordirectly, bypassing the configured error handler even though it's already initialized at line 46. While this may be intentional for basic marshaling errors, usingcfg.ErrorHandler(w, r, ...)would provide more flexibility and consistency with the rest of the function.♻️ Proposed refactor for consistency
bytes, err := io.ReadAll(r.Body) if err != nil { - handleError(w, r, errors.MalformedRequest("Unable to read request body: %s", err)) + cfg.ErrorHandler(w, r, errors.MalformedRequest("Unable to read request body: %s", err)) return } err = json.Unmarshal(bytes, &cfg.MarshalInto) if err != nil { - handleError(w, r, errors.MalformedRequest("Invalid request format: %s", err)) + cfg.ErrorHandler(w, r, errors.MalformedRequest("Invalid request format: %s", err)) return }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (2)
data/generated/openapi/openapi.gois excluded by!**/generated/**go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
cmd/maestro/main.gocmd/maestro/server/grpc_broker.gocmd/maestro/server/logging/request_logging_middleware.gocmd/maestro/server/logging/writer.goexamples/cloudevents/grpcclient.goexamples/manifestwork/README.mdexamples/manifestwork/client-a/main.goexamples/manifestwork/client-b/main.goexamples/manifestwork/client.goexamples/manifestwork/client/README.mdexamples/manifestwork/client/main.goexamples/manifestwork/manifestwork.jsongo.modopenapi/openapi.yamlpkg/api/openapi/.openapi-generator/VERSIONpkg/api/openapi/README.mdpkg/api/openapi/api/openapi.yamlpkg/api/openapi/api_default.gopkg/api/openapi/docs/DefaultAPI.mdpkg/auth/auth_middleware.gopkg/auth/helpers.gopkg/client/cloudevents/grpcsource/client.gopkg/client/cloudevents/grpcsource/pager.gopkg/client/cloudevents/grpcsource/util.gopkg/client/cloudevents/grpcsource/watcherstore.gopkg/db/transaction_middleware.gopkg/handlers/errors.gopkg/handlers/framework.gopkg/logger/operationid_middleware.gopkg/services/resource.gopkg/services/resource_test.gotest/e2e/pkg/suite_test.gotest/performance/pkg/hub/store/createonly.gotest/store.go
💤 Files with no reviewable changes (8)
- examples/cloudevents/grpcclient.go
- test/e2e/pkg/suite_test.go
- pkg/client/cloudevents/grpcsource/client.go
- examples/manifestwork/client-a/main.go
- cmd/maestro/main.go
- examples/manifestwork/client/main.go
- examples/manifestwork/client-b/main.go
- examples/manifestwork/client/README.md
🚧 Files skipped from review as they are similar to previous changes (9)
- pkg/auth/auth_middleware.go
- pkg/db/transaction_middleware.go
- pkg/api/openapi/.openapi-generator/VERSION
- pkg/api/openapi/api/openapi.yaml
- cmd/maestro/server/logging/writer.go
- pkg/client/cloudevents/grpcsource/util.go
- test/performance/pkg/hub/store/createonly.go
- openapi/openapi.yaml
- pkg/services/resource_test.go
🧰 Additional context used
🧬 Code graph analysis (6)
pkg/auth/helpers.go (2)
pkg/errors/errors.go (1)
ServiceErrorCode(61-61)pkg/logger/operationid_middleware.go (1)
OpIDHeader(17-17)
pkg/handlers/framework.go (2)
pkg/errors/errors.go (2)
ServiceError(92-99)MalformedRequest(189-191)pkg/logger/operationid_middleware.go (1)
OpIDHeader(17-17)
cmd/maestro/server/logging/request_logging_middleware.go (2)
cmd/maestro/server/logging/writer.go (1)
NewLoggingWriter(9-11)cmd/maestro/server/logging/formatter_json.go (1)
NewJSONLogFormatter(11-13)
pkg/handlers/errors.go (1)
pkg/errors/errors.go (1)
NotImplemented(177-179)
pkg/client/cloudevents/grpcsource/watcherstore.go (3)
pkg/client/cloudevents/grpcsource/pager.go (1)
PageList(21-84)pkg/api/openapi/model_list.go (1)
List(23-28)pkg/api/resource_types.go (1)
ResourceList(29-29)
pkg/services/resource.go (3)
pkg/api/openapi/model_list.go (1)
List(23-28)pkg/db/migrations/202311151850_add_resources.go (1)
Resource(10-24)pkg/api/resource_types.go (1)
Resource(14-27)
🪛 markdownlint-cli2 (0.18.1)
pkg/api/openapi/docs/DefaultAPI.md
382-382: Hard tabs
Column: 1
(MD010, no-hard-tabs)
383-383: Hard tabs
Column: 1
(MD010, no-hard-tabs)
385-385: Hard tabs
Column: 1
(MD010, no-hard-tabs)
386-386: Hard tabs
Column: 1
(MD010, no-hard-tabs)
387-387: Hard tabs
Column: 1
(MD010, no-hard-tabs)
413-413: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
413-413: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
520-520: Hard tabs
Column: 1
(MD010, no-hard-tabs)
521-521: Hard tabs
Column: 1
(MD010, no-hard-tabs)
523-523: Hard tabs
Column: 1
(MD010, no-hard-tabs)
524-524: Hard tabs
Column: 1
(MD010, no-hard-tabs)
525-525: Hard tabs
Column: 1
(MD010, no-hard-tabs)
examples/manifestwork/README.md
52-52: Hard tabs
Column: 1
(MD010, no-hard-tabs)
66-66: Hard tabs
Column: 1
(MD010, no-hard-tabs)
69-69: Hard tabs
Column: 1
(MD010, no-hard-tabs)
70-70: Hard tabs
Column: 1
(MD010, no-hard-tabs)
71-71: Hard tabs
Column: 1
(MD010, no-hard-tabs)
77-77: Bare URL used
(MD034, no-bare-urls)
93-93: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
94-94: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
95-95: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
96-96: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
97-97: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
98-98: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
99-99: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
100-100: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
101-101: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: e2e
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e-with-istio
- GitHub Check: upgrade
- GitHub Check: e2e-broadcast-subscription
🔇 Additional comments (25)
examples/manifestwork/client.go (3)
282-295: LGTM!Clean implementation with proper error handling and formatted JSON output.
320-361: LGTM!Well-structured apply logic with proper existence checking and appropriate use of create vs. patch operations. The error handling distinguishes between "not found" and other errors correctly.
363-377: LGTM!Good practice to retrieve the UID before deletion for auditing. The sequential Get-then-Delete approach is appropriate for a CLI tool.
go.mod (4)
51-55: k8s.io version consistency looks good.All Kubernetes ecosystem modules are consistently bumped to v0.34.3 (from v0.34.2), including both direct and indirect dependencies. This patch-level consistency across the k8s.io dependency tree is appropriate for context-aware API surface changes.
Also applies to: 197-199
45-45: gRPC minor version bump aligns with context propagation changes.The bump to google.golang.org/grpc v1.78.0 is a minor version increment, typical for adding context-aware features. This is consistent with the PR's goal of propagating operation IDs via context.
127-127: The v0.9.0 upgrade is justified and compatible.The version bump from v0.7.7 → v0.9.0 is driven by Go toolchain improvements and bugfixes, not API-breaking changes. Your project's Go 1.25.0 requirement is well-aligned with v0.9.0. Since your OpenAPI code generation tooling (v5.4.0) is current, the upgrade poses no serialization or compatibility risk.
58-60: Verify OCM module versions with the open-cluster-management.io project.The open-cluster-management.io modules use pseudo-versions with 2026-01-08 timestamps (api v1.1.1-0.20260108015315-68cef17a0643, ocm v1.1.1-0.20260108015314-635b0ff7e9cf, sdk-go v1.1.1-0.20260108080638-c607eaaa5d12). While these follow valid Go pseudo-version syntax, no public release information exists for OCM v1.1.1 pre-releases; the latest documented release is v1.0.0 (June 2025). Confirm with the OCM maintainers that these versions correspond to actual commits in the respective repositories and are accessible via the Go module proxy.
pkg/api/openapi/README.md (1)
8-12: Doc version bump looks fine—just keep it in sync with the pinned generator version source-of-truth.test/store.go (1)
3-6: Signature update forList(ctx, ...)is fine for the test store.Also applies to: 129-131
cmd/maestro/server/logging/request_logging_middleware.go (1)
19-47: Verify middleware ordering: request logs will only include opID if the opID logger is already inr.Context()before this runs.
Since request logging happens beforenext.ServeHTTP(...), any opID injection done in later middleware won’t show up on the request log line.pkg/client/cloudevents/grpcsource/pager.go (1)
29-50: The review comment is incorrect. ThegetOperationIDfunction guarantees that bothopIDKeyandoperationIDare either set together or both empty—it cannot return a non-emptyoperationIDwith an emptyopIDKey. The key is derived from a staticcontextTracingOPIDKeythat is always initialized with a non-empty value at startup. WhenopIDis found in the context,string(key)is always non-empty; whenopIDis not found, both return values are empty strings. The existing pattern used inwatcherstore.goconfirms this is safe without additional guards.Likely an incorrect or invalid review comment.
pkg/client/cloudevents/grpcsource/watcherstore.go (5)
75-112: LGTM! Context propagation for GetWatcher is well-implemented.The method correctly accepts and propagates
ctxthroughPageList,registerWatcher, and logging calls. This enables proper operation ID tracing for watch operations.
127-152: LGTM! Get method properly propagates context for API calls and logging.The context is correctly used for the REST API call, operation ID extraction via
getOperationID(ctx), and debug logging.
157-193: LGTM! List method context propagation is correct.Context flows through to
PageListand logging, enabling operation tracing for list operations.
219-257: Sync uses receiver contextm.ctxintentionally.This is appropriate since
Sync()is called from internal goroutines (line 68) and doesn't receive an external context. The receiver contextm.ctxis the correct choice here.
290-304: LGTM! registerWatcher correctly uses the provided context.The context is properly propagated to
newWorkWatcherand logging calls.pkg/api/openapi/api_default.go (2)
845-981: Generated code for X-Operation-ID-Key support looks correct.The auto-generated code properly adds the
xOperationIDKeyfield, builder method, and header emission for the ResourceBundles GET endpoint. This aligns with the OpenAPI spec changes for operation ID tracing.
1195-1277: Generated code for ResourceBundlesIdGet X-Operation-ID-Key support is correct.The changes mirror the list endpoint pattern, adding proper header support for single resource retrieval.
pkg/handlers/errors.go (1)
68-78: LGTM! Error handler calls updated consistently.All three stub methods (Create, Patch, Delete) now pass the http.Request to
handleError, enabling operation ID extraction for error responses. This aligns with the PR objective of tracing operations via operation IDs.pkg/auth/helpers.go (1)
14-32: LGTM! Operation ID tracing properly implemented in auth error handling.The changes correctly:
- Extract the operation ID from the
X-Operation-IDheader- Include it in log messages for both client (4xx) and server (5xx) errors
- Pass it to
AsOpenapiErrorfor inclusion in the error response payloadThis enables end-to-end operation tracing through auth failures.
pkg/services/resource.go (2)
36-36: LGTM! Interface updated to accept context.The
Listmethod signature now properly includescontext.Context, enabling cancellation, deadline propagation, and tracing.
327-333: LGTM! Proper context propagation replacing context.TODO().The implementation now correctly passes the received
ctxtoFindByConsumerNameinstead of usingcontext.TODO(). This is a good improvement that enables proper context propagation through the DAO layer.pkg/handlers/framework.go (3)
30-30: LGTM! Signature change aligns with operation ID tracing design.The shift from
context.Contextto*http.Requestenables error handlers to access the operation ID header, supporting the PR's goal of request-based tracing.
80-128: LGTM! Error handling updated consistently.The
handleDelete,handleGet, andhandleListfunctions have been correctly updated to use the new error handler signature. All invocations consistently pass(w, r, err)and properly initialize the default error handler.
33-42: Well-implemented request-based error handling with verified middleware integration.The function correctly extracts the logger from the request context and retrieves the operation ID from the request header. The
OperationIDMiddleware(inpkg/logger/operationid_middleware.go) properly ensures thatmaestrologger.OpIDHeaderis set before handlers execute—either from an incoming header or by generating a new operation ID.
Signed-off-by: Wei Liu <liuweixa@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/manifestwork/README.md (1)
218-302: Add language specifiers to fenced code blocks.Several code blocks in the example output sections are missing language specifiers, which affects documentation quality and consistent rendering. Add appropriate language identifiers (e.g.,
text,console, orjson) to these blocks.Based on static analysis tool markdownlint-cli2, the following lines need language specifiers:
- Line 218: Output example for "Creating a work"
- Line 224: Output example for "Updating an existing work"
- Line 230: Output example for "Watching changes"
- Line 263: Output example for "Getting work"
- Line 296: Output example for "Listing works"
📝 Proposed fixes
**Creating a work:** -``` +```text Apply manifestwork (opid=3bd7d...) Work 77112687-f4ac-42d1-b8b8-6f999428c19d/nginx-work (uid=abc123...) created successfullyUpdating an existing work:
-+text
Apply manifestwork (opid=5ad7d...)
Work 77112687-f4ac-42d1-b8b8-6f999428c19d/nginx-work (uid=abc123...) updated successfully**Watching changes with `--print-work-details`:** -``` +```text Watch manifestwork (opid=9ed7d...) [MODIFIED] Work: 77112687-f4ac-42d1-b8b8-6f999428c19d/nginx-work (uid=abc123...) +``` + +```json { "metadata": { "name": "nginx-work", @@ -260,7 +262,7 @@Getting work:
-+text
Get manifestwork 114916d9-f470-4905-8793-785d58ee0f8d/nginx-work (opid=7cd7d...)
++ +json
{
"metadata": {
"name": "nginx-work",
@@ -292,7 +296,7 @@**Listing works:** -``` +```text List manifestworks (opid=9dd7d...): Consumer Name UID Created 77112687... nginx-work 941681df... 2026-01-08T17:04:58+08:00Note: For the "Watching changes" and "Getting work" sections, split the text output and JSON output into separate code blocks for better syntax highlighting.
🤖 Fix all issues with AI agents
In @cmd/maestro/server/logging/request_logging_middleware.go:
- Around line 17-29: The current middleware sets logLevel := 2 (and doLog true)
which may cause excessive/ sensitive request/response logging if
NewJSONLogFormatter includes headers/bodies; change the default verbosity to a
safer higher level (e.g., logLevel := 4) and/or make it configurable (read from
config/env or a passed option) so production uses the higher default while
allowing a dev override; update the logic around path, doLog and logLevel in the
request_logging_middleware (the variables path, doLog, logLevel and the
NewJSONLogFormatter usage) to honour the new configurable default and document
or gate any lower verbosity to non-production environments only.
In @pkg/api/openapi/docs/DefaultAPI.md:
- Around line 382-383: Add descriptive text for the two header parameters in the
OpenAPI spec by updating the parameter entries for X-Operation-ID-Key and
X-Operation-ID (the header parameter names) to include clear descriptions; for
example set X-Operation-ID-Key to "Context key for storing the operation ID in
distributed tracing" and add a similar explanatory description for
X-Operation-ID (e.g., "Operation identifier propagated in headers for request
tracing"), so the generated docs (DefaultAPI.md) show non-empty description
cells.
🧹 Nitpick comments (4)
pkg/api/openapi/docs/DefaultAPI.md (1)
382-383: Replace hard tabs with spaces in code examples.The static analysis tool flagged hard tabs in the Go code examples. While this is generated documentation, it should use spaces for consistency with Go conventions.
If you control the OpenAPI generator configuration, ensure it's set to use spaces instead of tabs for code examples in generated documentation.
Also applies to: 387-387, 520-521, 525-525
examples/manifestwork/client.go (2)
54-81: Consider simplifying the argument parsing logic.The custom parsing logic to allow flags anywhere is more complex than necessary. Go's
flagpackage already supports flags before or after positional arguments if you useflag.Parse()after defining all flags, and then accessflag.Args()for remaining arguments.This would simplify the code significantly while maintaining the same functionality.
♻️ Simplified approach
- // Custom argument parsing to allow flags anywhere - var command string - var commandArgs []string - var otherArgs []string - - // Separate command/args from flags - for i := 1; i < len(os.Args); i++ { - arg := os.Args[i] - if strings.HasPrefix(arg, "-") { - // Keep the flag token itself - otherArgs = append(otherArgs, arg) - - // If this flag has a value (-flag=value or -flag value). Do not consume - // the next token for bool flags (e.g. -print-work-details). - if !strings.Contains(arg, "=") && i+1 < len(os.Args) && !strings.HasPrefix(os.Args[i+1], "-") { - name := strings.TrimLeft(arg, "-") - if f := flag.CommandLine.Lookup(name); f != nil { - type boolFlag interface{ IsBoolFlag() bool } - if bf, ok := f.Value.(boolFlag); ok && bf.IsBoolFlag() { - continue - } - i++ - otherArgs = append(otherArgs, os.Args[i]) - } - } - } else { - if command == "" { - command = arg - } else { - commandArgs = append(commandArgs, arg) - } - } - } - - // Parse flags - flag.CommandLine.Parse(otherArgs) + // Parse flags (supports flags anywhere) + flag.Parse() + + // Get command and args + args := flag.Args() + var command string + var commandArgs []string + if len(args) > 0 { + command = args[0] + commandArgs = args[1:] + }
207-266: Reduce code duplication in TLS configuration setup.Both the certificate-based and token-based TLS setup paths contain duplicated logic:
- Creating
certConfigand callingEmbedCerts()- Defining a reload function that repeats the same steps
- Calling
AutoLoadTLSConfigConsider extracting this into a helper function to improve maintainability.
♻️ Proposed refactoring
Add a helper function:
func setupTLSConfig( caFile, clientCertFile, clientKeyFile string, dialer *grpc.GRPCDialer, ) (*tls.Config, error) { certConfig := cert.CertConfig{ CAFile: caFile, ClientCertFile: clientCertFile, ClientKeyFile: clientKeyFile, } if err := certConfig.EmbedCerts(); err != nil { return nil, err } reloadFunc := func() (*cert.CertConfig, error) { cfg := cert.CertConfig{ CAFile: caFile, ClientCertFile: clientCertFile, ClientKeyFile: clientKeyFile, } if err := cfg.EmbedCerts(); err != nil { return nil, err } return &cfg, nil } return cert.AutoLoadTLSConfig(certConfig, reloadFunc, dialer) }Then simplify both branches:
if *grpcServerCAFile != "" && *grpcClientCertFile != "" && *grpcClientKeyFile != "" { - // Setup TLS if certificates are provided - certConfig := cert.CertConfig{ - CAFile: *grpcServerCAFile, - ClientCertFile: *grpcClientCertFile, - ClientKeyFile: *grpcClientKeyFile, - } - if err := certConfig.EmbedCerts(); err != nil { - return nil, err - } - tlsConfig, err := cert.AutoLoadTLSConfig( - certConfig, - func() (*cert.CertConfig, error) { - certConfig := cert.CertConfig{ - CAFile: *grpcServerCAFile, - ClientCertFile: *grpcClientCertFile, - ClientKeyFile: *grpcClientKeyFile, - } - if err := certConfig.EmbedCerts(); err != nil { - return nil, err - } - return &certConfig, nil - }, - grpcOptions.Dialer, - ) + tlsConfig, err := setupTLSConfig( + *grpcServerCAFile, + *grpcClientCertFile, + *grpcClientKeyFile, + grpcOptions.Dialer, + ) if err != nil { return nil, err } grpcOptions.Dialer.TLSConfig = tlsConfig } else if *grpcServerCAFile != "" && *grpcClientTokenFile != "" { - // Setup token if provided token, err := os.ReadFile(*grpcClientTokenFile) if err != nil { return nil, err } grpcOptions.Dialer.Token = strings.TrimSpace(string(token)) - certConfig := cert.CertConfig{ - CAFile: *grpcServerCAFile, - } - if err := certConfig.EmbedCerts(); err != nil { - return nil, err - } - tlsConfig, err := cert.AutoLoadTLSConfig( - certConfig, - func() (*cert.CertConfig, error) { - certConfig := cert.CertConfig{ - CAFile: *grpcServerCAFile, - } - if err := certConfig.EmbedCerts(); err != nil { - return nil, err - } - return &certConfig, nil - }, - grpcOptions.Dialer, - ) + tlsConfig, err := setupTLSConfig(*grpcServerCAFile, "", "", grpcOptions.Dialer) if err != nil { return nil, err } grpcOptions.Dialer.TLSConfig = tlsConfig }cmd/maestro/server/logging/writer.go (1)
32-38: Good change: per-call verbosity vialogLevel, but consider avoiding “magic ints” at call sites.
This aligns the writer with middleware-controlled verbosity; I’d just recommend drivinglogLevelvia named constants (or a typed alias) to keep “2/4” from spreading.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (2)
data/generated/openapi/openapi.gois excluded by!**/generated/**go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
cmd/maestro/main.gocmd/maestro/server/grpc_broker.gocmd/maestro/server/logging/request_logging_middleware.gocmd/maestro/server/logging/writer.goexamples/cloudevents/grpcclient.goexamples/manifestwork/README.mdexamples/manifestwork/client-a/main.goexamples/manifestwork/client-b/main.goexamples/manifestwork/client.goexamples/manifestwork/client/README.mdexamples/manifestwork/client/main.goexamples/manifestwork/manifestwork.jsongo.modopenapi/openapi.yamlpkg/api/openapi/.openapi-generator/VERSIONpkg/api/openapi/README.mdpkg/api/openapi/api/openapi.yamlpkg/api/openapi/api_default.gopkg/api/openapi/docs/DefaultAPI.mdpkg/auth/auth_middleware.gopkg/auth/helpers.gopkg/client/cloudevents/grpcsource/client.gopkg/client/cloudevents/grpcsource/pager.gopkg/client/cloudevents/grpcsource/util.gopkg/client/cloudevents/grpcsource/watcherstore.gopkg/db/transaction_middleware.gopkg/handlers/errors.gopkg/handlers/framework.gopkg/logger/operationid_middleware.gopkg/services/resource.gopkg/services/resource_test.gotest/e2e/pkg/suite_test.gotest/performance/pkg/hub/store/createonly.gotest/store.go
💤 Files with no reviewable changes (8)
- examples/manifestwork/client/README.md
- examples/manifestwork/client/main.go
- examples/cloudevents/grpcclient.go
- test/e2e/pkg/suite_test.go
- examples/manifestwork/client-a/main.go
- cmd/maestro/main.go
- pkg/client/cloudevents/grpcsource/client.go
- examples/manifestwork/client-b/main.go
🚧 Files skipped from review as they are similar to previous changes (12)
- pkg/db/transaction_middleware.go
- pkg/api/openapi/README.md
- pkg/client/cloudevents/grpcsource/pager.go
- openapi/openapi.yaml
- cmd/maestro/server/grpc_broker.go
- test/store.go
- pkg/services/resource.go
- pkg/handlers/errors.go
- go.mod
- pkg/api/openapi/.openapi-generator/VERSION
- pkg/auth/helpers.go
- pkg/api/openapi/api/openapi.yaml
🧰 Additional context used
🧬 Code graph analysis (5)
cmd/maestro/server/logging/request_logging_middleware.go (2)
cmd/maestro/server/logging/writer.go (1)
NewLoggingWriter(9-11)cmd/maestro/server/logging/formatter_json.go (1)
NewJSONLogFormatter(11-13)
test/performance/pkg/hub/store/createonly.go (2)
pkg/api/openapi/model_list.go (1)
List(23-28)pkg/api/resource_types.go (1)
ResourceList(29-29)
examples/manifestwork/client.go (2)
pkg/client/cloudevents/grpcsource/client.go (1)
NewMaestroGRPCSourceWorkClient(19-63)pkg/client/cloudevents/grpcsource/util.go (1)
ToWorkPatch(199-243)
pkg/client/cloudevents/grpcsource/watcherstore.go (1)
pkg/client/cloudevents/grpcsource/pager.go (1)
PageList(21-84)
pkg/auth/auth_middleware.go (1)
pkg/errors/errors.go (1)
ErrorUnauthorized(43-43)
🪛 markdownlint-cli2 (0.18.1)
examples/manifestwork/README.md
218-218: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
224-224: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
230-230: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
263-263: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
296-296: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
pkg/api/openapi/docs/DefaultAPI.md
382-382: Hard tabs
Column: 1
(MD010, no-hard-tabs)
383-383: Hard tabs
Column: 1
(MD010, no-hard-tabs)
385-385: Hard tabs
Column: 1
(MD010, no-hard-tabs)
386-386: Hard tabs
Column: 1
(MD010, no-hard-tabs)
387-387: Hard tabs
Column: 1
(MD010, no-hard-tabs)
413-413: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
413-413: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
520-520: Hard tabs
Column: 1
(MD010, no-hard-tabs)
521-521: Hard tabs
Column: 1
(MD010, no-hard-tabs)
523-523: Hard tabs
Column: 1
(MD010, no-hard-tabs)
524-524: Hard tabs
Column: 1
(MD010, no-hard-tabs)
525-525: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: e2e-with-istio
- GitHub Check: e2e
- GitHub Check: upgrade
- GitHub Check: e2e-broadcast-subscription
- GitHub Check: e2e-grpc-broker
🔇 Additional comments (27)
pkg/api/openapi/api_default.go (3)
845-890: LGTM! Generated code correctly implements X-Operation-ID-Key support.The struct fields and builder method for
xOperationIDKeyare properly implemented, following the same pattern as the existingxOperationIDfield. The use of pointer types for optional parameters is appropriate.
976-981: LGTM! Header emission logic is correct.The X-Operation-ID-Key header is correctly emitted before X-Operation-ID, which is the appropriate order for context setup followed by the actual operation ID value.
1195-1277: LGTM! Consistent implementation across both endpoints.The changes for
ApiMaestroV1ResourceBundlesIdGetRequestmirror the implementation for the list endpoint, maintaining consistency across the API client.examples/manifestwork/client.go (3)
188-191: Verify TLS 1.3 minimum version requirement.The TLS configuration enforces
MinVersion: tls.VersionTLS13, which is quite strict. While TLS 1.3 offers the best security, TLS 1.2 is still widely used and considered secure. This requirement might cause connectivity issues with older servers.Is this strict requirement intentional, or should
tls.VersionTLS12be considered for broader compatibility?
113-116: LGTM! Good operation ID tracing implementation.The operation ID generation and context propagation correctly implements the PR's tracing objective. The inline comment about customizing the key using
grpcsource.SetContextTracingOPIDKeyis helpful.
282-407: LGTM! Command handlers are well-implemented.All command handlers demonstrate good practices:
- Proper error handling with context propagation
applyWorkcorrectly usesToWorkPatchfor updates, aligning with the external snippetdeleteWorkfetches the UID before deletion for better audit loggingwatchWorkshandles graceful shutdown via context cancellation- Consistent error messages and user feedback
examples/manifestwork/README.md (3)
12-21: LGTM! Constructor documentation is accurate.The documented constructor signature correctly matches the implementation shown in the external code snippet, including all five parameters: ctx, logger, maestroAPIClient, maestroGRPCOptions, and sourceID.
102-145: LGTM! Clear documentation for local development.The quick start guide provides clear, executable examples for local development. The use of
--insecure-skip-verifyis appropriately limited to local testing scenarios and is clearly documented as insecure in the CLI help.
178-192: LGTM! Important RBAC documentation.The documentation correctly highlights the RBAC requirements for gRPC pub/sub permissions, which is essential for production deployments. The example ClusterRole configuration is clear and actionable.
pkg/auth/auth_middleware.go (1)
31-31: LGTM!The updated
handleErrorcall correctly passes the requestrto align with the refactored error handling that now sources operation IDs from request headers rather than context.pkg/client/cloudevents/grpcsource/util.go (3)
26-36: LGTM - proper initialization pattern.The use of
atomic.Valuewithsync.Onceis appropriate for thread-safe one-time initialization. Theinit()function ensurescontextTracingOPIDKeyis always initialized before any reads occur, preventing nil panics ingetOperationID.
289-298: LGTM!The function correctly uses atomic load for thread-safe access and handles missing/invalid context values gracefully by returning empty strings.
259-277: The reviewed code file and function cannot be located in the repository. The file pathpkg/client/cloudevents/grpcsource/util.goand functionSetContextTracingOPIDKeydo not exist in the current codebase despite comprehensive searches. Manual verification is required to confirm the code location and current state.pkg/services/resource_test.go (1)
89-107: LGTM!The test correctly passes
context.Background()to the updatedListmethod signature. Usingcontext.Background()is appropriate for unit tests where no cancellation or tracing context is needed.test/performance/pkg/hub/store/createonly.go (1)
21-53: LGTM!The stub methods correctly accept the new
context.Contextparameter to satisfy the updated interface contract. The context is intentionally unused in these no-op implementations, which is appropriate for aCreateOnlyWatcherStorethat explicitly returns "unsupported" for read operations.pkg/handlers/framework.go (2)
30-42: LGTM - Clean refactor to request-based operation ID handling.The changes correctly:
- Extract the logger from
r.Context()(set up byOperationIDMiddleware)- Read the operation ID from the request header for error responses
- Maintain appropriate log levels (info for 4xx, error for 5xx)
This aligns with the PR's goal of enabling per-request operation ID tracing from headers.
44-128: LGTM!All error handler call sites are consistently updated to pass
(w, r, err), and the default error handler assignment pattern (if cfg.ErrorHandler == nil) is preserved in each handler function.pkg/client/cloudevents/grpcsource/watcherstore.go (5)
75-113: LGTM - Proper context propagation in GetWatcher.The method correctly:
- Accepts
ctxparameter and propagates it toPageList(line 92)- Passes
ctxtoregisterWatcher(line 97)- Uses
ctxfor logging (line 106)This enables per-request operation ID tracing through the entire call chain.
127-153: LGTM!The
Getmethod correctly propagates the context and operation ID:
- Uses
ctxfor the API request- Extracts operation ID via
getOperationID(ctx)and sets bothXOperationIDKeyandXOperationIDheaders when present- Uses
ctxfor logging
157-198: LGTM!
Listcorrectly propagatesctxtoPageListand logging.ListAllis documented as a no-op for this store type, so the unusedctxparameter is acceptable to satisfy the interface contract.
290-304: LGTM!The
registerWatchermethod correctly propagates the context to the watcher creation and logging, enabling per-request tracing for watcher registration.
219-257: No changes needed.Sync()correctly usesm.ctxwhich is the client's lifecycle context captured during watcherStore initialization. SinceSync()is called from a background goroutine that monitors this same context's cancellation, adding a context parameter would be redundant. The current design is intentional and appropriate.pkg/logger/operationid_middleware.go (4)
16-17: LGTM - Clear header constant definitions.The header constants
OpIDKeyHeader("X-Operation-ID-Key") andOpIDHeader("X-Operation-ID") are clearly named and follow HTTP header conventions.
20-60: LGTM - Well-designed dynamic operation ID handling.The middleware correctly:
- Validates the custom key header with
isSafeOpIDKeybefore use- Falls back to the default key when header is empty or invalid
- Sets the generated operation ID in the request header (line 41) for downstream handlers
- Propagates the operation ID to span attributes, Sentry, and klog
62-71: LGTM!The function correctly uses the dynamic
opIDKeyfor both lookup and storage, and the empty string check prevents reusing invalid values.
78-89: LGTM - Good input validation.The function properly restricts operation ID keys to a safe character set (
[a-zA-Z0-9_.-]{1,64}), preventing potential injection or parsing issues with crafted header values.cmd/maestro/server/logging/request_logging_middleware.go (1)
31-34: The opid correlation is present and working correctly—OperationIDMiddleware runs first and enriches the context with a logger containing opid values before RegisterLoggerMiddleware executes.The upstream
OperationIDMiddleware(line 38 in routes.go) runs beforeRegisterLoggerMiddleware(line 41), so whenklog.FromContext(reqCtx)is called here, it retrieves the enriched logger with opid fields already attached. The correlation is maintained.That said, the defensive suggestion is valid: if this middleware ever enriches the context (e.g., adding a structured field), it should use
r.WithContext(reqCtx)when callingnextto propagate those changes downstream, as currently the rawris passed without context updates.
1 participant
