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 resolvers #4129

Merged
merged 5 commits into from
Feb 15, 2017

Conversation

garagatyi
Copy link

@garagatyi garagatyi commented Feb 13, 2017

What does this PR do?

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

CHE_DNS_RESOLVERS=10.10.10.10,8.8.8.8

What issues does this PR fix or reference?

Related to codenvy/codenvy#1749

Changelog

Add possibility to set custom DNS resolvers for workspace (master too) container runtimes.

Release Notes

We now let you override the default DNS resolvers used by Che and you workspace runtimes. The default behavior is for Che and workspaces to inherit DNS resolution from the host. You can add CHE_DNS_RESOLVERS=<ip-list> to che.env to customize how DNS resolution will occur within the Che server and workspace containers. Services such as agents and servers running within Che workspaces will usually require custom DNS resolution when there are internal providers behind a proxy or firewall.

Docs PR

eclipse-che/che-docs#139

@garagatyi garagatyi added the kind/enhancement A feature request - must adhere to the feature request template. label Feb 13, 2017
@garagatyi
Copy link
Author

@bmicklea @eivantsov @JamesDrummond Please review.

@TylerJewell
Copy link

Let's change it to be CHE_DNS_RESOLVERS inside of che.env.

If you can provide an explanation of what this does (or when an admin would need to configure this) then I can write release notes and identify docs changes that are needed.

Is there a matching codenvy PR?

@TylerJewell
Copy link

@garagatyi - I will review this - this will not need @bmicklea or @JamesDrummond review.

Copy link
Contributor

@riuvshin riuvshin left a comment

Choose a reason for hiding this comment

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

LGTM from configuration point of view

@codenvy-ci
Copy link

@JamesDrummond
Copy link
Contributor

JamesDrummond commented Feb 13, 2017

I think this needs docs. I created PR for docs that is in review.

@JamesDrummond JamesDrummond self-requested a review February 13, 2017 23:56
@TylerJewell
Copy link

@JamesDrummond - the dock PR links to a che PR - it's not a docs PR.

I am not sure this requires docs - we don't document every configuration property in or docs.

Signed-off-by: Alexander Garagatyi <[email protected]>
@garagatyi
Copy link
Author

@TylerJewell I updated env variable name. Also I added my comments for docs in bold, please take a look.

@garagatyi
Copy link
Author

Also not all the parts of the system inherit DNS resolution rules from host. Nginx container doesn't respect that and use 8.8.8.8 for dns resolution. So user might be surprised if by default we inherit from host but WSs don't start because of Nginx.

@garagatyi
Copy link
Author

@JamesDrummond If you still need docs please use my comments from this PR to change explanation in your PR.

@codenvy-ci
Copy link

Alexander Garagatyi added 2 commits February 14, 2017 11:13
@garagatyi
Copy link
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

@garagatyi
Copy link
Author

@TylerJewell please take a look at release notes. I added my comments into it.

@TylerJewell
Copy link

Ok for you to merge. I can fix release notes after merge when have time to review.

@JamesDrummond
Copy link
Contributor

@TylerJewell I updated the docs PR link. I somehow copied the wrong url. I think documentation on this would be helpful to those evaluating the product features.

@garagatyi What is master that you refer to above?

@codenvy-ci
Copy link

@garagatyi
Copy link
Author

@JamesDrummond I meant ws-master aka che-server

@JamesDrummond
Copy link
Contributor

JamesDrummond commented Feb 14, 2017

@garagatyi I rewrote the docs to include che server. https://github.com/eclipse/che-docs/pull/139/files#diff-ba9f2804ca5dc20e010ba41f64ef1cac . Please review.

@TylerJewell
Copy link

I updated the release notes based upon @garagatyi comments

@garagatyi garagatyi merged commit 6032c59 into eclipse-che:master Feb 15, 2017
@garagatyi garagatyi deleted the CODENVY-1749 branch February 15, 2017 10:47
@JamesDrummond JamesDrummond added this to the 5.3.0 milestone Feb 17, 2017
@JamesDrummond JamesDrummond mentioned this pull request Feb 17, 2017
9 tasks
JPinkney pushed a commit to JPinkney/che that referenced this pull request Aug 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A feature request - must adhere to the feature request template.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants