-
Notifications
You must be signed in to change notification settings - Fork 93
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
Add option to disable tls certificate #1421
Conversation
Existing functionality is preserved (i.e. adding a certificate of type
The logs for this pod also don't seem to indicate any problem in particular either. Trying to change or update the http redirects here doesn't seem to resolve the issue either unfortunately. I've attempted a few other workarounds to no avail. |
@iameskild bummer this looks like the blocking issue that we need to solve. It may be that keycloak requires https. |
@costrouc this might be a keycloak issue but I'm also starting to wonder if this is a traefik issue (or possibly both). @viniciusdc and I worked on this together (and async), here are some of our findings thus far:
Remove:
Endpoints become unreachable, although the servers/pods are running.
Remove:
Failure.
Remove:
and Update:
Failure but a different, (possibly worse) error message:
Update:
(also tried with This seems to work but I'm not confident TLS is truly disabled. This is because if I replace The
This also resulted in a |
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 think that your PR #1421 is good. I don't have any way to test this but I believe it will work for the client (I am confident that this PR will not break anything existing). Since in their case the load-balancer automatically enables a certificate (for our tests we don't have a good way to do this). I'm happy with approving your PR and merging it. I think if we go down the road of making the keycloak connection without https. We should likely just include in the docs what this setting is for and that is assumes that the user is responsible for providing tls via the loadbalancer e.g. https://docs.aws.amazon.com/elasticloadbalancing/latest/network/create-tls-listener.html.
@@ -14,6 +14,7 @@ class CertificateEnum(str, enum.Enum): | |||
letsencrypt = "lets-encrypt" | |||
selfsigned = "self-signed" | |||
existing = "existing" | |||
disabled = "disabled" |
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 like how you've integrated this into the existing configuration options.
qhub/template/stages/04-kubernetes-ingress/modules/kubernetes/ingress/main.tf
Show resolved
Hide resolved
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.
LGTM, all the available options are working as expected, and we don't have any issues with the prior behavior for self-assigned nor let-encrypt.
When using disabled
we get an error with openID connection of Keycloak, but this is to be expected due the the way the routing currently works (I guess):
[terraform]: │ Error: error initializing keycloak provider
[terraform]: │
[terraform]: │ with provider["registry.terraform.io/mrparkers/keycloak"],
[terraform]: │ on providers.tf line 1, in provider "keycloak":
[terraform]: │ 1: provider "keycloak" {
[terraform]: │
[terraform]: │ failed to perform initial login to Keycloak: error sending POST request to
[terraform]: │ https://github-actions.qhub.dev/auth/realms/master/protocol/openid-connect/token:
[terraform]: │ 404 Not Found
Fixes | Closes | Resolves #1418
Changes introduced in this PR:
Types of changes
What types of changes does your PR introduce?
Put an
x
in the boxes that applyTesting
Requires testing
In case you checked yes, did you write tests?
Documentation
Does your contribution include breaking changes or deprecations?
If so have you updated the documentation?
Further comments (optional)
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered and more.