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.

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

CODENVY-1749: add possibility to set DNS resolver #1771

Merged
merged 8 commits into from
Feb 15, 2017

Conversation

garagatyi
Copy link
Contributor

@garagatyi garagatyi commented Feb 13, 2017

What does this PR do?

Add possibility to set DNS resolving servers.
If Codenvy is set to use custom DNS that is resolvable using custom DNS resolving server admin should set env var: CODENVY_DNS_RESOLVERS
It may contain several values in comma-separated format, e.g.

CODENVY_DNS_RESOLVERS=10.10.10.10,8.8.8.8

What issues does this PR fix or reference?

Fixes #1749

Changelog

Add possibility to set DNS resolving servers.

Release Notes

We now let you override the default DNS resolvers that are created in your workspace runtimes. In Codenvy, each workspace is given a set of runtimes. Agents and other services that run within those runtimes inherit networking properties from Che including proxy and security configuration. In some environments, custom resolution of DNS entries (usually to an internal DNS provider) is required to enable the workspace runtimes to have lookup ability from services within the workspace. You can now add CODENVY_DNS_RESOLVERS=<ip-list> to codenvy.env to customize the DNS injection into default containers. The default behavior is to inherit the list of DNS resolver servers from the host.

Docs PR

??

@garagatyi
Copy link
Contributor Author

@codenvy/pm Please review

@@ -272,6 +272,10 @@
# please leave this as it is if you don't need no_proxy configuration
$no_proxy_for_codenvy_workspaces = getValue("CODENVY_NO_PROXY_FOR_CODENVY_WORKSPACES","")

################################
# DNS resolver configuration
$dns_resolvers = getValue("CODENVY_DNS_RESOLVERS","")

Choose a reason for hiding this comment

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

default value must be 'NULL' because you check on 'NULL' in template

Copy link
Contributor Author

Choose a reason for hiding this comment

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

10x

@@ -62,8 +62,7 @@ error_log /var/log/nginx/error.log error;
#gzip_proxied expired no-cache no-store private auth;
#gzip_types text/plain text/css text/xml text/javascript application/x-javascript application/xml;

resolver 127.0.0.1 8.8.8.8 ipv6=off;

resolver <% if @dns_resolvers.empty? -%>8.8.8.8<% else -%><%= @dns_resolvers.gsub(',', ' ') %><% end -%> ipv6=off;

Choose a reason for hiding this comment

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

you shoul che on != 'NULL' same as in compose file if @dns_resolvers != 'NULL'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

10x

@JamesDrummond
Copy link

Needs docs. Waiting on associated Che doc review before creating it for codenvy.

@garagatyi
Copy link
Contributor Author

@riuvshin I fixed PR.
@TylerJewell I commented docs in Che PR. Please consider changes in docs and port them into this PR also.

@@ -62,8 +62,7 @@ error_log /var/log/nginx/error.log error;
#gzip_proxied expired no-cache no-store private auth;
#gzip_types text/plain text/css text/xml text/javascript application/x-javascript application/xml;

resolver 127.0.0.1 8.8.8.8 ipv6=off;

resolver <% if @dns_resolvers != 'NULL' -%>8.8.8.8<% else -%><%= @dns_resolvers.gsub(',', ' ') %><% end -%> ipv6=off;

Choose a reason for hiding this comment

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

if statement look strange maybe I was confused you by asking check != null
but seems in that case it should looks like
<% if @dns_resolvers == 'NULL' -%>8.8.8.8<% else -%><%= @dns_resolvers.gsub(',', ' ') %><% end -%>
otherwise 8.8.8.8 will never be by defaul

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thx

@garagatyi
Copy link
Contributor Author

@riuvshin I had to rework PR, so now empty string is used instead of NULL. It is needed because Java code can't interpret NULL as empty array

@riuvshin
Copy link

@garagatyi I saw this and approved

@garagatyi garagatyi merged commit 16bdd83 into codenvy:master Feb 15, 2017
@garagatyi garagatyi deleted the CODENVY-1749 branch February 15, 2017 10:48
@bmicklea
Copy link
Contributor

@garagatyi was the merged to 5.3? There's no milestone set.

@garagatyi garagatyi added this to the 5.3.0 milestone Feb 15, 2017
@bmicklea bmicklea changed the title CODENVY-1749: add possibility to set dns resolver CODENVY-1749: add possibility to set DNS resolver Feb 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Respect dns options of a Docker daemon for workspaces
5 participants