Skip to content

OCPBUGS-77557: propagate additionalTrustBundle to AWS control plane components #7907

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

Closed
sdminonne wants to merge 1 commit into from
Closed

OCPBUGS-77557: propagate additionalTrustBundle to AWS control plane components #7907

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
4 changes: 4 additions & 0 deletions ...-plane-operator/controllers/hostedcontrolplane/v2/awsnodeterminationhandler/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ func adaptDeployment(cpContext component.WorkloadContext, deployment *appsv1.Dep
}
})

if hcp.Spec.AdditionalTrustBundle != nil {

@bryan-cox bryan-cox Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: This component is already gated to AWS by its predicate, so the missing platform check is correct. But a one-line comment like // No platform check needed — component predicate restricts to AWS would prevent future reviewers from flagging it as a bug. Same applies to karpenter/deployment.go and cloud_controller_manager/aws/deployment.go.

podspec.DeploymentAddAWSCABundleVolume(hcp.Spec.AdditionalTrustBundle, deployment, cpContext.ReleaseImageProvider.GetImage(podspec.CPOImageName))

@bryan-cox bryan-cox Jun 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I thought the cpContext had a copy of the hcp you could pass in. Do you know if that is true?

@bryan-cox bryan-cox Jun 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah you are actually using it here https://github.com/openshift/hypershift/pull/7907/changes#diff-db92db45ba677a590c2e7e4cd186a414d3496b6f62954e4f98218c44518e24cbR25. So maybe the HCP should be referenced from that.

@sdminonne sdminonne Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point — updated to use cpContext.HCP consistently.

}

// Set replicas based on whether termination handler is needed
// If the disable annotation is present, scale to 0 replicas
deployment.Spec.Replicas = ptr.To[int32](1)
Expand Down
91 changes: 91 additions & 0 deletions ...e-operator/controllers/hostedcontrolplane/v2/awsnodeterminationhandler/deployment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ import (
hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
assets "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/v2/assets"
controlplanecomponent "github.com/openshift/hypershift/support/controlplane-component"
"github.com/openshift/hypershift/support/testutil"

corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

Expand Down Expand Up @@ -181,3 +183,92 @@ func TestGetTerminationHandlerQueueURL(t *testing.T) {
})
}
}

func TestAdaptDeploymentAWSCABundle(t *testing.T) {
testCases := []struct {
name string
additionalTrust *corev1.LocalObjectReference
expectCABundle bool
}{
{
name: "When additional trust bundle is set it should add combined CA bundle with init container",
additionalTrust: &corev1.LocalObjectReference{Name: "user-ca-bundle"},
expectCABundle: true,
},
{
name: "When no additional trust bundle is set it should not add CA bundle resources",
additionalTrust: nil,
expectCABundle: false,
},
}

hcp := &hyperv1.HostedControlPlane{
ObjectMeta: metav1.ObjectMeta{
Name: "test-cluster",
Namespace: "clusters-test-cluster",
},
Spec: hyperv1.HostedControlPlaneSpec{
InfraID: "test-infra-id",
Platform: hyperv1.PlatformSpec{
Type: hyperv1.AWSPlatform,
AWS: &hyperv1.AWSPlatformSpec{
Region: "us-east-1",
},
},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
g := NewGomegaWithT(t)

hcp.Spec.AdditionalTrustBundle = tc.additionalTrust

cpContext := controlplanecomponent.WorkloadContext{

@bryan-cox bryan-cox Jun 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you pull this block outside the loop instead of making it over and over?

@sdminonne sdminonne Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ack — will hoist the HCP/cpContext setup outside the loop.

@bryan-cox bryan-cox Jun 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This still looks like it's inside the loop. You can build the base HCP once outside and just set hcp.Spec.AdditionalTrustBundle = tc.additionalTrust inside each iteration.

Context: t.Context(),
HCP: hcp,
ReleaseImageProvider: testutil.FakeImageProvider(),
}

deployment, err := assets.LoadDeploymentManifest(ComponentName)
g.Expect(err).ToNot(HaveOccurred())

err = adaptDeployment(cpContext, deployment)
g.Expect(err).ToNot(HaveOccurred())

volumes := deployment.Spec.Template.Spec.Volumes
initContainers := deployment.Spec.Template.Spec.InitContainers
container := deployment.Spec.Template.Spec.Containers[0]

if tc.expectCABundle {
g.Expect(volumes).To(ContainElement(SatisfyAll(
HaveField("Name", "user-ca-bundle"),
HaveField("VolumeSource.ConfigMap.Name", "user-ca-bundle"),
)))
g.Expect(volumes).To(ContainElement(SatisfyAll(
HaveField("Name", "aws-ca-bundle"),
HaveField("VolumeSource.EmptyDir", Not(BeNil())),
)))
g.Expect(initContainers).To(ContainElement(SatisfyAll(
HaveField("Name", "setup-aws-ca-bundle"),
HaveField("Image", "controlplane-operator"),
)))
g.Expect(container.VolumeMounts).To(ContainElement(SatisfyAll(
HaveField("Name", "aws-ca-bundle"),
HaveField("MountPath", "/etc/pki/ca-trust/extracted/hypershift"),
HaveField("ReadOnly", true),
)))
g.Expect(container.Env).To(ContainElement(SatisfyAll(
HaveField("Name", "AWS_CA_BUNDLE"),
HaveField("Value", "/etc/pki/ca-trust/extracted/hypershift/combined-ca-bundle.pem"),
)))
} else {
g.Expect(volumes).ToNot(ContainElement(HaveField("Name", "user-ca-bundle")))
g.Expect(volumes).ToNot(ContainElement(HaveField("Name", "aws-ca-bundle")))
g.Expect(initContainers).ToNot(ContainElement(HaveField("Name", "setup-aws-ca-bundle")))
g.Expect(container.VolumeMounts).ToNot(ContainElement(HaveField("Name", "aws-ca-bundle")))
g.Expect(container.Env).ToNot(ContainElement(HaveField("Name", "AWS_CA_BUNDLE")))
}
})
}
}

