Merge lp:~ahasenack/charms/precise/postgresql/nicer-fix-to-avoid-cnames-in-pg_hba into lp:charms/postgresql

Proposed by Andreas Hasenack
Status: Work in progress
Proposed branch: lp:~ahasenack/charms/precise/postgresql/nicer-fix-to-avoid-cnames-in-pg_hba
Merge into: lp:charms/postgresql
Diff against target: 23 lines (+2/-6)
1 file modified
hooks/ (+2/-6)
To merge this branch: bzr merge lp:~ahasenack/charms/precise/postgresql/nicer-fix-to-avoid-cnames-in-pg_hba
Reviewer Review Type Date Requested Status
Cory Johns Needs Information
Stuart Bishop Needs Information
Review via email:

Commit message

Use a much cheaper call to gethostbyname() instead of calling out to "dig" to avoid using CNAMEs in pg_hba.conf.

Description of the change

Use a much cheaper call to gethostbyname() instead of calling out to "dig" to avoid using CNAMEs in pg_hba.conf.

To post a comment you must log in.
Stuart Bishop (stub) wrote :

This is going to be a permanent fix as I understanding, as charms supoprting MaaS that need this sort of IP filtering (Most services? Few services?) will be required to do the CNAME dereferencing themselves. This will mean MaaS needs to guarantee that the CNAME will never be changed, and always point to the same A record. This still allows MaaS to change the IP addresses of machines, as the CNAME points to an A record and the IP address associated with that A record may be changed.

For a permanent fix, I don't think we should have the bare except in here. I think that if the DNS lookups fail, it is a good reason to have the hook fail.

This code isn't quite doing the same thing as the dig lookup. The dig lookup is only dereferencing CNAMES to an A record. The address this A record points to can change, and everything will still be happy. The new lookup deferences both CNAMES and A records to IP addresses. If a juju provider supports changing the IP address of a machine it could do so by giving a DNS name rather than an IP address as the private-address. It can then change where that name points to without needing to kick off all the -changed hooks, as the standard DNS TTL mechanism will sort things out. If a juju provider did this, it would break the proposed use of socket.gethostbyname to replace dig.

I'd vote to leave it like it is, and just remove the exception handler since the dig lookup seems stable. This is not performance critical.

review: Needs Information
Andreas Hasenack (ahasenack) wrote :

Apparently things changed overnight. Now the MAAS team is saying they are fixing the bug. Let's put this on hold.

Stuart Bishop (stub) wrote : means my worries may be unfounded.

Cory Johns (johnsca) wrote :

Touching base on this, since it hasn't been updated in some time. The MAAS bug is now in Fixed Released, and the juju-core bug is In Progress.

My understanding is that this proposal is now essentially moot and we should be able to (now or soon) remove the workaround entirely. Is this correct? Should this MP be closed?

review: Needs Information
Andreas Hasenack (ahasenack) wrote :

MAAS 1.6 doesn't use CNAMEs anymore, this can be dropped.

Andreas Hasenack (ahasenack) wrote :

In fact, I should probably make a new MP to drop the current code that calls out to dig, but let me run that test case again first to be sure.

Unmerged revisions

86. By Andreas Hasenack on 2014-02-05

Since we are returning an IP this time, mask it with /32.

85. By Andreas Hasenack on 2014-02-05

Use gethostbyname instead of calling out to dig, much simpler/better.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'hooks/'
2--- hooks/ 2014-02-03 20:56:40 +0000
3+++ hooks/ 2014-02-05 20:52:52 +0000
4@@ -476,17 +476,13 @@
5 return "%s/32" % addr
6 except socket.error:
7 # It's not an IP address.
8- # XXX workaround for MAAS bug
9- #
10 # If it's a CNAME, use the A record it points to.
11 # If it fails for some reason, return the original address
12 try:
13- output = run("dig +short -t CNAME %s" % addr, True).strip()
14+ remote_ip = socket.gethostbyname(addr)
15 except:
16 return addr
17- if len(output) != 0:
18- return output.rstrip(".") # trailing dot
19- return addr
20+ return "%s/32" % remote_ip
22 config_data = hookenv.config()
23 allowed_units = set()


People subscribed via source and target branches