binderhub
binderhub copied to clipboard
jupyterhub
Add AWS ECR support (round two)
Closes https://github.com/jupyterhub/binderhub/issues/705 and follow-on/alternative to https://github.com/jupyterhub/binderhub/pull/920.
Includes updated user documentation on how to configure AWS IAM and BinderHub to use AWS ECR as the Docker registry. Modifications are additive, in that existing configurations should not be affected, and have been tested with ECR.
Thanks for this PR.
Has someone been running this code on AWS?
Having different registry classes is a good idea. Could also be used for GitLab like registries (xref #986 #965)
Yes we are running this code on AWS with ECR and no further modifications.
Hi guys,
Any news about this PR? This is the missing point to me to adopt binderhub as my private notebooks provisioning pipeline.
Can I help in anything?
Best regards, Clovis Chedid
Hi @clovischedid! Thanks for your interest and offer. I went ahead and responded to all the conversations and applied the naming change requested. I have also re-requested a review from @betatim. Hopefully it's up to snuff now. Otherwise I will try to be quicker on the iteration this time around. :)
Status update: addressed all unresolved review comments. Awaiting re-review. :)
Nice work!
@clovischedid do you have a moment to give this a spin on your deployment? Our tests in this area aren't mega so the knowledge of "someone actually used this feature and it worked" would be great to have.
Hello guys,
I'm sorry for my delay.
@betatim I will do my best, but currently, I do not have a production ready deployment. At this time, I'm focusing myself on make binderhub running with AWS managed Kubernets service EKS. I'm facing some network problems as newer version o EKS do not make bridge network as default. By the way, I will try to make AWS ECR as my container registry, but, could you help me? I can't realize how to make helm deploy binderhub from a github branch instead of published version on helm repo.
Regards, Clovis Chedid
Nice work!
@clovischedid do you have a moment to give this a spin on your deployment? Our tests in this area aren't mega so the knowledge of "someone actually used this feature and it worked" would be great to have.
Hello guys,
I'm sorry for my delay.
@betatim I will do my best, but currently, I do not have a production ready deployment. At this time, I'm focusing myself on make binderhub running with AWS managed Kubernets service EKS. I'm facing some network problems as newer version o EKS do not make bridge network as default. By the way, I will try to make AWS ECR as my container registry, but, could you help me? I can't realize how to make helm deploy binderhub from a github branch instead of published version on helm repo.
Regards, Clovis Chedid
Rebuild the BinderHub image from source and set the image to it with helm, @clovischedid.
I might be able to test this PR in a couple of weeks.
Thanks @ivan-gomes for the work!
@clovischedid @chicocvenancio just checking in if you had a chance to test yet, no pressure.
@betatim do you think it would be a good idea to merge so it would be easier for those interested like @clovischedid to try it out without having to build from source? For what it's worth, I have it deployed as documented with ECR and this addition doesn't affect default behavior. Hoping to avoid merge conflicts with master again.
@zeyaddeeb that would be fantastic! Please ping me if I can help with or clarify anything.
It would be great to have someone test drive these instructions. Though given how long they've already been "in flight" it might be easier to merge them and then update them later as we discover things. With it being merged we might get more eyeballs on it.
@betatim I totally agree, especially considering we just resolved another round of merge conflicts in documentation.
I'll be testing this out over the next few days - will report back.
UPDATE: @ivan-gomes / @betatim - ~I got this to work~ (edit: false alarm, still trying) but with a few ammendments/clarifications to the docs. Are you still planning to merge this? I can then make a new PR with my changes.
@TomasBeuzen you can submit a PR to https://github.com/ivan-gomes/binderhub/tree/aws-ecr-support and I can merge it in. If it's minor and easier for you, you can also describe it here and I would be happy to incorporate the feedback into this PR.
@ivan-gomes - so sorry I've been super busy preparing for the new school year starting (in Canada). This is on my list but probably not for another 2-3 weeks (feel free to proceed without my comments, otherwise I'll update you ASAP).
Though given how long they've already been "in flight" it might be easier to merge them and then update them later as we discover things. With it being merged we might get more eyeballs on it.
@betatim I think we should go for it.
Apologies for delays on this, I got this up and running. I just made a PR for a few potentially helpful clarifications to the docs but otherwise thanks for all your work on this!
That's great news @TomasBeuzen! I merged in your helpful changes. Thanks!
@betatim just wanted to put this back on your radar. Hope you don't mind. :)
Pinging this as it would be nice to have it merged 🙂 Happy to help in any way I can!
Hi folks, would love to see this functionality if it's still in the roadmap!
I would be happy to re-resolve conflicts once this PR has been tentatively / conditionally approved. :)
I think one issue to consider is how to test this. In the short term we can probably ask a few AWS users to test it for real and sign off on it. In the long term how do we ensure we don't inadvertently break it? It looks like the AWS registry works in a completely different way to a standard docker registry, so it's not going to be regularly tested. Do you have any ideas?
Further to my previous comment. I figured out how to test this properly running a dev build of Binderhub in a Docker container by building the Docker images and installing the helm-chart locally (if someone wants to see how I did this, look here https://github.com/teticio/binderhub at the commented out section of bootstrap.sh). I had been following the instructions here https://binderhub.readthedocs.io/en/latest/contribute.html.
I got it to work but I would still recommend to take out the hard-coded references in _patch_docker_secret in registry.py as follows:
self.kube_client.patch_namespaced_secret(
self.parent.push_secret, self.parent.build_namespace, {"data": {"config.json": secret_data}}
)
You can also ignore my previous comments on the config file, which work as per the instructions when installing.
