SettingsAddress should take the private-address from the default binding #7707

wupeka · 2017-08-07T11:45:17Z

Description of change

SettingsAddress must work in the same way as unit-get private-address - that is be space aware and take the address from the space for the default binding, not the machine PrivateAddress

QA steps

In an environment with spaces, deploy an app "app1" with multiple spaces and default binding set to a space NOT containing the machines' private address.
Deploy app "app2" and relate it to app1.
Then:

juju run --unit app1/0 'unit-get private-address'
should give the same result as 
juju run --unit app2/0 'relation-get -r `relation-ids RELATION_NAME` - app1/0'

Documentation changes

None

Bug reference

https://bugs.launchpad.net/juju/+bug/1708492

…ing space, not the machines private address

dshcherb · 2017-08-07T15:29:07Z

Bootstrapped a controller from scratch - could not reproduce the issue with a patched version.

juju run --unit memcached/0 'relation-get -r relation-ids cache - gnocchi/0'
private-address: 10.232.5.25

dshcherb · 2017-08-07T12:09:05Z

state/relationunit.go

-// the unit, but if this is a cross-model relation then it will be the
-// public address. If this is cross-model and there's no public
-// address for the unit, return an error.
+// of this relation. Generally this will be the address of the unit in


Maybe we can rephrase the whole comment a little bit.
If a default space binding is specified, then this address will be a primary address of the unit in the default binding space. Otherwise, it is less well-defined and private address logic is used to find a value for this setting. If the relation is a cross-model relation then public address of the unit is returned if present or an error indicating that it is not.

Not sure if we can guarantee a primary-address here though. We are taking the first one ([0]) in an array of addresses.

dshcherb · 2017-08-07T12:11:07Z

state/relationunit.go

+		if !ok || len(info.NetworkInfos) == 0 || len(info.NetworkInfos[0].Addresses) == 0 {
+			privateAddress, err := unit.PrivateAddress()
+			if err == nil {
+				logger.Warningf("Can't find an address for default binding space %q, falling back to units' private address %q", space, privateAddress.String())


Cannot find an address in a default binding space, a unit's private address will be used instead.

jameinel · 2017-08-09T08:49:26Z

Would a better test for this be to deploy 2 applications to the same machine, with inverted spaces? eg:

juju deploy app1 --to 0 --bind space-a
juju deploy app2 --to 0 --bind space-b
[ $(juju run --unit app1/0 'unit-get private-address') = $(juju run --unit app2/0 'relation-get -r `relation-ids RELATION_NAME` - app1/0') ]
[ $(juju run --unit app2/0 'unit-get private-address') = $(juju run --unit app1/0 'relation-get -r `relation-ids RELATION_NAME` - app2/0') ]

By doing the bidirectional test, you know that at least one of them cannot be the machine's private address.

I think you also need to do a test that the app1's address is in space-a, and the app2's address is in space-b.
(I'm mostly commenting on this so that we can write up a nice CI test for this to make sure we have the right fix, and that we don't regress.)

jameinel · 2017-08-09T08:51:08Z

state/relationunit.go

@@ -482,7 +483,26 @@ func (ru *RelationUnit) SettingsAddress() (network.Address, error) {
 	if crossmodel, err := ru.relation.IsCrossModel(); err != nil {
 		return network.Address{}, errors.Trace(err)
 	} else if !crossmodel {
-		return unit.PrivateAddress()
+		space, err := unit.GetSpaceForBinding("")


This feels big enough that we should probably pull it out into a separate helper function (possibly exported for direct testing.)

Also, this shouldn't be the default space (I think). I think it should be the space for the binding on this end of the relation.

for example, if you do:
juju deploy app1 --space "bar endpt1=foo"

then
juju relate app1:endpt1 app2
should expose 'foo' as the private address, while if you do:
juju relate app1:endpt2 app3
that should expose 'bar' as the private address.

I think the right value is:
ru.endpoint.Relation.Name
(it embeds, so it might also just be ru.endpoint.Name)

Given this, we probably need more tests, including: juju deploy app --space "1 A=2 B=3"
and then various relations between those and other applications.

Also, this shouldn't be the default space (I think). I think it should be the space for the binding on this end of the relation.

Terminology-wise I think we should have a good (and short) way to say that. I agree that we should mention the side as well.

Here we have:
"default option for any interfaces not specified"
"default-space"
https://jujucharms.com/docs/2.2/charms-deploying#deploying-to-spaces

I used the following instead:

a default space binding ~ when talking about a binding and the fact that we have an ability to specify a default binding for all endpoints of an application for which there is no explicit binding;
a default binding space ~ when talking about a space specifically, not a binding, i.e. "a space that I have used in a default binding".

The term "default space" is kind of confusing: one might think that this space can be applied bundle-wise. I think this is something that could be added as a model property (not sure if it fits into the current data model we have) - any charms without a default binding would get one from a model property. The problem is that not all providers support network spaces. We have a workaround for that with yaml aliases but this gets cumbersome as we have to specify that for all charms.

wupeka · 2017-09-05T13:24:36Z

$$merge$$

jujubot · 2017-09-05T13:26:05Z

Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju

SettingsAddress should take the private-address from the default bind…

6a38574

…ing space, not the machines private address

dshcherb reviewed Aug 8, 2017

View reviewed changes

jameinel requested changes Aug 9, 2017

View reviewed changes

Merge branch '2.2' into 2.2-fix-SettingsAddress-in-spaced-environs

1d5a2bf

jujubot merged commit ba58f43 into juju:2.2 Sep 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SettingsAddress should take the private-address from the default binding #7707

SettingsAddress should take the private-address from the default binding #7707

wupeka commented Aug 7, 2017 •

edited by jameinel

dshcherb commented Aug 7, 2017

dshcherb Aug 7, 2017

dshcherb Aug 7, 2017

dshcherb Aug 7, 2017

jameinel commented Aug 9, 2017

jameinel Aug 9, 2017

jameinel Aug 9, 2017

jameinel Aug 9, 2017

dshcherb Aug 11, 2017

wupeka commented Sep 5, 2017

jujubot commented Sep 5, 2017

SettingsAddress should take the private-address from the default binding #7707

SettingsAddress should take the private-address from the default binding #7707

Conversation

wupeka commented Aug 7, 2017 • edited by jameinel

Description of change

QA steps

Documentation changes

Bug reference

dshcherb commented Aug 7, 2017

dshcherb Aug 7, 2017

Choose a reason for hiding this comment

dshcherb Aug 7, 2017

Choose a reason for hiding this comment

dshcherb Aug 7, 2017

Choose a reason for hiding this comment

jameinel commented Aug 9, 2017

jameinel Aug 9, 2017

Choose a reason for hiding this comment

jameinel Aug 9, 2017

Choose a reason for hiding this comment

jameinel Aug 9, 2017

Choose a reason for hiding this comment

dshcherb Aug 11, 2017

Choose a reason for hiding this comment

wupeka commented Sep 5, 2017

jujubot commented Sep 5, 2017

wupeka commented Aug 7, 2017 •

edited by jameinel