Handle empty array for shadowsocks.dnsServers #2
Conversation
templates/deployment.yaml
Outdated
| @@ -55,7 +55,7 @@ spec: | |||
| {{- end }} | |||
| {{- with .Values.shadowsocks.dnsServers }} | |||
| - name: DNS_ADDRS | |||
| value: {{ join "," . }} | |||
| value: {{ quote join "," . }} | |||
There was a problem hiding this comment.
You forgot the parentheses here. This will cause the following exception:
Error: template: shadowsocks/templates/deployment.yaml:58:30: executing "shadowsocks/templates/deployment.yaml" at <join>: wrong number of args for join: want 2 got 0
There was a problem hiding this comment.
Hi! Thanks for the feedback, I got confused in the tests, my change wasn't even working.
I force-pushed a fix. To test it please create a value-test.yaml with the following content:
shadowsocks:
dnsServers: []; and then run helm install --dry-run --values values-test.yaml --debug test .
You should get the following:
env:
- ...
- name: DNS_ADDRS
value: ""
alexandre-abrioux
force-pushed
the
quote-dns
branch
from
8bf76fd
to
e8abfdc
Compare
alexandre-abrioux
changed the title
alexandre-abrioux
force-pushed
the
quote-dns
branch
from
e8abfdc
to
b7d9623
Compare
| @@ -53,10 +53,8 @@ spec: | |||
| key: {{ .existingSecret.passwordKey }} | |||
| {{- end }} | |||
| {{- end }} | |||
| {{- with .Values.shadowsocks.dnsServers }} | |||
| - name: DNS_ADDRS | |||
There was a problem hiding this comment.
I think we should probably surround this part with {{- if .Values.shadowsocks.dnsServers }} ... {{- end }}. Passing an empty DNS_ADDRS value to the shadowsocks/shadowsocks-libev image would get the following error:
2021-12-16 14:08:40 ERROR: failed to set nameservers
There was a problem hiding this comment.
@predatorray You are right, there is a bug in the shadowsocks/shadowsocks-libev docker image. For the moment you are not able to use an empty DNS_ADDRS value, which means you are not able to use Kubernetes's internal DNS server (the one that is already setup in /etc/resolv.conf). I've submitted a fix on the same day that I've opened this PR, but it hasn't been merged yet: shadowsocks/shadowsocks-libev#2866. I will keep you posted once it's released!
There was a problem hiding this comment.
I don't quite understand the bug you mentioned in that issue. As I tested on my laptop, the process can be started if we either provide a non-empty DNS_ADDRS or just leave it unset. The problem only occurs if we provide an empty string. Maybe the simplest way is the solution I suggested in the previous comment.
There was a problem hiding this comment.
The solution you suggested won't work. Let me rephrase: the goal of this PR is to be able to set an empty string for DNS_ADDRS in order to use Kubernetes' internal DNS server, instead of the default 8.8.8.8. Doing so will allow shadowsocks proxy users to access any of the cluster's services, like a VPN hosted on the cluster for instance.
To use Kubernetes' internal DNS server, I need to set the DNS_ADDRS env var as an empty string. That way, the -d option of ss-server won't be used, and the DNS server IP address will be fetched from /etc/resolv.conf.
See the documentation of ss-server:
-d <addr>
Setup name servers for internal DNS resolver (libc-ares). The default server is fetched from /etc/resolv.conf.
The bug is that, as of today, even if you pass an empty string for DNS_ADDRS in the docker image, the -d option is still used, so you get
ERROR: failed to set nameservers.
This will be fixed in shadowsocks/shadowsocks-libev#2866
There was a problem hiding this comment.
Thanks for your explanation! Yes, this is indeed a problem. The image does not provide any mechanism to reset the -d option to its default behavior but instead using 8.8.8.8,8.8.4.4 as its default value. Let's see what is the suggestion from the maintainer.
There was a problem hiding this comment.
Hi @predatorray! The fix for the Docker image was merged in shadowsocks/shadowsocks-libev#2866. The proposed change for the Helm Chart is now relevant 🙂 I am using it in production, and it works like a charm!
There was a problem hiding this comment.
Thanks @alexandre-abrioux for your efforts to push this feature forward. Considering it as an incompatible change, since setting DNS_ADDRS to be an empty will fail using the latest stable image, let's wait for the release of a new stable image for this and change the appVersion in the Chart.yaml accordingly.
This PR makes sure that an empty array
[]for the value ofshadowsocks.dnsServerswill create an emptyDNS_ADDRSenvironment variable.