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
Contributor

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
Some checks failed
continuous-integration/drone/pr Build is failing
af4840d1d3
Add loadbalancersourceranges to ssh service
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
All checks were successful
continuous-integration/drone/pr Build is passing
f5d3acc736
Typo
techknowlogick requested changes 2021-01-25 02:47:15 +00:00
Dismissed
@ -9,6 +9,10 @@ metadata:
spec:
type: {{ .Values.service.ssh.type }}
{{- if and .Values.service.ssh.loadBalancerIP (eq .Values.service.ssh.type "LoadBalancer") }}
loadBalancerSourceRanges:

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
All checks were successful
continuous-integration/drone/pr Build is passing
42fc53d38b
Add conditional to loadBalancerSourceRanges
Author
Contributor

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

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
@ -8,6 +8,12 @@ metadata:
{{- toYaml .Values.service.ssh.annotations | nindent 4 }}
spec:
type: {{ .Values.service.ssh.type }}
{{- if and .Values.service.ssh.loadBalancerSourceRanges }}
Member

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 }} ```
Author
Contributor

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

thanks for the feedback @luhahn I've pushed the changes
JPRbrs added 1 commit 2021-01-26 15:51:42 +00:00
All checks were successful
continuous-integration/drone/pr Build is passing
43d5751088
Address review comments
Fit the LoadBalancersourceranges within the service type Loadbalancer conditional
luhahn requested changes 2021-01-26 17:39:09 +00:00
Dismissed
@ -10,6 +10,12 @@ spec:
type: {{ .Values.service.ssh.type }}
{{- if and .Values.service.ssh.loadBalancerIP (eq .Values.service.ssh.type "LoadBalancer") }}
Member

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 }}
Author
Contributor

@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)
Member

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 ```
Author
Contributor

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 :)
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
All checks were successful
continuous-integration/drone/pr Build is passing
ac1d80d2ce
Separate LoadBalancerIP and LoadBalancerSourceRange conditions
luhahn requested changes 2021-01-28 08:13:20 +00:00
Dismissed
luhahn left a comment
Member

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
All checks were successful
continuous-integration/drone/pr Build is passing
518c214cd2
Adressing review comments
Author
Contributor

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.

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.
luhahn approved these changes 2021-01-28 13:38:56 +00:00
Dismissed
luhahn left a comment
Member

looks good to me now :)

looks good to me now :)
Author
Contributor

@techknowlogick can you please take a look and see if it looks good to you?
thanks in advance!

@techknowlogick can you please take a look and see if it looks good to you? thanks in advance!
techknowlogick approved these changes 2021-02-04 20:41:41 +00:00
Dismissed
techknowlogick added 1 commit 2021-02-04 20:41:47 +00:00
All checks were successful
continuous-integration/drone/pr Build is passing
36ce015a44
Merge branch 'master' into master
techknowlogick merged commit 28e94f96e3 into master 2021-02-04 20:42:42 +00:00
Sign in to join this conversation.
No description provided.