gitea/helm-chart
45
54
Fork 128
Code Issues 31 Pull Requests 7 Actions Releases 87 Activity

Add loadbalancersourceranges to ssh service #105

Merged
techknowlogick merged 7 commits from JPRbrs/helm-chart:master into master 2021-02-04 20:42:42 +00:00
Conversation 3 Commits 7 Files Changed 2 +10 -1
2 changed files with 10 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
templates/gitea/ssh-svc.yaml
View File

@ -8,8 +8,16 @@ metadata:
{{- toYaml .Values.service.ssh.annotations | nindent 4 }}
spec:
type: {{ .Values.service.ssh.type }}
{{- if and .Values.service.ssh.loadBalancerIP (eq .Values.service.ssh.type "LoadBalancer") }}
{{- if eq .Values.service.ssh.type "LoadBalancer" }}
luhahn commented 2021-01-25 16:49:54 +00:00
Outdated
Review
Copy Link

The "and" seems to be wrong here.

I think it would be better, if we make one if to check if type == loadBalancer.
Including two other ifs to check if sourceRanges and/or an ip is provided.

Something like this:

{{- if eq .Values.service.ssh.type "LoadBalancer" }}
{{- if .Values.service.ssh.loadBalancerSourceRanges }}
...
{{- end }}
{{- if .Values.service.ssh.loadBalancerIP }}
....
{{- end }}
{{- end }}
The "and" seems to be wrong here. I think it would be better, if we make one if to check if type == loadBalancer. Including two other ifs to check if sourceRanges and/or an ip is provided. Something like this: ```yaml {{- if eq .Values.service.ssh.type "LoadBalancer" }} {{- if .Values.service.ssh.loadBalancerSourceRanges }} ... {{- end }} {{- if .Values.service.ssh.loadBalancerIP }} .... {{- end }} {{- end }} ```
JPRbrs commented 2021-01-26 15:52:40 +00:00
Outdated
Review
Copy Link

thanks for the feedback @luhahn I've pushed the changes

thanks for the feedback @luhahn I've pushed the changes
luhahn commented 2021-01-26 17:38:53 +00:00
Outdated
Review
Copy Link

This will now only trigger, if loadBalancerIP is set AND type == LoadBalancer.
I guess you would want to use loadBalancerSourceRanges with and without a loadBalancerIP.

Please split the if above like that:

{{- if eq .Values.service.ssh.type "LoadBalancer" }}
{{- if .Values.service.ssh.loadBalancerIP }}
...
{{- end }}
{{- if .Values.service.ssh.loadBalancerSourceRanges }}
...
{{- end }}
{{- end }}
This will now only trigger, if loadBalancerIP is set AND type == LoadBalancer. I guess you would want to use loadBalancerSourceRanges with and without a loadBalancerIP. Please split the if above like that: ```yaml {{- if eq .Values.service.ssh.type "LoadBalancer" }} {{- if .Values.service.ssh.loadBalancerIP }} ... {{- end }} {{- if .Values.service.ssh.loadBalancerSourceRanges }} ... {{- end }} {{- end }}
JPRbrs commented 2021-01-27 06:43:27 +00:00
Outdated
Review
Copy Link

@luhahn if I'm not mistaken, loadBalancerSourceRanges is only effective for LoadBancer type services, so there is no need to use it if the service type is not LoadBalancer

It doesn't say so specifically but that is my uderstanding from the documentatoin

@luhahn if I'm not mistaken, loadBalancerSourceRanges is only effective for LoadBancer type services, so there is no need to use it if the service type is not LoadBalancer It doesn't say so specifically but that is my uderstanding from the [documentatoin](https://cloud.google.com/kubernetes-engine/docs/how-to/service-parameters#lb_source_ranges)
luhahn commented 2021-01-27 08:19:42 +00:00
Outdated
Review
Copy Link

Yes, I know. But currently your loadBalancerSourceRanges would only apply if also a loadBalancerIP is given.

Please have a closer look at my suggested if.

{{- if eq .Values.service.ssh.type "LoadBalancer" }} # if type == loadBalancer
  {{- if .Values.service.ssh.loadBalancerIP }} # if a load balancer IP is given
    loadBalancerIP: 
  {{- end }} # end if loadBalancerIP
  {{- if .Values.service.ssh.loadBalancerSourceRanges }} # if sourceRanges are given
    loadBalancerSourceRanges: ...
  {{- end }} # end if loadBalancerSourceRanges
{{- end }} # end if type == loadBalancer
Yes, I know. But currently your loadBalancerSourceRanges would only apply if also a loadBalancerIP is given. Please have a closer look at my suggested if. ```yaml {{- if eq .Values.service.ssh.type "LoadBalancer" }} # if type == loadBalancer {{- if .Values.service.ssh.loadBalancerIP }} # if a load balancer IP is given loadBalancerIP: {{- end }} # end if loadBalancerIP {{- if .Values.service.ssh.loadBalancerSourceRanges }} # if sourceRanges are given loadBalancerSourceRanges: ... {{- end }} # end if loadBalancerSourceRanges {{- end }} # end if type == loadBalancer ```
JPRbrs commented 2021-01-27 16:27:21 +00:00
Outdated
Review
Copy Link

Oh, I see! Many thanks for your detailed explanation, I reckon that's way better. I've just pushed the commit :)

Oh, I see! Many thanks for your detailed explanation, I reckon that's way better. I've just pushed the commit :)
{{- if .Values.service.ssh.loadBalancerIP }}
techknowlogick commented 2021-01-25 02:47:11 +00:00
Outdated
Review
Copy Link

Can you wrap a conditional around loadBalancerSourceRanges in case the array is empty?

Can you wrap a conditional around `loadBalancerSourceRanges` in case the array is empty?
loadBalancerIP: {{ .Values.service.ssh.loadBalancerIP }}
{{- end -}}
{{- if .Values.service.ssh.loadBalancerSourceRanges }}
loadBalancerSourceRanges:
{{- range .Values.service.ssh.loadBalancerSourceRanges }}
- {{ . }}
{{- end }}
{{- end }}
{{- end }}
{{- if and .Values.service.ssh.clusterIP (eq .Values.service.ssh.type "ClusterIP") }}
clusterIP: {{ .Values.service.ssh.clusterIP }}

1
values.yaml
View File

@ -29,6 +29,7 @@ service:
#nodePort:
#externalTrafficPolicy:
#externalIPs:
loadBalancerSourceRanges: []
annotations:
ingress: