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.

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

Template External Url #2138

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

neevnuv
Copy link

@neevnuv neevnuv commented Feb 15, 2025

Description

This PR introduces templating support for the externalURL value inside the core-cm ConfigMap by wrapping it with tpl, allowing for dynamic evaluation of Helm templates.

Changes

Updated core-cm to use {{ tpl .Values.externalURL . }} instead of a static value.

Rationale

Enables users to reference other values or use Helm functions within externalURL.
Increases flexibility for dynamic configurations in different environments.

@neevnuv neevnuv force-pushed the template-external-url branch from 60d9d32 to a105ab2 Compare February 15, 2025 16:14
@neevnuv
Copy link
Author

neevnuv commented Feb 16, 2025

Hi @MinerYang, I would love if you could give this PR a view 😁.

@MinerYang MinerYang self-requested a review February 17, 2025 07:29
@MinerYang
Copy link
Collaborator

Hi @neevnuv ,

Thanks for contribution to harbor-helm.

By getting some contexts of using tpl functions for helm template, the only "potential benefit" I could thought would be dynamically rendering the templating at the the runtime. However when you doing this, other configs like certificate for nginx tls config would be inconsistent and causing issues. And using the chain templates function would increasing our helm template complexity, from my perspective, a static rendering is enough for this value.

However, I will keep this PR open and let's see if we will have same requirements from the community in the future. In the meantime, you could use this at your own fork repository.

Best,
Miner

@neevnuv
Copy link
Author

neevnuv commented Feb 20, 2025

Hi @MinerYang,

Thanks for your feedback! I understand the concerns about consistency and complexity.

We use ArgoCD on OpenShift to deploy Harbor across multiple clusters via ApplicationSets. Each cluster’s externalURL must follow harbor.apps..example.com, and we use .Values.global.clusterName for templating. This PR enables us to set externalURL dynamically, reducing manual effort and misconfigurations.

Notably, the only appearance of externalURL is in the core-cm.yaml ConfigMap, minimizing the risk of inconsistencies with other configurations like TLS.

While tpl adds some complexity, it simplifies multi-cluster management. Would you consider making this feature optional? It could benefit other users with similar needs. Looking forward to your thoughts!

@MinerYang
Copy link
Collaborator

Hi @MinerYang,
Notably, the only appearance of externalURL is in the core-cm.yaml ConfigMap, minimizing the risk of inconsistencies with other configurations like TLS.

Thanks @neevnuv for more details here. I will try to catch up more about ArgoCD and multi-cluster things about templating config.
A mainly concern here is once we dynamically change the externalURL here , we also need dynamically rotate the tls cert as well , so would you like to share how could we handle this properly?

@neevnuv
Copy link
Author

neevnuv commented Feb 25, 2025

@MinerYang, please correct me if I’m wrong, but it seems that tls.key and tls.crt do not depend on .Values.externalURL.

The TLS key and certificate are defined in core-secret.yaml.

  • If a user does not specify a custom tls.crt/key, the CA is generated using a random generator.
  • If a user does provide a custom tls.crt/key, then the externalURL shouldn’t affect it, since the user is responsible for managing the certificate and key.

Additionally, I believe externalURL is separate from the service/ingress values, as it mainly informs the Harbor core pod of the external URL users will use to push and pull images.

Since this PR only applies tpl to the variable without adding complex logic, it shouldn’t impact existing configurations. Let me know what you think!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants