Closed MoisesGSalas closed 1 year ago
Thanks for the pull request, @MoisesGSalas! Please note that it may take us up to several weeks or months to complete a review and merge your PR.
Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.
Please let us know once your PR is ready for our review and all tests are green.
Awesome work here @MoisesGSalas ! I read through the code and left a few comments/questions but I think it looks great. I'll test it next week and then give it my 👍🏻
Thanks a lot for the comments @bradenmacdonald. I tried to address all of them, tell me your thoughts whenever you have time.
Thanks @MoisesGSalas! Sorry for the delay - I'll try to test and approve this soon :)
I'm trying to test this now on a fresh kubernetes cluster but getting an error:
$ helm install --namespace tutor-multi --create-namespace -f values.yaml tutor-multi ./tutor-multi-chart
Error: INSTALLATION FAILED: unable to build kubernetes objects from release manifest: unable to recognize "": no matches for kind "ClusterIssuer" in version "cert-manager.io/v1"
It took me a while but here's what I figured out:
The Helm docs say that charts should put their CRDs into a special crds
subfolder (e.g. as seen in this example chart) and if they were there it would install them first. But cert-manager
doesn't want to do that because Helm doesn't support updating/deleting such CRDs. So cert-manager
puts them in as regular k8s yaml templates. But because of the order of operations that Helm uses, this means that when we use cert-manager as a subchart, Helm will try to install the issuer.yaml
file that you made in our chart before the CRDs have been installed. And then of course Kubernetes throws an error.
Some possible solutions to this could be:
kubectl apply -f https://github.com/cert-manager/cert-manager/releases/download/v1.10.1/cert-manager.crds.yaml --namespace=tutor-multi
and then set installCRDs: false
for the subchart.
Sorry for the late response @bradenmacdonald, I'm in favor managing the CRDs for cert-manager outside the chart especially if we consider that doing that is an actual use case that is mentioned in the cert-manager docs.
I've updated the Readme to address this caveat.
Hey @bradenmacdonald, do you have any updates on when you'll be able to continue reviewing here?
@MoisesGSalas @itsjeyd So sorry for the delay. I have finally been able to test this again this week and the CRDs are no longer an issue but something doesn't seem to be working.
My new test instance is returning a 404: http://braden-harmony-test01-01.opencraft.com/ And the HTTPS cert is not auto-provisioned: https://braden-harmony-test01-01.opencraft.com/
It will take me a bit more time to figure out what the issue is. Hopefully tomorrow.
Don't worry @bradenmacdonald, I appreciate your help.
I think the error might be related with the ingressClass name. I threw a default value for TUTOR_MULTI_INGRESS_CLASS_NAME
but now I realize I didn't add that same name to the default values for the chart. Maybe your ingress uses the ingress class tutor-multi-nginx
but the chart deployed the default nginx
one.
Ah, I think you're right @MoisesGSalas. Actually there are two issues: first, you'll need to update this PR to use a consistent ingress class name in both places. I can see the ingress controller is using nginx
as the ingress class name but Tutor is configured to use tutor-multi-nginx
as you said. Second, I think I didn't update my Tutor plugin properly when testing your PR because the actual ingress object that got deployed is still using tutor-multi-traefik
.
For now I manually changed the Ingress object's ingressClassName to nginx
and now it's working for HTTP at http://braden-harmony-test01-01.opencraft.com/
However the HTTPS is still showing the default cert, and the HTTP is not redirecting to the HTTPS. I'll have to investigate on Monday as I wasn't able to figure out the issue with a quick look just now.
I've adjusted the default values to match, I think it should be good now. I created a new cluster and was able to deploy two instances.
I will be on holidays until later next week, so I apologize in advance for not being able to answer the PR until then.
Nice, thanks - it's working now :)
Hmm, this is ready to merge but it seems I don't actually have merge rights here. @felipemontoya do you?
Nop, I also don't. Im waiting on https://github.com/openedx/tcril-engineering/issues/665
Ah, right. Thanks for the link.
@bradenmacdonald you should have permissions now.
@MoisesGSalas 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.
Description
This is a first attempt to add support for
ingress-nginx
andcert-manager
in response to #1.For the most part it works the same as before, cert-manager and ingress-nginx were added as dependencies and are installed by default, with the possibility of simply using traefik.
The the helm chart now provisions a
clusterissuer
to provide the certificates used globally. and the plugin includes a list of additional hosts to be routed to the caddy instance.There doesn't seem to be a nice way to discover the public hosts used by a tutor installation, for example, in addition to LMS, CMS, MFE and PREVIEW one may need to route traffic from ECOMMERCE, DISCOVERY, MINIO or any arbitrary plugin to caddy. I tried to think of a nice way to do this but wasn't able to come up with anything. I wonder what could be a nice way for plugins to announce their hosts.