Merge pull request juju#12343 from manadart/2.8-netget-resolve-server…

…-side juju#12343 Moves the resolution of host-names server-side instead of resolving within the `network-get` tool. This will allow us in a forthcoming patch to apply host-name resolution selectively. ## QA steps Same as for the original client-side change under juju#8672. ## Documentation changes None ## Bug reference N/A
sdg0919 · Nov 21, 2020 · eb7556a · eb7556a
2 parents f3602d4 + 6770078
commit eb7556a
Show file tree

Hide file tree

Showing 6 changed files with 163 additions and 214 deletions.
diff --git a/apiserver/facades/agent/uniter/networkinfo.go b/apiserver/facades/agent/uniter/networkinfo.go
@@ -183,6 +183,82 @@ func (n *NetworkInfoBase) getEgressForRelation(
 	return subnetsForAddresses(ingress.Values()), nil
 }
 
+func (n *NetworkInfoBase) resolveResultHostNames(netInfoResult params.NetworkInfoResult) params.NetworkInfoResult {
+	// Maintain a cache of host-name -> address resolutions.
+	resolved := make(map[string]string)
+	addressForHost := func(hostName string) string {
+		resolvedAddr, ok := resolved[hostName]
+		if !ok {
+			resolvedAddr = n.resolveHostAddress(hostName)
+			resolved[hostName] = resolvedAddr
+		}
+		return resolvedAddr
+	}
+
+	// Resolve addresses in Info.
+	for i, info := range netInfoResult.Info {
+		for j, addr := range info.Addresses {
+			if ip := net.ParseIP(addr.Address); ip == nil {
+				// If the address is not an IP, we assume it is a host name.
+				addr.Hostname = addr.Address
+				addr.Address = addressForHost(addr.Hostname)
+				netInfoResult.Info[i].Addresses[j] = addr
+			}
+		}
+	}
+
+	// Resolve addresses in IngressAddresses.
+	// This is slightly different to the addresses above in that we do not
+	// include anything that does not resolve to a usable address.
+	var newIngress []string
+	for _, addr := range netInfoResult.IngressAddresses {
+		if ip := net.ParseIP(addr); ip != nil {
+			newIngress = append(newIngress, addr)
+			continue
+		}
+		if ipAddr := addressForHost(addr); ipAddr != "" {
+			newIngress = append(newIngress, ipAddr)
+		}
+	}
+	netInfoResult.IngressAddresses = newIngress
+
+	return netInfoResult
+}
+
+func (n *NetworkInfoBase) resolveHostAddress(hostName string) string {
+	resolved, err := n.lookupHost(hostName)
+	if err != nil {
+		logger.Errorf("resolving %q: %v", hostName, err)
+		return ""
+	}
+
+	// This preserves prior behaviour from when resolution was done client-side
+	// within the network-get tool.
+	// This check is probably no longer necessary, but is preserved here
+	// conservatively.
+	for _, addr := range resolved {
+		if ip := net.ParseIP(addr); ip != nil && !ip.IsLoopback() {
+			return addr
+		}
+	}
+
+	if len(resolved) == 0 {
+		logger.Warningf("no addresses resolved for host %q", hostName)
+	} else {
+		// If we got results, but they were all filtered out, then we need to
+		// help out operators with some advice.
+		logger.Warningf(
+			"no usable addresses resolved for host %q\n\t"+
+				"resolved: %v\n\t"+
+				"consider editing the hosts file, or changing host resolution order via /etc/nsswitch.conf",
+			hostName,
+			resolved,
+		)
+	}
+
+	return ""
+}
+
 // subnetsForAddresses wraps the core/network method of the same name,
 // limiting the return to container at most one result.
 // TODO (manadart 2020-11-19): This preserves prior behaviour,

