-
-
Notifications
You must be signed in to change notification settings - Fork 24
support custom science images #1163
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
for more information, see https://pre-commit.ci
This reverts commit 86d536d.
…/ssl-hep/ServiceX into 1162-support-custom-topcp-images
…62-support-custom-topcp-images
for more information, see https://pre-commit.ci
…62-support-custom-topcp-images
…/ssl-hep/ServiceX into 1162-support-custom-topcp-images
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1163 +/- ##
===========================================
+ Coverage 86.25% 86.56% +0.30%
===========================================
Files 95 95
Lines 3281 3304 +23
Branches 376 381 +5
===========================================
+ Hits 2830 2860 +30
+ Misses 377 373 -4
+ Partials 74 71 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…62-support-custom-topcp-images
for more information, see https://pre-commit.ci
…/ssl-hep/ServiceX into 1162-support-custom-topcp-images
for more information, see https://pre-commit.ci
|
@ponyisi this PR has been updated to support CERN's GitLab instance. It also now validates all science images. |
| if registry == "docker.io": | ||
| return self._dockerhub_get_image_by_tag(repo, image, tag) | ||
|
|
||
| if registry == "gitlab-registry.cern.ch": |
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 there really no way to make this generic? This is pretty 🤢
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 added crane to the image and simplified this considerably
for more information, see https://pre-commit.ci
…/ssl-hep/ServiceX into 1162-support-custom-topcp-images
for more information, see https://pre-commit.ci
helm/servicex/values.yaml
Outdated
| sqlalchemyEngineOptions: null | ||
| allowedDockerRegistries: | ||
| "docker.io": | ||
| allowedImagePrefixes: |
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 a prefix mandatory or can this just be "" ?
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.
Right now a prefix is mandatory however it can just be "". I don't think we should include anything with "" in the default values.yaml but a cluster operator could choose to do so. Do you think we should keep KyungEon's GitLab repository by default? @kyungeonchoi do you know how easy it would be to setup a shared namespace in CERN's GitLab like sslhep-customimages that isn't tied to a specific user name?
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.
Personally I'd tend to say we just whitelist the full gitlab-registry.cern.ch by default
|
|
||
| request_rec.image = codegen_transformer_image | ||
| try: | ||
| jquery = json.loads(args["selection"]) |
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 is not generically going to work for all codegens, many do not use JSON for their selection and even when they are they do not all have an "image" key.
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 has been moved to the TopCP Code Gen's standard image setting logic.
| registry = jquery.get("registry", "docker.io") | ||
|
|
||
| transformer_image = codegen_transformer_image | ||
| if "image" in jquery: |
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.
Again I don't really understand the point of doing this. If the codegen is doing the correct thing, codegen_transformer_image, which is under the control of the codegen which is interpreting the selection request, will have the image we want. We should not have any logic here that tries to peek into the selection, this must be kept opaque at this level. Basically, we should take codegen_transformer_image and prefix it with docker.io if necessary, that's 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.
This has been moved to the TopCP Code Gen's standard image setting logic. Now the code moves immediately to validate the image name.
code_generator_TopCPToolkit/servicex/TopCP_code_generator/request_translator.py
Show resolved
Hide resolved
…62-support-custom-topcp-images
…/ssl-hep/ServiceX into 1162-support-custom-topcp-images
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
code_generator_TopCPToolkit/servicex/TopCP_code_generator/request_translator.py
Show resolved
Hide resolved
helm/servicex/values.yaml
Outdated
| sqlalchemyEngineOptions: null | ||
| allowedDockerRegistries: | ||
| "docker.io": | ||
| allowedImagePrefixes: |
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.
Personally I'd tend to say we just whitelist the full gitlab-registry.cern.ch by default
…/ssl-hep/ServiceX into 1162-support-custom-topcp-images
for more information, see https://pre-commit.ci
…/ssl-hep/ServiceX into 1162-support-custom-topcp-images
ponyisi
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.
OK, let's go
Backend work for supporting custom TopCP images from whitelisted Docker registry repositories: ssl-hep/ServiceX_frontend#658
Allowed images can be defined using the
app.allowedDockerRegistriesvalue, where each key should be a Docker registry and each value should be a list of repositories/images to validate against. Each string in the list will be tested to see if it is a valid prefix of the custom image. If it is, it will be allowed. The default value will allow any tags for the Docker Hubsslhep/repository.Sample files that work with a custom TopCP image but not with the default/base image: