CNTRLPLANE-625: Add support for custom trust bundle for AWS API endpoints#8280
CNTRLPLANE-625: Add support for custom trust bundle for AWS API endpoints#8280hypershift-jira-solve-ci[bot] wants to merge 10 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-625 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 story 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. |
openshift-ci
Bot
added
area/control-plane-operator
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds exported AWS constant Sequence DiagramsequenceDiagram
participant Reconciler as Reconciler
participant MgmtCluster as Management Cluster
participant GuestCluster as Guest Cluster
Reconciler->>Reconciler: Detect platform == AWS
Reconciler->>MgmtCluster: Get AWS provider-config ConfigMap (provider key)
MgmtCluster-->>Reconciler: Return provider config or NotFound/Error
alt AdditionalTrustBundle or TrustedCA set
Reconciler->>MgmtCluster: Get managed trust-bundle ConfigMap
alt Trust-bundle exists and non-empty
MgmtCluster-->>Reconciler: Return CA bundle data
else Missing or empty
MgmtCluster-->>Reconciler: NotFound or empty
end
end
Reconciler->>GuestCluster: Create/Update openshift-config/cloud-provider-config with provider config
alt CA bundle available
Reconciler->>GuestCluster: Set aws.CABundleKey = CA data
else No CA bundle
Reconciler->>GuestCluster: Ensure aws.CABundleKey absent
end
GuestCluster-->>Reconciler: Acknowledge write or return error
Reconciler->>Reconciler: Finish reconciliation
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go`:
- Line 2446: Check that reference.Data[aws.ProviderConfigKey] is present and
non-empty before assigning it into cm.Data[aws.ProviderConfigKey]; if it's
missing or empty, return an error (fail fast) instead of writing an empty
aws.conf. Update the sync logic around cm and reference (the block that
currently does cm.Data[aws.ProviderConfigKey] =
reference.Data[aws.ProviderConfigKey]) to validate the value, log/return a
descriptive error when absent, and only set cm.Data when the provider config is
valid.
🪄 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: Pro Plus
Run ID: 34913917-4951-4a83-8093-81ebed65cba8
📒 Files selected for processing (5)
control-plane-operator/controllers/hostedcontrolplane/cloud/aws/providerconfig.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/reconcile_cloud_config_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gosupport/globalconfig/infrastructure.gosupport/globalconfig/infrastructure_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8280 +/- ##
==========================================
+ Coverage 43.77% 43.79% +0.02%
==========================================
Files 772 772
Lines 95941 95999 +58
==========================================
+ Hits 41994 42041 +47
- Misses 51035 51043 +8
- Partials 2912 2915 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
7c4531d to
55275ba
Compare
|
Re: review PRR_kwDOE7ekcc72sY6X Thanks for the review. The actionable finding about validating AI-assisted response via Claude Code |
|
Re: #8280 (comment) Thanks for the walkthrough summary — it accurately describes the changes. No action needed. AI-assisted response via Claude Code |
csrwng
left a comment
csrwng
left a comment
There was a problem hiding this comment.
This change is ok, but it doesn't enable the use of custom trust bundles for the entire control plane. Maybe handling the rest of the control plane components (capi provider, control-plane-operator, etc) can be done under a separate story.
|
|
||
| switch platformType { | ||
| case hyperv1.AWSPlatform: | ||
| infra.Spec.CloudConfig.Name = "cloud-provider-config" |
There was a problem hiding this comment.
This should use a constant. Ideally the same constant used by resources.go
There was a problem hiding this comment.
Done. Defined CloudProviderCMName constant in the globalconfig package and updated both infrastructure.go and resources.go to use it. The resources package now references globalconfig.CloudProviderCMName instead of its own duplicate literal. Also updated the infrastructure tests to use the constant.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Done. Extracted a CloudProviderConfigName constant to the support/globalconfig package and updated both infrastructure.go and resources.go to use it. The CloudProviderCMName in resources.go now references globalconfig.CloudProviderConfigName instead of a local string literal.
AI-assisted response via Claude Code
| } | ||
|
|
||
| var caBundle string | ||
| if hcp.Spec.AdditionalTrustBundle != nil { |
There was a problem hiding this comment.
We should use the aggregate of of spec.AdditionalTrustBundle and .spec.configuration.Proxy.TrustedCA
There was a problem hiding this comment.
Done. Switched to using the aggregated trusted-ca-bundle-managed ConfigMap (via cpomanifests.TrustedCABundleConfigMap) which the HCP controller already populates by combining both spec.AdditionalTrustBundle and spec.Configuration.Proxy.TrustedCA. The condition now triggers on either source being set. Added a test case for the proxy TrustedCA path.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Done. Switched from UserCAConfigMap (which only contains AdditionalTrustBundle) to TrustedCABundleConfigMap (trusted-ca-bundle-managed), which already contains the aggregate of both spec.AdditionalTrustBundle and .spec.configuration.Proxy.TrustedCA — built by the CPO's reconcileManagedTrustedCABundle. Also updated the condition to check for either source being set, and added a test case for proxy-only TrustedCA.
AI-assisted response via Claude Code
|
Re: review PRR_kwDOE7ekcc73B2Hd Agreed — this PR focuses on the cloud-provider-config for the guest cluster's AWS SDK usage. Extending custom trust bundle support to the remaining control plane components (CAPI provider, control-plane-operator, etc.) should be tracked under a separate story to keep the scope manageable. AI-assisted response via Claude Code |
|
Re: review 4144456157 Agreed — this PR focuses on the cloud-provider-config use case for AWS. Extending custom trust bundle support to other control plane components (CAPI provider, control-plane-operator, etc.) can be tracked and handled under a separate story. AI-assisted response via Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go`:
- Around line 2423-2443: The reconcile can stall because when
cpomanifests.TrustedCABundleConfigMap(hcp.Namespace) or the AWS provider source
(cpomanifests.AWSProviderConfig) is expected but not present the controller
neither watches those management-cluster ConfigMaps nor requeues for retry;
update the controller to either (A) add watches for those management-cluster
ConfigMap sources in SetupWithManager (watch ConfigMap resources filtered by
name/namespace for the TrustedCABundle and aws-cloud-config so guest reconciles
are triggered when they appear/change) or (B) when cpClient.Get(...) returns
IsNotFound for the managed trusted CA (the branch inside the reconcile where you
currently log and skip), return a requeue result (e.g.,
ctrl.Result{RequeueAfter: <reasonable backoff>}) so the reconcile will retry
until the bundle appears; reference cpomanifests.TrustedCABundleConfigMap,
cpomanifests.AWSProviderConfig, aws.ProviderConfigKey, SetupWithManager and the
reconcile path that calls r.cpClient.Get.
🪄 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: Pro Plus
Run ID: e8d0cfdf-1e1d-470d-8230-bd36caa5c574
📒 Files selected for processing (4)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/reconcile_cloud_config_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gosupport/globalconfig/infrastructure.gosupport/globalconfig/infrastructure_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- support/globalconfig/infrastructure_test.go
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/reconcile_cloud_config_test.go
| reference := cpomanifests.AWSProviderConfig(hcp.Namespace) | ||
| if err := r.cpClient.Get(ctx, client.ObjectKeyFromObject(reference), reference); err != nil { | ||
| return fmt.Errorf("failed to fetch %s/%s configmap from management cluster: %w", reference.Namespace, reference.Name, err) | ||
| } | ||
| providerConfig, ok := reference.Data[aws.ProviderConfigKey] | ||
| if !ok || strings.TrimSpace(providerConfig) == "" { | ||
| return fmt.Errorf("source configmap %s/%s is missing required key %q", reference.Namespace, reference.Name, aws.ProviderConfigKey) | ||
| } | ||
|
|
||
| // Use the aggregated trust bundle that combines spec.AdditionalTrustBundle | ||
| // and spec.Configuration.Proxy.TrustedCA, managed by the HCP controller. | ||
| var caBundle string | ||
| if hcp.Spec.AdditionalTrustBundle != nil || | ||
| (hcp.Spec.Configuration != nil && hcp.Spec.Configuration.Proxy != nil && hcp.Spec.Configuration.Proxy.TrustedCA.Name != "") { | ||
| managedTrustBundle := cpomanifests.TrustedCABundleConfigMap(hcp.Namespace) | ||
| if err := r.cpClient.Get(ctx, client.ObjectKeyFromObject(managedTrustBundle), managedTrustBundle); err != nil { | ||
| if !apierrors.IsNotFound(err) { | ||
| return fmt.Errorf("failed to fetch managed trusted CA bundle from management cluster: %w", err) | ||
| } | ||
| ctrl.LoggerFrom(ctx).Info("managed trusted CA bundle ConfigMap not found, skipping CA bundle sync", "configmap", client.ObjectKeyFromObject(managedTrustBundle)) | ||
| } else { |
There was a problem hiding this comment.
This sync can get stuck after bootstrap.
When a trust bundle is configured, Line 2438 logs and skips if trusted-ca-bundle-managed is absent. But this controller does not watch the management-cluster trusted-ca-bundle-managed or aws-cloud-config ConfigMaps, so the guest cloud-provider-config may never pick up the CA bundle once it appears, or later source updates, until some unrelated reconcile happens. Please add CP-cluster watches for those source ConfigMaps, or explicitly requeue while the managed bundle is expected but missing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go`
around lines 2423 - 2443, The reconcile can stall because when
cpomanifests.TrustedCABundleConfigMap(hcp.Namespace) or the AWS provider source
(cpomanifests.AWSProviderConfig) is expected but not present the controller
neither watches those management-cluster ConfigMaps nor requeues for retry;
update the controller to either (A) add watches for those management-cluster
ConfigMap sources in SetupWithManager (watch ConfigMap resources filtered by
name/namespace for the TrustedCABundle and aws-cloud-config so guest reconciles
are triggered when they appear/change) or (B) when cpClient.Get(...) returns
IsNotFound for the managed trusted CA (the branch inside the reconcile where you
currently log and skip), return a requeue result (e.g.,
ctrl.Result{RequeueAfter: <reasonable backoff>}) so the reconcile will retry
until the bundle appears; reference cpomanifests.TrustedCABundleConfigMap,
cpomanifests.AWSProviderConfig, aws.ProviderConfigKey, SetupWithManager and the
reconcile path that calls r.cpClient.Get.
There was a problem hiding this comment.
The concern is valid in the abstract, but in practice this controller is not at risk of stalling for this case. Here's why:
-
HCP is watched (line 288): The HCP controller creates the
trusted-ca-bundle-managedConfigMap during HCP reconciliation, and HCP status updates are frequent — any HCP object change triggers an HCCO reconcile. -
Broad guest-cluster watches (lines 245-276): The controller watches 20+ resource types on the guest cluster (ConfigMaps, Secrets, Services, etc.), so reconciles fire very frequently during and after bootstrap.
-
Intentional graceful degradation: The "log and skip" pattern at line 2442 is deliberate — the cloud-provider-config should be created promptly even if the optional CA bundle isn't available yet. The next reconcile (triggered by HCP or guest-cluster changes) picks it up. Returning an error or requeueing would block the entire reconcile for a non-critical optional field.
-
Consistent with existing patterns: Other management-cluster resources like the
aws-cloud-configConfigMap (line 2423), pull secrets (line 366), and numerous othercpClient.Getcalls throughout this controller also lack explicit CP-cluster watches — they all rely on HCP or guest-cluster changes to trigger reconciles. Adding watches only for these two ConfigMaps would be inconsistent.
The aws-cloud-config case at line 2423 does return a hard error (which causes controller-runtime to requeue), but that's because the provider config is required — the cloud config cannot be built without it. The trust bundle is optional additional trust, and the ConfigMap is still created successfully without it.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
There was a problem hiding this comment.
This concern is not actionable for this controller. The HCP watch at line 288 ensures any HostedControlPlane change triggers a full reconcile — and HCP status updates are frequent during and after bootstrap. This is the same mechanism that drives every other cpClient.Get call in this controller (pull secrets, root CAs, OIDC secrets, etc.), none of which have dedicated ConfigMap watches either.
The "log and skip" pattern here is intentional: the cloud-provider-config ConfigMap should be created promptly with the provider config even if the optional CA bundle isn't available yet. The next HCP- or guest-cluster-triggered reconcile picks it up. Returning an error would block the entire reconcile for an optional field.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
There was a problem hiding this comment.
Only repository collaborators, contributors, or members can run CodeRabbit commands.
There was a problem hiding this comment.
Confirming the previous responses are correct — this is not a practical concern.
The HCCO resources controller watches HostedControlPlane (line 198: For(&hyperv1.HostedControlPlane{})), and the HCP controller is what creates the trusted-ca-bundle-managed ConfigMap. HCP status updates are frequent during and after bootstrap, which triggers this controller to re-reconcile and pick up the ConfigMap once it exists. This is the same pattern used by all ~20+ cpClient.Get calls in this controller (pull secrets, certs, cloud configs, etc.) — none have dedicated watches; they all rely on the HCP watch for re-reconciliation.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
There was a problem hiding this comment.
Reviewed. The concern is not actionable — the controller already watches HostedControlPlane (line 288) on the CP cluster, so any HCP change (including when the HCP controller creates the managed trust bundle ConfigMap) triggers a full reconcile. The "log and skip" pattern here is intentional graceful degradation for an optional field, consistent with how all other cpClient.Get calls in this controller work. No watches or requeue needed.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
|
Re: #8280 (review) Thanks for the review. The one actionable inline comment about potential stalling of the trust bundle sync has been addressed with an inline reply — the current "log and skip" pattern is intentional graceful degradation, consistent with the controller's existing architecture. See the inline thread for detailed rationale. AI-assisted response via Claude Code |
|
Re: #8280 (review) Acknowledged. The single actionable comment about the reconcile potentially stalling has been addressed in the inline thread. In short: the concern is valid in the abstract, but not a practical risk here. The AI-assisted response via Claude Code |
The comment analyzer flags review bodies and review threads independently. When an approved bot (e.g. CodeRabbit) submits a review with both a body and inline comments, both get flagged as separate needs_attention items. Claude addresses each independently, producing duplicate responses. Observed on openshift/hypershift#8280: CodeRabbit review #4146142149 had a body ("Actionable comments posted: 1") and one inline comment about trust bundle sync stalling. The analyzer returned both as separate items. Claude posted an inline reply to the thread at 11:04:10Z, then two issue comments at 11:04:20Z and 11:06:19Z — both "Re: #pullrequestreview-4146142149" with overlapping content. Root cause: analyze_review_bodies uses per-comment reply matching (checking if any bot comment references the review ID/URL). Since no bot comment had yet referenced review 4146142149, it was flagged. Meanwhile, analyze_review_threads independently flagged the same review's inline thread. Both items entered the needs_attention list. Fix: add comments { totalCount } to the reviews GraphQL query and skip approved bot review bodies when inline comments exist. Bot review bodies are machine-generated summaries of their inline comments (already handled by analyze_review_threads). Human review bodies are always kept since they often contain distinct feedback. Verified against PR #8280: - CodeRabbit review 4146142149: SKIPPED (approved_bot, inline_count=1) - CodeRabbit review 4138831511: SKIPPED (approved_bot, inline_count=1) - csrwng review 4144456157: KEPT (human, inline_count=2) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/reconcile_cloud_config_test.go (2)
301-305: Make error-path assertions specific to avoid false positives.The current
expectErrorbranch only checks that some error occurred. A different unintended failure would still pass. Please assert error type/message per case.Proposed hardening
type testCase struct { name string hcp *hyperv1.HostedControlPlane cpObjects []client.Object guestObjects []client.Object expectError bool + errContains string verify func(g Gomega, guestClient client.Client) } // ... { name: "When aws-cloud-config ConfigMap is missing, it should return an error", // ... cpObjects: []client.Object{}, expectError: true, + errContains: "not found", }, // ... { name: "When aws-cloud-config ConfigMap is missing the provider config key, it should return an error", // ... expectError: true, + errContains: aws.ProviderConfigKey, }, // ... err := r.reconcileCloudConfig(context.Background(), tc.hcp) if tc.expectError { g.Expect(err).To(HaveOccurred()) + if tc.errContains != "" { + g.Expect(err.Error()).To(ContainSubstring(tc.errContains)) + } return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/hostedclusterconfigoperator/controllers/resources/reconcile_cloud_config_test.go` around lines 301 - 305, The test's error branch only checks for any error; make it assert specific error types/messages to avoid false positives by adding an expected error indicator on each test case (e.g., tc.expectedErr or tc.expectedErrMsg) and then change the branch that calls r.reconcileCloudConfig to assert both presence and content/type: when tc.expectError is true assert the error occurred and that err.Error() contains tc.expectedErrMsg (or use errors.Is/As against tc.expectedErr for typed errors) instead of only HaveOccurred, referencing the test case variable tc and the reconcileCloudConfig(...) call to locate where to add the stricter assertions.
202-230: Add parity coverage for missing managed trust bundle withProxy.TrustedCA.You already test graceful fallback when
AdditionalTrustBundleis set and the managed trust bundle ConfigMap is missing. Please add the equivalent case forSpec.Configuration.Proxy.TrustedCAto lock in behavior for both triggers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/hostedclusterconfigoperator/controllers/resources/reconcile_cloud_config_test.go` around lines 202 - 230, Add a parallel test case to the existing one that covers the scenario where Spec.Configuration.Proxy.TrustedCA is set but the managed trust bundle ConfigMap is missing: create a HostedControlPlane with Spec.Configuration.Proxy.TrustedCA pointing to a LocalObjectReference (instead of AdditionalTrustBundle), include fakeAWSCloudConfig() in cpObjects, and in the verify closure fetch the ConfigMap by CloudProviderCMName and assert cm.Data has aws.ProviderConfigKey with the base config and does NOT have aws.CABundleKey; reuse the same structure and assertions as the AdditionalTrustBundle test but target HostedControlPlane.Spec.Configuration.Proxy.TrustedCA to ensure parity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/reconcile_cloud_config_test.go`:
- Around line 301-305: The test's error branch only checks for any error; make
it assert specific error types/messages to avoid false positives by adding an
expected error indicator on each test case (e.g., tc.expectedErr or
tc.expectedErrMsg) and then change the branch that calls r.reconcileCloudConfig
to assert both presence and content/type: when tc.expectError is true assert the
error occurred and that err.Error() contains tc.expectedErrMsg (or use
errors.Is/As against tc.expectedErr for typed errors) instead of only
HaveOccurred, referencing the test case variable tc and the
reconcileCloudConfig(...) call to locate where to add the stricter assertions.
- Around line 202-230: Add a parallel test case to the existing one that covers
the scenario where Spec.Configuration.Proxy.TrustedCA is set but the managed
trust bundle ConfigMap is missing: create a HostedControlPlane with
Spec.Configuration.Proxy.TrustedCA pointing to a LocalObjectReference (instead
of AdditionalTrustBundle), include fakeAWSCloudConfig() in cpObjects, and in the
verify closure fetch the ConfigMap by CloudProviderCMName and assert cm.Data has
aws.ProviderConfigKey with the base config and does NOT have aws.CABundleKey;
reuse the same structure and assertions as the AdditionalTrustBundle test but
target HostedControlPlane.Spec.Configuration.Proxy.TrustedCA to ensure parity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: a8eaacd9-0e27-4e9b-9f37-4a9f557c2411
📒 Files selected for processing (1)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/reconcile_cloud_config_test.go
|
Re: #8280 (review) Done. Addressed both nitpick suggestions:
All 9 test cases pass. AI-assisted response via Claude Code |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/reconcile_cloud_config_test.go (2)
233-255: Add a whitespace-only provider-config test case for full branch coverage.Line 234 currently validates a missing
aws.ProviderConfigKey, but the reconciler also rejects whitespace-only values. Adding that case will lock in thestrings.TrimSpace(...) == ""behavior.➕ Proposed test case
{ name: "When aws-cloud-config ConfigMap is missing the provider config key, it should return an error", hcp: &hyperv1.HostedControlPlane{ @@ expectError: true, errContains: aws.ProviderConfigKey, }, + { + name: "When aws-cloud-config ConfigMap has whitespace-only provider config, it should return an error", + hcp: &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: hcpNamespace, + }, + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AWSPlatform, + }, + }, + }, + cpObjects: []client.Object{ + func() *corev1.ConfigMap { + cm := cpomanifests.AWSProviderConfig(hcpNamespace) + cm.Data = map[string]string{ + aws.ProviderConfigKey: " \n\t", + } + return cm + }(), + }, + expectError: true, + errContains: aws.ProviderConfigKey, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/hostedclusterconfigoperator/controllers/resources/reconcile_cloud_config_test.go` around lines 233 - 255, Add a new test case in reconcile_cloud_config_test.go mirroring the existing missing-key case but instead set the aws provider config key to a whitespace-only value (e.g., cm.Data[aws.ProviderConfigKey] = " "), keep expectError: true and errContains: aws.ProviderConfigKey so the test exercises the strings.TrimSpace(... ) == "" branch; place it alongside the current case so it covers the whitespace-only rejection path.
73-319: Consider extracting a small assertion helper to reduce repeated verify closures.The repeated
guestClient.Get(...)+ provider-key assertions across multiple cases add maintenance overhead; a helper would make future case additions simpler and less error-prone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/hostedclusterconfigoperator/controllers/resources/reconcile_cloud_config_test.go` around lines 73 - 319, Extract a small test helper (e.g., assertCloudProviderConfig or verifyCloudProviderConfig) that wraps the repeated guestClient.Get call and the assertions against aws.ProviderConfigKey and aws.CABundleKey (and accepts parameters like Gomega, client.Client, expected provider config value, and optional expected CA bundle or a flag indicating absence) and replace the inline verify closures in tests that use fakeAWSCloudConfig() and fakeManagedTrustBundle() with calls to this helper; update test cases that expect no CA bundle to call the helper with an empty/absent CA parameter and cases that expect a bundle to pass the exact string, keeping references to guestClient.Get, aws.ProviderConfigKey, aws.CABundleKey, fakeAWSCloudConfig, and fakeManagedTrustBundle to locate where to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/reconcile_cloud_config_test.go`:
- Around line 233-255: Add a new test case in reconcile_cloud_config_test.go
mirroring the existing missing-key case but instead set the aws provider config
key to a whitespace-only value (e.g., cm.Data[aws.ProviderConfigKey] = " "),
keep expectError: true and errContains: aws.ProviderConfigKey so the test
exercises the strings.TrimSpace(... ) == "" branch; place it alongside the
current case so it covers the whitespace-only rejection path.
- Around line 73-319: Extract a small test helper (e.g.,
assertCloudProviderConfig or verifyCloudProviderConfig) that wraps the repeated
guestClient.Get call and the assertions against aws.ProviderConfigKey and
aws.CABundleKey (and accepts parameters like Gomega, client.Client, expected
provider config value, and optional expected CA bundle or a flag indicating
absence) and replace the inline verify closures in tests that use
fakeAWSCloudConfig() and fakeManagedTrustBundle() with calls to this helper;
update test cases that expect no CA bundle to call the helper with an
empty/absent CA parameter and cases that expect a bundle to pass the exact
string, keeping references to guestClient.Get, aws.ProviderConfigKey,
aws.CABundleKey, fakeAWSCloudConfig, and fakeManagedTrustBundle to locate where
to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 71bd9b3b-ea57-4f82-ab8b-5dff896f5cc7
📒 Files selected for processing (1)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/reconcile_cloud_config_test.go
|
Re: #8280 (review) Thanks for the nitpick suggestions!
AI-assisted response via Claude Code |
dcd520b to
8f48ddb
Compare
…hift#78134) The comment analyzer flags review bodies and review threads independently. When an approved bot (e.g. CodeRabbit) submits a review with both a body and inline comments, both get flagged as separate needs_attention items. Claude addresses each independently, producing duplicate responses. Observed on openshift/hypershift#8280: CodeRabbit review #4146142149 had a body ("Actionable comments posted: 1") and one inline comment about trust bundle sync stalling. The analyzer returned both as separate items. Claude posted an inline reply to the thread at 11:04:10Z, then two issue comments at 11:04:20Z and 11:06:19Z — both "Re: #pullrequestreview-4146142149" with overlapping content. Root cause: analyze_review_bodies uses per-comment reply matching (checking if any bot comment references the review ID/URL). Since no bot comment had yet referenced review 4146142149, it was flagged. Meanwhile, analyze_review_threads independently flagged the same review's inline thread. Both items entered the needs_attention list. Fix: add comments { totalCount } to the reviews GraphQL query and skip approved bot review bodies when inline comments exist. Bot review bodies are machine-generated summaries of their inline comments (already handled by analyze_review_threads). Human review bodies are always kept since they often contain distinct feedback. Verified against PR openshift#8280: - CodeRabbit review 4146142149: SKIPPED (approved_bot, inline_count=1) - CodeRabbit review 4138831511: SKIPPED (approved_bot, inline_count=1) - csrwng review 4144456157: KEPT (human, inline_count=2) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
csrwng
left a comment
csrwng
left a comment
There was a problem hiding this comment.
Overall the approach is sound — follows the existing Azure/OpenStack pattern, gates on trust bundle presence, and has good test coverage. Two minor nits on the implementation.
|
|
||
| if !hasTrustBundle { | ||
| cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: ConfigNamespace, Name: CloudProviderCMName}} | ||
| if err := r.client.Delete(ctx, cm); err != nil && !apierrors.IsNotFound(err) { |
There was a problem hiding this comment.
nit: prefer using k8sutil.DeleteIfNeeded here for consistency with the rest of the file (e.g., lines 2105, 2130, 3122).
There was a problem hiding this comment.
Done. Switched to k8sutil.DeleteIfNeeded for both cloud-provider-config and the new kube-cloud-config ConfigMaps.
AI-assisted response via Claude Code
| if !apierrors.IsNotFound(err) { | ||
| return fmt.Errorf("failed to fetch managed trusted CA bundle from management cluster: %w", err) | ||
| } | ||
| ctrl.LoggerFrom(ctx).Info("managed trusted CA bundle ConfigMap not found, skipping CA bundle sync", "configmap", client.ObjectKeyFromObject(managedTrustBundle)) |
There was a problem hiding this comment.
nit: The log says "skipping CA bundle sync" but execution continues into CreateOrUpdate below — nothing is actually skipped. The cloud-provider-config ConfigMap is still created/updated (just without the CA bundle key, because caBundle stays empty). Consider rewording to something like "managed trusted CA bundle ConfigMap not found, proceeding without CA bundle" to match the actual behavior.
There was a problem hiding this comment.
Done. Reworded to "managed trusted CA bundle ConfigMap not found, proceeding without CA bundle" to match the actual behavior.
AI-assisted response via Claude Code
|
/test address-review-comments |
|
Review agent triggered. View job |
|
/rebase |
openshift-ci
Bot
added
the
needs-rebase
|
🤖 Rebasing PR onto main: workflow run |
… for AWS Guest cluster operators (e.g. cloud-network-config-controller, cluster-ingress-operator) need the additional trust bundle to communicate with AWS API endpoints that use custom certificates, such as when using a proxy or private AWS endpoints. Previously, the trust bundle was only synced to the user-ca-bundle ConfigMap in the guest cluster's openshift-config namespace. With this change, we also sync it to the cloud-provider-config ConfigMap in the guest cluster, and set infra.Spec.CloudConfig on the Infrastructure resource to reference it, following the same pattern used by Azure and OpenStack platforms. The AWS case in reconcileCloudConfig now: - Always creates a cloud-provider-config ConfigMap with the AWS cloud config (aws.conf) - Includes the CA bundle (ca-bundle.pem) when additionalTrustBundle is set - Removes the CA bundle key when additionalTrustBundle is unset Ref: CNTRLPLANE-625 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ensure the new AWS cloud config reconciliation behavior is covered by unit tests: - Verify cloud-provider-config is created with CA bundle when additionalTrustBundle is set - Verify cloud-provider-config is created without CA bundle when additionalTrustBundle is not set - Verify CA bundle key is removed from cloud-provider-config when trust bundle is removed - Verify Infrastructure CloudConfig is set correctly for AWS platform Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…-ca-bundle gracefully Replace magic string "ca-bundle.crt" with certs.UserCABundleMapKey constant in reconcileCloudConfig and tests. Handle apierrors.IsNotFound for user-ca-bundle ConfigMap so the base aws.conf is still synced during bootstrap when the CA bundle isn't ready yet. Add error path and edge case tests for missing ConfigMaps and empty CA bundle values. Ticket: CNTRLPLANE-625 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…egate trust bundles Use globalconfig.CloudProviderCMName constant in both infrastructure.go and resources.go instead of duplicated string literals. Also switch from using only spec.AdditionalTrustBundle to the aggregated managed trust bundle (trusted-ca-bundle-managed) that combines both spec.AdditionalTrustBundle and spec.configuration.Proxy.TrustedCA, consistent with the HCP controller's existing aggregation pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move configv1 import to the github.com/openshift group, separate from the github.com/openshift/hypershift group, to satisfy the project's gci linter configuration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add specific error message assertions (errContains) to error-path test cases to prevent false positives from unintended failures. Add parity test coverage for the Proxy.TrustedCA code path when the managed trust bundle ConfigMap is missing, matching existing AdditionalTrustBundle coverage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…balconfig Move CABundleKey and cloud config key constants to support/globalconfig to fix the import direction anti-pattern where support/ imported from control-plane-operator/. This also deduplicates CABundleKey which was identical between the AWS and OpenStack packages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Make infra.Spec.CloudConfig and cloud-provider-config ConfigMap creation conditional on having a trust bundle configured (AdditionalTrustBundle or Proxy.TrustedCA). This avoids introducing new CloudConfig and ConfigMap resources into all existing AWS guest clusters, which could change guest operator behavior. When no trust bundle is configured, any previously-synced cloud-provider-config ConfigMap is cleaned up and CloudConfig is cleared. Also replaces aws.ProviderConfigKey usage in resources.go with the shared globalconfig.AWSProviderConfigKey constant, removing the import anti-pattern of support/ depending on control-plane-operator/. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use k8sutil.DeleteIfNeeded for consistency with the rest of the file - Reword misleading "skipping CA bundle sync" log message to match actual behavior (execution continues into CreateOrUpdate) - Add kube-cloud-config ConfigMap in openshift-config-managed for AWS, mirroring the OpenStack pattern so CNO can pass the config to cloud-network-config-controller Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ee5d5f1 to
4864818
Compare
openshift-ci
Bot
removed
the
needs-rebase
csrwng
added
the
tide/merge-method-squash
|
/approve |
openshift-ci
Bot
added
the
approved
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, csrwng, hypershift-jira-solve-ci[bot] 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 |
|
Now I have all the data I need. Let me produce the final report: Test Failure Analysis CompleteJob InformationJob 1: e2e-v2-aws
Job 2: e2e-aws
Test Failure AnalysisErrorSummaryBoth jobs failed from different root causes. e2e-v2-aws is a pure CI infrastructure flake — all 473 executed tests passed, but two post-phase cleanup pods were evicted due to DiskPressure on the build01 node; a Root Causee2e-v2-aws: CI Infrastructure Flake (NOT PR-related) The build01 CI cluster node experienced DiskPressure, causing Kubernetes to evict two post-phase pods ( e2e-aws: MCO Ignition Payload Generation Failure (Potentially PR-related) The
The test file ( Recommendations
Evidence
|
|
/retest |
|
@hypershift-jira-solve-ci[bot]: 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. |
5 participants
What this PR does / why we need it:
Guest cluster operators (e.g.
cloud-network-config-controller,cluster-ingress-operator) need the additional trust bundle to communicate with AWS API endpoints that use custom certificates, such as when using a proxy or private AWS endpoints. Previously, the trust bundle was only synced to theuser-ca-bundleConfigMap in the guest cluster'sopenshift-confignamespace, but AWS guest operators did not pick it up from there.This PR syncs the additional trust bundle into the
cloud-provider-configConfigMap in the guest cluster and setsinfra.Spec.CloudConfigon the Infrastructure resource to reference it, following the same pattern already used by Azure and OpenStack platforms.Changes
reconcileCloudConfignow creates acloud-provider-configConfigMap containing the AWS cloud config (aws.conf) and, whenadditionalTrustBundleis set, the CA bundle (ca-bundle.pem). When the trust bundle is removed, the CA bundle key is cleaned up.ReconcileInfrastructurenow setsinfra.Spec.CloudConfig.Nameandinfra.Spec.CloudConfig.Keyfor AWS, so guest operators discover the cloud config via the standard Infrastructure resource.user-ca-bundleConfigMap is not yet available (e.g. during bootstrap), the reconciler logs and continues syncing the base AWS config without the CA bundle, rather than failing.aws.CABundleKeyconstant instead of magic strings.Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-625
Special notes for your reviewer:
reconcileCloudConfigAzure case) and OpenStack.user-ca-bundleConfigMap may not exist during early bootstrap; the code handlesIsNotFoundgracefully to avoid blocking reconciliation.Checklist:
Always review AI generated responses prior to use.
Generated with Claude Code via
/jira:solve [CNTRLPLANE-625](https://redhat.atlassian.net/browse/CNTRLPLANE-625)Summary by CodeRabbit
New Features
Bug Fixes
Tests