Skip to content

fix: OCPBUGS-81675: hcco: merge additionalTrustBundle into proxy trusted CA bundle #8520

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dpateriya wants to merge 1 commit into
base: main
Choose a base branch
from
+276 −13
Open

fix: OCPBUGS-81675: hcco: merge additionalTrustBundle into proxy trusted CA bundle #8520

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 44 additions & 13 deletions control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import (
hyperapi "github.com/openshift/hypershift/support/api"
"github.com/openshift/hypershift/support/azureutil"
"github.com/openshift/hypershift/support/capabilities"
"github.com/openshift/hypershift/support/certs"
"github.com/openshift/hypershift/support/config"
"github.com/openshift/hypershift/support/globalconfig"
"github.com/openshift/hypershift/support/releaseinfo"
Expand Down Expand Up @@ -1071,6 +1072,13 @@ func (r *reconciler) reconcileConfig(ctx context.Context, hcp *hyperv1.HostedCon
proxy := globalconfig.ProxyConfig()
if _, err := r.CreateOrUpdate(ctx, r.client, proxy, func() error {
globalconfig.ReconcileInClusterProxyConfig(proxy, hcp.Spec.Configuration)
// When no proxy.trustedCA is set but additionalTrustBundle is, point the
// Proxy object at openshift-config/user-ca-bundle (already written by
// reconcileUserCertCABundle) so CNO merges it into
// openshift-config-managed/trusted-ca-bundle.
if proxy.Spec.TrustedCA.Name == "" && hcp.Spec.AdditionalTrustBundle != nil {
proxy.Spec.TrustedCA.Name = manifests.UserCABundle().Name
}
return nil
}); err != nil {
errs = append(errs, fmt.Errorf("failed to reconcile proxy config: %w", err))
Expand Down Expand Up @@ -1131,19 +1139,23 @@ func (r *reconciler) reconcileProxyTrustedCAConfigMap(ctx context.Context, hcp *

currentConfigMapRef := proxy.Spec.TrustedCA.Name
if currentConfigMapRef != "" && currentConfigMapRef != configMapRef {
// cleanup old configMaps
cm := &corev1.ConfigMap{}
cm.Name = currentConfigMapRef

// log and ignore deletion errors, should not disrupt normal workflow
cm.Namespace = hcp.Namespace
if err := r.cpClient.Delete(ctx, cm); err != nil {
log.Error(err, "failed to delete configmap", "name", cm.Name, "namespace", cm.Namespace)
}
// Do not clean up user-ca-bundle when it is being managed via additionalTrustBundle;
// reconcileConfig sets proxy.Spec.TrustedCA.Name to user-ca-bundle in that case.
if !(currentConfigMapRef == manifests.UserCABundle().Name && hcp.Spec.AdditionalTrustBundle != nil) {
// cleanup old configMaps
cm := &corev1.ConfigMap{}
cm.Name = currentConfigMapRef

// log and ignore deletion errors, should not disrupt normal workflow
cm.Namespace = hcp.Namespace
if err := r.cpClient.Delete(ctx, cm); err != nil {
log.Error(err, "failed to delete configmap", "name", cm.Name, "namespace", cm.Namespace)
}

cm.Namespace = manifests.ProxyTrustedCAConfigMap("").Namespace
if err := r.client.Delete(ctx, cm); err != nil {
log.Error(err, "failed to delete configmap in hosted cluster", "name", cm.Name, "namespace", cm.Namespace)
cm.Namespace = manifests.ProxyTrustedCAConfigMap("").Namespace
if err := r.client.Delete(ctx, cm); err != nil {
log.Error(err, "failed to delete configmap in hosted cluster", "name", cm.Name, "namespace", cm.Namespace)
}
}
}

Expand All @@ -1158,7 +1170,26 @@ func (r *reconciler) reconcileProxyTrustedCAConfigMap(ctx context.Context, hcp *

destCM := manifests.ProxyTrustedCAConfigMap(sourceCM.Name)
if _, err := r.CreateOrUpdate(ctx, r.client, destCM, func() error {
destCM.Data = sourceCM.Data
destCM.Data = make(map[string]string)
for k, v := range sourceCM.Data {
destCM.Data[k] = v
}
// Also merge additionalTrustBundle so that CNO writes both CAs into
// openshift-config-managed/trusted-ca-bundle, benefiting all consumers
// (including CVO's update service TLS verification).
if hcp.Spec.AdditionalTrustBundle != nil {
atbCM := &corev1.ConfigMap{}
if err := r.cpClient.Get(ctx, client.ObjectKey{Namespace: hcp.Namespace, Name: hcp.Spec.AdditionalTrustBundle.Name}, atbCM); err != nil {
return fmt.Errorf("failed to get additionalTrustBundle configmap %s/%s: %w", hcp.Namespace, hcp.Spec.AdditionalTrustBundle.Name, err)
}
Comment on lines +1180 to +1184

@coderabbitai coderabbitai Bot May 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use the canonical control-plane ATB ConfigMap here.

This path reads hcp.Spec.AdditionalTrustBundle.Name, but reconcileUserCertCABundle below reads cpomanifests.UserCAConfigMap(hcp.Namespace). If those names ever diverge, the guest user-ca-bundle sync still works while this merge path fails with a not-found.

Suggested fix 
-			atbCM := &corev1.ConfigMap{}
-			if err := r.cpClient.Get(ctx, client.ObjectKey{Namespace: hcp.Namespace, Name: hcp.Spec.AdditionalTrustBundle.Name}, atbCM); err != nil {
-				return fmt.Errorf("failed to get additionalTrustBundle configmap %s/%s: %w", hcp.Namespace, hcp.Spec.AdditionalTrustBundle.Name, err)
+			atbCM := cpomanifests.UserCAConfigMap(hcp.Namespace)
+			if err := r.cpClient.Get(ctx, client.ObjectKeyFromObject(atbCM), atbCM); err != nil {
+				return fmt.Errorf("failed to get additionalTrustBundle configmap %s/%s: %w", atbCM.Namespace, atbCM.Name, err)
 			}
🤖 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
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go`
around lines 1180 - 1184, The code is fetching the AdditionalTrustBundle
ConfigMap by hcp.Spec.AdditionalTrustBundle.Name but reconcileUserCertCABundle
uses cpomanifests.UserCAConfigMap(hcp.Namespace), so change the lookup to use
the canonical control-plane ATB name: call r.cpClient.Get with
client.ObjectKey{Name: cpomanifests.UserCAConfigMap(hcp.Namespace).Name,
Namespace: hcp.Namespace} (or otherwise use
cpomanifests.UserCAConfigMap(hcp.Namespace) as the source of truth) when
populating atbCM so the merge path and reconcileUserCertCABundle use the same
ConfigMap name.

if v := atbCM.Data[certs.UserCABundleMapKey]; v != "" {
existing := destCM.Data[certs.UserCABundleMapKey]
if existing != "" && !strings.HasSuffix(existing, "\n") {
existing += "\n"
}
destCM.Data[certs.UserCABundleMapKey] = existing + v
}
}
return nil
}); err != nil {
return fmt.Errorf("failed to reconcile referenced TrustedCA config map %s/%s: %w", destCM.Namespace, destCM.Name, err)
Expand Down
232 changes: 232 additions & 0 deletions control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,6 +637,238 @@ func TestReconcileUserCertCABundle(t *testing.T) {
}
}

func TestReconcileProxyTrustedCAConfigMap(t *testing.T) {
const (
testNamespace = "master-cluster1"
testHCPName = "cluster1"
proxyCAName = "my-proxy-ca"
)

proxyCAData := "-----BEGIN CERTIFICATE-----\nMIIBfakeProxyCACert==\n-----END CERTIFICATE-----\n"
atbCAData := "-----BEGIN CERTIFICATE-----\nMIIBfakeAdditionalTrustBundleCert==\n-----END CERTIFICATE-----\n"

proxyCAConfigMap := func() *corev1.ConfigMap {
return &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Name: proxyCAName, Namespace: testNamespace},
Data: map[string]string{"ca-bundle.crt": proxyCAData},
}
}
additionalTrustBundleCM := func() *corev1.ConfigMap {
return &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Name: "user-ca-bundle", Namespace: testNamespace},
Data: map[string]string{"ca-bundle.crt": atbCAData},
}
}
guestProxyObj := func() *configv1.Proxy {
return globalconfig.ProxyConfig()
}

tests := map[string]struct {
inputHCP *hyperv1.HostedControlPlane
mgmtObjects []client.Object
guestObjects []client.Object
wantErr bool
wantDestCMExists bool
wantDestCMData string
}{
"When neither proxy.trustedCA nor additionalTrustBundle is set, it should not create a dest ConfigMap": {
inputHCP: &hyperv1.HostedControlPlane{
ObjectMeta: metav1.ObjectMeta{Name: testHCPName, Namespace: testNamespace},
},
mgmtObjects: []client.Object{},
guestObjects: []client.Object{guestProxyObj()},
wantErr: false,
wantDestCMExists: false,
},
"When only proxy.trustedCA is set, it should create dest ConfigMap with proxy CA data only": {
inputHCP: &hyperv1.HostedControlPlane{
ObjectMeta: metav1.ObjectMeta{Name: testHCPName, Namespace: testNamespace},
Spec: hyperv1.HostedControlPlaneSpec{
Configuration: &hyperv1.ClusterConfiguration{
Proxy: &configv1.ProxySpec{TrustedCA: configv1.ConfigMapNameReference{Name: proxyCAName}},
},
},
},
mgmtObjects: []client.Object{proxyCAConfigMap()},
guestObjects: []client.Object{guestProxyObj()},
wantErr: false,
wantDestCMExists: true,
wantDestCMData: proxyCAData,
},
"When both proxy.trustedCA and additionalTrustBundle are set, it should merge both CA bundles": {
inputHCP: &hyperv1.HostedControlPlane{
ObjectMeta: metav1.ObjectMeta{Name: testHCPName, Namespace: testNamespace},
Spec: hyperv1.HostedControlPlaneSpec{
AdditionalTrustBundle: &corev1.LocalObjectReference{Name: "user-ca-bundle"},
Configuration: &hyperv1.ClusterConfiguration{
Proxy: &configv1.ProxySpec{TrustedCA: configv1.ConfigMapNameReference{Name: proxyCAName}},
},
},
},
mgmtObjects: []client.Object{proxyCAConfigMap(), additionalTrustBundleCM()},
guestObjects: []client.Object{guestProxyObj()},
wantErr: false,
wantDestCMExists: true,
wantDestCMData: proxyCAData + atbCAData,
},
"When only additionalTrustBundle is set, it should return early without creating dest ConfigMap": {
inputHCP: &hyperv1.HostedControlPlane{
ObjectMeta: metav1.ObjectMeta{Name: testHCPName, Namespace: testNamespace},
Spec: hyperv1.HostedControlPlaneSpec{
AdditionalTrustBundle: &corev1.LocalObjectReference{Name: "user-ca-bundle"},
},
},
mgmtObjects: []client.Object{additionalTrustBundleCM()},
guestObjects: []client.Object{guestProxyObj()},
wantErr: false,
wantDestCMExists: false,
},
"When additionalTrustBundle ConfigMap does not exist on management cluster, it should return error": {
inputHCP: &hyperv1.HostedControlPlane{
ObjectMeta: metav1.ObjectMeta{Name: testHCPName, Namespace: testNamespace},
Spec: hyperv1.HostedControlPlaneSpec{
AdditionalTrustBundle: &corev1.LocalObjectReference{Name: "user-ca-bundle"},
Configuration: &hyperv1.ClusterConfiguration{
Proxy: &configv1.ProxySpec{TrustedCA: configv1.ConfigMapNameReference{Name: proxyCAName}},
},
},
},
mgmtObjects: []client.Object{proxyCAConfigMap()},
guestObjects: []client.Object{guestProxyObj()},
wantErr: true,
wantDestCMExists: false,
},
"When proxy data has no trailing newline and ATB is set, it should insert newline separator": {
inputHCP: &hyperv1.HostedControlPlane{
ObjectMeta: metav1.ObjectMeta{Name: testHCPName, Namespace: testNamespace},
Spec: hyperv1.HostedControlPlaneSpec{
AdditionalTrustBundle: &corev1.LocalObjectReference{Name: "user-ca-bundle"},
Configuration: &hyperv1.ClusterConfiguration{
Proxy: &configv1.ProxySpec{TrustedCA: configv1.ConfigMapNameReference{Name: proxyCAName}},
},
},
},
mgmtObjects: []client.Object{
&corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Name: proxyCAName, Namespace: testNamespace},
Data: map[string]string{"ca-bundle.crt": "-----BEGIN CERTIFICATE-----\nnotrailingnewline\n-----END CERTIFICATE-----"},
},
additionalTrustBundleCM(),
},
guestObjects: []client.Object{guestProxyObj()},
wantErr: false,
wantDestCMExists: true,
wantDestCMData: "-----BEGIN CERTIFICATE-----\nnotrailingnewline\n-----END CERTIFICATE-----\n" + atbCAData,
},
}

for name, tc := range tests {
t.Run(name, func(t *testing.T) {
t.Parallel()
g := NewGomegaWithT(t)
r := &reconciler{
client: fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(tc.guestObjects...).Build(),
CreateOrUpdateProvider: &simpleCreateOrUpdater{},
cpClient: fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(tc.mgmtObjects...).Build(),
hcpName: testHCPName,
hcpNamespace: testNamespace,
}
err := r.reconcileProxyTrustedCAConfigMap(t.Context(), tc.inputHCP)
if tc.wantErr {
g.Expect(err).To(HaveOccurred())
} else {
g.Expect(err).ToNot(HaveOccurred())
}

if tc.wantDestCMExists {
destCM := manifests.ProxyTrustedCAConfigMap(proxyCAName)
g.Expect(r.client.Get(t.Context(), client.ObjectKeyFromObject(destCM), destCM)).To(Succeed())
g.Expect(destCM.Data["ca-bundle.crt"]).To(Equal(tc.wantDestCMData))
} else {
destCM := manifests.ProxyTrustedCAConfigMap(proxyCAName)
getErr := r.client.Get(t.Context(), client.ObjectKeyFromObject(destCM), destCM)
g.Expect(apierrors.IsNotFound(getErr)).To(BeTrue())
}
})
}
}

func TestReconcileConfigProxyTrustedCA(t *testing.T) {
const (
testNamespace = "master-cluster1"
testHCPName = "cluster1"
proxyCAName = "my-proxy-ca"
)

tests := map[string]struct {
inputHCP *hyperv1.HostedControlPlane
guestObjects []client.Object
wantTrustedCAName string
}{
"When ATB is set and no proxy config, it should set trustedCA.name to user-ca-bundle": {
inputHCP: &hyperv1.HostedControlPlane{
ObjectMeta: metav1.ObjectMeta{Name: testHCPName, Namespace: testNamespace},
Spec: hyperv1.HostedControlPlaneSpec{
AdditionalTrustBundle: &corev1.LocalObjectReference{Name: "user-ca-bundle"},
},
Status: hyperv1.HostedControlPlaneStatus{
ControlPlaneEndpoint: hyperv1.APIEndpoint{Host: "api.example.com"},
},
},
guestObjects: []client.Object{globalconfig.ProxyConfig()},
wantTrustedCAName: manifests.UserCABundle().Name,
},
"When both ATB and proxy trustedCA are set, it should preserve the proxy trustedCA name": {
inputHCP: &hyperv1.HostedControlPlane{
ObjectMeta: metav1.ObjectMeta{Name: testHCPName, Namespace: testNamespace},
Spec: hyperv1.HostedControlPlaneSpec{
AdditionalTrustBundle: &corev1.LocalObjectReference{Name: "user-ca-bundle"},
Configuration: &hyperv1.ClusterConfiguration{
Proxy: &configv1.ProxySpec{TrustedCA: configv1.ConfigMapNameReference{Name: proxyCAName}},
},
},
Status: hyperv1.HostedControlPlaneStatus{
ControlPlaneEndpoint: hyperv1.APIEndpoint{Host: "api.example.com"},
},
},
guestObjects: []client.Object{globalconfig.ProxyConfig()},
wantTrustedCAName: proxyCAName,
},
"When neither ATB nor proxy trustedCA is set, it should leave trustedCA.name empty": {
inputHCP: &hyperv1.HostedControlPlane{
ObjectMeta: metav1.ObjectMeta{Name: testHCPName, Namespace: testNamespace},
Status: hyperv1.HostedControlPlaneStatus{
ControlPlaneEndpoint: hyperv1.APIEndpoint{Host: "api.example.com"},
},
},
guestObjects: []client.Object{globalconfig.ProxyConfig()},
wantTrustedCAName: "",
},
}

for name, tc := range tests {
t.Run(name, func(t *testing.T) {
t.Parallel()
g := NewGomegaWithT(t)
r := &reconciler{
client: fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(tc.guestObjects...).Build(),
CreateOrUpdateProvider: &simpleCreateOrUpdater{},
cpClient: fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(tc.inputHCP).Build(),
hcpName: testHCPName,
hcpNamespace: testNamespace,
}
// reconcileConfig will produce errors for other resources (infra, dns, etc.)
// that aren't seeded, but we only care about the Proxy mutation.
_ = r.reconcileConfig(t.Context(), tc.inputHCP)

proxy := globalconfig.ProxyConfig()
err := r.client.Get(t.Context(), client.ObjectKeyFromObject(proxy), proxy)
g.Expect(err).To(BeNil())
g.Expect(proxy.Spec.TrustedCA.Name).To(Equal(tc.wantTrustedCAName))
})
}
}

var _ manifestReconciler = manifestAndReconcile[*rbacv1.ClusterRole]{}

func TestDestroyCloudResources(t *testing.T) {
Expand Down