environs: Add Networking.ProviderSpaceInfo and .IsSpaceRoutable #7291

Merged
merged 3 commits into from May 2, 2017

Conversation

Projects
None yet
5 participants
Member

babbageclunk commented Apr 27, 2017

Description of change

These new methods on the provider networking interface will be used when setting up cross-model relations to determine whether connections can use the cloud-local address for communication between units, or if they need to fall back to using the public address.

ProviderSpaceInfo gets all the information that might be needed about a space in this environ to allow a different environ to determine whether it is directly accessible.

IsSpaceRoutable takes a ProviderSpaceInfo (which will have been obtained from a different environ) and returns whether the cloud-local address can be used.

This adds dummy implementations to all the existing NetworkingEnviron types. Followup PRs will implement the methods for different providers.

QA steps

None yet - these will be used in the API when creating cross-model relations.

@babbageclunk babbageclunk changed the title from WIP environs: Add Networking.ProviderSpaceInfo and .IsRoutable to environs: Add Networking.ProviderSpaceInfo and .IsRoutable Apr 27, 2017

Member

babbageclunk commented Apr 27, 2017

Not sure about the names. Also, while typing the description I got confused - is it sensible to talk about routability between an Environ and some space? Surely it can only be between spaces? In which case maybe the routability check should be on EnvironProvider and take two ProviderSpaceInfos?

environs/networking.go
+ // ProviderSpaceInfo returns the details of the space requested as
+ // a ProviderSpaceInfo. This will contain everything needed to
+ // decide whether an Environ of the same type in another
+ // controller could route to the space. If "" is passed as
@wallyworld

wallyworld Apr 27, 2017

Owner

Can we define a const DefaultSpace or something for ""
Or maybe we have one. But if not, we should.

@babbageclunk

babbageclunk Apr 27, 2017

Member

Yeah, good point, I'll dig one up/create one.

environs/networking.go
+
+ // IsRoutable returns whether this Environ can connect to the
+ // given space via cloud-local addresses.
+ IsRoutable(targetSpace ProviderSpaceInfo) (bool, error)
@wallyworld

wallyworld Apr 27, 2017

Owner

And then targetSpace would become a pointer also.

@babbageclunk

babbageclunk Apr 28, 2017

Member

Is this the second comment to one that never got added? I'm guessing it was "make ProviderSpaceInfo return a pointer"?

Will do on both counts.

I think you should also ask jam to review this. I know he's been thinking about space routability too.

environs/networking.go
+
+ // IsRoutable returns whether this Environ can connect to the
+ // given space via cloud-local addresses.
+ IsRoutable(targetSpace ProviderSpaceInfo) (bool, error)
@axw

axw Apr 27, 2017

Member

maybe put Space in the name somewhere? IsSpaceRoutable?

environs/networking.go
+type ProviderSpaceInfo struct {
+ network.SpaceInfo
+
+ // This identifies the cloud containing the space. Care must be
@axw

axw Apr 27, 2017

Member

I'm still not convinced that we need any structure around the information. This is better than the layer above passing it in, but some of the bits in CloudSpec are never going to be relevant: IdentityEndpoint, StorageEndpoint.

I guess my only question is: do we think we'll ever want to use the CloudSpec field outside the provider? Maybe show which cloud/region a remote model is in to the user? Then some structure would be useful. But within the provider, the provider creates the information so it can apply its own structure to it.

@babbageclunk

babbageclunk Apr 28, 2017

Member

I don't really have a strong opinion about this. In the implementation it's more convenient to work with fields that have a defined type rather than pulling everything from a map[string]interface{} and do checking for presence and type. It seems like every cloud is going to care about type, name, region and credential, with endpoint being useful for non-built-in clouds. You're right that identity and storage endpoints are very unlikely to be useful, though.

I can also see that having the structure is kind of inviting the client code to look at it (although I guess it could be private).

@babbageclunk

babbageclunk May 1, 2017

Member

In a hangout Andrew pointed out that having a CloudSpec that wasn't quite a real CloudSpec was likely to bite us at some point (by some provider not scrubbing secrets and just using its real CloudSpec, for example). Changed the ProviderSpaceInfo just to have CloudType and everything else will live inside ProviderAttributes.

jamesbeedy commented Apr 28, 2017

@babbageclunk possibly keep this bug in mind while you are poking around here

babbageclunk added some commits Apr 27, 2017

Add Networking.ProviderSpaceInfo and .IsRoutable
These will be used when setting up cross-model relations to determine
whether connections should use the cloud-local or public address for
units.

@babbageclunk babbageclunk changed the title from environs: Add Networking.ProviderSpaceInfo and .IsRoutable to environs: Add Networking.ProviderSpaceInfo and .IsSpaceRoutable Apr 30, 2017

Member

babbageclunk commented May 2, 2017

I'm landing this for now, given that the sprint in Montreal is understandably making it hard for people to get to this - @jameinel if you have suggestions or changes later I'll gladly make them in a subsequent PR.

$$merge$$

Contributor

jujubot commented May 2, 2017

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

@jujubot jujubot merged commit abf46a7 into juju:develop May 2, 2017

1 check passed

github-check-merge-juju Built PR, ran unit tests, and tested LXD deploy. Use !!.*!! to request another build. IE, !!build!!, !!retry!!
Details

@babbageclunk babbageclunk deleted the babbageclunk:provider-remote-details branch May 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment