-
Notifications
You must be signed in to change notification settings - Fork 388
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 AWS ECR support (round two) #1055
Open
ivan-gomes
wants to merge
55
commits into
jupyterhub:main
Choose a base branch
from
ivan-gomes:aws-ecr-support
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+246
−1
Open
Changes from 28 commits
Commits
Show all changes
55 commits
Select commit
Hold shift + click to select a range
7c74f8a
add registry_class config value
chicocvenancio 81f92ca
add boto3 requirement
chicocvenancio 7d6f775
add AWSElasticContainerRegistry class
chicocvenancio b0b438c
rbac.yaml: allow manipulation of secrets
chicocvenancio 6cb5b6a
doc-requirements: add boto3
chicocvenancio 884af25
Merge branch 'master' into aws_ecr
chicocvenancio e25e597
Fix syntax
chicocvenancio 56dd9cf
Merge commit 'e25e59763ef2e33fb829e2feb36329721b28e015' into aws-ecr-…
ivan-gomes bd961a8
Revert RBAC modification and remove handling in AWSElasticContainerRe…
ivan-gomes af56e93
Add password and url fetching for ECR
ivan-gomes 296772b
Add AWS ECR documentation
ivan-gomes ade3a8b
Fix wording in AWS ECR documentation
ivan-gomes c1c37dd
Add ECR host to `image_prefix` in documentation
ivan-gomes b9ecea4
Re-add binder-push-secret patching. Add check for correct auth token.
ivan-gomes f9fd86c
Remove duplicate GetAuthorizationToken in AWS ECR doc
ivan-gomes a5edcd3
Lazily initialize kube_client in AWSElasticContainerRegistry to preve…
ivan-gomes a9aa576
Update description of `registry_class`
ivan-gomes 468bffc
Merge remote-tracking branch 'upstream/master' into aws-ecr-support
ivan-gomes 1047f9e
Rename `registry_class` to `docker_registry_class` for clarity
ivan-gomes 88e4914
Move boto3 and kubernetes calls to own threadpool
ivan-gomes 8309e97
Merge remote-tracking branch 'upstream/master' into aws-ecr-support
ivan-gomes c6939fe
Merge remote-tracking branch 'upstream/master' into aws-ecr-support
ivan-gomes 019a9fe
Restore newline lost during merge
ivan-gomes 00048e3
Merge remote-tracking branch 'upstream/master' into aws-ecr-support
ivan-gomes a74d814
general word-smithing for clarity
TomasBeuzen dddecf8
remove stray comma that AWS doesn't like in the policy
TomasBeuzen a729986
one more small clarification
TomasBeuzen 95a2f96
Merge pull request #2 from TomasBeuzen/aws-ecr-support-TB
ivan-gomes 9a4ad32
add boto3 requirement
chicocvenancio dfcb921
add AWSElasticContainerRegistry class
chicocvenancio a2a670a
rbac.yaml: allow manipulation of secrets
chicocvenancio 25faed2
Revert RBAC modification and remove handling in AWSElasticContainerRe…
ivan-gomes 60c8ed4
Add password and url fetching for ECR
ivan-gomes 6e6a61c
Add AWS ECR documentation
ivan-gomes b9877a7
Fix wording in AWS ECR documentation
ivan-gomes a5f21e8
Add ECR host to `image_prefix` in documentation
ivan-gomes 54c1b64
Re-add binder-push-secret patching. Add check for correct auth token.
ivan-gomes 6266cdd
Remove duplicate GetAuthorizationToken in AWS ECR doc
ivan-gomes dc676c4
Lazily initialize kube_client in AWSElasticContainerRegistry to preve…
ivan-gomes 5a4894a
Rename `registry_class` to `docker_registry_class` for clarity
ivan-gomes eeb7661
Move boto3 and kubernetes calls to own threadpool
ivan-gomes ad97b5a
general word-smithing for clarity
TomasBeuzen e7624d9
remove stray comma that AWS doesn't like in the policy
TomasBeuzen 378f539
one more small clarification
TomasBeuzen 762443e
Fix rebase mishap
thomas-bc 40ddeeb
update registry_class documentation for ECR
thomas-bc 42a28bf
moving optional boto3 import inside class
thomas-bc 7cf26bf
_patch_docker_config_secret docs and robustness
thomas-bc d9a983f
make RBAC increase conditional
thomas-bc 482cdb9
Merge branch 'jupyterhub:master' into aws-ecr-support
thomas-bc 591be68
rename RBAC increase to rbac.patchSecrets
thomas-bc c2b3f6e
Merge branch 'aws-ecr-support' of https://github.com/thomas-bc/binder…
thomas-bc 56f9e81
Merge branch 'aws-ecr-support' of https://github.com/ivan-gomes/binde…
thomas-bc 6fbc789
Merge pull request #4 from thomas-bc/aws-ecr-support
ivan-gomes 8a25e45
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,4 +11,4 @@ jupyterhub | |
jsonschema | ||
pycurl | ||
tornado>=5.1 | ||
|
||
boto3 |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 boto3 async?
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.
Not officially. There are third-party wrappers like aioboto3 - https://github.com/terrycain/aioboto3 - but we may not want to go there.
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.
BinderHub is written using tornado so we can't make calls over the network with a library which blocks. If we do then all of the BinderHub process will block while that network request is happening. We either need to use a threadpool to execute the boto3 calls or use a async library that you can
await
.Depending on the complexity of the requests you are making another option would be to implement the HTTP call yourself using the
AsyncHTTPClient
class.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.
Ah I see. There is really only one request that needs to be made currently and it's relatively simple to do ourselves. Unless you see a value in integrating something like
aioboto3
we can implement the request ourselves usingAsyncHTTPClient
. Drawing inspiration from elsewhere, it seems this is the route that FargateSpawner for JupyterHub went as well when making AWS calls - ref: https://github.com/uktrade/fargatespawner/blob/c614a54ffd80d0fb8886d1ef9e8de2c938de7759/fargatespawner/fargatespawner.py#L322-L346.If you approve of this path, I will start work on implementing and testing it.
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.
Sounds good to me.
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.
After biting into AWS's request signing process - ref: https://docs.aws.amazon.com/general/latest/gr/signature-version-4.html - I realized heading down the request reimplementation may not be the best approach. Additionally, the kubernetes call is also not async, so I switched to using a threadpool as you suggested. It has now been implemented and tested.