Add loadbalancersourceranges to ssh service #105

JPRbrs · 2021-01-22T11:59:23Z

JPRbrs commented

2021-01-22 11:59:23 +00:00

SSH service might want to limit the a range of source IPs. LoadBalancerSourceRanges
enables to limit them just passing a list of CIDR addresses to whitelist

SSH service might want to limit the a range of source IPs. LoadBalancerSourceRanges enables to limit them just passing a list of CIDR addresses to whitelist

JPRbrs added 1 commit 2021-01-22 11:59:24 +00:00

Add loadbalancersourceranges to ssh service

continuous-integration/drone/pr Build is failing

Details

af4840d1d3

SSH service might want to limit the a range of source IPs. LoadBalancerSourceRanges enables to limit them just passing a list of CIDR addresses to whitelist

JPRbrs added 1 commit 2021-01-22 12:02:00 +00:00

Typo

continuous-integration/drone/pr Build is passing

Details

f5d3acc736

techknowlogick requested changes 2021-01-25 02:47:15 +00:00

Dismissed

templates/gitea/ssh-svc.yaml Outdated

              @@ -9,6 +9,10 @@ metadata:  
     spec:  
      type: {{ .Values.service.ssh.type }}  
      {{- if and .Values.service.ssh.loadBalancerIP (eq .Values.service.ssh.type "LoadBalancer") }}  
      loadBalancerSourceRanges:  
  

techknowlogick commented

2021-01-25 02:47:11 +00:00

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?

JPRbrs added 1 commit 2021-01-25 10:39:48 +00:00

Add conditional to loadBalancerSourceRanges

continuous-integration/drone/pr Build is passing

Details

42fc53d38b

JPRbrs commented

2021-01-25 10:40:37 +00:00

Thanks for your feedback @techknowlogick, I've just pushed a commit with the changes

luhahn requested changes 2021-01-25 16:50:04 +00:00

Dismissed

templates/gitea/ssh-svc.yaml Outdated

              @@ -8,6 +8,12 @@ metadata:  
      {{- toYaml .Values.service.ssh.annotations | nindent 4 }}  
     spec:  
      type: {{ .Values.service.ssh.type }}  
      {{- if and .Values.service.ssh.loadBalancerSourceRanges }}  
  

luhahn commented

2021-01-25 16:49:54 +00:00

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

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

JPRbrs added 1 commit 2021-01-26 15:51:42 +00:00

Address review comments

continuous-integration/drone/pr Build is passing

Details

43d5751088

Fit the LoadBalancersourceranges within the service type Loadbalancer conditional

luhahn requested changes 2021-01-26 17:39:09 +00:00

Dismissed

templates/gitea/ssh-svc.yaml Outdated

              @@ -10,6 +10,12 @@ spec:  
      type: {{ .Values.service.ssh.type }}  
      {{- if and .Values.service.ssh.loadBalancerIP (eq .Values.service.ssh.type "LoadBalancer") }}  
  

luhahn commented

2021-01-26 17:38:53 +00:00

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

@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

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

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

JPRbrs requested review from techknowlogick 2021-01-27 16:23:36 +00:00

JPRbrs requested review from luhahn 2021-01-27 16:23:41 +00:00

JPRbrs added 1 commit 2021-01-27 16:26:53 +00:00

Separate LoadBalancerIP and LoadBalancerSourceRange conditions

continuous-integration/drone/pr Build is passing

Details

ac1d80d2ce

luhahn requested changes 2021-01-28 08:13:20 +00:00

Dismissed

luhahn left a comment

Still not correct.

your current change:

{{- if and .Values.service.ssh.loadBalancerIP (eq .Values.service.ssh.type "LoadBalancer") }}  loadBalancerIP: {{ .Values.service.ssh.loadBalancerIP }} {{- end -}} {{- if .Values.service.ssh.loadBalancerSourceRanges }}  loadBalancerSourceRanges:  {{- range .Values.service.ssh.loadBalancerSourceRanges }}  - {{ . }}  {{- end }} {{- end }}

But you still got if and .Values.service.ssh.loadBalancerIP (eq .Values.service.ssh.type "loadBalancer")

It needs to be this (without the comments of course):

{{- if eq .Values.service.ssh.type "LoadBalancer" }} # loadBalancer  {{- if .Values.service.ssh.loadBalancerIP }} # loadBalancerIP  loadBalancerIP: {{ .Values.service.ssh.loadBalancerIP }}  {{- end -}} # end loadBalancerIP  {{- if .Values.service.ssh.loadBalancerSourceRanges }} # loadBalancerSourceRanges  loadBalancerSourceRanges:  {{- range .Values.service.ssh.loadBalancerSourceRanges }} # loop  - {{ . }}  {{- end }} # end loop  {{- end }} # end loadBalancerSourceRanges {{- end }} # end loadBalancer

Still not correct. your current change: ```yaml {{- if and .Values.service.ssh.loadBalancerIP (eq .Values.service.ssh.type "LoadBalancer") }} loadBalancerIP: {{ .Values.service.ssh.loadBalancerIP }} {{- end -}} {{- if .Values.service.ssh.loadBalancerSourceRanges }} loadBalancerSourceRanges: {{- range .Values.service.ssh.loadBalancerSourceRanges }} - {{ . }} {{- end }} {{- end }} ``` But you still got if and .Values.service.ssh.loadBalancerIP (eq .Values.service.ssh.type "loadBalancer") It needs to be this (without the comments of course): ```yaml {{- if eq .Values.service.ssh.type "LoadBalancer" }} # loadBalancer {{- if .Values.service.ssh.loadBalancerIP }} # loadBalancerIP loadBalancerIP: {{ .Values.service.ssh.loadBalancerIP }} {{- end -}} # end loadBalancerIP {{- if .Values.service.ssh.loadBalancerSourceRanges }} # loadBalancerSourceRanges loadBalancerSourceRanges: {{- range .Values.service.ssh.loadBalancerSourceRanges }} # loop - {{ . }} {{- end }} # end loop {{- end }} # end loadBalancerSourceRanges {{- end }} # end loadBalancer ```

JPRbrs added 1 commit 2021-01-28 09:19:36 +00:00

Adressing review comments

continuous-integration/drone/pr Build is passing

Details

518c214cd2

JPRbrs commented

2021-01-28 09:19:52 +00:00

Many thanks for your patience @luhahn, I've just uploaded the change slightly changing the indentation as running tests locally rendered some lines slighly off.