tailcfg: add ServiceName

Rather than using a string everywhere and needing to clarify that the
string should have the svc: prefix, create a separate type for Service
names.

Updates tailscale/corp#24607

Change-Id: I720e022f61a7221644bb60955b72cacf42f59960
Signed-off-by: Adrian Dewhurst <adrian@tailscale.com>

cmd/k8s-operator/ingress-for-pg.go

@@ -222,13 +222,14 @@ func (a *IngressPGReconciler) maybeProvision(ctx context.Context, hostname strin
 			},
 		},
 	}
+	serviceName := tailcfg.ServiceName("svc:" + hostname)
 	var gotCfg *ipn.ServiceConfig
 	if cfg != nil && cfg.Services != nil {
-		gotCfg = cfg.Services[hostname]
+		gotCfg = cfg.Services[serviceName]
 	}
 	if !reflect.DeepEqual(gotCfg, ingCfg) {
 		logger.Infof("Updating serve config")
-		mak.Set(&cfg.Services, hostname, ingCfg)
+		mak.Set(&cfg.Services, serviceName, ingCfg)
 		cfgBytes, err := json.Marshal(cfg)
 		if err != nil {
 			return fmt.Errorf("error marshaling serve config: %w", err)
@@ -309,7 +310,7 @@ func (a *IngressPGReconciler) maybeCleanupProxyGroup(ctx context.Context, proxyG
 		found := false
 		for _, i := range ingList.Items {
 			ingressHostname := hostnameForIngress(&i)
-			if ingressHostname == vipHostname {
+			if ingressHostname == vipHostname.WithoutPrefix() {
 				found = true
 				break
 			}
@@ -317,7 +318,7 @@ func (a *IngressPGReconciler) maybeCleanupProxyGroup(ctx context.Context, proxyG
 
 		if !found {
 			logger.Infof("VIPService %q is not owned by any Ingress, cleaning up", vipHostname)
-			svc, err := a.getVIPService(ctx, vipHostname, logger)
+			svc, err := a.getVIPService(ctx, vipHostname.WithoutPrefix(), logger)
 			if err != nil {
 				errResp := &tailscale.ErrResponse{}
 				if errors.As(err, &errResp) && errResp.Status == http.StatusNotFound {
@@ -329,7 +330,7 @@ func (a *IngressPGReconciler) maybeCleanupProxyGroup(ctx context.Context, proxyG
 			}
 			if isVIPServiceForAnyIngress(svc) {
 				logger.Infof("cleaning up orphaned VIPService %q", vipHostname)
-				if err := a.tsClient.deleteVIPServiceByName(ctx, vipHostname); err != nil {
+				if err := a.tsClient.deleteVIPServiceByName(ctx, vipHostname.WithoutPrefix()); err != nil {
 					errResp := &tailscale.ErrResponse{}
 					if !errors.As(err, &errResp) || errResp.Status != http.StatusNotFound {
 						return fmt.Errorf("deleting VIPService %q: %w", vipHostname, err)
@@ -374,11 +375,12 @@ func (a *IngressPGReconciler) maybeCleanup(ctx context.Context, hostname string,
 	if err != nil {
 		return fmt.Errorf("error getting ProxyGroup serve config: %w", err)
 	}
+	serviceName := tailcfg.ServiceName("svc:" + hostname)
 	// VIPService is always first added to serve config and only then created in the Tailscale API, so if it is not
 	// found in the serve config, we can assume that there is no VIPService. TODO(irbekrm): once we have ingress
 	// ProxyGroup, we will probably add currently exposed VIPServices to its status. At that point, we can use the
 	// status rather than checking the serve config each time.
-	if cfg == nil || cfg.Services == nil || cfg.Services[hostname] == nil {
+	if cfg == nil || cfg.Services == nil || cfg.Services[serviceName] == nil {
 		return nil
 	}
 	logger.Infof("Ensuring that VIPService %q configuration is cleaned up", hostname)
@@ -390,7 +392,7 @@ func (a *IngressPGReconciler) maybeCleanup(ctx context.Context, hostname string,
 
 	// 3. Remove the VIPService from the serve config for the ProxyGroup.
 	logger.Infof("Removing VIPService %q from serve config for ProxyGroup %q", hostname, pg)
-	delete(cfg.Services, hostname)
+	delete(cfg.Services, serviceName)
 	cfgBytes, err := json.Marshal(cfg)
 	if err != nil {
 		return fmt.Errorf("error marshaling serve config: %w", err)

cmd/k8s-operator/ingress-for-pg_test.go

@@ -137,7 +137,7 @@ func TestIngressPGReconciler(t *testing.T) {
 		t.Fatalf("unmarshaling serve config: %v", err)
 	}
 
-	if cfg.Services["my-svc"] == nil {
+	if cfg.Services["svc:my-svc"] == nil {
 		t.Error("expected serve config to contain VIPService configuration")
 	}
 

cmd/tailscale/cli/advertise.go

@@ -66,7 +66,7 @@ func parseServiceNames(servicesArg string) ([]string, error) {
 	if servicesArg != "" {
 		services = strings.Split(servicesArg, ",")
 		for _, svc := range services {
-			err := tailcfg.CheckServiceName(svc)
+			err := tailcfg.ServiceName(svc).Validate()
 			if err != nil {
 				return nil, fmt.Errorf("service %q: %s", svc, err)
 			}

ipn/ipn_clone.go

@@ -106,7 +106,7 @@ func (src *ServeConfig) Clone() *ServeConfig {
 		}
 	}
 	if dst.Services != nil {
-		dst.Services = map[string]*ServiceConfig{}
+		dst.Services = map[tailcfg.ServiceName]*ServiceConfig{}
 		for k, v := range src.Services {
 			if v == nil {
 				dst.Services[k] = nil
@@ -133,7 +133,7 @@ func (src *ServeConfig) Clone() *ServeConfig {
 var _ServeConfigCloneNeedsRegeneration = ServeConfig(struct {
 	TCP         map[uint16]*TCPPortHandler
 	Web         map[HostPort]*WebServerConfig
-	Services    map[string]*ServiceConfig
+	Services    map[tailcfg.ServiceName]*ServiceConfig
 	AllowFunnel map[HostPort]bool
 	Foreground  map[string]*ServeConfig
 	ETag        string

ipn/ipn_view.go

@@ -195,7 +195,7 @@ func (v ServeConfigView) Web() views.MapFn[HostPort, *WebServerConfig, WebServer
 	})
 }
 
-func (v ServeConfigView) Services() views.MapFn[string, *ServiceConfig, ServiceConfigView] {
+func (v ServeConfigView) Services() views.MapFn[tailcfg.ServiceName, *ServiceConfig, ServiceConfigView] {
 	return views.MapFnOf(v.ж.Services, func(t *ServiceConfig) ServiceConfigView {
 		return t.View()
 	})
@@ -216,7 +216,7 @@ func (v ServeConfigView) ETag() string { return v.ж.ETag }
 var _ServeConfigViewNeedsRegeneration = ServeConfig(struct {
 	TCP         map[uint16]*TCPPortHandler
 	Web         map[HostPort]*WebServerConfig
-	Services    map[string]*ServiceConfig
+	Services    map[tailcfg.ServiceName]*ServiceConfig
 	AllowFunnel map[HostPort]bool
 	Foreground  map[string]*ServeConfig
 	ETag        string

ipn/ipnlocal/local.go

@@ -3417,13 +3417,13 @@ func generateInterceptVIPServicesTCPPortFunc(svcAddrPorts map[netip.Addr]func(ui
 
 // setVIPServicesTCPPortsIntercepted populates b.shouldInterceptVIPServicesTCPPortAtomic with an
 // efficient func for ShouldInterceptTCPPort to use, which is called on every incoming packet.
-func (b *LocalBackend) setVIPServicesTCPPortsIntercepted(svcPorts map[string][]uint16) {
+func (b *LocalBackend) setVIPServicesTCPPortsIntercepted(svcPorts map[tailcfg.ServiceName][]uint16) {
 	b.mu.Lock()
 	defer b.mu.Unlock()
 	b.setVIPServicesTCPPortsInterceptedLocked(svcPorts)
 }
 
-func (b *LocalBackend) setVIPServicesTCPPortsInterceptedLocked(svcPorts map[string][]uint16) {
+func (b *LocalBackend) setVIPServicesTCPPortsInterceptedLocked(svcPorts map[tailcfg.ServiceName][]uint16) {
 	if len(svcPorts) == 0 {
 		b.shouldInterceptVIPServicesTCPPortAtomic.Store(func(netip.AddrPort) bool { return false })
 		return
@@ -6025,7 +6025,7 @@ func (b *LocalBackend) reloadServeConfigLocked(prefs ipn.PrefsView) {
 // b.mu must be held.
 func (b *LocalBackend) setTCPPortsInterceptedFromNetmapAndPrefsLocked(prefs ipn.PrefsView) {
 	handlePorts := make([]uint16, 0, 4)
-	var vipServicesPorts map[string][]uint16
+	var vipServicesPorts map[tailcfg.ServiceName][]uint16
 
 	if prefs.Valid() && prefs.RunSSH() && envknob.CanSSHD() {
 		handlePorts = append(handlePorts, 22)
@@ -7815,7 +7815,7 @@ func (b *LocalBackend) vipServiceHash(services []*tailcfg.VIPService) string {
 
 func (b *LocalBackend) vipServicesFromPrefsLocked(prefs ipn.PrefsView) []*tailcfg.VIPService {
 	// keyed by service name
-	var services map[string]*tailcfg.VIPService
+	var services map[tailcfg.ServiceName]*tailcfg.VIPService
 	if !b.serveConfig.Valid() {
 		return nil
 	}
@@ -7828,12 +7828,13 @@ func (b *LocalBackend) vipServicesFromPrefsLocked(prefs ipn.PrefsView) []*tailcf
 	}
 
 	for _, s := range prefs.AdvertiseServices().All() {
-		if services == nil || services[s] == nil {
+		sn := tailcfg.ServiceName(s)
-			mak.Set(&services, s, &tailcfg.VIPService{
+		if services == nil || services[sn] == nil {
-				Name: s,
+			mak.Set(&services, sn, &tailcfg.VIPService{
+				Name: sn,
 			})
 		}
-		services[s].Active = true
+		services[sn].Active = true
 	}
 
 	return slicesx.MapValues(services)

ipn/ipnlocal/local_test.go

@@ -2715,7 +2715,7 @@ func TestTCPHandlerForDstWithVIPService(t *testing.T) {
 
 	err = b.setServeConfigLocked(
 		&ipn.ServeConfig{
-			Services: map[string]*ipn.ServiceConfig{
+			Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
 				"svc:foo": {
 					TCP: map[uint16]*ipn.TCPPortHandler{
 						882: {HTTP: true},
@@ -4747,7 +4747,7 @@ func TestGetVIPServices(t *testing.T) {
 			"served-only",
 			[]string{},
 			&ipn.ServeConfig{
-				Services: map[string]*ipn.ServiceConfig{
+				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
 					"svc:abc": {Tun: true},
 				},
 			},
@@ -4762,7 +4762,7 @@ func TestGetVIPServices(t *testing.T) {
 			"served-and-advertised",
 			[]string{"svc:abc"},
 			&ipn.ServeConfig{
-				Services: map[string]*ipn.ServiceConfig{
+				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
 					"svc:abc": {Tun: true},
 				},
 			},
@@ -4778,7 +4778,7 @@ func TestGetVIPServices(t *testing.T) {
 			"served-and-advertised-different-service",
 			[]string{"svc:def"},
 			&ipn.ServeConfig{
-				Services: map[string]*ipn.ServiceConfig{
+				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
 					"svc:abc": {Tun: true},
 				},
 			},
@@ -4797,7 +4797,7 @@ func TestGetVIPServices(t *testing.T) {
 			"served-with-port-ranges-one-range-single",
 			[]string{},
 			&ipn.ServeConfig{
-				Services: map[string]*ipn.ServiceConfig{
+				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
 					"svc:abc": {TCP: map[uint16]*ipn.TCPPortHandler{
 						80: {HTTPS: true},
 					}},
@@ -4814,7 +4814,7 @@ func TestGetVIPServices(t *testing.T) {
 			"served-with-port-ranges-one-range-multiple",
 			[]string{},
 			&ipn.ServeConfig{
-				Services: map[string]*ipn.ServiceConfig{
+				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
 					"svc:abc": {TCP: map[uint16]*ipn.TCPPortHandler{
 						80: {HTTPS: true},
 						81: {HTTPS: true},
@@ -4833,7 +4833,7 @@ func TestGetVIPServices(t *testing.T) {
 			"served-with-port-ranges-multiple-ranges",
 			[]string{},
 			&ipn.ServeConfig{
-				Services: map[string]*ipn.ServiceConfig{
+				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
 					"svc:abc": {TCP: map[uint16]*ipn.TCPPortHandler{
 						80:   {HTTPS: true},
 						81:   {HTTPS: true},
@@ -4866,7 +4866,7 @@ func TestGetVIPServices(t *testing.T) {
 			}
 			got := lb.vipServicesFromPrefsLocked(prefs.View())
 			slices.SortFunc(got, func(a, b *tailcfg.VIPService) int {
-				return strings.Compare(a.Name, b.Name)
+				return strings.Compare(a.Name.String(), b.Name.String())
 			})
 			if !reflect.DeepEqual(tt.want, got) {
 				t.Logf("want:")

ipn/ipnlocal/serve.go

@@ -55,7 +55,7 @@ var serveHTTPContextKey ctxkey.Key[*serveHTTPContext]
 
 type serveHTTPContext struct {
 	SrcAddr       netip.AddrPort
-	ForVIPService string // VIP service name, empty string means local
+	ForVIPService tailcfg.ServiceName // "" means local
 	DestPort      uint16
 
 	// provides funnel-specific context, nil if not funneled
@@ -1006,7 +1006,7 @@ func allNumeric(s string) bool {
 	return s != ""
 }
 
-func (b *LocalBackend) webServerConfig(hostname string, forVIPService string, port uint16) (c ipn.WebServerConfigView, ok bool) {
+func (b *LocalBackend) webServerConfig(hostname string, forVIPService tailcfg.ServiceName, port uint16) (c ipn.WebServerConfigView, ok bool) {
 	key := ipn.HostPort(fmt.Sprintf("%s:%v", hostname, port))
 
 	b.mu.Lock()
@@ -1021,7 +1021,7 @@ func (b *LocalBackend) webServerConfig(hostname string, forVIPService string, po
 	return b.serveConfig.FindWeb(key)
 }
 
-func (b *LocalBackend) getTLSServeCertForPort(port uint16, forVIPService string) func(hi *tls.ClientHelloInfo) (*tls.Certificate, error) {
+func (b *LocalBackend) getTLSServeCertForPort(port uint16, forVIPService tailcfg.ServiceName) func(hi *tls.ClientHelloInfo) (*tls.Certificate, error) {
 	return func(hi *tls.ClientHelloInfo) (*tls.Certificate, error) {
 		if hi == nil || hi.ServerName == "" {
 			return nil, errors.New("no SNI ServerName")

ipn/ipnlocal/serve_test.go

@@ -354,7 +354,7 @@ func TestServeConfigServices(t *testing.T) {
 		{
 			name: "one-incorrectly-configured-service",
 			conf: &ipn.ServeConfig{
-				Services: map[string]*ipn.ServiceConfig{
+				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
 					"svc:foo": {
 						TCP: map[uint16]*ipn.TCPPortHandler{
 							80: {HTTP: true},
@@ -369,7 +369,7 @@ func TestServeConfigServices(t *testing.T) {
 			// one correctly configured service with packet should be intercepted
 			name: "one-service-intercept-packet",
 			conf: &ipn.ServeConfig{
-				Services: map[string]*ipn.ServiceConfig{
+				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
 					"svc:foo": {
 						TCP: map[uint16]*ipn.TCPPortHandler{
 							80: {HTTP: true},
@@ -388,7 +388,7 @@ func TestServeConfigServices(t *testing.T) {
 			// one correctly configured service with packet should not be intercepted
 			name: "one-service-not-intercept-packet",
 			conf: &ipn.ServeConfig{
-				Services: map[string]*ipn.ServiceConfig{
+				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
 					"svc:foo": {
 						TCP: map[uint16]*ipn.TCPPortHandler{
 							80: {HTTP: true},
@@ -406,10 +406,10 @@ func TestServeConfigServices(t *testing.T) {
 			intercepted: false,
 		},
 		{
-			//multiple correctly configured service with packet should be intercepted
+			// multiple correctly configured service with packet should be intercepted
 			name: "multiple-service-intercept-packet",
 			conf: &ipn.ServeConfig{
-				Services: map[string]*ipn.ServiceConfig{
+				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
 					"svc:foo": {
 						TCP: map[uint16]*ipn.TCPPortHandler{
 							80: {HTTP: true},
@@ -437,7 +437,7 @@ func TestServeConfigServices(t *testing.T) {
 			// multiple correctly configured service with packet should not be intercepted
 			name: "multiple-service-not-intercept-packet",
 			conf: &ipn.ServeConfig{
-				Services: map[string]*ipn.ServiceConfig{
+				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
 					"svc:foo": {
 						TCP: map[uint16]*ipn.TCPPortHandler{
 							80: {HTTP: true},

ipn/serve.go

@@ -57,7 +57,7 @@ type ServeConfig struct {
 
 	// Services maps from service name (in the form "svc:dns-label") to a ServiceConfig.
 	// Which describes the L3, L4, and L7 forwarding information for the service.
-	Services map[string]*ServiceConfig `json:",omitempty"`
+	Services map[tailcfg.ServiceName]*ServiceConfig `json:",omitempty"`
 
 	// AllowFunnel is the set of SNI:port values for which funnel
 	// traffic is allowed, from trusted ingress peers.
@@ -618,7 +618,7 @@ func (v ServeConfigView) Webs() iter.Seq2[HostPort, WebServerConfigView] {
 }
 
 // FindServiceTCP return the TCPPortHandlerView for the given service name and port.
-func (v ServeConfigView) FindServiceTCP(svcName string, port uint16) (res TCPPortHandlerView, ok bool) {
+func (v ServeConfigView) FindServiceTCP(svcName tailcfg.ServiceName, port uint16) (res TCPPortHandlerView, ok bool) {
 	svcCfg, ok := v.Services().GetOk(svcName)
 	if !ok {
 		return res, ok
@@ -626,7 +626,7 @@ func (v ServeConfigView) FindServiceTCP(svcName string, port uint16) (res TCPPor
 	return svcCfg.TCP().GetOk(port)
 }
 
-func (v ServeConfigView) FindServiceWeb(svcName string, hp HostPort) (res WebServerConfigView, ok bool) {
+func (v ServeConfigView) FindServiceWeb(svcName tailcfg.ServiceName, hp HostPort) (res WebServerConfigView, ok bool) {
 	if svcCfg, ok := v.Services().GetOk(svcName); ok {
 		if res, ok := svcCfg.Web().GetOk(hp); ok {
 			return res, ok

tailcfg/tailcfg.go

@@ -717,21 +717,6 @@ func CheckTag(tag string) error {
 	return nil
 }
 
-// CheckServiceName validates svc for use as a service name.
-// We only allow valid DNS labels, since the expectation is that these will be
-// used as parts of domain names.
-func CheckServiceName(svc string) error {
-	var ok bool
-	svc, ok = strings.CutPrefix(svc, "svc:")
-	if !ok {
-		return errors.New("services must start with 'svc:'")
-	}
-	if svc == "" {
-		return errors.New("service names must not be empty")
-	}
-	return dnsname.ValidLabel(svc)
-}
-
 // CheckRequestTags checks that all of h.RequestTags are valid.
 func (h *Hostinfo) CheckRequestTags() error {
 	if h == nil {
@@ -897,16 +882,51 @@ type Hostinfo struct {
 	//       require changes to Hostinfo.Equal.
 }
 
+// ServiceName is the name of a service, of the form `svc:dns-label`. Services
+// represent some kind of application provided for users of the tailnet with a
+// MagicDNS name and possibly dedicated IP addresses. Currently (2024-01-21),
+// the only type of service is [VIPService].
+// This is not related to the older [Service] used in [Hostinfo.Services].
+type ServiceName string
+
+// Validate validates if the service name is formatted correctly.
+// We only allow valid DNS labels, since the expectation is that these will be
+// used as parts of domain names.
+func (sn ServiceName) Validate() error {
+	bareName, ok := strings.CutPrefix(string(sn), "svc:")
+	if !ok {
+		return errors.New("services must start with 'svc:'")
+	}
+	if bareName == "" {
+		return errors.New("service names must not be empty")
+	}
+	return dnsname.ValidLabel(bareName)
+}
+
+// String implements [fmt.Stringer].
+func (sn ServiceName) String() string {
+	return string(sn)
+}
+
+// WithoutPrefix is the name of the service without the `svc:` prefix, used for
+// DNS names. If the name does not include the prefix (which means
+// [ServiceName.Validate] would return an error) then it returns "".
+func (sn ServiceName) WithoutPrefix() string {
+	bareName, ok := strings.CutPrefix(string(sn), "svc:")
+	if !ok {
+		return ""
+	}
+	return bareName
+}
+
 // VIPService represents a service created on a tailnet from the
 // perspective of a node providing that service. These services
 // have an virtual IP (VIP) address pair distinct from the node's IPs.
 type VIPService struct {
-	// Name is the name of the service, of the form `svc:dns-label`.
+	// Name is the name of the service. The Name uniquely identifies a service
-	// See CheckServiceName for a validation func.
+	// on a particular tailnet, and so also corresponds uniquely to the pair of
-	// Name uniquely identifies a service on a particular tailnet,
+	// IP addresses belonging to the VIP service.
-	// and so also corresponds uniquely to the pair of IP addresses
+	Name ServiceName
-	// belonging to the VIP service.
-	Name string
 
 	// Ports specify which ProtoPorts are made available by this node
 	// on the service's IPs.
@@ -2972,10 +2992,10 @@ type EarlyNoise struct {
 // vs NodeKey)
 const LBHeader = "Ts-Lb"
 
-// ServiceIPMappings maps service names (strings that conform to
+// ServiceIPMappings maps ServiceName to lists of IP addresses. This is used
-// [CheckServiceName]) to lists of IP addresses. This is used as the value of
+// as the value of the [NodeAttrServiceHost] capability, to inform service hosts
-// the [NodeAttrServiceHost] capability, to inform service hosts what IP
+// what IP addresses they need to listen on for each service that they are
-// addresses they need to listen on for each service that they are advertising.
+// advertising.
 //
 // This is of the form:
 //
@@ -2988,4 +3008,4 @@ const LBHeader = "Ts-Lb"
 // provided in AllowedIPs, but this lets the client know which services
 // correspond to those IPs. Any services that don't correspond to a service
 // this client is hosting can be ignored.
-type ServiceIPMappings map[string][]netip.Addr
+type ServiceIPMappings map[ServiceName][]netip.Addr

types/netmap/netmap.go

@@ -427,10 +427,10 @@ const (
 )
 
 // IPServiceMappings maps IP addresses to service names. This is the inverse of
-// [ServiceIPMappings], and is used to inform clients which services is an VIP
+// [tailcfg.ServiceIPMappings], and is used to inform track which service a VIP
-// address associated with. This is set to b.ipVIPServiceMap every time the
+// is associated with. This is set to b.ipVIPServiceMap every time the netmap is
-// netmap is updated. This is used to reduce the cost for looking up the service
+// updated. This is used to reduce the cost for looking up the service name for
-// name for the dst IP address in the netStack packet processing workflow.
+// the dst IP address in the netStack packet processing workflow.
 //
 // This is of the form:
 //
@@ -440,4 +440,4 @@ const (
 //	  "100.102.42.3": "svc:web",
 //	  "fd7a:115c:a1e0::abcd": "svc:web",
 //	}
-type IPServiceMappings map[netip.Addr]string
+type IPServiceMappings map[netip.Addr]tailcfg.ServiceName