NO-ISSUE: Add opt-in kube-rbac-proxy sidecar for pprof endpoint (AWS)#1502
NO-ISSUE: Add opt-in kube-rbac-proxy sidecar for pprof endpoint (AWS)#1502perdasilva wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@perdasilva: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds conditional pprof profiling endpoint to the machine-controller deployment. When the Changespprof Endpoint Exposure on AWS
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[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.
🧹 Nitpick comments (1)
pkg/operator/sync.go (1)
805-808: LGTM!Operational note: If this pprof port is later exposed via a Kubernetes Service, ensure proper access controls are in place. Profiling endpoints can expose sensitive runtime information (memory dumps, goroutine stacks, CPU profiles). Consider restricting access to cluster-internal traffic only or implementing authentication/authorization mechanisms.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/sync.go` around lines 805 - 808, The pprof container port entry (Name: "pprof", ContainerPort: pprofPort) exposes a profiling endpoint that must be protected if you create a Kubernetes Service for it; update any Service/Ingress that exposes this port to use ClusterIP (no external exposure), add NetworkPolicy rules to restrict traffic to trusted pods/namespaces, and/or place an auth-proxy or mTLS ingress in front of the endpoint so only authenticated users can reach it; locate the pprof port definition (Name "pprof", variable pprofPort) and ensure associated Service/NetworkPolicy/Ingress manifests or code paths enforce these access controls before exposing the port.
🤖 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.
Nitpick comments:
In `@pkg/operator/sync.go`:
- Around line 805-808: The pprof container port entry (Name: "pprof",
ContainerPort: pprofPort) exposes a profiling endpoint that must be protected if
you create a Kubernetes Service for it; update any Service/Ingress that exposes
this port to use ClusterIP (no external exposure), add NetworkPolicy rules to
restrict traffic to trusted pods/namespaces, and/or place an auth-proxy or mTLS
ingress in front of the endpoint so only authenticated users can reach it;
locate the pprof port definition (Name "pprof", variable pprofPort) and ensure
associated Service/NetworkPolicy/Ingress manifests or code paths enforce these
access controls before exposing the port.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a34fb9da-94e1-4760-bdb0-50dea8a56586
📒 Files selected for processing (1)
pkg/operator/sync.go
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
/retest |
|
/retest |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@perdasilva: all tests passed! 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. |
| @@ -801,6 +802,10 @@ func newContainers(config *OperatorConfig, features map[string]bool, tlsArgs []s | |||
| Name: "healthz", | |||
| ContainerPort: defaultMachineHealthPort, | |||
| }, | |||
| { | |||
| Name: "pprof", | |||
| ContainerPort: pprofPort, | |||
| }, | |||
There was a problem hiding this comment.
Depending on where we go with: https://github.com/openshift/machine-api-provider-aws/pull/189/changes#r3434353653, this may need changing
Add an --enable-pprof flag to machine-api-operator that, when set, configures the AWS machine controller with a pprof profiling endpoint and a kube-rbac-proxy sidecar to securely expose it. When --enable-pprof is passed to MAO and the platform is AWS: - The machine controller receives --enable-pprof and --pprof-bind-address=127.0.0.1:6061 to serve plaintext pprof on loopback - A kube-rbac-proxy-pprof sidecar is added, listening on port 6060 with centralized TLS, proxying to the upstream pprof server The flag is off by default and only meaningful on AWS (the only platform whose machine controller supports pprof). On all other platforms, the flag is accepted but has no effect. Signed-off-by: Per G. da Silva <pegoncal@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
perdasilva
force-pushed
the
fix/pprof-container-port
branch
from
31f027b to
641d58e
Compare
perdasilva
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/operator/sync_test.go (1)
666-666: ⚡ Quick winCover the new
withPprofProxybranch in this unit test.
TestNewKubeProxyContainersalways passesfalsefor the new parameter, so the pprof proxy path is not directly validated here.Proposed diff
func TestNewKubeProxyContainers(t *testing.T) { testCases := []struct { name string image string withMHCProxy bool + withPprofProxy bool tlsProfile configv1.TLSProfileSpec expectedCipherSuitesInArgs bool expectedPorts map[string]int32 }{ { name: "TLS 1.2 Intermediate profile with cipher suites", image: "test-image:latest", withMHCProxy: true, + withPprofProxy: false, tlsProfile: configv1.TLSProfileSpec{ Ciphers: []string{ "TLS_AES_128_GCM_SHA256", @@ expectedPorts: map[string]int32{ "kube-rbac-proxy-machineset-mtrc": machineSetExposeMetricsPort, "kube-rbac-proxy-machine-mtrc": machineExposeMetricsPort, "kube-rbac-proxy-mhc-mtrc": machineHealthCheckExposeMetricsPort, }, }, + { + name: "TLS 1.2 Intermediate profile with pprof proxy", + image: "test-image:latest", + withMHCProxy: false, + withPprofProxy: true, + tlsProfile: configv1.TLSProfileSpec{ + Ciphers: []string{ + "ECDHE-ECDSA-AES128-GCM-SHA256", + "ECDHE-RSA-AES128-GCM-SHA256", + }, + MinTLSVersion: configv1.VersionTLS12, + }, + expectedCipherSuitesInArgs: true, + expectedPorts: map[string]int32{ + "kube-rbac-proxy-machineset-mtrc": machineSetExposeMetricsPort, + "kube-rbac-proxy-machine-mtrc": machineExposeMetricsPort, + "kube-rbac-proxy-pprof": pprofExposePort, + }, + }, @@ - containers := newKubeProxyContainers(tc.image, tc.withMHCProxy, false, getTLSArgs(tc.tlsProfile)) + containers := newKubeProxyContainers(tc.image, tc.withMHCProxy, tc.withPprofProxy, getTLSArgs(tc.tlsProfile))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/sync_test.go` at line 666, The test `TestNewKubeProxyContainers` currently passes `false` for the `withPprofProxy` parameter in the `newKubeProxyContainers` function call, which means the code path for pprof proxy is never tested. Add test coverage for the `withPprofProxy` parameter by either adding additional test cases with `withPprofProxy` set to `true` or modifying the existing test structure to validate behavior in both the true and false scenarios for this parameter.
🤖 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.
Nitpick comments:
In `@pkg/operator/sync_test.go`:
- Line 666: The test `TestNewKubeProxyContainers` currently passes `false` for
the `withPprofProxy` parameter in the `newKubeProxyContainers` function call,
which means the code path for pprof proxy is never tested. Add test coverage for
the `withPprofProxy` parameter by either adding additional test cases with
`withPprofProxy` set to `true` or modifying the existing test structure to
validate behavior in both the true and false scenarios for this parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a2ba5be1-b66f-4f34-911c-9cc18a0bc7e0
📒 Files selected for processing (5)
cmd/machine-api-operator/start.gopkg/operator/config.gopkg/operator/operator.gopkg/operator/sync.gopkg/operator/sync_test.go
3 participants
Description
Add an
--enable-pprofflag tomachine-api-operatorthat, when set, configures the AWS machine controller with a pprof profiling endpoint and a kube-rbac-proxy sidecar to securely expose it.When
--enable-pprofis passed to MAO and the platform is AWS:--enable-pprofand--pprof-bind-address=127.0.0.1:6061to serve plaintext pprof on loopbackkube-rbac-proxy-pprofsidecar is added, listening on port 6060 with centralized TLS, proxying to the upstream pprof serverThe flag is off by default and only meaningful on AWS (the only platform whose machine controller supports pprof). On all other platforms, the flag is accepted but has no effect.
Changes
cmd/machine-api-operator/start.go— new--enable-pprofCLI flag (default:false, help: "AWS only"), passed through tooperator.Newpkg/operator/config.go—EnablePproffield added toOperatorConfigpkg/operator/operator.go—enablePproffield onOperatorstruct, threaded throughNew()→OperatorConfigpkg/operator/sync.go— whenEnablePprof && PlatformType == AWS: adds--enable-pprof+--pprof-bind-addressargs to machine controller, addskube-rbac-proxy-pprofsidecarpkg/operator/sync_test.go— new test case verifying pprof proxy + args when enabled; existing AWS tests verify pprof is absent by defaultDependencies
--enable-pprofflag support in the AWS machine controller🤖 Generated with Claude Code
Summary by CodeRabbit
--enable-pprofCLI flag to optionally enable a profiling endpoint for the machine controller (AWS-only), exposed via an added proxy container and bound to a local upstream.