/juju

Initial support for IPv6 #6620

Merged
merged 2 commits into from Nov 29, 2016

Conversation

Reviewers

@jameinel jameinel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@macgreagoir @bz2 @jujubot @jameinel
@macgreagoir
Contributor

macgreagoir commented Nov 25, 2016

Adds support for IPv6, but without requiring IPv6.

There are some known issues that should not break, or be broken by, this
commit:

  • Manual is the only supportable provider for IPv6
  • RFC 6761 (6.3), expecting localhost to resolve to both the IPv4 and
    IPv6 loopback addresses, is not always satisfied in Ubuntu builds
    (lp:1644009), but
  • Ubuntu adds both IPv4 and IPv6 loopback addresses regardless of the
    version specified for lo in ENI, so loopback will resolve to a usable
    loopback address in any case
  • Code to be revised when these issues are resolved in Ubuntu are
    commented with TODO(macgreagoir) IPv6. ...
  • A bug around enabled-ha using existing machines (lp:1642618)
    requires a work-around for HA

QA steps:

  • IPv4, simply bootstrap as usual
  • IPv6
    • Build a machine with a working IPv6 address (which may require something like an IPv6 tunnel from Hurricane Electric) and no IPv4 addresses apart from loopback. (Feel welcome to contact me about this directly.)
    • Deploy to this machine with the manual provider
  • The expected result is a working environment in both IP version cases (noting the HA bug with the manual provider)

@macgreagoir macgreagoir changed the base branch from staging to develop Nov 28, 2016

@macgreagoir
Contributor

macgreagoir commented Nov 28, 2016

!!retry!!
@macgreagoir
Bootstrap with IPv6 using the Manual provider 
Adds support for IPv6, but without requiring IPv6.

There are some known issues that should not break, or be broken by, this
commit:

 * Manual is the only supportable provider for IPv6
 * RFC 6761 (6.3), expecting localhost to resolve to both the IPv4 and
   IPv6 loopback addresses, is not always satisfied in Ubuntu builds
   (lp:1644009), but
 * Ubuntu adds both IPv4 and IPv6 loopback addresses regardless of the
   version specified for lo in ENI, so loopback will resolve to a usable
   loopback address in any case
 * Code to be revised when these issues are resolved in Ubuntu are
   commented with `TODO(macgreagoir) IPv6. ...`
 * A bug around `enabled-ha` using existing machines (lp:1642618)
   requires a work-around for HA
8728e90
@jameinel

jameinel approved these changes Nov 28, 2016
View changes

I have a couple of small tweaks, but I think the overall changes of switching to 'localhost' from '127.0.0.1' is all good.

I'm a little concerned still about machineLocal=true, and what actual addresses are getting filtered by the 'ip.IP4() == nil' check.

api/apiclient.go
+ // We won't reconnect when there's an X509
+ // error because we're not going to succeed if
+ // we retry in that case.
+ logger.Errorf("certificate error dialing %q: %v", cfg.Location, err)
@jameinel

jameinel Nov 28, 2016

Owner

This is just to improve the error messages, right?

@macgreagoir

macgreagoir Nov 28, 2016

Contributor

It is, aye. It's something I added for clarity in my own testing and thought was worth keeping.