diff --git a/apiserver/facades/agent/uniter/networkinfo_test.go b/apiserver/facades/agent/uniter/networkinfo_test.go
@@ -4,6 +4,7 @@
 package uniter_test
 
 import (
+	"errors"
 	"fmt"
 	"math/rand"
 	"time"
@@ -60,7 +61,7 @@ func (s *networkInfoSuite) TestNetworksForRelation(c *gc.C) {
 	)
 	c.Assert(err, jc.ErrorIsNil)
 
-	netInfo := s.newNetworkInfo(c, prr.pu0.UnitTag(), nil)
+	netInfo := s.newNetworkInfo(c, prr.pu0.UnitTag(), nil, nil)
 	boundSpace, ingress, egress, err := netInfo.NetworksForRelation("", prr.rel, true)
 	c.Assert(err, jc.ErrorIsNil)
 
@@ -70,28 +71,6 @@ func (s *networkInfoSuite) TestNetworksForRelation(c *gc.C) {
 	c.Assert(egress, gc.DeepEquals, []string{"10.2.3.4/32"})
 }
 
-func (s *networkInfoSuite) TestNetworksForRelationHostNameNoEgress(c *gc.C) {
-	prr := s.newProReqRelation(c, charm.ScopeGlobal)
-	err := prr.pu0.AssignToNewMachine()
-	c.Assert(err, jc.ErrorIsNil)
-	id, err := prr.pu0.AssignedMachineId()
-	c.Assert(err, jc.ErrorIsNil)
-	machine, err := s.State.Machine(id)
-	c.Assert(err, jc.ErrorIsNil)
-
-	addr := network.NewSpaceAddress("host.at.somewhere")
-	err = machine.SetProviderAddresses(addr)
-	c.Assert(err, jc.ErrorIsNil)
-
-	netInfo := s.newNetworkInfo(c, prr.pu0.UnitTag(), testingRetryFactory)
-	boundSpace, ingress, egress, err := netInfo.NetworksForRelation("", prr.rel, true)
-	c.Assert(err, jc.ErrorIsNil)
-
-	c.Assert(boundSpace, gc.Equals, network.AlphaSpaceId)
-	c.Assert(ingress, gc.DeepEquals, network.SpaceAddresses{addr})
-	c.Assert(egress, gc.HasLen, 0)
-}
-
 func (s *networkInfoSuite) addDevicesWithAddresses(c *gc.C, machine *state.Machine, addresses ...string) {
 	for _, address := range addresses {
 		name := fmt.Sprintf("e%x", rand.Int31())
@@ -117,7 +96,7 @@ func (s *networkInfoSuite) addDevicesWithAddresses(c *gc.C, machine *state.Machi
 	}
 }
 
-func (s *networkInfoSuite) TestNetworksForBinding(c *gc.C) {
+func (s *networkInfoSuite) TestProcessAPIRequestForBinding(c *gc.C) {
 	// Add subnets for the addresses that the machine will have.
 	// We are testing a space-less deployment here.
 	_, err := s.State.AddSubnet(network.SubnetInfo{
@@ -158,7 +137,7 @@ func (s *networkInfoSuite) TestNetworksForBinding(c *gc.C) {
 
 	s.addDevicesWithAddresses(c, machine, "10.2.3.4/16", "100.2.3.4/24")
 
-	netInfo := s.newNetworkInfo(c, unit.UnitTag(), nil)
+	netInfo := s.newNetworkInfo(c, unit.UnitTag(), nil, nil)
 	result, err := netInfo.ProcessAPIRequest(params.NetworkInfoParams{
 		Unit:      unit.UnitTag().String(),
 		Endpoints: []string{"server-admin"},
@@ -178,11 +157,63 @@ func (s *networkInfoSuite) TestNetworksForBinding(c *gc.C) {
 	c.Check(ingress[0], gc.Equals, "100.2.3.4")
 }
 
+func (s *networkInfoSuite) TestAPIRequestForRelationHostNameNoEgress(c *gc.C) {
+	prr := s.newProReqRelation(c, charm.ScopeGlobal)
+	err := prr.pu0.AssignToNewMachine()
+	c.Assert(err, jc.ErrorIsNil)
+	id, err := prr.pu0.AssignedMachineId()
+	c.Assert(err, jc.ErrorIsNil)
+	machine, err := s.State.Machine(id)
+	c.Assert(err, jc.ErrorIsNil)
+
+	// The only address is a host-name, resolvable to the IP below.
+	host := "host.at.somewhere"
+	ip := "100.2.3.4"
+
+	addr := network.NewSpaceAddress(host)
+	err = machine.SetProviderAddresses(addr)
+	c.Assert(err, jc.ErrorIsNil)
+
+	lookup := func(addr string) ([]string, error) {
+		if addr == host {
+			return []string{ip}, nil
+		}
+		return nil, errors.New("bad horsey")
+	}
+
+	netInfo := s.newNetworkInfo(c, prr.pu0.UnitTag(), nil, lookup)
+
+	rID := prr.rel.Id()
+	result, err := netInfo.ProcessAPIRequest(params.NetworkInfoParams{
+		Unit:       names.NewUnitTag(prr.pru0.UnitName()).String(),
+		Endpoints:  []string{"server"},
+		RelationId: &rID,
+	})
+	c.Assert(err, jc.ErrorIsNil)
+
+	res := result.Results
+	c.Assert(res, gc.HasLen, 1)
+
+	binding, ok := res["server"]
+	c.Assert(ok, jc.IsTrue)
+
+	ingress := binding.IngressAddresses
+	c.Assert(ingress, gc.HasLen, 1)
+	c.Check(ingress[0], gc.Equals, ip)
+
+	c.Assert(binding.Info, gc.HasLen, 1)
+
+	addrs := binding.Info[0].Addresses
+	c.Check(addrs, gc.HasLen, 1)
+	c.Check(addrs[0].Hostname, gc.Equals, host)
+	c.Check(addrs[0].Address, gc.Equals, ip)
+}
+
 func (s *networkInfoSuite) TestNetworksForRelationWithSpaces(c *gc.C) {
-	_ = s.setupSpace(c, "space-1", "1.2.0.0/16", false)
-	_ = s.setupSpace(c, "space-2", "2.2.0.0/16", false)
-	spaceID3 := s.setupSpace(c, "space-3", "10.2.0.0/16", false)
-	_ = s.setupSpace(c, "public-4", "4.2.0.0/16", true)
+	_ = s.setupSpace(c, "space-1", "1.2.0.0/16")
+	_ = s.setupSpace(c, "space-2", "2.2.0.0/16")
+	spaceID3 := s.setupSpace(c, "space-3", "10.2.0.0/16")
+	_ = s.setupSpace(c, "public-4", "4.2.0.0/16")
 
 	// We want to have all bindings set so that no actual binding is
 	// really set to the default.
@@ -211,7 +242,7 @@ func (s *networkInfoSuite) TestNetworksForRelationWithSpaces(c *gc.C) {
 
 	s.addDevicesWithAddresses(c, machine, "1.2.3.4/16", "2.2.3.4/16", "10.2.3.4/16", "4.3.2.1/16")
 
-	netInfo := s.newNetworkInfo(c, prr.pu0.UnitTag(), nil)
+	netInfo := s.newNetworkInfo(c, prr.pu0.UnitTag(), nil, nil)
 	boundSpace, ingress, egress, err := netInfo.NetworksForRelation("", prr.rel, true)
 	c.Assert(err, jc.ErrorIsNil)
 
@@ -236,7 +267,7 @@ func (s *networkInfoSuite) TestNetworksForRelationRemoteRelation(c *gc.C) {
 	)
 	c.Assert(err, jc.ErrorIsNil)
 
-	netInfo := s.newNetworkInfo(c, prr.ru0.UnitTag(), nil)
+	netInfo := s.newNetworkInfo(c, prr.ru0.UnitTag(), nil, nil)
 	boundSpace, ingress, egress, err := netInfo.NetworksForRelation("", prr.rel, true)
 	c.Assert(err, jc.ErrorIsNil)
 
@@ -260,7 +291,7 @@ func (s *networkInfoSuite) TestNetworksForRelationRemoteRelationNoPublicAddr(c *
 	)
 	c.Assert(err, jc.ErrorIsNil)
 
-	netInfo := s.newNetworkInfo(c, prr.ru0.UnitTag(), nil)
+	netInfo := s.newNetworkInfo(c, prr.ru0.UnitTag(), nil, nil)
 	boundSpace, ingress, egress, err := netInfo.NetworksForRelation("", prr.rel, true)
 	c.Assert(err, jc.ErrorIsNil)
 
@@ -294,7 +325,7 @@ func (s *networkInfoSuite) TestNetworksForRelationRemoteRelationDelayedPublicAdd
 		}
 	}
 
-	netInfo := s.newNetworkInfo(c, prr.ru0.UnitTag(), retryFactory)
+	netInfo := s.newNetworkInfo(c, prr.ru0.UnitTag(), retryFactory, nil)
 	boundSpace, ingress, egress, err := netInfo.NetworksForRelation("", prr.rel, true)
 	c.Assert(err, jc.ErrorIsNil)
 
@@ -342,7 +373,7 @@ func (s *networkInfoSuite) TestNetworksForRelationRemoteRelationDelayedPrivateAd
 		}
 	}
 
-	netInfo := s.newNetworkInfo(c, prr.ru0.UnitTag(), retryFactory)
+	netInfo := s.newNetworkInfo(c, prr.ru0.UnitTag(), retryFactory, nil)
 	boundSpace, ingress, egress, err := netInfo.NetworksForRelation("", prr.rel, true)
 	c.Assert(err, jc.ErrorIsNil)
 
@@ -365,7 +396,7 @@ func (s *networkInfoSuite) TestNetworksForRelationCAASModel(c *gc.C) {
 	prr := newProReqRelationForApps(c, st, mysql, gitlab)
 
 	// We need to instantiate this with the new CAAS model state.
-	netInfo, err := uniter.NewNetworkInfoForStrategy(st, prr.pu0.UnitTag(), testingRetryFactory, nil)
+	netInfo, err := uniter.NewNetworkInfoForStrategy(st, prr.pu0.UnitTag(), nil, nil)
 	c.Assert(err, jc.ErrorIsNil)
 
 	// First no address.
@@ -392,9 +423,9 @@ func (s *networkInfoSuite) TestNetworksForRelationCAASModel(c *gc.C) {
 }
 
 func (s *networkInfoSuite) TestMachineNetworkInfos(c *gc.C) {
-	spaceIDDefault := s.setupSpace(c, "default", "10.0.0.0/24", true)
-	spaceIDDMZ := s.setupSpace(c, "dmz", "10.10.0.0/24", true)
-	_ = s.setupSpace(c, "private", "10.20.0.0/24", false)
+	spaceIDDefault := s.setupSpace(c, "default", "10.0.0.0/24")
+	spaceIDDMZ := s.setupSpace(c, "dmz", "10.10.0.0/24")
+	_ = s.setupSpace(c, "private", "10.20.0.0/24")
 
 	app := s.AddTestingApplication(c, "wordpress", s.AddTestingCharm(c, "wordpress"))
 
@@ -420,7 +451,7 @@ func (s *networkInfoSuite) TestMachineNetworkInfos(c *gc.C) {
 		network.NewScopedSpaceAddress("10.20.0.20", network.ScopeCloudLocal))
 	c.Assert(err, jc.ErrorIsNil)
 
-	ni := s.newNetworkInfo(c, unit.UnitTag(), nil)
+	ni := s.newNetworkInfo(c, unit.UnitTag(), nil, nil)
 	netInfo := ni.(*uniter.NetworkInfoIAAS)
 
 	res, err := netInfo.MachineNetworkInfos(spaceIDDefault, spaceIDDMZ, "666", network.AlphaSpaceId)
@@ -487,7 +518,7 @@ func (s *networkInfoSuite) TestMachineNetworkInfosAlphaNoSubnets(c *gc.C) {
 		network.NewScopedSpaceAddress("10.20.0.20", network.ScopeCloudLocal))
 	c.Assert(err, jc.ErrorIsNil)
 
-	ni := s.newNetworkInfo(c, unit.UnitTag(), nil)
+	ni := s.newNetworkInfo(c, unit.UnitTag(), nil, nil)
 	netInfo := ni.(*uniter.NetworkInfoIAAS)
 
 	res, err := netInfo.MachineNetworkInfos(network.AlphaSpaceId)
@@ -504,7 +535,7 @@ func (s *networkInfoSuite) TestMachineNetworkInfosAlphaNoSubnets(c *gc.C) {
 	c.Check(resEmpty.NetworkInfos[0].Addresses[0].CIDR, gc.Equals, "10.20.0.0/24")
 }
 
-func (s *networkInfoSuite) setupSpace(c *gc.C, spaceName, cidr string, public bool) string {
+func (s *networkInfoSuite) setupSpace(c *gc.C, spaceName, cidr string) string {
 	space, err := s.State.AddSpace(spaceName, network.Id(spaceName), nil, true)
 	c.Assert(err, jc.ErrorIsNil)
 
@@ -558,7 +589,7 @@ func (s *networkInfoSuite) createNICWithIP(
 }
 
 func (s *networkInfoSuite) newNetworkInfo(
-	c *gc.C, tag names.UnitTag, retryFactory func() retry.CallArgs,
+	c *gc.C, tag names.UnitTag, retryFactory func() retry.CallArgs, lookupHost func(string) ([]string, error),
 ) uniter.NetworkInfo {
 	// Allow the caller to supply nil if this is not important.
 	// We fill it with an optimistic default.
@@ -572,33 +603,35 @@ func (s *networkInfoSuite) newNetworkInfo(
 		}
 	}
 
-	ni, err := uniter.NewNetworkInfoForStrategy(s.State, tag, retryFactory, nil)
+	ni, err := uniter.NewNetworkInfoForStrategy(s.State, tag, retryFactory, lookupHost)
 	c.Assert(err, jc.ErrorIsNil)
 	return ni
 }
 
 func (s *networkInfoSuite) newProReqRelationWithBindings(
-	c *gc.C, scope charm.RelationScope, pbindings, rbindings map[string]string,
+	c *gc.C, scope charm.RelationScope, pBindings, rBindings map[string]string,
 ) *ProReqRelation {
-	papp := s.AddTestingApplicationWithBindings(c, "mysql", s.AddTestingCharm(c, "mysql"), pbindings)
+	papp := s.AddTestingApplicationWithBindings(c, "mysql", s.AddTestingCharm(c, "mysql"), pBindings)
 	var rapp *state.Application
 	if scope == charm.ScopeGlobal {
-		rapp = s.AddTestingApplicationWithBindings(c, "wordpress", s.AddTestingCharm(c, "wordpress"), rbindings)
+		rapp = s.AddTestingApplicationWithBindings(c, "wordpress", s.AddTestingCharm(c, "wordpress"), rBindings)
 	} else {
-		rapp = s.AddTestingApplicationWithBindings(c, "logging", s.AddTestingCharm(c, "logging"), rbindings)
+		rapp = s.AddTestingApplicationWithBindings(c, "logging", s.AddTestingCharm(c, "logging"), rBindings)
 	}
 	return newProReqRelationForApps(c, s.State, papp, rapp)
 }
 
 func (s *networkInfoSuite) newProReqRelation(c *gc.C, scope charm.RelationScope) *ProReqRelation {
-	papp := s.AddTestingApplication(c, "mysql", s.AddTestingCharm(c, "mysql"))
-	var rapp *state.Application
+	pApp := s.AddTestingApplication(c, "mysql", s.AddTestingCharm(c, "mysql"))
+
+	var rApp *state.Application
 	if scope == charm.ScopeGlobal {
-		rapp = s.AddTestingApplication(c, "wordpress", s.AddTestingCharm(c, "wordpress"))
+		rApp = s.AddTestingApplication(c, "wordpress", s.AddTestingCharm(c, "wordpress"))
 	} else {
-		rapp = s.AddTestingApplication(c, "logging", s.AddTestingCharm(c, "logging"))
+		rApp = s.AddTestingApplication(c, "logging", s.AddTestingCharm(c, "logging"))
 	}
-	return newProReqRelationForApps(c, s.State, papp, rapp)
+
+	return newProReqRelationForApps(c, s.State, pApp, rApp)
 }
 
 func (s *networkInfoSuite) newRemoteProReqRelation(c *gc.C) *RemoteProReqRelation {
@@ -694,11 +727,3 @@ func addRemoteRU(c *gc.C, rel *state.Relation, unitName string) *state.RelationU
 	c.Assert(err, jc.ErrorIsNil)
 	return ru
 }
-
-var testingRetryFactory = func() retry.CallArgs {
-	return retry.CallArgs{
-		Clock:       clock.WallClock,
-		Delay:       1 * time.Millisecond,
-		MaxDuration: 3 * time.Millisecond,
-	}
-}
diff --git a/apiserver/facades/agent/uniter/networkinfocaas.go b/apiserver/facades/agent/uniter/networkinfocaas.go
@@ -115,7 +115,7 @@ func (n *NetworkInfoCAAS) ProcessAPIRequest(args params.NetworkInfoParams) (para
 			info.EgressSubnets = subnetsForAddresses(info.IngressAddresses)
 		}
 
-		result.Results[endpoint] = info
+		result.Results[endpoint] = n.resolveResultHostNames(info)
 	}
 
 	return result, nil

diff --git a/apiserver/facades/agent/uniter/networkinfoiaas.go b/apiserver/facades/agent/uniter/networkinfoiaas.go
@@ -87,7 +87,7 @@ func (n *NetworkInfoIAAS) ProcessAPIRequest(args params.NetworkInfoParams) (para
 			info.EgressSubnets = subnetsForAddresses(info.IngressAddresses)
 		}
 
-		result.Results[endpoint] = info
+		result.Results[endpoint] = n.resolveResultHostNames(info)
 	}
 
 	return result, nil