CCXDEV-16594: add custom proxy field to insights config - #1329
Conversation
Allow configuring a custom CA certificate for trusting the archive upload endpoint, scoped to the Insights Operator. This avoids requiring changes to the cluster-wide proxy trusted CA bundle. Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
Introduce Config interface to thread Insights configuration into the HTTP client. Rename Client to InsightsClient and getTrustedCABundle to getRootCAs. The custom CACert from the insights-config ConfigMap is now appended to the trusted CA pool alongside the proxy CA bundle. Only the following endpoints should use the custom proxy configuration: uploadEndpoint, downloadEndpoint, downloadEndpointTechPreview and processingStatusEndpoint Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@opokornyy: This pull request references CCXDEV-16594 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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. |
📝 WalkthroughWalkthroughThe change adds serialized and runtime CA certificate support, merges CA data into effective configuration, applies it to Insights client TLS transports, and replaces the former client type with ChangesInsights CA configuration and client refactor
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant ConfigAggregator
participant GatherJob
participant InsightsClient
participant TLSTransport
participant InsightsAPI
ConfigAggregator->>GatherJob: provide effective configuration
GatherJob->>InsightsClient: construct with configAggregator
InsightsClient->>TLSTransport: configure request transport with CACert
TLSTransport->>InsightsAPI: send HTTPS request
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/config/configobserver/config_aggregator.go (1)
155-157: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for removing a previously configured CA.
This branch intentionally replaces a non-empty existing certificate with an empty/nil incoming value. Add a regression case that starts with an existing CA and applies a configuration without
CACert, then asserts the CA is cleared.🤖 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/config/configobserver/config_aggregator.go` around lines 155 - 157, Add a regression test for the configuration aggregation flow around the CACert update branch: initialize the existing/default configuration with a non-empty DataReporting.CACert, apply an incoming configuration where CACert is omitted or nil, and assert that the resulting CA value is cleared.
🤖 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 `@pkg/config/types.go`:
- Line 39: Update getRootCAs to initialize the pool from x509.SystemCertPool()
rather than x509.NewCertPool(), then append CACert and any trusted CA bundle to
that pool. Preserve appropriate fallback handling if the system pool cannot be
loaded while ensuring custom certificates are still included.
In `@pkg/insights/insightsclient/insightsclient.go`:
- Around line 124-145: Update the certificate pool initialization in the shown
TLS configuration flow to start from x509.SystemCertPool() rather than
x509.NewCertPool(), preserving system roots while still appending configured and
trusted CA certificates. Handle any SystemCertPool error appropriately before
processing CACert and the trusted CA bundle, keeping the existing invalid-PEM
and missing-bundle behavior unchanged.
---
Nitpick comments:
In `@pkg/config/configobserver/config_aggregator.go`:
- Around line 155-157: Add a regression test for the configuration aggregation
flow around the CACert update branch: initialize the existing/default
configuration with a non-empty DataReporting.CACert, apply an incoming
configuration where CACert is omitted or nil, and assert that the resulting CA
value is cleared.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 321bed84-0298-4536-b519-1ad9b3f6cb37
📒 Files selected for processing (16)
pkg/config/config.gopkg/config/config_test.gopkg/config/configobserver/config_aggregator.gopkg/config/configobserver/config_aggregator_test.gopkg/config/types.gopkg/controller/gather_commands.gopkg/controller/operator.gopkg/gather/gather.gopkg/insights/insightsclient/insightsclient.gopkg/insights/insightsclient/insightsclient_test.gopkg/insights/insightsclient/requests.gopkg/insights/insightsclient/requests_test.gopkg/insights/insightsreport/insightsreport.gopkg/insights/insightsuploader/insightsuploader.gopkg/ocm/clustertransfer/cluster_transfer_test.gopkg/ocm/sca/sca.go
| ProcessingStatusEndpoint string `json:"processingStatusEndpoint,omitempty"` | ||
| Obfuscation Obfuscation `json:"obfuscation,omitempty"` | ||
| DisableRuntimeExtractor string `json:"disableRuntimeExtractor,omitempty"` | ||
| CACert string `json:"CACert,omitempty"` |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'NewCertPool|SystemCertPool|RootCAs|trusted-ca-bundle' pkg/insights/insightsclientRepository: openshift/insights-operator
Length of output: 1140
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '110,190p' pkg/insights/insightsclient/insightsclient.go
printf '\n--- tests ---\n'
sed -n '150,260p' pkg/insights/insightsclient/insightsclient_test.goRepository: openshift/insights-operator
Length of output: 4480
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' pkg/insights/insightsclient/insightsclient.goRepository: openshift/insights-operator
Length of output: 2796
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'func GetTLSConfigFromAPIServer|SystemCertPool|RootCAs' pkg/insights/insightsclient
sed -n '1,220p' pkg/insights/insightsclient/*.go | sed -n '/func GetTLSConfigFromAPIServer/,/}/p'Repository: openshift/insights-operator
Length of output: 1117
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n pkg/insights/insightsclient/apiserver_config.goRepository: openshift/insights-operator
Length of output: 3791
Preserve system trust roots when building the CA pool. getRootCAs starts from x509.NewCertPool(), so setting tlsConfig.RootCAs drops the default roots whenever CACert or the trusted CA bundle is present. Start from x509.SystemCertPool() and append the custom bundles instead.
🤖 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/config/types.go` at line 39, Update getRootCAs to initialize the pool
from x509.SystemCertPool() rather than x509.NewCertPool(), then append CACert
and any trusted CA bundle to that pool. Preserve appropriate fallback handling
if the system pool cannot be loaded while ensuring custom certificates are still
included.
| certs := x509.NewCertPool() | ||
| hasAnyCert := false | ||
|
|
||
| if config != nil && len(config.Config().DataReporting.CACert) > 0 { | ||
| if ok := certs.AppendCertsFromPEM(config.Config().DataReporting.CACert); !ok { | ||
| return nil, errors.New("error loading configured CACert: invalid PEM data") | ||
| } | ||
| hasAnyCert = true | ||
| } | ||
|
|
||
| caBundleBytes, err := os.ReadFile("/var/run/configmaps/trusted-ca-bundle/ca-bundle.crt") | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| return nil, nil | ||
| if !os.IsNotExist(err) { | ||
| return nil, err | ||
| } | ||
| return nil, err | ||
| klog.Infof("ca-bundle does not exist") | ||
| } | ||
| if len(caBytes) == 0 { | ||
| return nil, nil | ||
| if len(caBundleBytes) > 0 { | ||
| if ok := certs.AppendCertsFromPEM(caBundleBytes); !ok { | ||
| return nil, errors.New("error loading trusted CA bundle: invalid PEM data") | ||
| } | ||
| hasAnyCert = true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
According to the official Go crypto/tls documentation, does assigning a non-nil tls.Config.RootCAs replace the platform system certificate pool?
💡 Result:
Yes, according to the official Go documentation, assigning a non-nil *x509.CertPool to tls.Config.RootCAs replaces the platform's system certificate pool for that TLS configuration [1][2]. The documentation explicitly states: "If RootCAs is nil, TLS uses the host's root CA set." [1][3][2]. Conversely, when you assign a specific *x509.CertPool to the RootCAs field, Go uses that pool instead of the default system roots to verify server certificates [1][2]. If you wish to augment the system pool rather than replace it, you must first load the system's certificate pool using x509.SystemCertPool and then add your additional certificates to that pool before assigning it to tls.Config.RootCAs.
Citations:
- 1: https://pkg.go.dev/crypto/tls
- 2: https://pkg.go.dev/crypto/tls@go1.26.4
- 3: https://golang.google.cn/pkg/crypto/tls/
Preserve the system root CA pool here. x509.NewCertPool() starts empty, so assigning it to tls.Config.RootCAs drops the host trust store for endpoints that rely on public/system CAs. Start from x509.SystemCertPool() and append the configured/trusted PEMs.
🤖 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/insights/insightsclient/insightsclient.go` around lines 124 - 145, Update
the certificate pool initialization in the shown TLS configuration flow to start
from x509.SystemCertPool() rather than x509.NewCertPool(), preserving system
roots while still appending configured and trusted CA certificates. Handle any
SystemCertPool error appropriately before processing CACert and the trusted CA
bundle, keeping the existing invalid-PEM and missing-bundle behavior unchanged.
|
/retest |
1 similar comment
|
/retest |
|
/cc @JiriPapousek |
|
@opokornyy: GitHub didn't allow me to request PR reviews from the following users: JiriPapousek. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JiriPapousek, opokornyy The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
@opokornyy: 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. |
3 participants
Allow configuring a custom CA certificate for trusting the archive upload endpoint, scoped to the Insights Operator. This avoids requiring changes to the cluster-wide proxy trusted CA bundle.
Categories
Sample Archive
NoneDocumentation
NoneUnit Tests
pkg/insights/insightsclient/requests_test.gopkg/ocm/clustertransfer/cluster_transfer_test.gopkg/insights/insightsclient/insightsclient_test.gopkg/config/configobserver/config_aggregator_test.gopkg/config/config_test.goPrivacy
Yes. There are no sensitive data in the newly collected information.
Changelog
NoneBreaking Changes
No
References
https://redhat.atlassian.net/browse/CCXDEV-16594
Summary by CodeRabbit
New Features
Bug Fixes