cmd/k8s-operator: advertise VIPServices in ProxyGroup config (#14946)

Now that packets flow for VIPServices, the last piece needed to start
serving them from a ProxyGroup is config to tell the proxy Pods which
services they should advertise.

Updates tailscale/corp#24795

Change-Id: Ic7bbeac8e93c9503558107bc5f6123be02a84c77
Signed-off-by: Tom Proctor <tomhjp@users.noreply.github.com>

cmd/k8s-operator/egress-services.go

@@ -630,7 +630,11 @@ func tailnetTargetFromSvc(svc *corev1.Service) egressservices.TailnetTarget {
 
 func portMap(p corev1.ServicePort) egressservices.PortMap {
 	// TODO (irbekrm): out of bounds check?
-	return egressservices.PortMap{Protocol: string(p.Protocol), MatchPort: uint16(p.TargetPort.IntVal), TargetPort: uint16(p.Port)}
+	return egressservices.PortMap{
+		Protocol:   string(p.Protocol),
+		MatchPort:  uint16(p.TargetPort.IntVal),
+		TargetPort: uint16(p.Port),
+	}
 }
 
 func isEgressSvcForProxyGroup(obj client.Object) bool {

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

@@ -99,7 +99,7 @@ func (a *IngressPGReconciler) Reconcile(ctx context.Context, req reconcile.Reque
 	hostname := hostnameForIngress(ing)
 	logger = logger.With("hostname", hostname)
 
-	if !ing.DeletionTimestamp.IsZero() || !a.shouldExpose(ing) {
+	if !ing.DeletionTimestamp.IsZero() || !shouldExpose(ing) {
 		return res, a.maybeCleanup(ctx, hostname, ing, logger)
 	}
 
@@ -122,6 +122,8 @@ func (a *IngressPGReconciler) maybeProvision(ctx context.Context, hostname strin
 		logger.Infof("[unexpected] no ProxyGroup annotation, skipping VIPService provisioning")
 		return nil
 	}
+	logger = logger.With("ProxyGroup", pgName)
+
 	pg := &tsapi.ProxyGroup{}
 	if err := a.Get(ctx, client.ObjectKey{Name: pgName}, pg); err != nil {
 		if apierrors.IsNotFound(err) {
@@ -148,8 +150,6 @@ func (a *IngressPGReconciler) maybeProvision(ctx context.Context, hostname strin
 		a.recorder.Event(ing, corev1.EventTypeWarning, "HTTPSNotEnabled", "HTTPS is not enabled on the tailnet; ingress may not work")
 	}
 
-	logger = logger.With("proxy-group", pg)
-
 	if !slices.Contains(ing.Finalizers, FinalizerNamePG) {
 		// This log line is printed exactly once during initial provisioning,
 		// because once the finalizer is in place this block gets skipped. So,
@@ -288,7 +288,13 @@ func (a *IngressPGReconciler) maybeProvision(ctx context.Context, hostname strin
 		}
 	}
 
-	// 5. Update Ingress status
+	// 5. Update tailscaled's AdvertiseServices config, which should add the VIPService
+	// IPs to the ProxyGroup Pods' AllowedIPs in the next netmap update if approved.
+	if err = a.maybeUpdateAdvertiseServicesConfig(ctx, pg.Name, serviceName, true, logger); err != nil {
+		return fmt.Errorf("failed to update tailscaled config: %w", err)
+	}
+
+	// 6. Update Ingress status
 	oldStatus := ing.Status.DeepCopy()
 	ports := []networkingv1.IngressPortStatus{
 		{
@@ -320,9 +326,9 @@ func (a *IngressPGReconciler) maybeProvision(ctx context.Context, hostname strin
 // maybeCleanupProxyGroup ensures that if an Ingress hostname has changed, any VIPService resources created for the
 // Ingress' ProxyGroup corresponding to the old hostname are cleaned up. A run of this function will ensure that any
 // VIPServices that are associated with the provided ProxyGroup and no longer owned by an Ingress are cleaned up.
-func (a *IngressPGReconciler) maybeCleanupProxyGroup(ctx context.Context, proxyGroupName string, logger *zap.SugaredLogger) error {
+func (a *IngressPGReconciler) maybeCleanupProxyGroup(ctx context.Context, pgName string, logger *zap.SugaredLogger) error {
 	// Get serve config for the ProxyGroup
-	cm, cfg, err := a.proxyGroupServeConfig(ctx, proxyGroupName)
+	cm, cfg, err := a.proxyGroupServeConfig(ctx, pgName)
 	if err != nil {
 		return fmt.Errorf("getting serve config: %w", err)
 	}
@@ -349,17 +355,16 @@ func (a *IngressPGReconciler) maybeCleanupProxyGroup(ctx context.Context, proxyG
 
 		if !found {
 			logger.Infof("VIPService %q is not owned by any Ingress, cleaning up", vipServiceName)
-			svc, err := a.getVIPService(ctx, vipServiceName, logger)
+
+			// Delete the VIPService from control if necessary.
+			svc, err := a.tsClient.GetVIPService(ctx, vipServiceName)
 			if err != nil {
 				errResp := &tailscale.ErrResponse{}
-				if errors.As(err, &errResp) && errResp.Status == http.StatusNotFound {
+				if ok := errors.As(err, errResp); !ok || errResp.Status != http.StatusNotFound {
-					delete(cfg.Services, vipServiceName)
-					serveConfigChanged = true
-					continue
-				}
 					return err
 				}
-			if isVIPServiceForAnyIngress(svc) {
+			}
+			if svc != nil && isVIPServiceForAnyIngress(svc) {
 				logger.Infof("cleaning up orphaned VIPService %q", vipServiceName)
 				if err := a.tsClient.DeleteVIPService(ctx, vipServiceName); err != nil {
 					errResp := &tailscale.ErrResponse{}
@@ -368,6 +373,11 @@ func (a *IngressPGReconciler) maybeCleanupProxyGroup(ctx context.Context, proxyG
 					}
 				}
 			}
+
+			// Make sure the VIPService is not advertised in tailscaled or serve config.
+			if err = a.maybeUpdateAdvertiseServicesConfig(ctx, pgName, vipServiceName, false, logger); err != nil {
+				return fmt.Errorf("failed to update tailscaled config services: %w", err)
+			}
 			delete(cfg.Services, vipServiceName)
 			serveConfigChanged = true
 		}
@@ -383,6 +393,7 @@ func (a *IngressPGReconciler) maybeCleanupProxyGroup(ctx context.Context, proxyG
 			return fmt.Errorf("updating serve config: %w", err)
 		}
 	}
+
 	return nil
 }
 
@@ -421,7 +432,12 @@ func (a *IngressPGReconciler) maybeCleanup(ctx context.Context, hostname string,
 		return fmt.Errorf("error deleting VIPService: %w", err)
 	}
 
-	// 3. Remove the VIPService from the serve config for the ProxyGroup.
+	// 3. Unadvertise the VIPService in tailscaled config.
+	if err = a.maybeUpdateAdvertiseServicesConfig(ctx, pg, serviceName, false, logger); err != nil {
+		return fmt.Errorf("failed to update tailscaled config services: %w", err)
+	}
+
+	// 4. 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, serviceName)
 	cfgBytes, err := json.Marshal(cfg)
@@ -501,7 +517,7 @@ func (a *IngressPGReconciler) tailnetCertDomain(ctx context.Context) (string, er
 }
 
 // shouldExpose returns true if the Ingress should be exposed over Tailscale in HA mode (on a ProxyGroup)
-func (a *IngressPGReconciler) shouldExpose(ing *networkingv1.Ingress) bool {
+func shouldExpose(ing *networkingv1.Ingress) bool {
 	isTSIngress := ing != nil &&
 		ing.Spec.IngressClassName != nil &&
 		*ing.Spec.IngressClassName == tailscaleIngressClassName
@@ -509,18 +525,6 @@ func (a *IngressPGReconciler) shouldExpose(ing *networkingv1.Ingress) bool {
 	return isTSIngress && pgAnnot != ""
 }
 
-func (a *IngressPGReconciler) getVIPService(ctx context.Context, name tailcfg.ServiceName, logger *zap.SugaredLogger) (*tailscale.VIPService, error) {
-	svc, err := a.tsClient.GetVIPService(ctx, name)
-	if err != nil {
-		errResp := &tailscale.ErrResponse{}
-		if ok := errors.As(err, errResp); ok && errResp.Status != http.StatusNotFound {
-			logger.Infof("error getting VIPService %q: %v", name, err)
-			return nil, fmt.Errorf("error getting VIPService %q: %w", name, err)
-		}
-	}
-	return svc, nil
-}
-
 func isVIPServiceForIngress(svc *tailscale.VIPService, ing *networkingv1.Ingress) bool {
 	if svc == nil || ing == nil {
 		return false
@@ -582,12 +586,16 @@ func (a *IngressPGReconciler) validateIngress(ing *networkingv1.Ingress, pg *tsa
 
 // deleteVIPServiceIfExists attempts to delete the VIPService if it exists and is owned by the given Ingress.
 func (a *IngressPGReconciler) deleteVIPServiceIfExists(ctx context.Context, name tailcfg.ServiceName, ing *networkingv1.Ingress, logger *zap.SugaredLogger) error {
-	svc, err := a.getVIPService(ctx, name, logger)
+	svc, err := a.tsClient.GetVIPService(ctx, name)
 	if err != nil {
+		errResp := &tailscale.ErrResponse{}
+		if ok := errors.As(err, errResp); ok && errResp.Status == http.StatusNotFound {
+			return nil
+		}
+
 		return fmt.Errorf("error getting VIPService: %w", err)
 	}
 
-	// isVIPServiceForIngress handles nil svc, so we don't need to check it here
 	if !isVIPServiceForIngress(svc, ing) {
 		return nil
 	}
@@ -606,3 +614,54 @@ func isHTTPEndpointEnabled(ing *networkingv1.Ingress) bool {
 	}
 	return ing.Annotations[annotationHTTPEndpoint] == "enabled"
 }
+
+func (a *IngressPGReconciler) maybeUpdateAdvertiseServicesConfig(ctx context.Context, pgName string, serviceName tailcfg.ServiceName, shouldBeAdvertised bool, logger *zap.SugaredLogger) (err error) {
+	logger.Debugf("Updating ProxyGroup tailscaled configs to advertise service %q: %v", serviceName, shouldBeAdvertised)
+
+	// Get all config Secrets for this ProxyGroup.
+	secrets := &corev1.SecretList{}
+	if err := a.List(ctx, secrets, client.InNamespace(a.tsNamespace), client.MatchingLabels(pgSecretLabels(pgName, "config"))); err != nil {
+		return fmt.Errorf("failed to list config Secrets: %w", err)
+	}
+
+	for _, secret := range secrets.Items {
+		var updated bool
+		for fileName, confB := range secret.Data {
+			var conf ipn.ConfigVAlpha
+			if err := json.Unmarshal(confB, &conf); err != nil {
+				return fmt.Errorf("error unmarshalling ProxyGroup config: %w", err)
+			}
+
+			// Update the services to advertise if required.
+			idx := slices.Index(conf.AdvertiseServices, serviceName.String())
+			isAdvertised := idx >= 0
+			switch {
+			case isAdvertised == shouldBeAdvertised:
+				// Already up to date.
+				continue
+			case isAdvertised:
+				// Needs to be removed.
+				conf.AdvertiseServices = slices.Delete(conf.AdvertiseServices, idx, idx+1)
+			case shouldBeAdvertised:
+				// Needs to be added.
+				conf.AdvertiseServices = append(conf.AdvertiseServices, serviceName.String())
+			}
+
+			// Update the Secret.
+			confB, err := json.Marshal(conf)
+			if err != nil {
+				return fmt.Errorf("error marshalling ProxyGroup config: %w", err)
+			}
+			mak.Set(&secret.Data, fileName, confB)
+			updated = true
+		}
+
+		if updated {
+			if err := a.Update(ctx, &secret); err != nil {
+				return fmt.Errorf("error updating ProxyGroup config Secret: %w", err)
+			}
+		}
+	}
+
+	return nil
+}

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

@@ -8,6 +8,7 @@ package main
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"maps"
 	"reflect"
 	"testing"
@@ -24,6 +25,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"tailscale.com/ipn"
 	"tailscale.com/ipn/ipnstate"
+	tsoperator "tailscale.com/k8s-operator"
 	tsapi "tailscale.com/k8s-operator/apis/v1alpha1"
 	"tailscale.com/tailcfg"
 	"tailscale.com/types/ptr"
@@ -63,6 +65,7 @@ func TestIngressPGReconciler(t *testing.T) {
 	expectReconciled(t, ingPGR, "default", "test-ingress")
 	verifyServeConfig(t, fc, "svc:my-svc", false)
 	verifyVIPService(t, ft, "svc:my-svc", []string{"443"})
+	verifyTailscaledConfig(t, fc, []string{"svc:my-svc"})
 
 	mustUpdate(t, fc, "default", "test-ingress", func(ing *networkingv1.Ingress) {
 		ing.Annotations["tailscale.com/tags"] = "tag:custom,tag:test"
@@ -122,6 +125,8 @@ func TestIngressPGReconciler(t *testing.T) {
 	verifyServeConfig(t, fc, "svc:my-svc", false)
 	verifyVIPService(t, ft, "svc:my-svc", []string{"443"})
 
+	verifyTailscaledConfig(t, fc, []string{"svc:my-svc", "svc:my-other-svc"})
+
 	// Delete second Ingress
 	if err := fc.Delete(context.Background(), ing2); err != nil {
 		t.Fatalf("deleting second Ingress: %v", err)
@@ -151,6 +156,8 @@ func TestIngressPGReconciler(t *testing.T) {
 		t.Error("second Ingress service config was not cleaned up")
 	}
 
+	verifyTailscaledConfig(t, fc, []string{"svc:my-svc"})
+
 	// Delete the first Ingress and verify cleanup
 	if err := fc.Delete(context.Background(), ing); err != nil {
 		t.Fatalf("deleting Ingress: %v", err)
@@ -175,6 +182,7 @@ func TestIngressPGReconciler(t *testing.T) {
 	if len(cfg.Services) > 0 {
 		t.Error("serve config not cleaned up")
 	}
+	verifyTailscaledConfig(t, fc, nil)
 }
 
 func TestValidateIngress(t *testing.T) {
@@ -464,6 +472,27 @@ func verifyServeConfig(t *testing.T, fc client.Client, serviceName string, wantH
 	}
 }
 
+func verifyTailscaledConfig(t *testing.T, fc client.Client, expectedServices []string) {
+	var expected string
+	if expectedServices != nil {
+		expectedServicesJSON, err := json.Marshal(expectedServices)
+		if err != nil {
+			t.Fatalf("marshaling expected services: %v", err)
+		}
+		expected = fmt.Sprintf(`,"AdvertiseServices":%s`, expectedServicesJSON)
+	}
+	expectEqual(t, fc, &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      pgConfigSecretName("test-pg", 0),
+			Namespace: "operator-ns",
+			Labels:    pgSecretLabels("test-pg", "config"),
+		},
+		Data: map[string][]byte{
+			tsoperator.TailscaledConfigFileName(106): []byte(fmt.Sprintf(`{"Version":""%s}`, expected)),
+		},
+	})
+}
+
 func setupIngressTest(t *testing.T) (*IngressPGReconciler, client.Client, *fakeTSClient) {
 	t.Helper()
 
@@ -494,9 +523,21 @@ func setupIngressTest(t *testing.T) (*IngressPGReconciler, client.Client, *fakeT
 		},
 	}
 
+	// Pre-create a config Secret for the ProxyGroup
+	pgCfgSecret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      pgConfigSecretName("test-pg", 0),
+			Namespace: "operator-ns",
+			Labels:    pgSecretLabels("test-pg", "config"),
+		},
+		Data: map[string][]byte{
+			tsoperator.TailscaledConfigFileName(106): []byte("{}"),
+		},
+	}
+
 	fc := fake.NewClientBuilder().
 		WithScheme(tsapi.GlobalScheme).
-		WithObjects(pg, pgConfigMap, tsIngressClass).
+		WithObjects(pg, pgCfgSecret, pgConfigMap, tsIngressClass).
 		WithStatusSubresource(pg).
 		Build()
 

cmd/k8s-operator/proxygroup.go

@@ -452,7 +452,7 @@ func (r *ProxyGroupReconciler) ensureConfigSecretsCreated(ctx context.Context, p
 	for i := range pgReplicas(pg) {
 		cfgSecret := &corev1.Secret{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:            fmt.Sprintf("%s-%d-config", pg.Name, i),
+				Name:            pgConfigSecretName(pg.Name, i),
 				Namespace:       r.tsNamespace,
 				Labels:          pgSecretLabels(pg.Name, "config"),
 				OwnerReferences: pgOwnerReference(pg),
@@ -596,10 +596,35 @@ func pgTailscaledConfig(pg *tsapi.ProxyGroup, class *tsapi.ProxyClass, idx int32
 		conf.AuthKey = key
 	}
 	capVerConfigs := make(map[tailcfg.CapabilityVersion]ipn.ConfigVAlpha)
+
+	// AdvertiseServices config is set by ingress-pg-reconciler, so make sure we
+	// don't overwrite it here.
+	if err := copyAdvertiseServicesConfig(conf, oldSecret, 106); err != nil {
+		return nil, err
+	}
 	capVerConfigs[106] = *conf
 	return capVerConfigs, nil
 }
 
+func copyAdvertiseServicesConfig(conf *ipn.ConfigVAlpha, oldSecret *corev1.Secret, capVer tailcfg.CapabilityVersion) error {
+	if oldSecret == nil {
+		return nil
+	}
+
+	oldConfB := oldSecret.Data[tsoperator.TailscaledConfigFileName(capVer)]
+	if len(oldConfB) == 0 {
+		return nil
+	}
+
+	var oldConf ipn.ConfigVAlpha
+	if err := json.Unmarshal(oldConfB, &oldConf); err != nil {
+		return fmt.Errorf("error unmarshalling existing config: %w", err)
+	}
+	conf.AdvertiseServices = oldConf.AdvertiseServices
+
+	return nil
+}
+
 func (r *ProxyGroupReconciler) validate(_ *tsapi.ProxyGroup) error {
 	return nil
 }

cmd/k8s-operator/proxygroup_specs.go

@@ -73,7 +73,7 @@ func pgStatefulSet(pg *tsapi.ProxyGroup, namespace, image, tsFirewallMode string
 				Name: fmt.Sprintf("tailscaledconfig-%d", i),
 				VolumeSource: corev1.VolumeSource{
 					Secret: &corev1.SecretVolumeSource{
-						SecretName: fmt.Sprintf("%s-%d-config", pg.Name, i),
+						SecretName: pgConfigSecretName(pg.Name, i),
 					},
 				},
 			})
@@ -236,7 +236,7 @@ func pgRole(pg *tsapi.ProxyGroup, namespace string) *rbacv1.Role {
 				ResourceNames: func() (secrets []string) {
 					for i := range pgReplicas(pg) {
 						secrets = append(secrets,
-							fmt.Sprintf("%s-%d-config", pg.Name, i), // Config with auth key.
+							pgConfigSecretName(pg.Name, i),   // Config with auth key.
 							fmt.Sprintf("%s-%d", pg.Name, i), // State.
 						)
 					}
@@ -349,6 +349,10 @@ func pgReplicas(pg *tsapi.ProxyGroup) int32 {
 	return 2
 }
 
+func pgConfigSecretName(pgName string, i int32) string {
+	return fmt.Sprintf("%s-%d-config", pgName, i)
+}
+
 func pgEgressCMName(pg string) string {
 	return fmt.Sprintf("%s-egress-config", pg)
 }

cmd/k8s-operator/proxygroup_test.go

@@ -24,6 +24,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"tailscale.com/client/tailscale"
+	"tailscale.com/ipn"
 	tsoperator "tailscale.com/k8s-operator"
 	tsapi "tailscale.com/k8s-operator/apis/v1alpha1"
 	"tailscale.com/kube/kubetypes"
@@ -446,6 +447,79 @@ func TestProxyGroupTypes(t *testing.T) {
 	})
 }
 
+func TestIngressAdvertiseServicesConfigPreserved(t *testing.T) {
+	fc := fake.NewClientBuilder().
+		WithScheme(tsapi.GlobalScheme).
+		Build()
+	reconciler := &ProxyGroupReconciler{
+		tsNamespace: tsNamespace,
+		proxyImage:  testProxyImage,
+		Client:      fc,
+		l:           zap.Must(zap.NewDevelopment()).Sugar(),
+		tsClient:    &fakeTSClient{},
+		clock:       tstest.NewClock(tstest.ClockOpts{}),
+	}
+
+	existingServices := []string{"svc1", "svc2"}
+	existingConfigBytes, err := json.Marshal(ipn.ConfigVAlpha{
+		AdvertiseServices: existingServices,
+		Version:           "should-get-overwritten",
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	const pgName = "test-ingress"
+	mustCreate(t, fc, &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      pgConfigSecretName(pgName, 0),
+			Namespace: tsNamespace,
+		},
+		// Write directly to Data because the fake client doesn't copy the write-only
+		// StringData field across to Data for us.
+		Data: map[string][]byte{
+			tsoperator.TailscaledConfigFileName(106): existingConfigBytes,
+		},
+	})
+
+	mustCreate(t, fc, &tsapi.ProxyGroup{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: pgName,
+			UID:  "test-ingress-uid",
+		},
+		Spec: tsapi.ProxyGroupSpec{
+			Type:     tsapi.ProxyGroupTypeIngress,
+			Replicas: ptr.To[int32](1),
+		},
+	})
+	expectReconciled(t, reconciler, "", pgName)
+
+	expectedConfigBytes, err := json.Marshal(ipn.ConfigVAlpha{
+		// Preserved.
+		AdvertiseServices: existingServices,
+
+		// Everything else got updated in the reconcile:
+		Version:      "alpha0",
+		AcceptDNS:    "false",
+		AcceptRoutes: "false",
+		Locked:       "false",
+		Hostname:     ptr.To(fmt.Sprintf("%s-%d", pgName, 0)),
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	expectEqual(t, fc, &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:            pgConfigSecretName(pgName, 0),
+			Namespace:       tsNamespace,
+			ResourceVersion: "2",
+		},
+		StringData: map[string]string{
+			tsoperator.TailscaledConfigFileName(106): string(expectedConfigBytes),
+		},
+	}, omitSecretData)
+}
+
 func verifyProxyGroupCounts(t *testing.T, r *ProxyGroupReconciler, wantIngress, wantEgress int) {
 	t.Helper()
 	if r.ingressProxyGroups.Len() != wantIngress {
@@ -501,7 +575,7 @@ func expectProxyGroupResources(t *testing.T, fc client.WithWatch, pg *tsapi.Prox
 		for i := range pgReplicas(pg) {
 			expectedSecrets = append(expectedSecrets,
 				fmt.Sprintf("%s-%d", pg.Name, i),
-				fmt.Sprintf("%s-%d-config", pg.Name, i),
+				pgConfigSecretName(pg.Name, i),
 			)
 		}
 	}
@@ -546,3 +620,11 @@ func addNodeIDToStateSecrets(t *testing.T, fc client.WithWatch, pg *tsapi.ProxyG
 		})
 	}
 }
+
+// The operator mostly writes to StringData and reads from Data, but the fake
+// client doesn't copy StringData across to Data on write. When comparing actual
+// vs expected Secrets, use this function to only check what the operator writes
+// to StringData.
+func omitSecretData(secret *corev1.Secret) {
+	secret.Data = nil
+}