Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
6 changes: 6 additions & 0 deletions charts/platform-operator/crds/platform.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,12 @@ spec:
type: object
required: [persistence]
properties:
registry_type:
type: string
enum:
- "docker"
- "harbor"
default: "docker"
persistence:
type: object
required: [storageClassName]
Expand Down
4 changes: 4 additions & 0 deletions charts/platform/Chart.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ dependencies:
version: "1.9.2"
repository: https://charts.helm.sh/stable
condition: dockerRegistryEnabled
- name: harbor
version: "1.17.0"
repository: https://helm.goharbor.io
condition: harborEnabled
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need a condition for harbor?

- name: minio
version: "5.0.33"
repository: https://charts.helm.sh/stable
Expand Down
83 changes: 79 additions & 4 deletions platform_operator/helm_values.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from .models import (
BucketsProvider,
DockerRegistryStorageDriver,
DockerRegistryType,
HelmChartNames,
IngressServiceType,
LabelsConfig,
Expand Down Expand Up @@ -61,7 +62,10 @@ def create_platform_values(self, platform: PlatformConfig) -> dict[str, Any]:
result: dict[str, Any] = {
"traefikEnabled": platform.ingress_controller_install,
"acmeEnabled": platform.ingress_acme_enabled,
"dockerRegistryEnabled": platform.registry.docker_registry_install,
"dockerRegistryEnabled": platform.registry.docker_registry_install
and platform.registry.docker_registry_type == DockerRegistryType.DOCKER,
"harborEnabled": platform.registry.docker_registry_install
and platform.registry.docker_registry_type == DockerRegistryType.HARBOR,
"appsPostgresOperatorEnabled": (
platform.apps_operator_config.postgres_operator_enabled
),
Expand Down Expand Up @@ -198,9 +202,13 @@ def create_platform_values(self, platform: PlatformConfig) -> dict[str, Any]:
else:
result["dockerHubConfigSecret"] = {"create": False}
if platform.registry.docker_registry_install:
result[HelmChartNames.docker_registry] = self.create_docker_registry_values(
platform
)
if platform.registry.docker_registry_type == DockerRegistryType.DOCKER:
result[HelmChartNames.docker_registry] = (
self.create_docker_registry_values(platform)
)
elif platform.registry.docker_registry_type == DockerRegistryType.HARBOR:
result[HelmChartNames.harbor] = self.create_harbor_values(platform)

if platform.buckets.minio_install:
assert platform.buckets.minio_public_url
result["ingress"]["minioHost"] = platform.buckets.minio_public_url.host
Expand Down Expand Up @@ -411,6 +419,73 @@ def create_docker_registry_values(self, platform: PlatformConfig) -> dict[str, A
}
return result

def create_harbor_values(self, platform: PlatformConfig) -> dict[str, Any]:
docker_registry_credentials_secret_name = (
f"{platform.release_name}-docker-registry"
)
harbor_external_host = f"harbor.apps.{platform.ingress_dns_name}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why harbor.apps...? It's not an app

result: dict[str, Any] = {
"expose": {
"tls": {"enabled": False},
"ingress": {
"hosts": {"core": harbor_external_host},
"className": "traefik",
"annotations": None,
},
},
"persistence": {
"persistentVolumeClaim": {
"registry": {},
},
"imageChartStorage": {},
},
"externalURL": harbor_external_host,
}

if (
platform.registry.docker_registry_username
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to save images to docker registry?

and platform.registry.docker_registry_password
):
assert platform.registry.docker_registry_username == "admin"
result["existingSecretAdminPassword"] = (
docker_registry_credentials_secret_name
)
result["existingSecretAdminPasswordKey"] = "password"

if (
platform.registry.docker_registry_storage_driver
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need to support persistence to disk?

== DockerRegistryStorageDriver.FILE_SYSTEM
):
result["persistence"]["imageChartStorage"]["type"] = "filesystem"
result["persistence"]["persistentVolumeClaim"]["registry"] = {
"storageClass": (
platform.registry.docker_registry_file_system_storage_class_name
),
"size": platform.registry.docker_registry_file_system_storage_size,
}
elif (
platform.registry.docker_registry_storage_driver
== DockerRegistryStorageDriver.S3
):
assert platform.registry.docker_registry_s3_endpoint
result["persistence"]["imageChartStorage"]["type"] = "s3"
result["persistence"]["imageChartStorage"]["s3"] = {
"bucket": str(platform.registry.docker_registry_s3_bucket),
"region": platform.registry.docker_registry_s3_region,
"regionendpoint": self._get_url_authority(
platform.registry.docker_registry_s3_endpoint
),
"accesskey": platform.registry.docker_registry_s3_access_key,
"secretkey": platform.registry.docker_registry_s3_secret_key,
"secure": platform.registry.docker_registry_s3_endpoint.scheme
== "https",
"rootdirectory": "/harbor",
}
result["persistence"]["imageChartStorage"]["disableredirect"] = (
platform.registry.docker_registry_s3_disable_redirect
)
return result

def create_minio_values(self, platform: PlatformConfig) -> dict[str, Any]:
assert platform.buckets.minio_public_url
return {
Expand Down
31 changes: 24 additions & 7 deletions platform_operator/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ class HelmReleaseNames:

class HelmChartNames:
docker_registry: str = "docker-registry"
harbor: str = "harbor"
minio: str = "minio"
minio_gateway: str = "minio-gateway"
traefik: str = "traefik"
Expand Down Expand Up @@ -596,6 +597,11 @@ class DockerRegistryStorageDriver(str, Enum):
S3 = "s3"


class DockerRegistryType(str, Enum):
DOCKER = "docker"
HARBOR = "harbor"


@dataclass(frozen=True)
class RegistryConfig:
provider: RegistryProvider
Expand All @@ -610,6 +616,7 @@ class RegistryConfig:
azure_password: str = ""

docker_registry_install: bool = False
docker_registry_type: DockerRegistryType = DockerRegistryType.DOCKER
docker_registry_url: URL | None = None
docker_registry_username: str = ""
docker_registry_password: str = ""
Expand Down Expand Up @@ -1035,7 +1042,7 @@ def create(self, platform_body: kopf.Body, cluster: Cluster) -> PlatformConfig:
idle_jobs=cluster.orchestrator.idle_jobs,
buckets=buckets_config,
registry=self._create_registry(
spec.registry, buckets_config=buckets_config
spec.registry, buckets_config=buckets_config, cluster=cluster
),
monitoring=self._create_monitoring(spec.monitoring),
disks_storage_limit_per_user=cluster.disks.storage_limit_per_user,
Expand Down Expand Up @@ -1222,7 +1229,7 @@ def _create_buckets(self, spec: BlobStorageSpec, cluster: Cluster) -> BucketsCon
raise ValueError("Bucket provider is not supported")

def _create_registry(
self, spec: RegistrySpec, *, buckets_config: BucketsConfig
self, spec: RegistrySpec, *, buckets_config: BucketsConfig, cluster: Cluster
) -> RegistryConfig:
if not spec:
raise ValueError("Registry spec is empty")
Expand Down Expand Up @@ -1256,14 +1263,24 @@ def _create_registry(
docker_registry_password=spec.docker_password,
)
if "kubernetes" in spec:
return RegistryConfig(
provider=RegistryProvider.DOCKER,
docker_registry_install=True,
docker_registry_url=URL.build(
reg_type = DockerRegistryType(
spec.get("kubernetes", {}).get("registry_type", "docker")
)
if reg_type == DockerRegistryType.HARBOR:
docker_registry_url = URL.build(
scheme="https", host=f"harbor.apps.{cluster.dns.name}"
Copy link
Contributor

Choose a reason for hiding this comment

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

why not in-cluster service hostname?

)
else:
docker_registry_url = URL.build(
scheme="http",
host=f"{self._config.helm_release_names.platform}-docker-registry",
port=5000,
),
)
return RegistryConfig(
provider=RegistryProvider.DOCKER,
docker_registry_install=True,
docker_registry_type=reg_type,
docker_registry_url=docker_registry_url,
docker_registry_storage_driver=DockerRegistryStorageDriver.FILE_SYSTEM,
docker_registry_file_system_storage_class_name=(
spec.kubernetes_storage_class_name
Expand Down
146 changes: 146 additions & 0 deletions tests/unit/test_helm_values.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
BucketsProvider,
DockerConfig,
DockerRegistryStorageDriver,
DockerRegistryType,
ExternalSecretObjectsSpec,
ExternalSecretsSpec,
IngressServiceType,
Expand All @@ -39,6 +40,7 @@ def test_create_gcp_platform_values(
"traefikEnabled": True,
"acmeEnabled": True,
"dockerRegistryEnabled": False,
"harborEnabled": False,
"appsPostgresOperatorEnabled": True,
"appsSparkOperatorEnabled": True,
"appsKedaEnabled": True,
Expand Down Expand Up @@ -356,6 +358,42 @@ def test_create_on_prem_platform_values_without_docker_registry(
assert result["dockerRegistryEnabled"] is False
assert "docker-registry" not in result

def test_create_on_prem_platform_values_with_harbor_registry(
self, on_prem_platform_config: PlatformConfig, factory: HelmValuesFactory
) -> None:
result = factory.create_platform_values(
replace(
on_prem_platform_config,
registry=replace(
on_prem_platform_config.registry,
docker_registry_type=DockerRegistryType.HARBOR,
),
)
)

assert result["dockerRegistryEnabled"] is False
assert result["harborEnabled"] is True
assert "harbor" in result
assert "docker-registry" not in result

def test_create_on_prem_platform_values_with_docker_registry_explicit(
self, on_prem_platform_config: PlatformConfig, factory: HelmValuesFactory
) -> None:
result = factory.create_platform_values(
replace(
on_prem_platform_config,
registry=replace(
on_prem_platform_config.registry,
docker_registry_type=DockerRegistryType.DOCKER,
),
)
)

assert result["dockerRegistryEnabled"] is True
assert result["harborEnabled"] is False
assert "docker-registry" in result
assert "harbor" not in result

def test_create_on_prem_platform_values_without_minio(
self, on_prem_platform_config: PlatformConfig, factory: HelmValuesFactory
) -> None:
Expand Down Expand Up @@ -515,6 +553,114 @@ def test_create_docker_registry_values_with_s3_minio_storage(
}
assert result["secrets"]["haSharedSecret"]

def test_create_harbor_values_with_filesystem_storage(
self, on_prem_platform_config: PlatformConfig, factory: HelmValuesFactory
) -> None:
on_prem_platform_config = replace(
on_prem_platform_config,
registry=replace(
on_prem_platform_config.registry,
docker_registry_type=DockerRegistryType.HARBOR,
),
)
result = factory.create_harbor_values(on_prem_platform_config)

assert result == {
"expose": {
"tls": {"enabled": False},
"ingress": {
"hosts": {
"core": "harbor.apps."
f"{on_prem_platform_config.ingress_dns_name}"
},
"className": "traefik",
"annotations": None,
},
},
"persistence": {
"persistentVolumeClaim": {
"registry": {
"storageClass": "registry-standard",
"size": "100Gi",
},
},
"imageChartStorage": {"type": "filesystem"},
},
"externalURL": f"harbor.apps.{on_prem_platform_config.ingress_dns_name}",
}

def test_create_harbor_values_with_s3_storage(
self, on_prem_platform_config: PlatformConfig, factory: HelmValuesFactory
) -> None:
on_prem_platform_config = replace(
on_prem_platform_config,
registry=replace(
on_prem_platform_config.registry,
docker_registry_type=DockerRegistryType.HARBOR,
docker_registry_storage_driver=DockerRegistryStorageDriver.S3,
docker_registry_s3_endpoint=URL("https://s3.amazonaws.com"),
docker_registry_s3_region="us-east-1",
docker_registry_s3_bucket="harbor-registry",
docker_registry_s3_access_key="aws-access-key",
docker_registry_s3_secret_key="aws-secret-key",
docker_registry_s3_disable_redirect=False,
),
)
result = factory.create_harbor_values(on_prem_platform_config)

assert result == {
"expose": {
"tls": {"enabled": False},
"ingress": {
"hosts": {
"core": "harbor.apps."
f"{on_prem_platform_config.ingress_dns_name}"
},
"className": "traefik",
"annotations": None,
},
},
"persistence": {
"persistentVolumeClaim": {
"registry": {},
},
"imageChartStorage": {
"type": "s3",
"s3": {
"bucket": "harbor-registry",
"region": "us-east-1",
"regionendpoint": "s3.amazonaws.com",
"accesskey": "aws-access-key",
"secretkey": "aws-secret-key",
"secure": True,
"rootdirectory": "/harbor",
},
"disableredirect": False,
},
},
"externalURL": f"harbor.apps.{on_prem_platform_config.ingress_dns_name}",
}

def test_create_harbor_values_with_admin_credentials(
self, on_prem_platform_config: PlatformConfig, factory: HelmValuesFactory
) -> None:
on_prem_platform_config = replace(
on_prem_platform_config,
registry=replace(
on_prem_platform_config.registry,
docker_registry_type=DockerRegistryType.HARBOR,
docker_registry_username="admin",
docker_registry_password="admin-password",
),
)
result = factory.create_harbor_values(on_prem_platform_config)

assert (
result["existingSecretAdminPassword"]
== f"{on_prem_platform_config.release_name}-docker-registry"
)
assert result["existingSecretAdminPasswordKey"] == "password"

def test_create_minio_values(
self, on_prem_platform_config: PlatformConfig, factory: HelmValuesFactory
) -> None:
Expand Down
Loading