Improve Ingress + TLS configurability #16
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,46 @@ | ||
| # File created using '.gitignore Generator' for Visual Studio Code: https://bit.ly/vscode-gig | ||
| # Created by https://www.toptal.com/developers/gitignore/api/windows,visualstudiocode,linux,helm,jetbrains | ||
| # Edit at https://www.toptal.com/developers/gitignore?templates=windows,visualstudiocode,linux,helm,jetbrains | ||
|
|
||
| ### Helm ### | ||
| # Chart dependencies | ||
| **/charts/*.tgz | ||
|
|
||
| ### IntelliJ ### | ||
| .idea/* | ||
| !.idea/icon.svg | ||
|
|
||
| ### Linux ### | ||
| *~ | ||
|
|
||
| # Linux trash folder which might appear on any partition or disk | ||
| .Trash-* | ||
|
|
||
| ### VisualStudioCode ### | ||
| .vscode/* | ||
| !.vscode/settings.json | ||
| !.vscode/tasks.json | ||
| !.vscode/launch.json | ||
| !.vscode/extensions.json | ||
| !.vscode/*.code-snippets | ||
|
|
||
| # Local History for Visual Studio Code | ||
| .history/ | ||
|
|
||
| ### VisualStudioCode Patch ### | ||
| # Ignore all local history of files | ||
| .history | ||
| .ionide | ||
|
|
||
| ### Windows ### | ||
| # Windows thumbnail cache files | ||
| Thumbs.db | ||
| Thumbs.db:encryptable | ||
| ehthumbs.db | ||
| ehthumbs_vista.db | ||
|
|
||
| # Dump file | ||
| *.stackdump | ||
|
|
||
| # Folder config file | ||
| [Dd]esktop.ini |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| # Patterns to ignore when building packages. | ||
| # This supports shell glob matching, relative path matching, and | ||
| # negation (prefixed with !). Only one pattern per line. | ||
| .DS_Store | ||
| # Common VCS dirs | ||
| .git/ | ||
| .gitignore | ||
| .bzr/ | ||
| .bzrignore | ||
| .hg/ | ||
| .hgignore | ||
| .svn/ | ||
| # Common backup files | ||
| *.swp | ||
| *.bak | ||
| *.tmp | ||
| *~ | ||
| # Various IDEs | ||
| .project | ||
| .idea/ | ||
| *.tmproj |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| dependencies: | ||
| - name: common | ||
| repository: oci://registry-1.docker.io/bitnamicharts | ||
| version: 2.19.1 | ||
| digest: sha256:f3d03c6da540e7e18162d5336c8c35627f8295f25fe68bfdc4fef57ba6046c1c | ||
| generated: "2024-04-12T17:57:55.376976539+02:00" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| apiVersion: v2 | ||
| name: dependency-track | ||
| version: 0.2.0 | ||
| type: application | ||
| appVersion: 4.10.1 | ||
| description: |- | ||
|
|
@@ -14,3 +14,7 @@ maintainers: | |
| - name: nscuro | ||
| email: [email protected] | ||
| url: https://github.com/nscuro | ||
| dependencies: | ||
| - name: common | ||
| repository: oci://registry-1.docker.io/bitnamicharts | ||
| version: ^2.19.1 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,29 +1,83 @@ | ||
| {{- if .Values.ingress.enabled }} | ||
| apiVersion: {{ include "common.capabilities.ingress.apiVersion" . }} | ||
|
There was a problem hiding this comment.
|
||
| kind: Ingress | ||
| metadata: | ||
| name: {{ include "common.names.fullname" . }} | ||
| namespace: {{ include "common.names.namespace" . | quote }} | ||
| labels: {{- include "common.labels.standard" ( dict "customLabels" .Values.commonLabels "context" $ ) | nindent 4 }} | ||
|
There was a problem hiding this comment. I feel like putting to much logic in the expressions makes it a lot harder to read the manifest files. Can we externalize some of it to There was a problem hiding this comment. Hi! Thank you for initiating the discussion. Let me explain few things: This refined template, utilized by Bitnami in their charts, constitutes a marked enhancement over the original Ingress, which was candidly inadequate and utterly unsuitable for standard deployments. Initially, configuration was virtually impossible; key elements such as My proposal has transformed this Helm chart into an exceptionally intuitive tool, harmonizing with the familiar structure of Values employed by many DevOps engineers who utilize Bitnami charts or their derivatives. So why attempt to reinvent the wheel? Moreover, this version supports TLS, cert-manager, and self-signed certificates (including auto-generation) right out of the box—because, as I have stated, I did not create it; I merely adopted it entirely, enhancing it just sufficiently to function as a cohesive unit. Furthermore, this template accommodates numerous modifications and supports various Kubernetes API versions back to version 1.19. It has also proven its efficacy in environments that are pervasive within the corporate sphere. I recognize the appeal of a streamlined and orderly Helm chart; however, simplicity alone does not equate to deployability. No accolades will be forthcoming for a "simple" Helm chart, particularly given that its deployment demanded a full day and subsequently required adjustments via Kustomize due to the absence of configuration options through Values. Consider the following: From my perspective, there is scant latitude for experimentation within this domain; it is prudent to adhere to the tried-and-tested industry standards. This template closely mirrors the Bitnami Nginx chart, available for review at Bitnami Charts. Although some elements might be streamlined, such reductions could prove counterproductive. This template is devoid of extraneous components; indeed, it exemplifies essential functionality. My recommendation is to prioritize product development over the solitary maintenance of a Helm chart, a task that would undoubtedly prove arduous. Moreover, despite the relative stability of the Kubernetes API, occasional shifts occur. Adhering to a tested implementation like this one, actively maintained by Bitnami, guarantees that the Helm chart is cross-platform—effective on OpenShift, AWS, Rancher, and microk8s. Should this not be our paramount concern? Furthermore, all new options in Values have been meticulously documented. Let the code fulfill its purpose when operational; ultimately, users prioritize user experience above all. Certainly, the decision to accept my proposal rests solely with you. I realize my comments might seem a bit forward, but please rest assured that they weren't meant personally. I endeavored to remain as objective as possible. Dependency Track is indeed a remarkable product. 🙂 There was a problem hiding this comment.
Well, see, this repository was created with the explicit intent to have it built by the community (DependencyTrack/dependency-track#3080 (comment)). It even has a disclaimer in the README about not being production ready: Lines 3 to 4 in 008c6a2 It's virtually impossible to foresee all the options people might need upfront. So from that perspective, your contribution is very much appreciated.
I do understand that there is functionality that you (and surely others as well) need, but in the end it will be us maintainers who have to literally maintain this thing going forward. In asking about simplifying things, and reducing dependencies on 3rd party code, I'm trying to manage risk of us losing grip on how this chart works. Apologies for the ping from the sideline, but @NissesSenap @heubeck can I get a second opinion on the proposed changes please? There was a problem hiding this comment.
Yes, I am aware of that—so I intended to help push it a bit further. That was meant to be constructive criticism, not a complaint. 🙂 Originally, my intention was to keep it as simple as possible, yet at a level that would satisfy a wide range of users with varying demands. Would adding comments help? However, I didn’t want to clutter the charter with unnecessary elements. There was a problem hiding this comment.
I fully understand. There was a problem hiding this comment.
Indeed, should my proposal be adopted, it would undoubtedly be prudent to revise the remainder of the chart accordingly—for consistency’s sake, as well as to ensure the entire chart maintains a uniform standard of excellence. However, I refrained from initiating more extensive modifications, as my primary objective centered around addressing the inadequacies of the poorly configurable Ingress. The amendments I suggested are confined to Would it be advisable to appropriately adjust the rest of the chart as well, integrating all changes simultaneously? If there’s consensus, I’d be happy to prepare the remainder, this time with enhanced internal documentation. I would certainly complete the documentation for the rest of |
||
| {{- if or .Values.ingress.annotations .Values.commonAnnotations }} | ||
| {{- $annotations := include "common.tplvalues.merge" ( dict "values" ( list .Values.ingress.annotations .Values.commonAnnotations ) "context" . ) }} | ||
| annotations: {{- include "common.tplvalues.render" ( dict "value" $annotations "context" $) | nindent 4 }} | ||
| {{- end }} | ||
| spec: | ||
| {{- if and .Values.ingress.ingressClassName (eq "true" (include "common.ingress.supportsIngressClassname" .)) }} | ||
| ingressClassName: {{ .Values.ingress.ingressClassName | quote }} | ||
| {{- end }} | ||
| rules: | ||
| {{- if .Values.ingress.hostname }} | ||
| - host: {{ .Values.ingress.hostname | quote }} | ||
| http: | ||
| paths: | ||
| {{- if .Values.ingress.extraPaths }} | ||
| {{- toYaml .Values.ingress.extraPaths | nindent 10 }} | ||
| {{- end }} | ||
| - path: {{ default "/api" .apiPath }} | ||
| {{- if eq "true" (include "common.ingress.supportsPathType" $) }} | ||
| pathType: {{ default "ImplementationSpecific" .pathType }} | ||
| {{- end }} | ||
| backend: {{- include "common.ingress.backend" (dict "serviceName" (include "dependencytrack.apiServerFullname" $) "servicePort" "http" "context" $) | nindent 14 }} | ||
| - path: {{ default "/" .path }} | ||
| {{- if eq "true" (include "common.ingress.supportsPathType" $) }} | ||
| pathType: {{ default "ImplementationSpecific" .pathType }} | ||
| {{- end }} | ||
| backend: {{- include "common.ingress.backend" (dict "serviceName" (include "dependencytrack.frontendFullname" $) "servicePort" "http" "context" $) | nindent 14 }} | ||
| {{- else if .Values.ingress.path }} | ||
| - http: | ||
| paths: | ||
| {{- if .Values.ingress.extraPaths }} | ||
| {{- toYaml .Values.ingress.extraPaths | nindent 10 }} | ||
| {{- end }} | ||
| - path: {{ default "/api" .apiPath }} | ||
| {{- if eq "true" (include "common.ingress.supportsPathType" $) }} | ||
| pathType: {{ default "ImplementationSpecific" .pathType }} | ||
| {{- end }} | ||
| backend: {{- include "common.ingress.backend" (dict "serviceName" (include "dependencytrack.apiServerFullname" $) "servicePort" "http" "context" $) | nindent 14 }} | ||
| - path: {{ default "/" .path }} | ||
| {{- if eq "true" (include "common.ingress.supportsPathType" $) }} | ||
| pathType: {{ default "ImplementationSpecific" .pathType }} | ||
| {{- end }} | ||
| backend: {{- include "common.ingress.backend" (dict "serviceName" (include "dependencytrack.frontendFullname" $) "servicePort" "http" "context" $) | nindent 14 }} | ||
| {{- end }} | ||
| {{- range .Values.ingress.extraHosts }} | ||
| - host: {{ .name | quote }} | ||
| http: | ||
| paths: | ||
| - path: {{ default "/api" .apiPath }} | ||
| {{- if eq "true" (include "common.ingress.supportsPathType" $) }} | ||
| pathType: {{ default "ImplementationSpecific" .pathType }} | ||
| {{- end }} | ||
| backend: {{- include "common.ingress.backend" (dict "serviceName" (include "dependencytrack.apiServerFullname" $) "servicePort" "http" "context" $) | nindent 14 }} | ||
| - path: {{ default "/" .path }} | ||
| {{- if eq "true" (include "common.ingress.supportsPathType" $) }} | ||
| pathType: {{ default "ImplementationSpecific" .pathType }} | ||
| {{- end }} | ||
| backend: {{- include "common.ingress.backend" (dict "serviceName" (include "dependencytrack.frontendFullname" $) "servicePort" "http" "context" $) | nindent 14 }} | ||
| {{- end }} | ||
| {{- if .Values.ingress.extraRules }} | ||
| {{- include "common.tplvalues.render" (dict "value" .Values.ingress.extraRules "context" $) | nindent 4 }} | ||
| {{- end }} | ||
| {{- if or (and .Values.ingress.tls (or (include "common.ingress.certManagerRequest" ( dict "annotations" .Values.ingress.annotations )) .Values.ingress.selfSigned (not (empty .Values.ingress.secrets)))) .Values.ingress.extraTls }} | ||
| tls: | ||
| {{- if and .Values.ingress.tls (or (include "common.ingress.certManagerRequest" ( dict "annotations" .Values.ingress.annotations )) .Values.ingress.selfSigned (not (empty .Values.ingress.secrets))) }} | ||
| - hosts: | ||
| - {{ .Values.ingress.hostname | quote }} | ||
| {{- if or (.Values.ingress.tlsWwwPrefix) (eq (index .Values.ingress.annotations "nginx.ingress.kubernetes.io/from-to-www-redirect") "true" ) }} | ||
| - {{ printf "www.%s" (tpl .Values.ingress.hostname $) | quote }} | ||
| {{- end }} | ||
| secretName: {{ printf "%s-tls" .Values.ingress.hostname }} | ||
| {{- end }} | ||
| {{- if .Values.ingress.extraTls }} | ||
| {{- include "common.tplvalues.render" (dict "value" .Values.ingress.extraTls "context" $) | nindent 4 }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- end }} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| {{- if and .Values.tls.enabled (not .Values.tls.existingSecret) }} | ||
| {{- $ca := genCA "dependency-track-ca" 365 }} | ||
| apiVersion: v1 | ||
| kind: Secret | ||
| metadata: | ||
| name: {{ printf "%s-tls" (include "common.names.fullname" .) }} | ||
| namespace: {{ include "common.names.namespace" . | quote }} | ||
| labels: {{- include "common.labels.standard" ( dict "customLabels" .Values.commonLabels "context" $ ) | nindent 4 }} | ||
| {{- if .Values.commonAnnotations }} | ||
| annotations: {{- include "common.tplvalues.render" ( dict "value" .Values.commonAnnotations "context" $ ) | nindent 4 }} | ||
| {{- end }} | ||
| type: kubernetes.io/tls | ||
| data: | ||
| {{- if .Values.tls.autoGenerated }} | ||
| {{- $cert := genSignedCert (include "common.names.fullname" .) nil (list (include "common.names.fullname" .) (printf "%s.%s" (include "common.names.fullname" .) (include "common.names.namespace" .)) (printf "%s.%s.svc" (include "common.names.fullname" .) (include "common.names.namespace" .)) (printf "%s.%s.svc.%s" (include "common.names.fullname" .) (include "common.names.namespace" .) .Values.clusterDomain)) 365 $ca }} | ||
| {{ .Values.tls.certFilename }}: {{ include "common.secrets.lookup" (dict "secret" (printf "%s-tls" (include "common.names.fullname" .)) "key" .Values.tls.certFilename "defaultValue" $cert.Cert "context" $) }} | ||
| {{ .Values.tls.certKeyFilename }}: {{ include "common.secrets.lookup" (dict "secret" (printf "%s-tls" (include "common.names.fullname" .)) "key" .Values.tls.certKeyFilename "defaultValue" $cert.Key "context" $) }} | ||
| {{ .Values.tls.certCAFilename }}: {{ include "common.secrets.lookup" (dict "secret" (printf "%s-tls" (include "common.names.fullname" .)) "key" .Values.tls.certCAFilename "defaultValue" $ca.Cert "context" $) }} | ||
| {{- else }} | ||
| {{- if .Values.tls.cert }} | ||
| {{ .Values.tls.certFilename }}: {{ .Values.tls.cert | b64enc }} | ||
| {{- end }} | ||
| {{- if .Values.tls.key }} | ||
| {{ .Values.tls.certKeyFilename }}: {{ .Values.tls.key | b64enc }} | ||
| {{- end }} | ||
| {{- if .Values.tls.ca }} | ||
| {{ .Values.tls.certCAFilename }}: {{ .Values.tls.ca | b64enc }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to achieve the desired outcome without adding 3rd party dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but in my opinion, that's unfortunate because Bitnami is quite stable regarding breaking changes. If it's pegged to major version 2, you can benefit from the fixes and improvements of that library. During the build, the entire library is downloaded, unpacked, and added to the final Helm chart, so it serves merely as a build dependency. The resulting chart is hermetically self-sufficient, eliminating any risk of issues related to unavailability, etc.
However, if you want to maintain control, you can add it as a subchart—the result will be the same, but this approach offers greater control over the origin, and, if necessary, it allows for manual updates.