Fix pprof broad exposition#2821
Merged
Merged
Conversation
When pprof was enabled on FLP or the agent, it was exposed on ":(port)" which stands for all net interfaces. FLP and agent configs are modified to accept the full listening address, not just the port (breaking changes). For FLP, the pprof config is exposed in FlowCollector. Force using "localhost" (which stands for 127.0.0.1 in ipv4 but also works on ipv6). We don't change the FlowCollector API, only the port is exposed. For the agent, the pprof config is not explicitely exposed but can be modified through env, so the user may configure whatever they want. Raise 3 warnings through the webhook: - when pprof is enbaled on FLP - when pprof is enabled on the agent - when the agent pprof uses broad interface exposition Add more info to the pprof doc about security.
📝 WalkthroughWalkthroughFlowCollector profiling and health-port configuration, webhook warnings, chart/manifests/docs, and NetObserv agent/pipeline config are updated. The PR also refreshes vendored dependency versions and behavior in fsnotify, grpc-gateway, OpenTelemetry, and x/sys. ChangesFlowCollector Profiling and QUIC Wiring
Vendored Dependency Updates
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Jun 25, 2026
jpinsonneau
previously approved these changes
Jun 25, 2026
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vendor/go.opentelemetry.io/otel/CONTRIBUTING.md (1)
873-899: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep the nil guard in the pooled-recording helper.
i.counter.Enabled(ctx)will panic if this helper is invoked on a nil or half-initialized instrumentation. Mirror thei == nil || i.inflight == nilpattern used above before touching the pool.Suggested fix
func (i *instrumentation) record(ctx context.Context, value int64, baseAttrs ...attribute.KeyValue) { - if !i.counter.Enabled(ctx) { + if i == nil || i.counter == nil || !i.counter.Enabled(ctx) { return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/go.opentelemetry.io/otel/CONTRIBUTING.md` around lines 873 - 899, The pooled-recording helper in instrumentation.record is missing the nil guard, so it can panic when called on a nil or half-initialized instrumentation. Add the same early-return check used elsewhere in this type, guarding both the receiver and the counter state before calling i.counter.Enabled(ctx) or touching attrPool/addOptPool, and keep the rest of the record flow unchanged.
🧹 Nitpick comments (1)
internal/controller/flp/flp_common_objects.go (1)
287-293: 🎯 Functional Correctness | 🔵 Trivialpprof localhost binding looks correct; minor condition inconsistency.
Binding
pprofAddrtolocalhostcorrectly scopes pprof to the loopback. Note the health check uses!= 0here whilepodTemplate(Line 123) uses> 0; harmless for valid ports but worth aligning for a negative value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/flp/flp_common_objects.go` around lines 287 - 293, The port checks in the FLP config setup are inconsistent between the health and pprof bindings, and the `HealthPort`/`ProfilePort` guards should be aligned to reject negative values consistently. Update the conditional logic in `flp_common_objects.go` around `advancedConfig.HealthPort` and `advancedConfig.ProfilePort` to use the same positive-port validation pattern already used in `podTemplate`, while keeping the `pprofAddr` localhost binding behavior in place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.mk/local.mk:
- Line 10: The helm dependency update step in the local.mk target is swallowing
failures because the trailing cd .. makes the overall shell command succeed even
when helm dependency update fails. Update this recipe so the dependency update
command in the helm context is the only result that determines success, and
ensure the Make target stops on a non-zero exit from helm dependency update
instead of proceeding to later steps.
In `@api/flowcollector/v1beta2/flowcollector_validation_webhook.go`:
- Around line 96-98: The broad-bind warning in flowcollector validation
currently only checks for ":" and "0.0.0.0:" prefixes, so it misses the IPv6
wildcard bind case. Update the warning logic in the validation webhook’s
environment address check to also detect the "[::]:" wildcard pattern alongside
the existing checks, so the profiling warning is emitted whenever PPROF_ADDR
binds all interfaces.
In `@internal/controller/flp/flp_common_objects.go`:
- Around line 123-128: The health probe setup in flp_common_objects.go can still
reference healthPortName even when HealthPort is 0, which leaves
liveness/startup probes pointing at a missing named port. Update the probe
wiring in the same flow that builds ports in the container spec so
EnableKubeProbes only configures probes when HealthPort is present and greater
than 0, or enforce that constraint in the webhook; make sure the logic around
healthPortName, advancedConfig.HealthPort, and the probe constructors stays
consistent.
In `@internal/pkg/helper/flowcollector.go`:
- Around line 156-161: The flow collector port assignment currently accepts
explicit 0 values for HealthPort and ProfilePort, which can disable the
endpoints and break probes. Update the port handling in flowcollector logic to
match the guarded pattern used for Port in flp_common_objects.go by only copying
specConfig.HealthPort and specConfig.ProfilePort into cfg when the pointers are
non-nil and the values are greater than 0. Keep the fix localized around the
existing cfg.HealthPort and cfg.ProfilePort assignments in the flow collector
helper.
In `@vendor/go.opentelemetry.io/otel/CONTRIBUTING.md`:
- Around line 1021-1034: The span export example calls e.inst.Enabled(ctx)
without first confirming e.inst is non-nil, which can panic when instrumentation
is disabled. Update the snippet around e.doExport and the
recordSpanExportStarted/recordSpanExportFailed/recordSpanExportSucceeded calls
to keep the earlier nil guard on e.inst and cache the enabled check once before
using it.
---
Outside diff comments:
In `@vendor/go.opentelemetry.io/otel/CONTRIBUTING.md`:
- Around line 873-899: The pooled-recording helper in instrumentation.record is
missing the nil guard, so it can panic when called on a nil or half-initialized
instrumentation. Add the same early-return check used elsewhere in this type,
guarding both the receiver and the counter state before calling
i.counter.Enabled(ctx) or touching attrPool/addOptPool, and keep the rest of the
record flow unchanged.
---
Nitpick comments:
In `@internal/controller/flp/flp_common_objects.go`:
- Around line 287-293: The port checks in the FLP config setup are inconsistent
between the health and pprof bindings, and the `HealthPort`/`ProfilePort` guards
should be aligned to reject negative values consistently. Update the conditional
logic in `flp_common_objects.go` around `advancedConfig.HealthPort` and
`advancedConfig.ProfilePort` to use the same positive-port validation pattern
already used in `podTemplate`, while keeping the `pprofAddr` localhost binding
behavior in place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d8de08c-b06b-473e-a26a-5f2117f3ceea
⛔ Files ignored due to path filters (6)
go.sumis excluded by!**/*.sumvendor/google.golang.org/genproto/googleapis/api/expr/v1alpha1/checked.pb.gois excluded by!**/*.pb.govendor/google.golang.org/genproto/googleapis/api/expr/v1alpha1/eval.pb.gois excluded by!**/*.pb.govendor/google.golang.org/genproto/googleapis/api/expr/v1alpha1/explain.pb.gois excluded by!**/*.pb.govendor/google.golang.org/genproto/googleapis/api/expr/v1alpha1/syntax.pb.gois excluded by!**/*.pb.govendor/google.golang.org/genproto/googleapis/api/expr/v1alpha1/value.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (119)
.mk/local.mkapi/flowcollector/v1beta2/flowcollector_types.goapi/flowcollector/v1beta2/flowcollector_validation_webhook.gobundle/manifests/flows.netobserv.io_flowcollectors.yamlconfig/crd/bases/flows.netobserv.io_flowcollectors.yamldocs/FlowCollector.mdgo.modhelm/crds/flows.netobserv.io_flowcollectors.yamlinternal/controller/ebpf/agent_controller_test.gointernal/controller/flp/flp_common_objects.gointernal/pkg/helper/flowcollector.govendor/github.com/fsnotify/fsnotify/.cirrus.ymlvendor/github.com/fsnotify/fsnotify/CHANGELOG.mdvendor/github.com/fsnotify/fsnotify/CONTRIBUTING.mdvendor/github.com/fsnotify/fsnotify/README.mdvendor/github.com/fsnotify/fsnotify/backend_fen.govendor/github.com/fsnotify/fsnotify/backend_inotify.govendor/github.com/fsnotify/fsnotify/backend_kqueue.govendor/github.com/fsnotify/fsnotify/backend_windows.govendor/github.com/fsnotify/fsnotify/fsnotify.govendor/github.com/fsnotify/fsnotify/internal/darwin.govendor/github.com/fsnotify/fsnotify/internal/debug_darwin.govendor/github.com/fsnotify/fsnotify/internal/debug_dragonfly.govendor/github.com/fsnotify/fsnotify/internal/debug_freebsd.govendor/github.com/fsnotify/fsnotify/internal/debug_kqueue.govendor/github.com/fsnotify/fsnotify/internal/debug_netbsd.govendor/github.com/fsnotify/fsnotify/internal/debug_openbsd.govendor/github.com/fsnotify/fsnotify/internal/freebsd.govendor/github.com/fsnotify/fsnotify/internal/unix.govendor/github.com/fsnotify/fsnotify/internal/unix2.govendor/github.com/fsnotify/fsnotify/internal/windows.govendor/github.com/grpc-ecosystem/grpc-gateway/v2/runtime/mux.govendor/github.com/netobserv/flowlogs-pipeline/pkg/api/encode_s3.govendor/github.com/netobserv/flowlogs-pipeline/pkg/api/redacted.govendor/github.com/netobserv/flowlogs-pipeline/pkg/config/config.govendor/github.com/netobserv/netobserv-ebpf-agent/pkg/config/config.govendor/github.com/netobserv/netobserv-ebpf-agent/pkg/maps/maps.govendor/go.opentelemetry.io/otel/.golangci.ymlvendor/go.opentelemetry.io/otel/AGENTS.mdvendor/go.opentelemetry.io/otel/CHANGELOG.mdvendor/go.opentelemetry.io/otel/CLAUDE.mdvendor/go.opentelemetry.io/otel/CONTRIBUTING.mdvendor/go.opentelemetry.io/otel/Makefilevendor/go.opentelemetry.io/otel/attribute/encoder.govendor/go.opentelemetry.io/otel/attribute/hash.govendor/go.opentelemetry.io/otel/attribute/key.govendor/go.opentelemetry.io/otel/attribute/kv.govendor/go.opentelemetry.io/otel/attribute/set.govendor/go.opentelemetry.io/otel/attribute/type_string.govendor/go.opentelemetry.io/otel/attribute/value.govendor/go.opentelemetry.io/otel/baggage/baggage.govendor/go.opentelemetry.io/otel/dependencies.Dockerfilevendor/go.opentelemetry.io/otel/exporters/otlp/otlptrace/internal/tracetransform/attribute.govendor/go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc/client.govendor/go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc/internal/observ/instrumentation.govendor/go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc/internal/otlpconfig/options.govendor/go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc/internal/version.govendor/go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc/options.govendor/go.opentelemetry.io/otel/exporters/otlp/otlptrace/version.govendor/go.opentelemetry.io/otel/metric/asyncfloat64.govendor/go.opentelemetry.io/otel/metric/asyncint64.govendor/go.opentelemetry.io/otel/metric/config.govendor/go.opentelemetry.io/otel/metric/doc.govendor/go.opentelemetry.io/otel/metric/instrument.govendor/go.opentelemetry.io/otel/metric/syncfloat64.govendor/go.opentelemetry.io/otel/metric/syncint64.govendor/go.opentelemetry.io/otel/propagation/baggage.govendor/go.opentelemetry.io/otel/sdk/resource/builtin.govendor/go.opentelemetry.io/otel/sdk/resource/container.govendor/go.opentelemetry.io/otel/sdk/resource/env.govendor/go.opentelemetry.io/otel/sdk/resource/host_id.govendor/go.opentelemetry.io/otel/sdk/resource/host_id_exec.govendor/go.opentelemetry.io/otel/sdk/resource/os.govendor/go.opentelemetry.io/otel/sdk/resource/os_unix.govendor/go.opentelemetry.io/otel/sdk/resource/os_windows.govendor/go.opentelemetry.io/otel/sdk/resource/process.govendor/go.opentelemetry.io/otel/sdk/trace/batch_span_processor.govendor/go.opentelemetry.io/otel/sdk/trace/internal/observ/batch_span_processor.govendor/go.opentelemetry.io/otel/sdk/trace/internal/observ/simple_span_processor.govendor/go.opentelemetry.io/otel/sdk/trace/internal/observ/tracer.govendor/go.opentelemetry.io/otel/sdk/trace/provider.govendor/go.opentelemetry.io/otel/sdk/trace/sampling.govendor/go.opentelemetry.io/otel/sdk/trace/span.govendor/go.opentelemetry.io/otel/sdk/trace/span_limits.govendor/go.opentelemetry.io/otel/sdk/version.govendor/go.opentelemetry.io/otel/semconv/v1.37.0/attribute_group.govendor/go.opentelemetry.io/otel/semconv/v1.39.0/attribute_group.govendor/go.opentelemetry.io/otel/semconv/v1.39.0/httpconv/metric.govendor/go.opentelemetry.io/otel/semconv/v1.40.0/README.mdvendor/go.opentelemetry.io/otel/semconv/v1.41.0/MIGRATION.mdvendor/go.opentelemetry.io/otel/semconv/v1.41.0/README.mdvendor/go.opentelemetry.io/otel/semconv/v1.41.0/attribute_group.govendor/go.opentelemetry.io/otel/semconv/v1.41.0/doc.govendor/go.opentelemetry.io/otel/semconv/v1.41.0/error_type.govendor/go.opentelemetry.io/otel/semconv/v1.41.0/exception.govendor/go.opentelemetry.io/otel/semconv/v1.41.0/otelconv/metric.govendor/go.opentelemetry.io/otel/semconv/v1.41.0/schema.govendor/go.opentelemetry.io/otel/trace/auto.govendor/go.opentelemetry.io/otel/trace/config.govendor/go.opentelemetry.io/otel/trace/internal/telemetry/span.govendor/go.opentelemetry.io/otel/version.govendor/go.opentelemetry.io/otel/versions.yamlvendor/golang.org/x/sys/unix/ztypes_linux.govendor/golang.org/x/sys/unix/ztypes_linux_386.govendor/golang.org/x/sys/unix/ztypes_linux_amd64.govendor/golang.org/x/sys/unix/ztypes_linux_arm.govendor/golang.org/x/sys/unix/ztypes_linux_arm64.govendor/golang.org/x/sys/unix/ztypes_linux_loong64.govendor/golang.org/x/sys/unix/ztypes_linux_mips.govendor/golang.org/x/sys/unix/ztypes_linux_mips64.govendor/golang.org/x/sys/unix/ztypes_linux_mips64le.govendor/golang.org/x/sys/unix/ztypes_linux_mipsle.govendor/golang.org/x/sys/unix/ztypes_linux_ppc.govendor/golang.org/x/sys/unix/ztypes_linux_ppc64.govendor/golang.org/x/sys/unix/ztypes_linux_ppc64le.govendor/golang.org/x/sys/unix/ztypes_linux_riscv64.govendor/golang.org/x/sys/unix/ztypes_linux_s390x.govendor/golang.org/x/sys/unix/ztypes_linux_sparc64.govendor/modules.txt
💤 Files with no reviewable changes (10)
- vendor/github.com/fsnotify/fsnotify/.cirrus.yml
- vendor/go.opentelemetry.io/otel/semconv/v1.40.0/README.md
- vendor/github.com/fsnotify/fsnotify/internal/debug_dragonfly.go
- vendor/github.com/fsnotify/fsnotify/internal/debug_darwin.go
- vendor/github.com/fsnotify/fsnotify/internal/debug_openbsd.go
- vendor/github.com/fsnotify/fsnotify/internal/windows.go
- vendor/github.com/fsnotify/fsnotify/internal/debug_netbsd.go
- vendor/github.com/fsnotify/fsnotify/internal/freebsd.go
- vendor/github.com/fsnotify/fsnotify/internal/darwin.go
- vendor/github.com/fsnotify/fsnotify/internal/unix.go
| VERSION_FOR_HELM := $(word 2,$(subst :, ,${IMAGE})) | ||
| .PHONY: helm-install | ||
| helm-install: prereqs-helm ## Install the operator and its pre-requisites to a running cluster, using Helm | ||
| cd helm && helm dependency update --skip-refresh ; cd .. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Failed helm dependency update is silently swallowed.
cd helm && helm dependency update --skip-refresh ; cd .. — the trailing ; cd .. always exits 0, masking a non-zero exit from helm dependency update. Make sees a successful step and proceeds to install with potentially stale/missing chart dependencies.
🛠️ Proposed fix
- cd helm && helm dependency update --skip-refresh ; cd ..
+ cd helm && helm dependency update --skip-refresh && cd ..📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cd helm && helm dependency update --skip-refresh ; cd .. | |
| cd helm && helm dependency update --skip-refresh && cd .. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.mk/local.mk at line 10, The helm dependency update step in the local.mk
target is swallowing failures because the trailing cd .. makes the overall shell
command succeed even when helm dependency update fails. Update this recipe so
the dependency update command in the helm context is the only result that
determines success, and ensure the Make target stops on a non-zero exit from
helm dependency update instead of proceeding to later steps.
Comment on lines
+96
to
+98
| if strings.HasPrefix(env, ":") || strings.HasPrefix(env, "0.0.0.0:") { | ||
| v.warnings = append(v.warnings, "Profiling is enabled for all network interfaces, make sure access is restricted e.g. with a network policy.") | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Broad-bind check misses the IPv6 wildcard [::]:.
PPROF_ADDR=[::]:6060 binds all interfaces but slips past the prefix checks, so the "all network interfaces" warning is never emitted for the IPv6 wildcard case.
🛡️ Proposed fix
- if strings.HasPrefix(env, ":") || strings.HasPrefix(env, "0.0.0.0:") {
+ if strings.HasPrefix(env, ":") || strings.HasPrefix(env, "0.0.0.0:") || strings.HasPrefix(env, "[::]:") {
v.warnings = append(v.warnings, "Profiling is enabled for all network interfaces, make sure access is restricted e.g. with a network policy.")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if strings.HasPrefix(env, ":") || strings.HasPrefix(env, "0.0.0.0:") { | |
| v.warnings = append(v.warnings, "Profiling is enabled for all network interfaces, make sure access is restricted e.g. with a network policy.") | |
| } | |
| if strings.HasPrefix(env, ":") || strings.HasPrefix(env, "0.0.0.0:") || strings.HasPrefix(env, "[::]:") { | |
| v.warnings = append(v.warnings, "Profiling is enabled for all network interfaces, make sure access is restricted e.g. with a network policy.") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/flowcollector/v1beta2/flowcollector_validation_webhook.go` around lines
96 - 98, The broad-bind warning in flowcollector validation currently only
checks for ":" and "0.0.0.0:" prefixes, so it misses the IPv6 wildcard bind
case. Update the warning logic in the validation webhook’s environment address
check to also detect the "[::]:" wildcard pattern alongside the existing checks,
so the profiling warning is emitted whenever PPROF_ADDR binds all interfaces.
Comment on lines
+123
to
+128
| if advancedConfig.HealthPort != nil && *advancedConfig.HealthPort > 0 { | ||
| ports = append(ports, corev1.ContainerPort{ | ||
| Name: healthPortName, | ||
| ContainerPort: *advancedConfig.HealthPort, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Probes can reference a missing named port when HealthPort is 0.
The health ContainerPort (healthPortName) is only appended when HealthPort > 0, but the liveness/startup probes at Lines 178-200 always resolve intstr.FromString(healthPortName). If a user sets HealthPort to 0 (now possible after the helper relaxation) with EnableKubeProbes=true, the named port won't exist and probes will fail, restarting the container. Gate the probes on the same condition or enforce it in the webhook.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/flp/flp_common_objects.go` around lines 123 - 128, The
health probe setup in flp_common_objects.go can still reference healthPortName
even when HealthPort is 0, which leaves liveness/startup probes pointing at a
missing named port. Update the probe wiring in the same flow that builds ports
in the container spec so EnableKubeProbes only configures probes when HealthPort
is present and greater than 0, or enforce that constraint in the webhook; make
sure the logic around healthPortName, advancedConfig.HealthPort, and the probe
constructors stays consistent.
Comment on lines
+156
to
161
| if specConfig.HealthPort != nil { | ||
| cfg.HealthPort = specConfig.HealthPort | ||
| } | ||
| if specConfig.ProfilePort != nil && *specConfig.ProfilePort > 0 { | ||
| if specConfig.ProfilePort != nil { | ||
| cfg.ProfilePort = specConfig.ProfilePort | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for webhook validation guarding HealthPort against 0
rg -nP -C3 '(HealthPort|healthPort)' api/flowcollector/v1beta2/flowcollector_validation_webhook.goRepository: netobserv/netobserv-operator
Length of output: 166
Explicit 0 for HealthPort or ProfilePort disables endpoints, causing probe failures.
Unlike Port, the code in internal/controller/flp/flp_common_objects.go lacks a > 0 guard:
Verify in `internal/controller/flp/flp_common_objects.go`
if specConfig.Port != nil && *specConfig.Port > 0 {
cfg.Port = specConfig.Port
}
if specConfig.HealthPort != nil {
cfg.HealthPort = specConfig.HealthPort
}
if specConfig.ProfilePort != nil {
cfg.ProfilePort = specConfig.ProfilePort
}No webhook validation rejects 0. A user can explicitly disable these ports while EnableKubeProbes is active, breaking health checks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/pkg/helper/flowcollector.go` around lines 156 - 161, The flow
collector port assignment currently accepts explicit 0 values for HealthPort and
ProfilePort, which can disable the endpoints and break probes. Update the port
handling in flowcollector logic to match the guarded pattern used for Port in
flp_common_objects.go by only copying specConfig.HealthPort and
specConfig.ProfilePort into cfg when the pointers are non-nil and the values are
greater than 0. Keep the fix localized around the existing cfg.HealthPort and
cfg.ProfilePort assignments in the flow collector helper.
Comment on lines
+1021
to
+1034
| if e.inst.Enabled(ctx) { | ||
| e.inst.recordSpanExportStarted(ctx, len(spans)) | ||
| } | ||
|
|
||
| err := e.doExport(ctx, spans) | ||
|
|
||
| if err != nil { | ||
| e.inst.recordSpanExportFailed(ctx, len(spans), err) | ||
| } else { | ||
| e.inst.recordSpanExportSucceeded(ctx, len(spans)) | ||
| if e.inst.Enabled(ctx) { | ||
| if err != nil { | ||
| e.inst.recordSpanExportFailed(ctx, len(spans), err) | ||
| } else { | ||
| e.inst.recordSpanExportSucceeded(ctx, len(spans)) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard e.inst before calling Enabled.
newInstrumentation() can return nil, so this example will panic when observability is off. Cache the enabled check once and keep the nil guard from the earlier snippet.
Suggested fix
func (e *Exporter) ExportSpans(ctx context.Context, spans []trace.ReadOnlySpan) error {
- if e.inst.Enabled(ctx) {
+ enabled := e.inst != nil && e.inst.Enabled(ctx)
+ if enabled {
e.inst.recordSpanExportStarted(ctx, len(spans))
}
err := e.doExport(ctx, spans)
- if e.inst.Enabled(ctx) {
+ if enabled {
if err != nil {
e.inst.recordSpanExportFailed(ctx, len(spans), err)
} else {
e.inst.recordSpanExportSucceeded(ctx, len(spans))
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if e.inst.Enabled(ctx) { | |
| e.inst.recordSpanExportStarted(ctx, len(spans)) | |
| } | |
| err := e.doExport(ctx, spans) | |
| if err != nil { | |
| e.inst.recordSpanExportFailed(ctx, len(spans), err) | |
| } else { | |
| e.inst.recordSpanExportSucceeded(ctx, len(spans)) | |
| if e.inst.Enabled(ctx) { | |
| if err != nil { | |
| e.inst.recordSpanExportFailed(ctx, len(spans), err) | |
| } else { | |
| e.inst.recordSpanExportSucceeded(ctx, len(spans)) | |
| } | |
| } | |
| enabled := e.inst != nil && e.inst.Enabled(ctx) | |
| if enabled { | |
| e.inst.recordSpanExportStarted(ctx, len(spans)) | |
| } | |
| err := e.doExport(ctx, spans) | |
| if enabled { | |
| if err != nil { | |
| e.inst.recordSpanExportFailed(ctx, len(spans), err) | |
| } else { | |
| e.inst.recordSpanExportSucceeded(ctx, len(spans)) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@vendor/go.opentelemetry.io/otel/CONTRIBUTING.md` around lines 1021 - 1034,
The span export example calls e.inst.Enabled(ctx) without first confirming
e.inst is non-nil, which can panic when instrumentation is disabled. Update the
snippet around e.doExport and the
recordSpanExportStarted/recordSpanExportFailed/recordSpanExportSucceeded calls
to keep the earlier nil guard on e.inst and cache the enabled check once before
using it.
|
@jotak: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When pprof was enabled on FLP or the agent, it was exposed on ":(port)" which stands for all net interfaces. FLP and agent configs are modified to accept the full listening address, not just the port (breaking changes).
For FLP, the pprof config is exposed in FlowCollector. Force using "localhost" (which stands for 127.0.0.1 in ipv4 but also works on ipv6). We don't change the FlowCollector API, only the port is exposed.
For the agent, the pprof config is not explicitely exposed but can be modified through env, so the user may configure whatever they want.
Raise 3 warnings through the webhook:
Add more info to the pprof doc about security.
Dependencies
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation