deis / registry-proxy

Deis Workflow Registry Proxy
https://deis.com
MIT License
6 stars 4 forks source link

Enable registry-proxy for CNI users #11

Closed Bregor closed 7 years ago

Bregor commented 7 years ago

Variables added:

Proofing image: quay.io/evilmartians/deis-registry-proxy:cni

Steps to setup:

kubectl delete ds -n deis deis-registry-proxy
helm install -n proxy --namespace=deis ./charts/registry-proxy --set global.registry_proxy_bind_addr=127.0.0.1:6666,global.use_cni=true

PR for https://github.com/deis/workflow is on a way.

deis-admin commented 7 years ago

Thanks for the contribution! Please ensure your commits follow our style guide. This code will be tested once a Deis maintainer reviews it.

Bregor commented 7 years ago

BTW, maybe only the use_cni variable makes sense? I mean:

        env:
        ...
        - name: BIND_ADDR
{{- if (.Values.global.registry_use_cni)}}
          value: "127.0.0.1:5555"
{{- else }}
          value: "80"
{{- end }}
Bregor commented 7 years ago

@mboersma what do you think which way is preferrable:

  1. two variables use_cni: <bool> and registry_proxy_bind_addr: <string>
  2. single variable use_cni: <bool> which will setup bind address automagically?
mboersma commented 7 years ago

@Bregor I think we might need the flexibility of allowing registry_proxy_bind_addr to be set separately. It's tempting to make the defaults be 127.0.0.1:5555 and 80, but I can imagine where someone might have things configured differently. Maybe this?

{{- if (.Values.global.registry_use_cni)}}
          value: {{ default "127.0.0.1:5555" .Values.global.registry_proxy_bind_addr }}
{{- else }}
          value: {{ default "80" .Values.global.registry_proxy_bind_addr }}
{{- end }}
vdice commented 7 years ago

Jenkins, test this please.

vdice commented 7 years ago

Artifacts that can be used for testing:

Bregor commented 7 years ago

@mboersma thanks, rewrote with your proposal, rebased, squashed and pushed

vdice commented 7 years ago

Jenkins, OK to test

mboersma commented 7 years ago

I did the negative test on minikube to ensure nothing is broken in the non-CNI case. Looks good!

vdice commented 7 years ago

Thank you for the great addition, @Bregor !

Bregor commented 7 years ago

Great! @vdice thanks for support and patience :)