Skip to content
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.

Sign up for GitHub

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

Support dynamic push credentials obtained from registry #1724

Merged
merged 3 commits into from
Jul 8, 2023
Merged

Support dynamic push credentials obtained from registry #1724

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a8d5049
Support dynamic push credentials obtained from registry
manics Feb 19, 2023
f381eb3
Replace `push_secret_content` with `registry_credentials`
manics Jun 27, 2023
cb6d328
Make registry_credentials a Dict
manics Jun 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 29 additions & 10 deletions binderhub/build.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,18 @@ class BuildExecutor(LoggingConfigurable):
config=True,
)

push_secret_content = Unicode(
"",
help=(
"Content of an implementation dependent secret for pushing image to a registry. "
"For example, if push tokens are temporary this can be used to pass the token "
"as an environment variable CONTAINER_ENGINE_REGISTRY_CREDENTIALS to "
"repo2docker."
"If provided this will be used instead of push_secret."
),
config=True,
)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the push_secret config has been challenging for me to understand well. Here are some notes on what I think can help.

  • Its not obvious that its a push_secret references a name (k8s Secret resource for example) as compared to content to put inside one.
  • Its not obvious that its going to be created or needs to be created manually
  • Its not obvious what kind of content to put in the referenced secret created manually

Thinking about what I'd appreciate from docs to understand this good enough, I'd say something like the following, where its not obvious whats in scope and not for this PR:

  1. I think "implementation dependant" could be written more verbosively to clarify that: its dependant on the BuildExecutor implementation, and no base implemention exist.
  2. push_secret and push_secret_content's help strings are both are updated to reflect that there are separate ways of providing container registry credentials to the builder, where push_secret references something existing by name, and push_secret_content is content.
  3. push_secret and push_secret_content's help strings are both are updated on how to know what kind of resource to reference and what content to provide. This may be implementation dependent and not something BuildExecutor can know, but I think we can declare here where to find that kind of details.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update the docs. AFAIK it should be fine to duplicate the traitlet in the subclass to have more specialist documentation?

If it's easier (I'm not sure whether it is), we could switch push_secret to be of type (Unicode, Dict). where Dict would for example take the form {content: <secret>} or {secret_name: <k8s-secret-name>}. Unicode would support the current default behaviour.

memory_limit = ByteSpecification(
0,
help="Memory limit for the build process in bytes (optional suffixes K M G T).",
Expand Down Expand Up @@ -394,7 +406,23 @@ def submit(self):
)
]

if self.push_secret:
env = [
client.V1EnvVar(name=key, value=value)
for key, value in self.extra_envs.items()
]
if self.git_credentials:
env.append(
client.V1EnvVar(name="GIT_CREDENTIAL_ENV", value=self.git_credentials)
)

if self.push_secret_content:
env.append(
client.V1EnvVar(
name="CONTAINER_ENGINE_REGISTRY_CREDENTIALS",
consideRatio marked this conversation as resolved.
Show resolved Hide resolved
value=self.push_secret_content,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is less secure than the push_secret option I think, because kubectl get pod can reveal content while when using a dedicated k8s Secret to house the content only kubectl get secret can reveal the content.

KubeSpawner has ended up dynamically creating also a k8s Secret for user pods to house misc information, for example info relevant for internal_ssl. It certainly adds complexity to do that, and this is far less complicated.

I think it may be acceptable to do this if we describe it clearly in the help string of the config.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it's not as good as using a secret, however we had a long discussion in #1577 over the pros and cons of dynamically creating secrets attached to pods, as the same problem arises with dynamic Git credentials.

)
)
elif self.push_secret:
volume_mounts.append(
client.V1VolumeMount(mount_path="/root/.docker", name="docker-config")
)
Expand All @@ -405,15 +433,6 @@ def submit(self):
)
)

env = [
client.V1EnvVar(name=key, value=value)
for key, value in self.extra_envs.items()
]
if self.git_credentials:
env.append(
client.V1EnvVar(name="GIT_CREDENTIAL_ENV", value=self.git_credentials)
)

self.pod = client.V1Pod(
metadata=client.V1ObjectMeta(
name=self.name,
Expand Down
11 changes: 9 additions & 2 deletions binderhub/builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,11 +381,12 @@ async def get(self, provider_prefix, _unescaped_spec):
.lower()
)

image_without_tag, image_tag = _get_image_basename_and_tag(image_name)
if self.settings["use_registry"]:
for _ in range(3):
try:
image_manifest = await self.registry.get_image_manifest(
*_get_image_basename_and_tag(image_name)
image_without_tag, image_tag
)
image_found = bool(image_manifest)
break
Expand Down Expand Up @@ -457,7 +458,13 @@ async def get(self, provider_prefix, _unescaped_spec):
image_name=image_name,
git_credentials=provider.git_credentials,
)
if not self.settings["use_registry"]:
if self.settings["use_registry"]:
push_token = await self.registry.get_credentials(
image_without_tag, image_tag
)
if push_token:
build.push_secret_content = json.dumps(push_token)
else:
build.push_secret = ""

self.build = build
Expand Down
8 changes: 8 additions & 0 deletions binderhub/registry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,14 @@ async def get_image_manifest(self, image, tag):
raise
return json.loads(resp.body.decode("utf-8"))

async def get_credentials(self, image, tag):
"""
If a dynamic token is required for pushing an image to the registry
return a dictionary of login credentials, otherwise return None
(caller should get credentials from some other source)
"""
return None


class FakeRegistry(DockerRegistry):
"""
Expand Down