-
Notifications
You must be signed in to change notification settings - Fork 548
Use container registry for authenticating kubernetes image pull #3771
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
base: develop
Are you sure you want to change the base?
Changes from 12 commits
8bc6e85
75622fa
8f2d890
b8ed918
7f983a2
064bf5b
2a8bec5
aeb2bc9
b99472d
eeaac76
38d7eec
0654a4c
57cc133
baf0245
ed26186
2172a0e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,16 +94,13 @@ def requires_authentication(self) -> bool: | |
def credentials(self) -> Optional[Tuple[str, str]]: | ||
"""Username and password to authenticate with this container registry. | ||
|
||
Service connector credentials take precedence over direct authentication secrets. | ||
|
||
Returns: | ||
Tuple with username and password if this container registry | ||
requires authentication, `None` otherwise. | ||
""" | ||
secret = self.get_typed_authentication_secret( | ||
expected_schema_type=BasicAuthSecretSchema | ||
) | ||
if secret: | ||
return secret.username, secret.password | ||
|
||
# Check service connector credentials first as they take precedence | ||
connector = self.get_connector() | ||
if connector: | ||
from zenml.service_connectors.docker_service_connector import ( | ||
|
@@ -116,6 +113,13 @@ def credentials(self) -> Optional[Tuple[str, str]]: | |
connector.config.password.get_secret_value(), | ||
) | ||
|
||
# Fall back to direct authentication secrets | ||
secret = self.get_typed_authentication_secret( | ||
expected_schema_type=BasicAuthSecretSchema | ||
) | ||
if secret: | ||
return secret.username, secret.password | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stefannica according to your comment the connector should be checked first, but in the credentials implementation it was the other way around - let me know if I am missing something or if it made sense to flip here |
||
return None | ||
|
||
@property | ||
|
@@ -232,6 +236,47 @@ def get_image_repo_digest(self, image_name: str) -> Optional[str]: | |
|
||
return cast(str, metadata.id.split(":")[-1]) | ||
|
||
@property | ||
def registry_server_uri(self) -> str: | ||
"""Get the normalized registry server URI. | ||
|
||
This property returns a normalized registry server URI suitable for | ||
authentication and API access. Subclasses can override this property | ||
to provide registry-specific normalization logic. | ||
|
||
Returns: | ||
The normalized registry server URI. | ||
""" | ||
registry_uri = self.config.uri | ||
|
||
# Check if there's a service connector with a different registry setting | ||
connector = self.get_connector() | ||
if connector: | ||
from zenml.service_connectors.docker_service_connector import ( | ||
DockerServiceConnector, | ||
) | ||
|
||
if ( | ||
isinstance(connector, DockerServiceConnector) | ||
and connector.config.registry | ||
): | ||
# Use the service connector's registry setting | ||
registry_uri = connector.config.registry | ||
|
||
# Normalize registry URI for consistency | ||
if registry_uri.startswith("https://"): | ||
registry_uri = registry_uri[8:] | ||
elif registry_uri.startswith("http://"): | ||
registry_uri = registry_uri[7:] | ||
|
||
# For generic registries, extract just the domain part for better image matching | ||
if not registry_uri.startswith(("http://", "https://")): | ||
domain = registry_uri.split("/")[0] | ||
return f"https://{domain}" | ||
|
||
return registry_uri | ||
|
||
|
||
|
||
class BaseContainerRegistryFlavor(Flavor): | ||
"""Base flavor for container registries.""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,16 +13,34 @@ | |
# permissions and limitations under the License. | ||
"""Implementation of a DockerHub Container Registry class.""" | ||
|
||
from typing import Optional | ||
from typing import Optional, Type | ||
|
||
from zenml.constants import DOCKER_REGISTRY_RESOURCE_TYPE | ||
from zenml.container_registries.base_container_registry import ( | ||
BaseContainerRegistry, | ||
BaseContainerRegistryFlavor, | ||
) | ||
from zenml.enums import ContainerRegistryFlavor | ||
from zenml.models import ServiceConnectorRequirements | ||
|
||
|
||
class DockerHubContainerRegistry(BaseContainerRegistry): | ||
"""Container registry implementation for DockerHub.""" | ||
|
||
@property | ||
def registry_server_uri(self) -> str: | ||
"""Get the DockerHub registry server URI. | ||
|
||
DockerHub requires authentication against the specific | ||
'https://index.docker.io/v1/' endpoint regardless of how | ||
the registry URI is configured. | ||
|
||
Returns: | ||
The DockerHub registry server URI. | ||
""" | ||
return "https://index.docker.io/v1/" | ||
|
||
|
||
Comment on lines
+27
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit unsure on this one as container registries seem like the ugly duckling among our stack components with very little actual implementation of the base classes. |
||
class DockerHubContainerRegistryFlavor(BaseContainerRegistryFlavor): | ||
"""Class for DockerHub Container Registry.""" | ||
|
||
|
@@ -80,3 +98,12 @@ def logo_url(self) -> str: | |
The flavor logo. | ||
""" | ||
return "https://public-flavor-logos.s3.eu-central-1.amazonaws.com/container_registry/docker.png" | ||
|
||
@property | ||
def implementation_class(self) -> Type[DockerHubContainerRegistry]: | ||
"""Implementation class for DockerHub container registry. | ||
|
||
Returns: | ||
The DockerHub container registry implementation class. | ||
""" | ||
return DockerHubContainerRegistry |
Uh oh!
There was an error while loading. Please reload this page.