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

dnsmasq integration #194

Merged
merged 2 commits into from
May 9, 2015
Merged

dnsmasq integration #194

merged 2 commits into from
May 9, 2015

Conversation

subnetmarco
Copy link
Member

This pull request fixes issue #182, #163 and #127.

The nginx resolver property specifies which DNS server nginx will use when resolving addresses.

The problem with the resolver is that by design nginx doesn't follow the system settings including the /etc/hosts file. In order to have a proper DNS resolution that respects the system DNS settings a new dependency needs to be introduces: dnsmasq.

dnsmasq is a DNS server that runs locally, can listen on a port (I decided to use 8053 for Kong), and can provide DNS resolution reading both /etc/hosts file and resolv.conf. It is the same dependency that we use for the official Kong Dockerfile (https://github.com/Mashape/docker-kong/blob/master/Dockerfile#L17).

I have been searching for another solution but everybody seems to be using dnsmasq for this use-case.

So this now works, and if in the future a better way to handle this issue will be found, I will be more than happy to remove the dependency to dnsmasq (which works very well by the way).

I will also update the distributions in a future commit to include dnsmasq as a dependency.

@montanaflynn
Copy link

Do you think this may also fix #195?

@subnetmarco
Copy link
Member Author

@montanaflynn not sure, we need to replicate and test that separately after this pull request has been merged into master

@montanaflynn
Copy link

I've been unable to replicate it, maybe he could build the new version from source and test for us.

@subnetmarco
Copy link
Member Author

This pull request is not finalized, I still need to implement some fixes.

@thibaultcha
Copy link
Member

It looks ok so far, except minor things I'll probably update for the CLI.

    1. I am very sad, we keep adding more and more dependencies to Kong, and they become more and more expansive (dnsmasq requires a port and runs as yet another process, sigh...)
    1. I tried playing with lua-resty-dns with no success so far, so I see no other solution.

On a side note:
We should stop relying on the Faker, as it makes the test data very sparse. Some tests end up using the same APIs as others tests and we lose isolation.

We need to use the spec_helper to pass directly through the DAO, and insert only the needed for the current tests at setup. And the same APIs need to be dropped at teardown.

@@ -33,6 +33,9 @@ Faker.FIXTURES = {
{ name = "API TESTS 6", public_dns = "cors1.com", target_url = "http://mockbin.com" },
{ name = "API TESTS 7", public_dns = "cors2.com", target_url = "http://mockbin.com" },

{ name = "API TESTS 8 (dns)", public_dns = "dns1.com", target_url = "http://127.0.0.1:7771" },
{ name = "API TESTS 9 (dns)", public_dns = "dns2.com", target_url = "http://localhost:7771" },
Copy link
Member Author

Choose a reason for hiding this comment

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

@thibaultcha did you mean like this? The name is unfortunate but I am testing DNS resolution so dns1.com and dns2.com make sense here.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it's nice it's enough to know it's for the DNS. We'll move them out of the faker and to their own respective tests later.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 66.31% when pulling 845bf59 on fix/dns into 8acfe1d on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 66.31% when pulling 845bf59 on fix/dns into 8acfe1d on master.

@subnetmarco
Copy link
Member Author

@thibaultcha if it looks good I can merge

@thibaultcha
Copy link
Member

I'm having an error when I try to run it:

kong|fix/dns ⇒ bin/kong start
[WARN] No configuration at: /etc/kong/kong.yml using default config instead.
[INFO] Using configuration: /usr/local/lib/luarocks/rocks/kong/0.2.1-1/conf/kong.yml
[INFO] Proxy port.........8000
       Admin API port.....8001
       dnsmasq port.......8053
       Database...........cassandra keepalive=60000 hosts=127.0.0.1 port=9042 timeout=1000 keyspace=kong
[INFO] Connecting to the database...
[ERR] /bin/bash: /usr/local/sbin/dnsmasq: No such file or directory

@subnetmarco
Copy link
Member Author

@thibaultcha yes, I need to update the distributions but that is in another branch. The new distributions will include dnsmasq.

Also I will need to update the website to list the new requirement, and to document a new configuration property dnsmasq_port.

@thibaultcha
Copy link
Member

Yeah makes sense, it's kinda late here lol, ok nothing to say otherwise, sounds good

@subnetmarco
Copy link
Member Author

Adding some tests to increase the coverage, then I will merge.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 66.51% when pulling 8219762 on fix/dns into 8acfe1d on master.

subnetmarco added a commit that referenced this pull request May 9, 2015
@subnetmarco subnetmarco merged commit 538c3a3 into master May 9, 2015
@subnetmarco subnetmarco deleted the fix/dns branch May 9, 2015 01:45
@subnetmarco
Copy link
Member Author

Related to Kong/docs.konghq.com@7c54540

ctranxuan pushed a commit to streamdataio/kong that referenced this pull request Aug 25, 2015
hutchic pushed a commit that referenced this pull request Jun 10, 2022
Co-authored-by: Renovate Bot <renovatebot@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants