-
Notifications
You must be signed in to change notification settings - Fork 97
chore: Allow external TLS configuration for ingress/routes #2025
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
Conversation
|
Skipping CI for Draft Pull Request. |
| // in order avoid resyncing by devworkspace controller | ||
| clusterIngress := &networkingv1.Ingress{} | ||
| if err := cl.Get(ctx, client.ObjectKey{Name: ingress.Name, Namespace: ingress.Namespace}, clusterIngress); err == nil { | ||
| ingress.Spec.TLS = clusterIngress.Spec.TLS |
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.
Is it possible to append the endpoint name to the secret-name ? It would allow the use of different secret name and allow Cert Manager to inject the certificate in the namespace
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, I am not very familiar how cert-manager works in details.
Does it create and set the secret? If there there are several ingress, will the same secret used?
What if secret is set TLS config?
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.
I see, cert-manager doen't set secret, it uses the on from the TLS config.
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, and the best use case is to have one Secret by FQDN/Ingress
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.
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 it is
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.
Could you provide steps to deploy and configure cert-manager on OpenShift cluster? I would like to test.
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.
This repo containe the needed step to have Cert manager handle Openshift Routes https://github.com/cert-manager/openshift-routes
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.
@batleforc
Minor changes, tested.
Please have a look.
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.
Look's good to me !
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
| }, | ||
| }) | ||
|
|
||
| assert.Equal(t, 3, len(objs.Ingresses)) |
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.
nit, Maybe we can replace assert with require for critical assertions here so that we fail fast.
| assert.Equal(t, 3, len(objs.Ingresses)) | |
| require.Equal(t, 3, len(objs.Ingresses)) |
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
|
/retest |
vinokurig
left a comment
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.
Tested according to the PR description.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rohanKanojia, tolusha, vinokurig The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
What does this PR do?
This PR enables external tools (e.g., cert-manager) to inject TLS configuration into workspace ingresses/routes.
When externalTLSConfig=true:
Screenshot/screencast of this PR
N/A
What issues does this PR fix or reference?
eclipse-che/che#22935
How to test this PR?
OpenShift
Create the CatalogSource and deploy Eclipse Che
Deploy cert-manager
on Minikube
Log into Eclipse Che
Create Issuer/Certificate resources
Start and stop a workspace
Check that:
For minikube:
For OpenShift:
Start the same workspace workspace again
Check that:
Start a new workspace.
Check that:
PR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or referenceandHow to test this PRcompletedReviewers
Reviewers, please comment how you tested the PR when approving it.