cmd/k8s-operator: fix some missing cleanup + unit tests

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

cmd/k8s-operator/api-server-proxy-pg.go

@@ -284,6 +284,10 @@ func (r *APIServerProxyServiceReconciler) maybeDeleteStaleServices(ctx context.C
 		if err := r.tsClient.DeleteVIPService(ctx, svc.Name); err != nil && !isErrorTailscaleServiceNotFound(err) {
 			return fmt.Errorf("error deleting Tailscale Service %s: %w", svc.Name, err)
 		}
+
+		if err = cleanupCertResources(ctx, r.Client, r.lc, r.tsNamespace, pg.Name, svc.Name); err != nil {
+			return fmt.Errorf("failed to clean up cert resources: %w", err)
+		}
 	}
 
 	return nil
@@ -339,6 +343,10 @@ func (r *APIServerProxyServiceReconciler) maybeAdvertiseServices(ctx context.Con
 	if err != nil {
 		return fmt.Errorf("error checking TLS credentials provisioned for Tailscale Service %q: %w", serviceName, err)
 	}
+	var advertiseServices []string
+	if shouldBeAdvertised {
+		advertiseServices = []string{serviceName.String()}
+	}
 
 	for _, s := range cfgSecrets.Items {
 		if len(s.Data[kubetypes.KubeAPIServerConfigFile]) == 0 {
@@ -352,7 +360,7 @@ func (r *APIServerProxyServiceReconciler) maybeAdvertiseServices(ctx context.Con
 		}
 
 		if cfg.Parsed.APIServerProxy == nil {
-			return fmt.Errorf("[unexpected] config Secret %q does not contain APIServerProxy config", s.Name)
+			return fmt.Errorf("config Secret %q does not contain APIServerProxy config", s.Name)
 		}
 
 		existingCfgSecret := s.DeepCopy()
@@ -364,18 +372,8 @@ func (r *APIServerProxyServiceReconciler) maybeAdvertiseServices(ctx context.Con
 		}
 
 		// Update the services to advertise if required.
-		idx := slices.Index(cfg.Parsed.AdvertiseServices, serviceName.String())
+		if !slices.Equal(cfg.Parsed.AdvertiseServices, advertiseServices) {
-		isAdvertised := idx >= 0
+			cfg.Parsed.AdvertiseServices = advertiseServices
-		switch {
-		case isAdvertised == shouldBeAdvertised:
-			// Already up to date.
-		case isAdvertised:
-			// Needs to be removed.
-			cfg.Parsed.AdvertiseServices = slices.Delete(cfg.Parsed.AdvertiseServices, idx, idx+1)
-			updated = true
-		case shouldBeAdvertised:
-			// Needs to be added.
-			cfg.Parsed.AdvertiseServices = append(cfg.Parsed.AdvertiseServices, serviceName.String())
 			updated = true
 		}
 

cmd/k8s-operator/api-server-proxy-pg_test.go

@@ -4,15 +4,254 @@
 package main
 
 import (
+	"encoding/json"
+	"reflect"
 	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"go.uber.org/zap"
+	corev1 "k8s.io/api/core/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/tools/record"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"tailscale.com/internal/client/tailscale"
+	"tailscale.com/ipn/ipnstate"
+	tsoperator "tailscale.com/k8s-operator"
 	tsapi "tailscale.com/k8s-operator/apis/v1alpha1"
+	"tailscale.com/kube/k8s-proxy/conf"
+	"tailscale.com/kube/kubetypes"
+	"tailscale.com/tailcfg"
+	"tailscale.com/tstest"
+	"tailscale.com/types/opt"
+	"tailscale.com/types/ptr"
 )
 
+func TestAPIServerProxyReconciler(t *testing.T) {
+	const (
+		pgName        = "test-pg"
+		pgUID         = "test-pg-uid"
+		ns            = "operator-ns"
+		defaultDomain = "test-pg.ts.net"
+	)
+	pg := &tsapi.ProxyGroup{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:       pgName,
+			Generation: 1,
+			UID:        pgUID,
+		},
+		Spec: tsapi.ProxyGroupSpec{
+			Type: tsapi.ProxyGroupTypeKubernetesAPIServer,
+		},
+		Status: tsapi.ProxyGroupStatus{
+			Conditions: []metav1.Condition{
+				{
+					Type:               string(tsapi.ProxyGroupAvailable),
+					Status:             metav1.ConditionTrue,
+					ObservedGeneration: 1,
+				},
+			},
+		},
+	}
+	initialCfg := &conf.VersionedConfig{
+		Version: "v1alpha1",
+		ConfigV1Alpha1: &conf.ConfigV1Alpha1{
+			AuthKey: ptr.To("test-key"),
+			APIServerProxy: &conf.APIServerProxyConfig{
+				Enabled: opt.NewBool(true),
+			},
+		},
+	}
+	expectedCfg := *initialCfg
+	initialCfgB, err := json.Marshal(initialCfg)
+	if err != nil {
+		t.Fatalf("marshaling initial config: %v", err)
+	}
+	pgCfgSecret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      pgConfigSecretName(pgName, 0),
+			Namespace: ns,
+			Labels:    pgSecretLabels(pgName, kubetypes.LabelSecretTypeConfig),
+		},
+		Data: map[string][]byte{
+			// Existing config should be preserved.
+			kubetypes.KubeAPIServerConfigFile: initialCfgB,
+		},
+	}
+	fc := fake.NewClientBuilder().
+		WithScheme(tsapi.GlobalScheme).
+		WithObjects(pg, pgCfgSecret).
+		WithStatusSubresource(pg).
+		Build()
+	expectCfg := func(c *conf.VersionedConfig) {
+		t.Helper()
+		cBytes, err := json.Marshal(c)
+		if err != nil {
+			t.Fatalf("marshaling expected config: %v", err)
+		}
+		pgCfgSecret.Data[kubetypes.KubeAPIServerConfigFile] = cBytes
+		expectEqual(t, fc, pgCfgSecret)
+	}
+
+	ft := &fakeTSClient{}
+
+	lc := &fakeLocalClient{
+		status: &ipnstate.Status{
+			CurrentTailnet: &ipnstate.TailnetStatus{
+				MagicDNSSuffix: "ts.net",
+			},
+		},
+	}
+
+	r := &APIServerProxyServiceReconciler{
+		Client:      fc,
+		tsClient:    ft,
+		defaultTags: []string{"tag:k8s"},
+		tsNamespace: ns,
+		logger:      zap.Must(zap.NewDevelopment()).Sugar(),
+		recorder:    record.NewFakeRecorder(10),
+		lc:          lc,
+		clock:       tstest.NewClock(tstest.ClockOpts{}),
+		operatorID:  "self-id",
+	}
+
+	// Create a Tailscale Service that will conflict with the initial config.
+	if err := ft.CreateOrUpdateVIPService(t.Context(), &tailscale.VIPService{
+		Name: "svc:" + pgName,
+	}); err != nil {
+		t.Fatalf("creating initial Tailscale Service: %v", err)
+	}
+	expectReconciled(t, r, "", pgName)
+	pg.ObjectMeta.Finalizers = []string{proxyPGFinalizerName}
+	tsoperator.SetProxyGroupCondition(pg, tsapi.PGTailscaleServiceValid, metav1.ConditionFalse, reasonPGTailscaleServiceInvalid, "", 1, r.clock, r.logger)
+	tsoperator.SetProxyGroupCondition(pg, tsapi.PGTailscaleServiceConfigured, metav1.ConditionFalse, reasonPGTailscaleServiceNoBackends, "", 1, r.clock, r.logger)
+	omitStatusConditionMessages := func(p *tsapi.ProxyGroup) {
+		for i := range p.Status.Conditions {
+			// Don't bother validating the message.
+			p.Status.Conditions[i].Message = ""
+		}
+	}
+	expectEqual(t, fc, pg, omitStatusConditionMessages)
+	expectMissing[corev1.Secret](t, fc, ns, defaultDomain)
+	expectMissing[rbacv1.Role](t, fc, ns, defaultDomain)
+	expectMissing[rbacv1.RoleBinding](t, fc, ns, defaultDomain)
+	expectEqual(t, fc, pgCfgSecret) // Unchanged.
+
+	// Delete Tailscale Service; should see Service created and valid condition updated to true.
+	if err := ft.DeleteVIPService(t.Context(), "svc:"+pgName); err != nil {
+		t.Fatalf("deleting initial Tailscale Service: %v", err)
+	}
+	expectReconciled(t, r, "", pgName)
+
+	tsSvc, err := ft.GetVIPService(t.Context(), "svc:"+pgName)
+	if err != nil {
+		t.Fatalf("getting Tailscale Service: %v", err)
+	}
+	if tsSvc == nil {
+		t.Fatalf("expected Tailscale Service to be created, but got nil")
+	}
+	expectedTSSvc := &tailscale.VIPService{
+		Name:    "svc:" + pgName,
+		Comment: managedTSServiceComment,
+		Annotations: map[string]string{
+			ownerAnnotation: `{"ownerRefs":[{"operatorID":"self-id","resource":{"kind":"ProxyGroup","name":"test-pg","uid":"test-pg-uid"}}]}`,
+		},
+		Ports: []string{"tcp:443"},
+		Tags:  []string{"tag:k8s"},
+		Addrs: []string{"5.6.7.8"},
+	}
+	if !reflect.DeepEqual(tsSvc, expectedTSSvc) {
+		t.Fatalf("expected Tailscale Service to be %+v, got %+v", expectedTSSvc, tsSvc)
+	}
+	tsoperator.SetProxyGroupCondition(pg, tsapi.PGTailscaleServiceValid, metav1.ConditionTrue, reasonPGTailscaleServiceValid, "", 1, r.clock, r.logger)
+	tsoperator.SetProxyGroupCondition(pg, tsapi.PGTailscaleServiceConfigured, metav1.ConditionFalse, reasonPGTailscaleServiceNoBackends, "", 1, r.clock, r.logger)
+	expectEqual(t, fc, pg, omitStatusConditionMessages)
+
+	expectedCfg.APIServerProxy.ServiceName = ptr.To(tailcfg.ServiceName("svc:" + pgName))
+	expectCfg(&expectedCfg)
+
+	expectEqual(t, fc, certSecret(pgName, ns, defaultDomain, pg))
+	expectEqual(t, fc, certSecretRole(pgName, ns, defaultDomain))
+	expectEqual(t, fc, certSecretRoleBinding(pg, ns, defaultDomain))
+
+	// Simulate certs being issued; should observe AdvertiseServices config change.
+	if err := populateTLSSecret(t.Context(), fc, pgName, defaultDomain); err != nil {
+		t.Fatalf("populating TLS Secret: %v", err)
+	}
+	expectReconciled(t, r, "", pgName)
+
+	expectedCfg.AdvertiseServices = []string{"svc:" + pgName}
+	expectCfg(&expectedCfg)
+
+	expectEqual(t, fc, pg, omitStatusConditionMessages) // Unchanged status.
+
+	// Simulate Pod prefs updated with advertised services; should see Configured condition updated to true.
+	mustCreate(t, fc, &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-pg-0",
+			Namespace: ns,
+			Labels:    pgSecretLabels(pgName, kubetypes.LabelSecretTypeState),
+		},
+		Data: map[string][]byte{
+			"_current-profile": []byte("profile-foo"),
+			"profile-foo":      []byte(`{"AdvertiseServices":["svc:test-pg"],"Config":{"NodeID":"node-foo"}}`),
+		},
+	})
+	expectReconciled(t, r, "", pgName)
+	tsoperator.SetProxyGroupCondition(pg, tsapi.PGTailscaleServiceConfigured, metav1.ConditionTrue, reasonPGTailscaleServiceConfigured, "", 1, r.clock, r.logger)
+	expectEqual(t, fc, pg, omitStatusConditionMessages)
+
+	// Rename the Tailscale Service - old one + cert resources should be cleaned up.
+	updatedServiceName := tailcfg.ServiceName("svc:test-pg-renamed")
+	updatedDomain := "test-pg-renamed.ts.net"
+	pg.Spec.KubeAPIServer = &tsapi.KubeAPIServerConfig{
+		ServiceName: updatedServiceName.String(),
+	}
+	mustUpdate(t, fc, "", pgName, func(p *tsapi.ProxyGroup) {
+		p.Spec.KubeAPIServer = pg.Spec.KubeAPIServer
+	})
+	expectReconciled(t, r, "", pgName)
+	tsSvc, err = ft.GetVIPService(t.Context(), "svc:"+pgName)
+	if !isErrorTailscaleServiceNotFound(err) {
+		t.Fatalf("Expected 404, got: %v", err)
+	}
+	tsSvc, err = ft.GetVIPService(t.Context(), updatedServiceName)
+	if err != nil {
+		t.Fatalf("Expected renamed svc, got error: %v", err)
+	}
+	expectedTSSvc.Name = updatedServiceName
+	if !reflect.DeepEqual(tsSvc, expectedTSSvc) {
+		t.Fatalf("expected Tailscale Service to be %+v, got %+v", expectedTSSvc, tsSvc)
+	}
+	// Check cfg and status reset until TLS certs are available again.
+	expectedCfg.APIServerProxy.ServiceName = ptr.To(updatedServiceName)
+	expectedCfg.AdvertiseServices = nil
+	expectCfg(&expectedCfg)
+	tsoperator.SetProxyGroupCondition(pg, tsapi.PGTailscaleServiceConfigured, metav1.ConditionFalse, reasonPGTailscaleServiceNoBackends, "", 1, r.clock, r.logger)
+	expectEqual(t, fc, pg, omitStatusConditionMessages)
+
+	expectEqual(t, fc, certSecret(pgName, ns, updatedDomain, pg))
+	expectEqual(t, fc, certSecretRole(pgName, ns, updatedDomain))
+	expectEqual(t, fc, certSecretRoleBinding(pg, ns, updatedDomain))
+	expectMissing[corev1.Secret](t, fc, ns, defaultDomain)
+	expectMissing[rbacv1.Role](t, fc, ns, defaultDomain)
+	expectMissing[rbacv1.RoleBinding](t, fc, ns, defaultDomain)
+
+	// Delete the ProxyGroup and verify Tailscale Service and cert resources are cleaned up.
+	if err := fc.Delete(t.Context(), pg); err != nil {
+		t.Fatalf("deleting ProxyGroup: %v", err)
+	}
+	expectReconciled(t, r, "", pgName)
+	expectMissing[corev1.Secret](t, fc, ns, updatedDomain)
+	expectMissing[rbacv1.Role](t, fc, ns, updatedDomain)
+	expectMissing[rbacv1.RoleBinding](t, fc, ns, updatedDomain)
+	tsSvc, err = ft.GetVIPService(t.Context(), updatedServiceName)
+	if !isErrorTailscaleServiceNotFound(err) {
+		t.Fatalf("Expected 404, got: %v", err)
+	}
+}
+
 func TestExclusiveOwnerAnnotations(t *testing.T) {
 	pg := &tsapi.ProxyGroup{
 		ObjectMeta: metav1.ObjectMeta{

cmd/k8s-operator/proxygroup_test.go

@@ -31,8 +31,11 @@ import (
 	kube "tailscale.com/k8s-operator"
 	tsoperator "tailscale.com/k8s-operator"
 	tsapi "tailscale.com/k8s-operator/apis/v1alpha1"
+	"tailscale.com/kube/k8s-proxy/conf"
 	"tailscale.com/kube/kubetypes"
+	"tailscale.com/tailcfg"
 	"tailscale.com/tstest"
+	"tailscale.com/types/opt"
 	"tailscale.com/types/ptr"
 )
 
@@ -1256,6 +1259,90 @@ func TestProxyGroupTypes(t *testing.T) {
 	})
 }
 
+func TestKubeAPIServerType_DoesNotOverwriteServicesConfig(t *testing.T) {
+	fc := fake.NewClientBuilder().
+		WithScheme(tsapi.GlobalScheme).
+		WithStatusSubresource(&tsapi.ProxyGroup{}).
+		Build()
+
+	reconciler := &ProxyGroupReconciler{
+		tsNamespace:  tsNamespace,
+		tsProxyImage: testProxyImage,
+		Client:       fc,
+		l:            zap.Must(zap.NewDevelopment()).Sugar(),
+		tsClient:     &fakeTSClient{},
+		clock:        tstest.NewClock(tstest.ClockOpts{}),
+	}
+
+	pg := &tsapi.ProxyGroup{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "test-k8s-apiserver",
+			UID:  "test-k8s-apiserver-uid",
+		},
+		Spec: tsapi.ProxyGroupSpec{
+			Type:     tsapi.ProxyGroupTypeKubernetesAPIServer,
+			Replicas: ptr.To[int32](1),
+			KubeAPIServer: &tsapi.KubeAPIServerConfig{
+				Mode: ptr.To(tsapi.APIServerProxyModeNoAuth), // Avoid needing to pre-create the static ServiceAccount.
+			},
+		},
+	}
+	if err := fc.Create(t.Context(), pg); err != nil {
+		t.Fatal(err)
+	}
+	expectReconciled(t, reconciler, "", pg.Name)
+
+	cfg := conf.VersionedConfig{
+		Version: "v1alpha1",
+		ConfigV1Alpha1: &conf.ConfigV1Alpha1{
+			AuthKey:  ptr.To("secret-authkey"),
+			State:    ptr.To(fmt.Sprintf("kube:%s", pgPodName(pg.Name, 0))),
+			App:      ptr.To(kubetypes.AppProxyGroupKubeAPIServer),
+			LogLevel: ptr.To("debug"),
+
+			Hostname: ptr.To("test-k8s-apiserver-0"),
+			APIServerProxy: &conf.APIServerProxyConfig{
+				Enabled:    opt.NewBool(true),
+				AuthMode:   opt.NewBool(false),
+				IssueCerts: opt.NewBool(true),
+			},
+		},
+	}
+	cfgB, err := json.Marshal(cfg)
+	if err != nil {
+		t.Fatalf("failed to marshal config: %v", err)
+	}
+
+	cfgSecret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:            pgConfigSecretName(pg.Name, 0),
+			Namespace:       tsNamespace,
+			Labels:          pgSecretLabels(pg.Name, kubetypes.LabelSecretTypeConfig),
+			OwnerReferences: pgOwnerReference(pg),
+		},
+		Data: map[string][]byte{
+			kubetypes.KubeAPIServerConfigFile: cfgB,
+		},
+	}
+	expectEqual(t, fc, cfgSecret)
+
+	// Now simulate the kube-apiserver services reconciler updating config,
+	// then check the proxygroup reconciler doesn't overwrite it.
+	cfg.APIServerProxy.ServiceName = ptr.To(tailcfg.ServiceName("svc:some-svc-name"))
+	cfg.AdvertiseServices = []string{"svc:should-not-be-overwritten"}
+	cfgB, err = json.Marshal(cfg)
+	if err != nil {
+		t.Fatalf("failed to marshal config: %v", err)
+	}
+	mustUpdate(t, fc, tsNamespace, cfgSecret.Name, func(s *corev1.Secret) {
+		s.Data[kubetypes.KubeAPIServerConfigFile] = cfgB
+	})
+	expectReconciled(t, reconciler, "", pg.Name)
+
+	cfgSecret.Data[kubetypes.KubeAPIServerConfigFile] = cfgB
+	expectEqual(t, fc, cfgSecret)
+}
+
 func TestIngressAdvertiseServicesConfigPreserved(t *testing.T) {
 	fc := fake.NewClientBuilder().
 		WithScheme(tsapi.GlobalScheme).