Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle empty array for shadowsocks.dnsServers #2

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Handle empty array for shadowsocks.dnsServers #2

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b7d9623
Handle empty array for shadowsocks.dnsServers
alexandre-abrioux Dec 8, 2021
cf1c347
Merge branch 'main' into quote-dns
alexandre-abrioux Dec 16, 2021
862b908
Merge branch 'main' into quote-dns
alexandre-abrioux Apr 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions templates/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,8 @@ spec:
key: {{ .existingSecret.passwordKey }}
{{- end }}
{{- end }}
{{- with .Values.shadowsocks.dnsServers }}
- name: DNS_ADDRS
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@alexandre-abrioux alexandre-abrioux Dec 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@alexandre-abrioux alexandre-abrioux Dec 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

value: {{ join "," . }}
{{- end }}
value: {{ .Values.shadowsocks.dnsServers | join "," | quote }}
{{- with .Values.shadowsocks.timeout }}
- name: TIMEOUT
value: {{ quote . }}
Expand Down