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

Add option to not expose Harbor. #1073

Closed
wants to merge 1 commit into from

Conversation

Vad1mo
Copy link
Member

@Vad1mo Vad1mo commented Oct 21, 2021

This contribution allows users to not expose Harbor so that they use their preferred way of exposing Harbor, that should be outside the scope of this Helm Chart.

The main reasons someone would not expose Harbor is because they are planning to expose Harbor in a way not supported by this Helm Chart:

Examples:

  • Istio Gateway
  • AWS ALB

Those two examples that have been requested frequently by the community.
This contribution would allow users to define whatever way they want to expose Harbor.

Closes #1132, #1006, #914, #295, #1672

@reasonerjt
Copy link
Contributor

Hi @Vad1mo

Thanks for the PR but seems you can just set the .Values.expose.type to something like none to achieve the same goal?
Have you tried that?

@Vad1mo
Copy link
Member Author

Vad1mo commented Oct 28, 2021

@reasonerjt I tried to set ingress to none but it didn't work.

helm template --set "expose.type=none" . > test-none.yaml
Error: execution error at (harbor/templates/nginx/secret.yaml:3:12): The "expose.tls.auto.commonName" is required!

After looking into the templates, it would have meant to do some more rewriting to make it work with none I decided not to do this because of Consistency over verbosity: enable=true is a common and known practice when working with Helm.

@Vad1mo Vad1mo requested a review from reasonerjt November 19, 2021 13:15
@ninjadq ninjadq self-requested a review December 9, 2021 07:53
@ninjadq ninjadq self-assigned this Dec 9, 2021
@ninjadq
Copy link
Member

ninjadq commented Dec 9, 2021

hi @Vad1mo , because the error is caused by commonName, how about just assign an arbitrary name for expose.tls.auto.commonName as a workaround? so we can avoid changes a lots of code here

@Vad1mo
Copy link
Member Author

Vad1mo commented Dec 21, 2021

Having this switch explicitly in the Harbor Helm Chart has a few advantages over implicit workarounds:

  1. Implicit workarounds can stop working and there is no clear documentation or lookup possibility in the code.
  2. Explicit documentation and workflow allow users to find exactly the solution to their problem.

Apart from implicit vs. explicit, this feature has a few advantages:
This functionality allows using Harbor with every ingress controller, and the Harbor Helm Chart doesn't have to support every use case.

@zyyw zyyw assigned zyyw and unassigned ninjadq Jan 13, 2022
@Vad1mo
Copy link
Member Author

Vad1mo commented Mar 25, 2022

Can I get your attention @reasonerjt @ninjadq on this PR? especially after my comment explaining the reason for the explicitly? ☝️

@CloudlightCoUk
Copy link

I share the use case above. For my project I don't want to use nginx as a proxy. The chart as it is either creates a nginx object, or trys to deploy nginx. I am using Contour with a HTTPProxy object for my front end proxy. The Harbor application architecture defines a proxy layer, so we should have the option to disable this in the chart to 'bring your own proxy'

rgarcia89 added a commit to rgarcia89/harbor-helm that referenced this pull request Jan 22, 2024
Signed-off-by: Raul Garcia Sanchez <[email protected]>
@Vad1mo
Copy link
Member Author

Vad1mo commented Jan 22, 2024

closed in favor #1687

@Vad1mo Vad1mo closed this Jan 22, 2024
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.

5 participants