@bryan-cox bryan-cox Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The ~25-line CA bundle verification block (check volumes, init containers, mounts, env vars) is copy-pasted across 7 test files in this PR. Consider extracting a shared helper like testutil.AssertAWSCABundleWiring(g, deployment, expectPresent). This is separate from the struct-hoisting feedback — the assertion logic itself is the duplication.

10 changes: 9 additions & 1 deletion control-plane-operator/controllers/hostedcontrolplane/v2/capi_provider/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package capiprovider

import (
hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
component "github.com/openshift/hypershift/support/controlplane-component"
"github.com/openshift/hypershift/support/k8sutil"
"github.com/openshift/hypershift/support/podspec"
"github.com/openshift/hypershift/support/proxy"

appsv1 "k8s.io/api/apps/v1"
Expand All @@ -11,6 +13,8 @@ import (

// TODO: create a separate component for each platform?
func (capi *CAPIProviderOptions) adaptDeployment(cpContext component.WorkloadContext, deployment *appsv1.Deployment) error {
hcp := cpContext.HCP

deployment.Spec = *capi.deploymentSpec
deployment.Spec.Selector = &metav1.LabelSelector{
MatchLabels: labels(),
Expand All @@ -20,10 +24,14 @@ func (capi *CAPIProviderOptions) adaptDeployment(cpContext component.WorkloadCon

proxy.SetEnvVars(&deployment.Spec.Template.Spec.Containers[0].Env)

if hcp.Spec.Platform.Type == hyperv1.AWSPlatform && hcp.Spec.AdditionalTrustBundle != nil {
podspec.DeploymentAddAWSCABundleVolume(hcp.Spec.AdditionalTrustBundle, deployment, cpContext.ReleaseImageProvider.GetImage(podspec.CPOImageName))
}

if deployment.Annotations == nil {
deployment.Annotations = make(map[string]string)
}
deployment.Annotations[k8sutil.HostedClusterAnnotation] = cpContext.HCP.Annotations[k8sutil.HostedClusterAnnotation]
deployment.Annotations[k8sutil.HostedClusterAnnotation] = hcp.Annotations[k8sutil.HostedClusterAnnotation]

return nil
}
Expand Down
118 changes: 118 additions & 0 deletions control-plane-operator/controllers/hostedcontrolplane/v2/capi_provider/deployment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
assets "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/v2/assets"
component "github.com/openshift/hypershift/support/controlplane-component"
"github.com/openshift/hypershift/support/k8sutil"
"github.com/openshift/hypershift/support/testutil"

appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -231,3 +232,120 @@ func TestAdaptDeployment_WithNilAnnotations(t *testing.T) {
g.Expect(deployment.Annotations).ToNot(BeNil())
g.Expect(deployment.Annotations[k8sutil.HostedClusterAnnotation]).To(Equal("test-namespace/test-cluster"))
}

func TestAdaptDeploymentAWSCABundle(t *testing.T) {
testCases := []struct {
name string
platformType hyperv1.PlatformType
additionalTrust *corev1.LocalObjectReference
expectCABundle bool
}{
{
name: "When AWS platform with additional trust bundle it should add combined CA bundle",
platformType: hyperv1.AWSPlatform,
additionalTrust: &corev1.LocalObjectReference{Name: "user-ca-bundle"},
expectCABundle: true,
},
{
name: "When AWS platform without additional trust bundle it should not add CA bundle",
platformType: hyperv1.AWSPlatform,
additionalTrust: nil,
expectCABundle: false,
},
{
name: "When non-AWS platform with additional trust bundle it should not add CA bundle",
platformType: hyperv1.KubevirtPlatform,
additionalTrust: &corev1.LocalObjectReference{Name: "user-ca-bundle"},
expectCABundle: false,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
g := NewWithT(t)

hcp := &hyperv1.HostedControlPlane{

@bryan-cox bryan-cox Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This could be outside the look instead of recreating it each iteration.

ObjectMeta: metav1.ObjectMeta{
Name: "test-cluster",
Namespace: "clusters-test-cluster",
Annotations: map[string]string{
"hypershift.openshift.io/cluster": "clusters/test-cluster",
},
},
Spec: hyperv1.HostedControlPlaneSpec{
Platform: hyperv1.PlatformSpec{
Type: tc.platformType,
},
AdditionalTrustBundle: tc.additionalTrust,
},
}

deploymentSpec := &appsv1.DeploymentSpec{
Template: corev1.PodTemplateSpec{
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{
Name: "manager",
Image: "test-image",
},
},
},
},
}

capi := &CAPIProviderOptions{
deploymentSpec: deploymentSpec,
}

cpContext := component.WorkloadContext{
Context: t.Context(),
HCP: hcp,
ReleaseImageProvider: testutil.FakeImageProvider(),
}

deployment := &appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
Name: ComponentName,
Namespace: hcp.Namespace,
},
}

err := capi.adaptDeployment(cpContext, deployment)
g.Expect(err).ToNot(HaveOccurred())

volumes := deployment.Spec.Template.Spec.Volumes
initContainers := deployment.Spec.Template.Spec.InitContainers
container := deployment.Spec.Template.Spec.Containers[0]

if tc.expectCABundle {
g.Expect(volumes).To(ContainElement(SatisfyAll(
HaveField("Name", "user-ca-bundle"),
HaveField("VolumeSource.ConfigMap.Name", "user-ca-bundle"),
)))
g.Expect(volumes).To(ContainElement(SatisfyAll(
HaveField("Name", "aws-ca-bundle"),
HaveField("VolumeSource.EmptyDir", Not(BeNil())),
)))
g.Expect(initContainers).To(ContainElement(SatisfyAll(
HaveField("Name", "setup-aws-ca-bundle"),
HaveField("Image", "controlplane-operator"),
)))
g.Expect(container.VolumeMounts).To(ContainElement(SatisfyAll(
HaveField("Name", "aws-ca-bundle"),
HaveField("MountPath", "/etc/pki/ca-trust/extracted/hypershift"),
HaveField("ReadOnly", true),
)))
g.Expect(container.Env).To(ContainElement(SatisfyAll(
HaveField("Name", "AWS_CA_BUNDLE"),
HaveField("Value", "/etc/pki/ca-trust/extracted/hypershift/combined-ca-bundle.pem"),
)))
} else {
g.Expect(volumes).ToNot(ContainElement(HaveField("Name", "user-ca-bundle")))
g.Expect(volumes).ToNot(ContainElement(HaveField("Name", "aws-ca-bundle")))
g.Expect(initContainers).ToNot(ContainElement(HaveField("Name", "setup-aws-ca-bundle")))
g.Expect(container.VolumeMounts).ToNot(ContainElement(HaveField("Name", "aws-ca-bundle")))
g.Expect(container.Env).ToNot(ContainElement(HaveField("Name", "AWS_CA_BUNDLE")))
}
})
}
}
1 change: 1 addition & 0 deletions ...lane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/aws/component.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ func (c *awsOptions) NeedsManagementKASAccess() bool {

func NewComponent() component.ControlPlaneComponent {
return component.NewDeploymentComponent(ComponentName, &awsOptions{}).
WithAdaptFunction(adaptDeployment).
WithPredicate(predicate).
WithManifestAdapter(
"config.yaml",
Expand Down
16 changes: 16 additions & 0 deletions ...ane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/aws/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package aws

import (
component "github.com/openshift/hypershift/support/controlplane-component"
"github.com/openshift/hypershift/support/podspec"

appsv1 "k8s.io/api/apps/v1"
)

func adaptDeployment(cpContext component.WorkloadContext, deployment *appsv1.Deployment) error {
hcp := cpContext.HCP
if hcp.Spec.AdditionalTrustBundle != nil {
podspec.DeploymentAddAWSCABundleVolume(hcp.Spec.AdditionalTrustBundle, deployment, cpContext.ReleaseImageProvider.GetImage(podspec.CPOImageName))
}
return nil
}
Loading