apiserver/common/networkingcommon/types.go
- logger.Debugf("skipping observed IPv6 address %q on %q: not fully supported yet", ip, interfaceName)
- // TODO(dimitern): Treat IPv6 addresses as empty until we can handle
- // them reliably.
+ if ip.To4() == nil && ip.IsLinkLocalUnicast() {
@jameinel

jameinel Nov 28, 2016

Owner

What do we do with the other IPv6 addresses?
Would it be more obvious if we did:

if ip.To6() != nil && ip.IsLinkLocalUnicast() ?

I almost wonder if for now we'd want to break this up into:

if ip.IP6() != nil {
if ip.IsLinkLocalUnicast() {
logger.Debugf("skipping observed IPv6 link-local...")
} else {
logger.Debugf("including ipv6 address %s")
}
}

@macgreagoir

macgreagoir Nov 28, 2016

Contributor

The net package doesn't have a direct check for IPv6, that I can see. To4() will tell us that we have a valid 4-byte representation, therefore an IPv4 address, but To16() can return a valid 16-byte address of either version, hence the use in this case filter out link-local but allow usable IPv6 addresses.

I could add our own checks for non-link-local IPv6 addresses to filter, but this does seem like code to maintain where we needn't. What do you think?

apiserver/common/networkingcommon/types_test.go
@@ -978,6 +978,8 @@ func (s *TypesSuite) TestGetObservedNetworkConfigLoopbackInfrerred(c *gc.C) {
ConfigType: "loopback", // since it is a loopback
}, {
DeviceIndex: 1,
+ CIDR: "::1/128",
@jameinel

jameinel Nov 28, 2016

Owner

Should this be a new section, rather than just adding to this scenario?

I feel like we should handle IPv4-only loopback as well, shouldn't we?

@macgreagoir

macgreagoir Nov 28, 2016

Contributor

I'm not sure this is still a likely use case. I see both versions everywhere.

mongo/mongo.go
- return network.SelectMongoHostPortsByScope(hostPorts, true)[0]
+ return network.SelectMongoHostPortsByScope(
+ hostPorts,
+ network.HostPortWithIPv4Address(hostPorts),
@jameinel

jameinel Nov 28, 2016

Owner

can you use a local variable so that the meaning of this makes more sense? When I read it at a glance it looks like we are only selecting IPv4 addresses.

allowMachineLocal := network.HostPortWithIPv4Address(hostPorts)

return network.SelectMongoHostPortsByScope(hostPorts, allowMachineLocal)

I'm still not 100% sold on using machine local. This feels more like "if we don't have a cloud-local then maybe we can fall back to localhost".
Given its the address we are putting into the replicaSet, it seems like it can't ever be right to give a link local address if you are ever actually in a peer group.

@macgreagoir

macgreagoir Nov 28, 2016

Contributor

I'm inclined to agree with you about the original, and continuing IPv4, behaviour. Falling back to loopback/localhost can seem like a bad idea for the replicaset config, though it doesn't cause a problem if HA is never enabled.

Can I suggest that this is machine-local question is a separate bug to be considered outside this change, as a new behaviour?

@macgreagoir

macgreagoir Nov 28, 2016

Contributor

Local variable done.

network/address.go
@@ -335,6 +335,20 @@ func SelectMongoHostPortsBySpaces(hostPorts []HostPort, spaces []SpaceName) ([]s
return HostPortsToStrings(filteredHostPorts), ok
}
+// HostPortWithIPv4Address returns true if the passed slice of HostPort
+// contains an IPv4 address that is not just a loopback address.
+func HostPortWithIPv4Address(hostPorts []HostPort) bool {
@jameinel

jameinel Nov 28, 2016

Owner

The name sounds like it is returning a HostPort, maybe:

HostPortsHaveIPv4Address()

Not sure what name, but something that makes it clearer the function is returning a boolean.

@macgreagoir

macgreagoir Nov 28, 2016

Contributor

Done.

network/address.go
+// contains an IPv4 address that is not just a loopback address.
+func HostPortWithIPv4Address(hostPorts []HostPort) bool {
+ for _, hp := range hostPorts {
+ logger.Debugf("found Address of Value %q, Type %q and Scope %q",
@jameinel

jameinel Nov 28, 2016

Owner

This feels a bit chatty, might be better as Tracef.

@macgreagoir

macgreagoir Nov 28, 2016

Contributor

Done.

provider/manual/environ.go
@@ -258,7 +258,7 @@ if [ $stopped -ne 1 ]; then
service %s stop
fi
rm -f /etc/init/juju*
-rm -f /etc/systemd/system/juju*
+rm -f /etc/systemd/system{,/multi-user.target.wants}/juju*
@jameinel

jameinel Nov 28, 2016

Owner

Does this have anything to do with IPv6 or is it just a drive by that you encountered. Should we be tagging a bug about this so people can know where/when it was fixed?

@macgreagoir

macgreagoir Nov 28, 2016

Contributor

Fair call :-) I really don't like drive-by changes myself.

PR juju/juju#6624

@bz2
Contributor

bz2 commented Nov 28, 2016

!!retry!!
@macgreagoir
Contributor

macgreagoir commented Nov 29, 2016

!!retry!!
@bz2
Contributor

bz2 commented Nov 29, 2016

!!retry!!
@macgreagoir
Contributor

macgreagoir commented Nov 29, 2016

$$merge$$
@jujubot
Contributor

jujubot commented Nov 29, 2016

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

jujubot commented Nov 29, 2016

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/9727
@macgreagoir
Contributor

macgreagoir commented Nov 29, 2016

@bz2 Thanks for the manual stop :-) I'll $$merge$$ with squash now.
@jujubot
Contributor

jujubot commented Nov 29, 2016

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

@jujubot jujubot merged commit aa221fa into juju:develop Nov 29, 2016
1 check passed

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

rogpeppe added a commit to rogpeppe/juju that referenced this pull request Jan 18, 2017

@rogpeppe
apiclient: don't log at error level when dialing 
PR #6620 changed the client logging so that it logs an
error when it can't connect to an API address.
It is common for these errors to occur when connecting
even though the actual connect succeeds, so this
PR changes the logging to debug level - it's part
of the implementation but not something that users
should always see.
208dd13

rogpeppe added a commit to rogpeppe/juju that referenced this pull request Jan 18, 2017

@rogpeppe
apiclient: don't log at error level when dialing 
PR #6620 changed the client logging so that it logs an
error when it can't connect to an API address.
It is common for these errors to occur when connecting
even though the actual connect succeeds, so this
PR changes the logging to debug level - it's part
of the implementation but not something that users
should always see.
5e4ac80

@rogpeppe rogpeppe referenced this pull request Jan 18, 2017

Merged

apiclient: don't log at error level when dialing #6830

rogpeppe added a commit to rogpeppe/juju that referenced this pull request Jan 18, 2017

@rogpeppe
apiclient: don't log at error level when dialing 
PR #6620 changed the client logging so that it logs an
error when it can't connect to an API address.
It is common for these errors to occur when connecting
even though the actual connect succeeds, so this
PR changes the logging to debug level - it's part
of the implementation but not something that users
should always see.
6f69b58

jujubot added a commit that referenced this pull request Jan 18, 2017

@jujubot
Merge pull request #6830 from rogpeppe/122-no-error-logging-on-failed… 
…-dial

apiclient: don't log at error level when dialing

PR #6620 changed the client logging so that it logs an
error when it can't connect to an API address.
It is common for these errors to occur when connecting
even though the actual connect succeeds, so this
PR changes the logging to debug level - it's part
of the implementation but not something that users
should always see.
0abde22

rogpeppe added a commit to rogpeppe/juju that referenced this pull request Feb 6, 2017

@rogpeppe
apiclient: don't log at error level when dialing 
PR #6620 changed the client logging so that it logs an
error when it can't connect to an API address.
It is common for these errors to occur when connecting
even though the actual connect succeeds, so this
PR changes the logging to debug level - it's part
of the implementation but not something that users
should always see.
2f3cc6a

@rogpeppe rogpeppe referenced this pull request Feb 6, 2017

Merged

apiclient: don't log at error level when dialing #6926

jujubot added a commit that referenced this pull request Feb 6, 2017

@jujubot
Merge pull request #6926 from rogpeppe/124-no-error-logging-on-failed… 
…-dial-2.1

apiclient: don't log at error level when dialing

PR #6620 changed the client logging so that it logs an
error when it can't connect to an API address.
It is common for these errors to occur when connecting
even though the actual connect succeeds, so this
PR changes the logging to debug level - it's part
of the implementation but not something that users
should always see.

This is a backport of #6830 to Juju 2.1, as the errors
are confusing people and 2.2 won't be out for a while.
20b593c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment