-
Notifications
You must be signed in to change notification settings - Fork 218
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.
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
base: master
Are you sure you want to change the base?
Conversation
Sets trustBundleName in order to customize the name of the configmap containing the CA cert so it doesn't clash with a standalone OSSM instance. Follow-up to openshift#1243. Signed-off-by: Steve Kriss <[email protected]>
|
@skriss: This pull request references OSSM-10865 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 "4.21.0" version, but no target version was set. In 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. |
|
Hi @skriss. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
| }, | ||
| IstioNamespace: ptr.To("openshift-ingress"), | ||
| PriorityClassName: ptr.To("system-cluster-critical"), | ||
| TrustBundleName: ptr.To(controller.OpenShiftGatewayCARootCertName), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 ;)
The "openshift.io/mesh": "true" only exist on our created CMs.
There was a problem hiding this comment.
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.
Signed-off-by: Steve Kriss <[email protected]>
|
/ok-to-test |
|
/retest |
|
/assign |
|
Thanks! /lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Miciah The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
The test failure on e2e-aws-pre-release-ossm is being verified at https://issues.redhat.com/browse/OCPBUGS-65939 |
|
/cc @rhamini3 @lihongan @ShudiLi @melvinjoseph86 for QE |
|
Hi @skriss I run pre-merge test with the PR and seems the configmap is still there after gateway is delete, is that expected ? |
Yeah, AFAIK this is expected, there's no code to delete the configmaps on Gateway deletion. |
|
@lihongan: This PR has been marked as verified by In 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. |
|
/retest |
|
/retest-required |
|
@skriss: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Sets trustBundleName in order to customize the
name of the configmap containing the CA cert so
it doesn't clash with a standalone OSSM instance.
Follow-up to #1243.