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

Jump to bottom

Add condition to disable nginx even if not exposing via ingress #1687

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

Add condition to disable nginx even if not exposing via ingress #1687

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
146e797
add condition to disable deployment of nginx even if you do not expos…
rgarcia89 Jan 19, 2024
3833764
update readme
rgarcia89 Jan 22, 2024
893345e
implemented Vadims #1073 PR changes
rgarcia89 Jan 22, 2024
0b1bef8
fix markdown
rgarcia89 Jan 22, 2024
e90f2fe
readme markdown foo
rgarcia89 Jan 22, 2024
439260e
revert changes in nginx changes
rgarcia89 Jan 22, 2024
3800c79
markdown next try
rgarcia89 Jan 22, 2024
4e4c994
resolve conflicting files
rgarcia89 Oct 8, 2024
b7c1b17
Merge branch 'main' into nginx
rgarcia89 Oct 8, 2024
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
4 changes: 4 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ The following items can be set via `--set` flag during installation or configure
- **NodePort**: Exposes the service on each Node’s IP at a static port (the NodePort). You’ll be able to contact the NodePort service, from outside the cluster, by requesting `NodeIP:NodePort`.
- **LoadBalancer**: Exposes the service externally using a cloud provider’s load balancer.

If your desired option to expose Harbor (e.g. Istio Gateway, AWS ALB) is not present you can set `expose.enabled` to false and expose Harbor manually with Kubernetes Manifests.

#### Configure the external URL

The external URL for Harbor core service is used to:
Expand Down Expand Up @@ -78,6 +80,7 @@ The following table lists the configurable parameters of the Harbor chart and th
| Parameter | Description | Default |
| -------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------- |
| **Expose** | | |
| `expose.enabled` | Set to false if no `ingress`, `clusterIP`, `nodePort` or `loadBalancer` should be created or you plan to expose Harbor in a way not offered by this chart. | `true` |
Copy link
Contributor

@wy65701436 wy65701436 Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of adding a new item, I prefer to include additional Enums such as "None" for the expose type. This approach will not overly complicate the code.

And when set the value of expose type to none, we do nothing for ingress and nginx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my humble opinion, I believe that an enable setter aligns better with the current chart structure.
internalTLS, persistence, trivy, metrics, and trace each have one.
So, essentially, all dispensable components have one. Why deviate from this approach here now?

Copy link
Member

Choose a reason for hiding this comment

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

I agree enable = true/false is common practice in Helm Chart development, we also use it in that chart. While it seem to be more verbose in code it is much easier and straightforward to understand for users what is going on and what is going to happen

Copy link
Collaborator

@MinerYang MinerYang Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

enabled more like a two sides thing, like whether using internalTLS or not, there's no third choice. But for expose, we have so many choices already, and made our control flow sometimes redundant with both type and enabled . We would like to make our templates as simple as possible and in the meantime fulfill the requirements.
I will take some time to maybe have an overall adjustment for this part, and please @rgarcia89 help to review on it if your are willing to.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understnad why you think this is to complex logic.

{{- if and (ne .Values.expose.type "ingress") .Values.nginx.enabled }}
  • This is common practice, in writing helm charts, every best practice blog mentions that practive.
  • it is much easier to test in the subsequent logic .Values.nginx.enabled instead of .if Values.expose.type == None
  • It doesn't follow that https://en.wikipedia.org/wiki/Single_responsibility_principle. Values.expose.type would have two different meaning and depending on the value cause implicitly different behaivor, which is difficult to reason about in the long run. especially for the average Chart Developer. (I never knew I would say that in terms of YAML, but its true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with @Vad1mo on that. I don't see why adding an enable switch should increase the complexity of the template. From my understanding, this is following a very simple approach and is not creating a misleading situation, which could be created by a type: none expose.

Copy link
Collaborator

@MinerYang MinerYang Jan 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree it is not complex, and my first thoughts was could we make it more simple when we only use expose.type as the only entry-point instead of combine with expose.type and nginx.enabled for the proxy usage.
maybe have something like this in values.yaml

expose:
    ## enum type Nginx, Ingress, ClusterIP,  NodePort, Loadbalancer, Others
    type:
    nginx:
        image:
        ...
    ingress:
       hosts:
       ...
    clusterIP:
        name:
        ...
    loadBalancer:
        name:
        ...
    nodeport:
        name:
        ...

While the templates would be something like this

{{- if Values.expose.type == nginx }}

Sure I will take your comments into consideration, And please point me out if I have something understood wrong. Thx~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any update here?

| `expose.type` | How to expose the service: `ingress`, `clusterIP`, `nodePort` or `loadBalancer`, other values will be ignored and the creation of service will be skipped. | `ingress` |
| `expose.tls.enabled` | Enable TLS or not. Delete the `ssl-redirect` annotations in `expose.ingress.annotations` when TLS is disabled and `expose.type` is `ingress`. Note: if the `expose.type` is `ingress` and TLS is disabled, the port must be included in the command when pulling/pushing images. Refer to https://github.com/goharbor/harbor/issues/5291 for details. | `true` |
| `expose.tls.certSource` | The source of the TLS certificate. Set as `auto`, `secret` or `none` and fill the information in the corresponding section: 1) auto: generate the TLS certificate automatically 2) secret: read the TLS certificate from the specified secret. The TLS certificate can be generated manually or by cert manager 3) none: configure no TLS certificate for the ingress. If the default TLS certificate is configured in the ingress controller, choose this option | `auto` |
Expand Down Expand Up @@ -185,6 +188,7 @@ The following table lists the configurable parameters of the Harbor chart and th
| `proxy.components` | The component list that the proxy settings apply to | core, jobservice, trivy |
| `enableMigrateHelmHook` | Run the migration job via helm hook, if it is true, the database migration will be separated from harbor-core, run with a preupgrade job migration-job | `false` |
| **Nginx** (if service exposed via `ingress`, Nginx will not be used) | | |
| `nginx.enabled` | Set to false if you plan to expose Harbor without nginx | `true` |
| `nginx.image.repository` | Image repository | `goharbor/nginx-photon` |
| `nginx.image.tag` | Image tag | `dev` |
| `nginx.replicas` | The replica count | `1` |
Expand Down
3 changes: 3 additions & 0 deletions templates/ingress/ingress.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
{{- if .Values.expose.enabled }}
{{- if eq .Values.expose.type "ingress" }}
{{- $ingress := .Values.expose.ingress -}}
{{- $tls := .Values.expose.tls -}}
Expand Down Expand Up @@ -143,3 +144,5 @@ spec:
{{- end }}

{{- end }}

{{- end }}
2 changes: 1 addition & 1 deletion templates/nginx/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{{- if ne .Values.expose.type "ingress" }}
{{- if and (ne .Values.expose.type "ingress") .Values.nginx.enabled }}
apiVersion: apps/v1
kind: Deployment
metadata:
Expand Down
4 changes: 3 additions & 1 deletion values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
expose:
enabled: true
# Set how to expose the service. Set the type as "ingress", "clusterIP", "nodePort" or "loadBalancer"
# and fill the information in the corresponding section
type: ingress
Expand Down Expand Up @@ -381,8 +382,9 @@ enableMigrateHelmHook: false
# contains a base64 encoded CA Certificate named `ca.crt`.
# uaaSecretName:

# If service exposed via "ingress", the Nginx will not be used
# If service exposed via "ingress", or not enabled the Nginx will not be used
nginx:
enabled: true
image:
repository: goharbor/nginx-photon
tag: dev
Expand Down
Loading