Skip to content

OSSM-10865: set trustBundleName in Istio global values #1288

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
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

OSSM-10865: set trustBundleName in Istio global values #1288

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
85b6fc9
set trustBundleName in Istio global values
skriss Oct 2, 2025
056f80f
don't use constant in unit test
skriss Oct 23, 2025
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
2 changes: 2 additions & 0 deletions pkg/operator/controller/gatewayclass/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client/fake"
"sigs.k8s.io/controller-runtime/pkg/reconcile"

"github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
testutil "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/test/util"
)

Expand Down Expand Up @@ -68,6 +69,7 @@ func Test_Reconcile(t *testing.T) {
},
IstioNamespace: ptr.To("openshift-ingress"),
PriorityClassName: ptr.To("system-cluster-critical"),
TrustBundleName: ptr.To(controller.OpenShiftGatewayCARootCertName),
Copy link
Member

@rikatz rikatz Oct 20, 2025

Choose a reason for hiding this comment

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

as a suggestion for the unit test, instead of relying on the variable value, rely on the real expected value (like line 70 where you explicit set "openshift-ingress") so in case something change on this controller.OpenShiftGatewayCARootCertName we know it will also break on the expected behavior.

As a side question: does setting this on an existing environment breaks something? Do we need to test any kind of upgrade? If this TrustBundleName is set on an existing environment, will it trigger a new reconciliation on the Istio resource that we need to be careful off?

Copy link
Author

@skriss skriss Oct 23, 2025

Choose a reason for hiding this comment

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

As a side question: does setting this on an existing environment breaks something? Do we need to test any kind of upgrade? If this TrustBundleName is set on an existing environment, will it trigger a new reconciliation on the Istio resource that we need to be careful off?

It's not expected to break anything; it will just configure istiod to write out the root cert in a new ConfigMap with the custom name (only within namespaces with Gateways). This will help avoid conflicts with any standalone OSSM installations that would be using the default name for the root cert ConfigMap.

cc @aslakknutsen - any hidden upgrade risks we're not thinking of?

Copy link
Contributor

@aslakknutsen aslakknutsen Nov 19, 2025

Choose a reason for hiding this comment

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

Sorry missed this.

I believe as @skriss states that we'll just get a new ConfigMap. I suppose it's an open question if we should try to clean up the old one or not as it'll be left in place. We don't really support Mesh in the same namespace as a Gateway at the moment so I believe it's relatively safe to delete them. Even if someone have configured mesh and gateway for the same namespace it wouldn't technically work as it would be flipping between the certs of the two control planes.

Copy link
Contributor

@Miciah Miciah Nov 19, 2025

Choose a reason for hiding this comment

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

@aslakknutsen, if we merge this PR now, do we make the problem of cleaning up the old configmaps more difficult?

I think we have discussed the problem of cleaning up old configmaps, and we might have decided that it is too risky, and that we should tell the user to do the cleanup manually, if desired. However, I am not sure where that decision was recorded, if my memory is even correct. Maybe @dgn remembers?

Copy link
Contributor

@aslakknutsen aslakknutsen Nov 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@Miciah Noting comes to mind. But I think we managed to set the PILOT_ENABLE_GATEWAY_API_CA_CERT_ONLY flag(41d7add#diff-a2d0fb9a1cce5ecf91b0fd80716edfb3b48bad924cc6f365dbb93d72d82f8956R100-R105) which would imply that the CM in question actually has a special label just for this case ;)

https://github.com/openshift-service-mesh/istio/blob/release-1.27/pilot/pkg/config/kube/gateway/gateway_ca_controller.go#L51C2-L51C16

The "openshift.io/mesh": "true" only exist on our created CMs.

Copy link

@dgn dgn Nov 21, 2025

Choose a reason for hiding this comment

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

Yes that is what I remember as well-- let's not attempt an automatic cleanup.

},
Pilot: &sailv1.PilotConfig{
Cni: &sailv1.CNIUsageConfig{
Expand Down
1 change: 1 addition & 0 deletions pkg/operator/controller/gatewayclass/istio.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ func desiredIstio(name types.NamespacedName, ownerRef metav1.OwnerReference, ist
},
IstioNamespace: ptr.To(controller.DefaultOperandNamespace),
PriorityClassName: ptr.To(systemClusterCriticalPriorityClassName),
TrustBundleName: ptr.To(controller.OpenShiftGatewayCARootCertName),
},
Pilot: &sailv1.PilotConfig{
Cni: &sailv1.CNIUsageConfig{
Expand Down