Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
network-get hook tool becomes relation aware #7760
Conversation
wallyworld
requested a review
from
jameinel
Aug 18, 2017
| + Bindings: bindings, | ||
| + RelationId: relationId, | ||
| + } | ||
| + if u.st.BestAPIVersion() < 7 { |
axw
Aug 23, 2017
Member
is this even possible? the controllers have to upgrade before models can upgrade
wallyworld
Aug 24, 2017
Owner
It may not be possible. It's just a sanity check to ensure that unexpected fields aren't sent to the v6 api, just in case we get strict about that at some point. The code here could very well be omitted, but I thought it reasonable to have as it also serves to highlight the v6 vs v7 difference.
axw
Aug 25, 2017
Member
I'd rather not carry unreachable code, particularly when it suggests compatibility requirements that we don't have. It'll end up getting copied.
| @@ -1599,6 +1616,12 @@ func (u *UniterAPI) SLALevel() (params.StringResult, error) { | ||
| } | ||
| // NetworkInfo returns network interfaces/addresses for specified bindings. | ||
| +func (u *UniterAPIV6) NetworkInfo(args params.NetworkInfoParams) (params.NetworkInfoResults, error) { |
wallyworld
Aug 24, 2017
Owner
The semantics are different and that's why we also need the version bump. NetworkInfoParams gains an optional RelationId attribute. We don't want callers expecting V7 behaviour to accidentally call a v6 API. The behaviour between V6 and v7 is indeed identical if the relation id is omitted when calling v7. So that's why v6 NetworkInfo simply delegates to v7 NetworkInfo.
axw
Aug 25, 2017
Member
The semantics aren't different, though, precisely because you're just calling the other one without any additional validation. As it is, you may as well not have this, and just allow the V7 method to be exposed to V6 clients. If you add args.RelationId = nil here, it would make it equivalent to the old behaviour.
| @@ -1628,6 +1651,19 @@ func (u *UniterAPI) NetworkInfo(args params.NetworkInfoParams) (params.NetworkIn | ||
| if err != nil { | ||
| return params.NetworkInfoResults{}, err | ||
| } | ||
| + defaultSpaceAddress, spaceAddressErr := machine.PrivateAddress() |
axw
Aug 23, 2017
Member
do this only if args.RelationId == nil? it's a bit wasteful to perform a DB lookup if we're not using the result
jameinel
Aug 23, 2017
Owner
There is no DB access here, it is part of the machine.doc already. PrivateAddress is already an attribute. The only error that can be generated is if the value of the object is "".
| + if err != nil { | ||
| + return params.NetworkInfoResults{}, err | ||
| + } | ||
| + defaultSpaceAddress, spaceAddressErr = ru.SettingsAddress() |
axw
Aug 23, 2017
Member
I think we should rename SettingsAddress. It looked alarmingly like you were pulling the address out of the unit's relation settings. Not sure what's a better name.
jameinel
Aug 23, 2017
Owner
For what its worth, I don't really like that we're re-using a variable named "defaultSpaceAddress" when it isn't the address in the default space (which is more what machine.PrivateAddress would be)
wallyworld
Aug 24, 2017
Owner
Perhaps a better name for the method would be "ExternalAddress". Since that's what we're really getting at here - what address to external callers use to contact this unit. Another option could be "IngressAddress"?
I called it defaultSpaceAddress because the GetNetworkInfoForSpaces method uses it in the network info result for the default space.
| @@ -1645,7 +1681,18 @@ func (u *UniterAPI) NetworkInfo(args params.NetworkInfoParams) (params.NetworkIn | ||
| } | ||
| } | ||
| - networkInfos := machine.GetNetworkInfoForSpaces(spaces) | ||
| + // Don't get the network info for the default space if there was an error |
axw
Aug 23, 2017
Member
why?
also, does it necessarily hold true that it's the default space for cross-model?
jameinel
Aug 23, 2017
Owner
I'm not sure what kind of error you would get for the default address that he's concerned about.
It does feel a little weird. And while I could see not loading an error if there was a problem with machine.PrivateAddress(), if you did call ru.SettingsAddress() then it is no longer the same space as the "DefaultSpaceName". which means you're taking an error and using it to modify something else, which doesn't feel quite right.
wallyworld
Aug 24, 2017
Owner
The logic is there to maintain existing behaviour - I was careful to retain the semantics of the GetNetworkInfoForSpaces method. The only difference is that we pass in the address for the default space instead of having the method get it via machine.PrivateAddress().
| + defaultSpaceNetworkErr = errors.Annotatef(err, "getting machine %q preferred primary address", machine.MachineTag()) | ||
| + } | ||
| + | ||
| + networkInfos := machine.GetNetworkInfoForSpaces(spaces, defaultSpaceAddress) |
axw
Aug 23, 2017
Member
it feels wrong to be passing an address into machine.GetNetworkInfoForSpaces. can we just remove the default space name from the spaces set, and add the address to the result? I'm not even sure if it makes sense to say that the cross-model relation address is in the default space. might need jam to weigh in here...
jameinel
Aug 23, 2017
Owner
Presumably this is because we want to override the 'default space address'. I feel like this loop becomes invalid with Ian's change:
if spaces.Contains(environs.DefaultSpaceName) && defaultSpaceAddress.Value == addr.Value() {
r := results[environs.DefaultSpaceName]
r.NetworkInfos, err = addAddressToResult(r.NetworkInfos, addr)
if err != nil {
r.Error = err
} else {
results[environs.DefaultSpaceName] = r
}
}
Because if you're using a Public address, that is likely not part of the machine description, and thus no addr.Value can == defaultSpaceAddress.Value
It would seem that the better fix would be to just call machine.GetNetworkInfoForSpaces, and then after that call set results[DefaultSpaceName] = OverriddenDefaultAddress
or something to that effect.
I'll still consider whether we should be changing the address for the "default space" because of a current relation. We should be changing the default value of the private address, certainly, but it doesn't feel like we're should be changing the address for the "default space" in what we send to the charm.
Do we know where the data is being used to just take the default value? Maybe it is unit-get private-address (which doesn't support bindings).
Anyway, I agree with Andrew that changing Machine.GetNetworkInfoForSpaces doesn't feel like the correct fix.
We only know about CMR in the context of a relation, and its a little odd that a relation would have context on other bindings that are not its own relation. So I'm not positive where it would sit. I would tend to do something with Relation.GetNetworkInfo, which could call down to its Unit which would talk to the machine.
But it doesn't feel like we should be jumping through those layers to go directly to the machine with CMR information.
wallyworld
Aug 24, 2017
Owner
A concrete case I have seen is a charm calling "unit-get private-address". But network-get also needs to be relation aware as well. The address for the default space does then become contextually dependent on whether it's a cross model relation, so long as the relation endpoint is bound to that space (I think that's the issue here? - we need to consider the endpoint binding and only if the binding is omitted or explicitly specifies the default space do we need to provide the calculated address?)
| @@ -214,6 +214,16 @@ func (f *contextFactory) HookContext(hookInfo hook.Info) (*HookContext, error) { | ||
| relation.cache.InvalidateMember(hookInfo.RemoteUnit) | ||
| } | ||
| hookName = fmt.Sprintf("%s-%s", relation.Name(), hookInfo.Kind) | ||
| + // Update the private address on the context from the relation settings |
axw
Aug 23, 2017
Member
this seems wrong, since ctx.privateAddress might be used as the basis for setting the private-address relation settings
jameinel
Aug 23, 2017
Owner
I think there are charms that override private-address. I'm guessing the idea is to not redo the logic we've already done to figure out what to use for privateAddress. But I'd rather we did something like:
PrivateAddressForHook rather than reading the relation settings to get the same content.
wallyworld
Aug 24, 2017
Owner
Right, there are 3 places where a charm could ask for "what address do I tell external callers to use to connect":
- private-address value in relation settings, set up by Juju when the unit enters scope, accessed via relation-get
- unit-get private-addtess
- network-get
Before this PR, only the first way was cmr aware. The other 2 always used machine private address. Part of the point of this PR is to have all 3 ways use common, cmr aware code.
If we used a new PrivateAddressForHook attribute on the context, are you saying that's what unit-get private-address would return etc, and if a charm did a relation-get and read the private-address value from there, the result should be either what Juju set initially, or what the charm may have overridden.
| @@ -51,11 +52,16 @@ type UniterAPI struct { | ||
| StorageAPI | ||
| } | ||
| +// UniterAPIV6 uses NetworkInfo as a replacement for NetworkConfig |
jameinel
Aug 23, 2017
Owner
This comment seems a little hard to parse. Is it more that NetworkConfig is being dropped in v7 ?
Would this be better stated as
// UniterAPIV6 adds NetworkInfo as a preferred method to calling NetworkConfig
| @@ -1628,6 +1651,19 @@ func (u *UniterAPI) NetworkInfo(args params.NetworkInfoParams) (params.NetworkIn | ||
| if err != nil { | ||
| return params.NetworkInfoResults{}, err | ||
| } | ||
| + defaultSpaceAddress, spaceAddressErr := machine.PrivateAddress() |
axw
Aug 23, 2017
Member
do this only if args.RelationId == nil? it's a bit wasteful to perform a DB lookup if we're not using the result
jameinel
Aug 23, 2017
Owner
There is no DB access here, it is part of the machine.doc already. PrivateAddress is already an attribute. The only error that can be generated is if the value of the object is "".
| + if err != nil { | ||
| + return params.NetworkInfoResults{}, err | ||
| + } | ||
| + defaultSpaceAddress, spaceAddressErr = ru.SettingsAddress() |
axw
Aug 23, 2017
Member
I think we should rename SettingsAddress. It looked alarmingly like you were pulling the address out of the unit's relation settings. Not sure what's a better name.
jameinel
Aug 23, 2017
Owner
For what its worth, I don't really like that we're re-using a variable named "defaultSpaceAddress" when it isn't the address in the default space (which is more what machine.PrivateAddress would be)
wallyworld
Aug 24, 2017
Owner
Perhaps a better name for the method would be "ExternalAddress". Since that's what we're really getting at here - what address to external callers use to contact this unit. Another option could be "IngressAddress"?
I called it defaultSpaceAddress because the GetNetworkInfoForSpaces method uses it in the network info result for the default space.
| @@ -1645,7 +1681,18 @@ func (u *UniterAPI) NetworkInfo(args params.NetworkInfoParams) (params.NetworkIn | ||
| } | ||
| } | ||
| - networkInfos := machine.GetNetworkInfoForSpaces(spaces) | ||
| + // Don't get the network info for the default space if there was an error |
axw
Aug 23, 2017
Member
why?
also, does it necessarily hold true that it's the default space for cross-model?
jameinel
Aug 23, 2017
Owner
I'm not sure what kind of error you would get for the default address that he's concerned about.
It does feel a little weird. And while I could see not loading an error if there was a problem with machine.PrivateAddress(), if you did call ru.SettingsAddress() then it is no longer the same space as the "DefaultSpaceName". which means you're taking an error and using it to modify something else, which doesn't feel quite right.
wallyworld
Aug 24, 2017
Owner
The logic is there to maintain existing behaviour - I was careful to retain the semantics of the GetNetworkInfoForSpaces method. The only difference is that we pass in the address for the default space instead of having the method get it via machine.PrivateAddress().
| + defaultSpaceNetworkErr = errors.Annotatef(err, "getting machine %q preferred primary address", machine.MachineTag()) | ||
| + } | ||
| + | ||
| + networkInfos := machine.GetNetworkInfoForSpaces(spaces, defaultSpaceAddress) |
axw
Aug 23, 2017
Member
it feels wrong to be passing an address into machine.GetNetworkInfoForSpaces. can we just remove the default space name from the spaces set, and add the address to the result? I'm not even sure if it makes sense to say that the cross-model relation address is in the default space. might need jam to weigh in here...
jameinel
Aug 23, 2017
Owner
Presumably this is because we want to override the 'default space address'. I feel like this loop becomes invalid with Ian's change:
if spaces.Contains(environs.DefaultSpaceName) && defaultSpaceAddress.Value == addr.Value() {
r := results[environs.DefaultSpaceName]
r.NetworkInfos, err = addAddressToResult(r.NetworkInfos, addr)
if err != nil {
r.Error = err
} else {
results[environs.DefaultSpaceName] = r
}
}
Because if you're using a Public address, that is likely not part of the machine description, and thus no addr.Value can == defaultSpaceAddress.Value
It would seem that the better fix would be to just call machine.GetNetworkInfoForSpaces, and then after that call set results[DefaultSpaceName] = OverriddenDefaultAddress
or something to that effect.
I'll still consider whether we should be changing the address for the "default space" because of a current relation. We should be changing the default value of the private address, certainly, but it doesn't feel like we're should be changing the address for the "default space" in what we send to the charm.
Do we know where the data is being used to just take the default value? Maybe it is unit-get private-address (which doesn't support bindings).
Anyway, I agree with Andrew that changing Machine.GetNetworkInfoForSpaces doesn't feel like the correct fix.
We only know about CMR in the context of a relation, and its a little odd that a relation would have context on other bindings that are not its own relation. So I'm not positive where it would sit. I would tend to do something with Relation.GetNetworkInfo, which could call down to its Unit which would talk to the machine.
But it doesn't feel like we should be jumping through those layers to go directly to the machine with CMR information.
wallyworld
Aug 24, 2017
Owner
A concrete case I have seen is a charm calling "unit-get private-address". But network-get also needs to be relation aware as well. The address for the default space does then become contextually dependent on whether it's a cross model relation, so long as the relation endpoint is bound to that space (I think that's the issue here? - we need to consider the endpoint binding and only if the binding is omitted or explicitly specifies the default space do we need to provide the calculated address?)
| + | ||
| + // Since it is a remote relation, private address is set to the | ||
| + // machine's public address. | ||
| + privateAddress, err := s.base.machine1.PublicAddress() |
| @@ -214,6 +214,16 @@ func (f *contextFactory) HookContext(hookInfo hook.Info) (*HookContext, error) { | ||
| relation.cache.InvalidateMember(hookInfo.RemoteUnit) | ||
| } | ||
| hookName = fmt.Sprintf("%s-%s", relation.Name(), hookInfo.Kind) | ||
| + // Update the private address on the context from the relation settings |
axw
Aug 23, 2017
Member
this seems wrong, since ctx.privateAddress might be used as the basis for setting the private-address relation settings
jameinel
Aug 23, 2017
Owner
I think there are charms that override private-address. I'm guessing the idea is to not redo the logic we've already done to figure out what to use for privateAddress. But I'd rather we did something like:
PrivateAddressForHook rather than reading the relation settings to get the same content.
wallyworld
Aug 24, 2017
Owner
Right, there are 3 places where a charm could ask for "what address do I tell external callers to use to connect":
- private-address value in relation settings, set up by Juju when the unit enters scope, accessed via relation-get
- unit-get private-addtess
- network-get
Before this PR, only the first way was cmr aware. The other 2 always used machine private address. Part of the point of this PR is to have all 3 ways use common, cmr aware code.
If we used a new PrivateAddressForHook attribute on the context, are you saying that's what unit-get private-address would return etc, and if a charm did a relation-get and read the private-address value from there, the result should be either what Juju set initially, or what the charm may have overridden.
| -} | ||
| - | ||
| -func (u *Unit) NetworkInfo(bindings []string) (map[string]params.NetworkInfoResult, error) { | ||
| +func (u *Unit) NetworkInfo(bindings []string, relationId *int) (map[string]params.NetworkInfoResult, error) { |
| + Bindings: bindings, | ||
| + RelationId: relationId, | ||
| + } | ||
| + if u.st.BestAPIVersion() < 7 { |
axw
Aug 23, 2017
Member
is this even possible? the controllers have to upgrade before models can upgrade
wallyworld
Aug 24, 2017
Owner
It may not be possible. It's just a sanity check to ensure that unexpected fields aren't sent to the v6 api, just in case we get strict about that at some point. The code here could very well be omitted, but I thought it reasonable to have as it also serves to highlight the v6 vs v7 difference.
axw
Aug 25, 2017
Member
I'd rather not carry unreachable code, particularly when it suggests compatibility requirements that we don't have. It'll end up getting copied.
| @@ -1599,6 +1616,12 @@ func (u *UniterAPI) SLALevel() (params.StringResult, error) { | ||
| } | ||
| // NetworkInfo returns network interfaces/addresses for specified bindings. | ||
| +func (u *UniterAPIV6) NetworkInfo(args params.NetworkInfoParams) (params.NetworkInfoResults, error) { |
wallyworld
Aug 24, 2017
Owner
The semantics are different and that's why we also need the version bump. NetworkInfoParams gains an optional RelationId attribute. We don't want callers expecting V7 behaviour to accidentally call a v6 API. The behaviour between V6 and v7 is indeed identical if the relation id is omitted when calling v7. So that's why v6 NetworkInfo simply delegates to v7 NetworkInfo.
axw
Aug 25, 2017
Member
The semantics aren't different, though, precisely because you're just calling the other one without any additional validation. As it is, you may as well not have this, and just allow the V7 method to be exposed to V6 clients. If you add args.RelationId = nil here, it would make it equivalent to the old behaviour.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
wallyworld commentedAug 18, 2017
•
Edited 1 time
-
wallyworld
Aug 25, 2017
Description of change
The time has come to make the network-get hook tool relation aware. If network-get is called in the context of a relation, and the relation endpoint is not bound to a space, or is bound to the default space, the address returned is in the context of that relation, and may be either a cloud local or public address depending on whether it is a cross model relation or not.
We also rename relation unit SettingsAddress() to IngressAddress() to better reflect its purpose. And we add a new relation settings "ingress-address" value which is preferred over "private-address".
Some old code is also deleted.
As a drive by, remove old code, and add a missing unit test for NetworkInfo.
QA steps
Charms will need to be updated to stop calling unit-get "private-address" in order to benefit from this PR. For now, just do a smoke test.
Bug reference
Partially fixes:
https://bugs.launchpad.net/juju/+bug/1711238