juju: use new DNS caching functionality #7438

Merged
merged 1 commit into from Jun 2, 2017

Conversation

Projects
None yet
4 participants
Owner

rogpeppe commented Jun 1, 2017

juju: use new DNS caching functionality

We rely on the new API client functionality to do our
DNS resolving for us, and store any resulting cached
names in the controllers.yaml entry.

The UnresolvedAPIEndpoints field is now redundant
and is replaced by the strictly advisory DNSCache
field. The APIEndpoints field now always contains all
the host names returned from the Login call with
the exception of addresses deemed unusable
(e.g. link-local addresses).

We also change rpcreflect.Value.FindMethod to accept any
version so that it's more useful for testing - this makes it
possible to server limited portions of the juju API from
a mocked API type without implementing a custom FindMethod.
And, in passing, fix rpc.NewConn so that it works with a nil obververFactory
as documented.

QA check that API connections still work OK, particularly on
MAAS setups where some of the returned host names will
not resolve on a client.

Also, check that:

juju register jaas
juju list-models

does not produce the "cannot validate certificate for because it
doesn't contain any IP SANs" error.

Fixes https://bugs.launchpad.net/juju/+bug/1692905

Owner

rogpeppe commented Jun 1, 2017

!!build!!

axw approved these changes Jun 2, 2017

very nice, just a few nits

@@ -0,0 +1,24 @@
+package testing
@axw

axw Jun 2, 2017

Member

copyright

@@ -0,0 +1,38 @@
+package testing
@axw

axw Jun 2, 2017

Member

copyright

- fakeUUID,
+ dialed := make(chan string, 10)
+ start := make(chan struct{})
+ // Wait for both dials to complete, so we
@axw

axw Jun 2, 2017

Member

neat :)

juju/api_test.go
+ })
+ c.Assert(err, jc.ErrorIsNil)
+ start := make(chan struct{})
+ // Wait for both dials to complete, so we
@axw

axw Jun 2, 2017

Member

this comment isn't right

mhilton approved these changes Jun 2, 2017

LGTM with some thoughts

juju/api.go
-// addrsChanged returns true iff the two
-// slices are not equal. Order is important.
+// addrsChanged reports whether the two slices
+// are not equal. Order is important.
@mhilton

mhilton Jun 2, 2017

Member

s/are not equal/are different/ ?

func (v Value) FindMethod(rootMethodName string, version int, objMethodName string) (MethodCaller, error) {
if !v.IsValid() {
panic("FindMethod called on invalid Value")
}
caller := methodCaller{
rootValue: v.rootValue,
}
- if version != 0 {
@mhilton

mhilton Jun 2, 2017

Member

This seems like a bold change to make under the guise of DNS caching. It's probably a good idea, but its effects are far reaching. This should probably have its own PR.

@rogpeppe

rogpeppe Jun 2, 2017

Owner

Value.FindMethod is only called once in the production code, with a zero version.
It's effects shouldn't be far reaching, but you're probably right that it should be
a separate PR.

@rogpeppe

rogpeppe Jun 2, 2017

Owner

Nonetheless I'm going to land this anyway, as we need the fix in and I'm sure it's OK to do.

// appropriate method will be called for every RPC request.
func NewConn(codec Codec, observerFactory ObserverFactory) *Conn {
+ if observerFactory == nil {
@mhilton

mhilton Jun 2, 2017

Member

thanks for fixing this one though.

juju: use new DNS caching functionality
We rely on the new API client functionality to do our
DNS resolving for us, and store any resulting cached
names in the controllers.yaml entry.

The UnresolvedAPIEndpoints field is now redundant
and is replaced by the strictly advisory DNSCache
field. The APIEndpoints field now always contains all
the host names returned from the Login call with
the exception of addresses deemed unusable
(e.g. link-local addresses).

We also change rpcreflect.Value.FindMethod to accept any
version so that it's more useful for testing - this makes it
possible to server limited portions of the juju API from
a mocked API type without implementing a custom FindMethod.
And, in passing, fix rpc.NewConn so that it works with a nil obververFactory
as documented.

QA check that API connections still work OK, particularly on
MAAS setups where some of the returned host names will
not resolve on a client.

Also, check that:

	juju register jaas
	juju list-models

does not produce the "cannot validate certificate for because it
doesn't contain any IP SANs" error.

Fixes https://bugs.launchpad.net/juju/+bug/1692905
Owner

rogpeppe commented Jun 2, 2017

$$merge$$

Contributor

jujubot commented Jun 2, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

Contributor

jujubot commented Jun 2, 2017

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/11078

@jujubot jujubot merged commit 8cb1f45 into juju:develop Jun 2, 2017

1 check failed

github-check-merge-juju Built PR, ran unit tests, and tested LXD deploy. Use !!.*!! to request another build. IE, !!build!!, !!retry!!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment