cmd/k8s-operator,internal/client/tailscale: use VIPService annotations for ownership tracking (#15356)

Switch from using the Comment field to a ts-scoped annotation for
tracking which operators are cooperating over ownership of a
VIPService.

Updates tailscale/corp#24795

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

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

@@ -228,12 +228,11 @@ func (r *HAIngressReconciler) maybeProvision(ctx context.Context, hostname strin
 			return false, fmt.Errorf("error getting VIPService %q: %w", hostname, err)
 		}
 	}
-	// Generate the VIPService comment for new or existing VIPService. This
+	// Generate the VIPService owner annotation for new or existing VIPService.
-	// checks and ensures that VIPService's owner references are updated for
+	// This checks and ensures that VIPService's owner references are updated
-	// this Ingress and errors if that is not possible (i.e. because it
+	// for this Ingress and errors if that is not possible (i.e. because it
-	// appears that the VIPService has been created by a non-operator
+	// appears that the VIPService has been created by a non-operator actor).
-	// actor).
+	updatedAnnotations, err := r.ownerAnnotations(existingVIPSvc)
-	svcComment, err := r.ownerRefsComment(existingVIPSvc)
 	if err != nil {
 		const instr = "To proceed, you can either manually delete the existing VIPService or choose a different MagicDNS name at `.spec.tls.hosts[0] in the Ingress definition"
 		msg := fmt.Sprintf("error ensuring ownership of VIPService %s: %v. %s", hostname, err, instr)
@@ -313,11 +312,13 @@ func (r *HAIngressReconciler) maybeProvision(ctx context.Context, hostname strin
 		vipPorts = append(vipPorts, "80")
 	}
 
+	const managedVIPServiceComment = "This VIPService is managed by the Tailscale Kubernetes Operator, do not modify"
 	vipSvc := &tailscale.VIPService{
-		Name:    serviceName,
+		Name:        serviceName,
-		Tags:    tags,
+		Tags:        tags,
-		Ports:   vipPorts,
+		Ports:       vipPorts,
-		Comment: svcComment,
+		Comment:     managedVIPServiceComment,
+		Annotations: updatedAnnotations,
 	}
 	if existingVIPSvc != nil {
 		vipSvc.Addrs = existingVIPSvc.Addrs
@@ -328,7 +329,7 @@ func (r *HAIngressReconciler) maybeProvision(ctx context.Context, hostname strin
 	if existingVIPSvc == nil ||
 		!reflect.DeepEqual(vipSvc.Tags, existingVIPSvc.Tags) ||
 		!reflect.DeepEqual(vipSvc.Ports, existingVIPSvc.Ports) ||
-		!strings.EqualFold(vipSvc.Comment, existingVIPSvc.Comment) {
+		!ownersAreSetAndEqual(vipSvc, existingVIPSvc) {
 		logger.Infof("Ensuring VIPService exists and is up to date")
 		if err := r.tsClient.CreateOrUpdateVIPService(ctx, vipSvc); err != nil {
 			return false, fmt.Errorf("error creating VIPService: %w", err)
@@ -669,34 +670,34 @@ func (r *HAIngressReconciler) cleanupVIPService(ctx context.Context, name tailcf
 	if svc == nil {
 		return false, nil
 	}
-	c, err := parseComment(svc)
+	o, err := parseOwnerAnnotation(svc)
 	if err != nil {
-		return false, fmt.Errorf("error parsing VIPService comment")
+		return false, fmt.Errorf("error parsing VIPService owner annotation")
 	}
-	if c == nil || len(c.OwnerRefs) == 0 {
+	if o == nil || len(o.OwnerRefs) == 0 {
 		return false, nil
 	}
 	// Comparing with the operatorID only means that we will not be able to
 	// clean up VIPServices in cases where the operator was deleted from the
 	// cluster before deleting the Ingress. Perhaps the comparison could be
 	// 'if or.OperatorID === r.operatorID || or.ingressUID == r.ingressUID'.
-	ix := slices.IndexFunc(c.OwnerRefs, func(or OwnerRef) bool {
+	ix := slices.IndexFunc(o.OwnerRefs, func(or OwnerRef) bool {
 		return or.OperatorID == r.operatorID
 	})
 	if ix == -1 {
 		return false, nil
 	}
-	if len(c.OwnerRefs) == 1 {
+	if len(o.OwnerRefs) == 1 {
 		logger.Infof("Deleting VIPService %q", name)
 		return false, r.tsClient.DeleteVIPService(ctx, name)
 	}
-	c.OwnerRefs = slices.Delete(c.OwnerRefs, ix, ix+1)
+	o.OwnerRefs = slices.Delete(o.OwnerRefs, ix, ix+1)
 	logger.Infof("Deleting VIPService %q", name)
-	json, err := json.Marshal(c)
+	json, err := json.Marshal(o)
 	if err != nil {
 		return false, fmt.Errorf("error marshalling updated VIPService owner reference: %w", err)
 	}
-	svc.Comment = string(json)
+	svc.Annotations[ownerAnnotation] = string(json)
 	return true, r.tsClient.CreateOrUpdateVIPService(ctx, svc)
 }
 
@@ -783,6 +784,15 @@ func (a *HAIngressReconciler) numberPodsAdvertising(ctx context.Context, pgName
 	return count, nil
 }
 
+const ownerAnnotation = "tailscale.com/owner-references"
+
+// ownerAnnotationValue is the content of the VIPService.Annotation[ownerAnnotation] field.
+type ownerAnnotationValue struct {
+	// OwnerRefs is a list of owner references that identify all operator
+	// instances that manage this VIPService.
+	OwnerRefs []OwnerRef `json:"ownerRefs,omitempty"`
+}
+
 // OwnerRef is an owner reference that uniquely identifies a Tailscale
 // Kubernetes operator instance.
 type OwnerRef struct {
@@ -790,48 +800,67 @@ type OwnerRef struct {
 	OperatorID string `json:"operatorID,omitempty"`
 }
 
-// comment is the content of the VIPService.Comment field.
+// ownerAnnotations returns the updated annotations required to ensure this
-type comment struct {
+// instance of the operator is included as an owner. If the VIPService is not
-	// OwnerRefs is a list of owner references that identify all operator
+// nil, but does not contain an owner we return an error as this likely means
-	// instances that manage this VIPService.
+// that the VIPService was created by somthing other than a Tailscale
-	OwnerRefs []OwnerRef `json:"ownerRefs,omitempty"`
+// Kubernetes operator.
-}
+func (r *HAIngressReconciler) ownerAnnotations(svc *tailscale.VIPService) (map[string]string, error) {
-
-// ownerRefsComment return VIPService Comment that includes owner reference for this
-// operator instance for the provided VIPService. If the VIPService is nil, a
-// new comment with owner ref is returned. If the VIPService is not nil, the
-// existing comment is returned with the owner reference added, if not already
-// present. If the VIPService is not nil, but does not contain a comment we
-// return an error as this likely means that the VIPService was created by
-// somthing other than a Tailscale Kubernetes operator.
-func (r *HAIngressReconciler) ownerRefsComment(svc *tailscale.VIPService) (string, error) {
 	ref := OwnerRef{
 		OperatorID: r.operatorID,
 	}
 	if svc == nil {
-		c := &comment{OwnerRefs: []OwnerRef{ref}}
+		c := ownerAnnotationValue{OwnerRefs: []OwnerRef{ref}}
 		json, err := json.Marshal(c)
 		if err != nil {
-			return "", fmt.Errorf("[unexpected] unable to marshal VIPService comment contents: %w, please report this", err)
+			return nil, fmt.Errorf("[unexpected] unable to marshal VIPService owner annotation contents: %w, please report this", err)
 		}
-		return string(json), nil
+		return map[string]string{
+			ownerAnnotation: string(json),
+		}, nil
 	}
-	c, err := parseComment(svc)
+	o, err := parseOwnerAnnotation(svc)
 	if err != nil {
-		return "", fmt.Errorf("error parsing existing VIPService comment: %w", err)
+		return nil, err
 	}
-	if c == nil || len(c.OwnerRefs) == 0 {
+	if o == nil || len(o.OwnerRefs) == 0 {
-		return "", fmt.Errorf("VIPService %s exists, but does not contain Comment field with owner references- not proceeding as this is likely a resource created by something other than a Tailscale Kubernetes Operator", svc.Name)
+		return nil, fmt.Errorf("VIPService %s exists, but does not contain owner annotation with owner references; not proceeding as this is likely a resource created by something other than the Tailscale Kubernetes operator", svc.Name)
 	}
-	if slices.Contains(c.OwnerRefs, ref) { // up to date
+	if slices.Contains(o.OwnerRefs, ref) { // up to date
-		return svc.Comment, nil
+		return svc.Annotations, nil
 	}
-	c.OwnerRefs = append(c.OwnerRefs, ref)
+	o.OwnerRefs = append(o.OwnerRefs, ref)
-	json, err := json.Marshal(c)
+	json, err := json.Marshal(o)
 	if err != nil {
-		return "", fmt.Errorf("error marshalling updated owner references: %w", err)
+		return nil, fmt.Errorf("error marshalling updated owner references: %w", err)
+	}
+
+	newAnnots := make(map[string]string, len(svc.Annotations)+1)
+	for k, v := range svc.Annotations {
+		newAnnots[k] = v
 	}
-	return string(json), nil
+	newAnnots[ownerAnnotation] = string(json)
+	return newAnnots, nil
+}
+
+// parseOwnerAnnotation returns nil if no valid owner found.
+func parseOwnerAnnotation(vipSvc *tailscale.VIPService) (*ownerAnnotationValue, error) {
+	if vipSvc.Annotations == nil || vipSvc.Annotations[ownerAnnotation] == "" {
+		return nil, nil
+	}
+	o := &ownerAnnotationValue{}
+	if err := json.Unmarshal([]byte(vipSvc.Annotations[ownerAnnotation]), o); err != nil {
+		return nil, fmt.Errorf("error parsing VIPService %s annotation %q: %w", ownerAnnotation, vipSvc.Annotations[ownerAnnotation], err)
+	}
+	return o, nil
+}
+
+func ownersAreSetAndEqual(a, b *tailscale.VIPService) bool {
+	return a != nil && b != nil &&
+		a.Annotations != nil && b.Annotations != nil &&
+		a.Annotations[ownerAnnotation] != "" &&
+		b.Annotations[ownerAnnotation] != "" &&
+		strings.EqualFold(a.Annotations[ownerAnnotation], b.Annotations[ownerAnnotation])
 }
 
 // ensureCertResources ensures that the TLS Secret for an HA Ingress and RBAC
@@ -877,18 +906,6 @@ func (r *HAIngressReconciler) cleanupCertResources(ctx context.Context, pgName s
 	return nil
 }
 
-// parseComment returns VIPService comment or nil if none found or not matching the expected format.
-func parseComment(vipSvc *tailscale.VIPService) (*comment, error) {
-	if vipSvc.Comment == "" {
-		return nil, nil
-	}
-	c := &comment{}
-	if err := json.Unmarshal([]byte(vipSvc.Comment), c); err != nil {
-		return nil, fmt.Errorf("error parsing VIPService Comment field %q: %w", vipSvc.Comment, err)
-	}
-	return c, nil
-}
-
 // requeueInterval returns a time duration between 5 and 10 minutes, which is
 // the period of time after which an HA Ingress, whose VIPService has been newly
 // created or changed, needs to be requeued. This is to protect against

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

@@ -745,8 +745,10 @@ func TestIngressPGReconciler_MultiCluster(t *testing.T) {
 
 	// Simulate existing VIPService from another cluster
 	existingVIPSvc := &tailscale.VIPService{
-		Name:    "svc:my-svc",
+		Name: "svc:my-svc",
-		Comment: `{"ownerrefs":[{"operatorID":"operator-2"}]}`,
+		Annotations: map[string]string{
+			ownerAnnotation: `{"ownerrefs":[{"operatorID":"operator-2"}]}`,
+		},
 	}
 	ft.vipServices = map[tailcfg.ServiceName]*tailscale.VIPService{
 		"svc:my-svc": existingVIPSvc,
@@ -763,17 +765,17 @@ func TestIngressPGReconciler_MultiCluster(t *testing.T) {
 		t.Fatal("VIPService not found")
 	}
 
-	c := &comment{}
+	o, err := parseOwnerAnnotation(vipSvc)
-	if err := json.Unmarshal([]byte(vipSvc.Comment), c); err != nil {
+	if err != nil {
-		t.Fatalf("parsing comment: %v", err)
+		t.Fatalf("parsing owner annotation: %v", err)
 	}
 
 	wantOwnerRefs := []OwnerRef{
 		{OperatorID: "operator-2"},
 		{OperatorID: "operator-1"},
 	}
-	if !reflect.DeepEqual(c.OwnerRefs, wantOwnerRefs) {
+	if !reflect.DeepEqual(o.OwnerRefs, wantOwnerRefs) {
-		t.Errorf("incorrect owner refs\ngot:  %+v\nwant: %+v", c.OwnerRefs, wantOwnerRefs)
+		t.Errorf("incorrect owner refs\ngot:  %+v\nwant: %+v", o.OwnerRefs, wantOwnerRefs)
 	}
 
 	// Delete the Ingress and verify VIPService still exists with one owner ref
@@ -790,15 +792,15 @@ func TestIngressPGReconciler_MultiCluster(t *testing.T) {
 		t.Fatal("VIPService was incorrectly deleted")
 	}
 
-	c = &comment{}
+	o, err = parseOwnerAnnotation(vipSvc)
-	if err := json.Unmarshal([]byte(vipSvc.Comment), c); err != nil {
+	if err != nil {
-		t.Fatalf("parsing comment after deletion: %v", err)
+		t.Fatalf("parsing owner annotation: %v", err)
 	}
 
 	wantOwnerRefs = []OwnerRef{
 		{OperatorID: "operator-2"},
 	}
-	if !reflect.DeepEqual(c.OwnerRefs, wantOwnerRefs) {
+	if !reflect.DeepEqual(o.OwnerRefs, wantOwnerRefs) {
-		t.Errorf("incorrect owner refs after deletion\ngot:  %+v\nwant: %+v", c.OwnerRefs, wantOwnerRefs)
+		t.Errorf("incorrect owner refs after deletion\ngot:  %+v\nwant: %+v", o.OwnerRefs, wantOwnerRefs)
 	}
 }

internal/client/tailscale/vip_service.go

@@ -27,6 +27,8 @@ type VIPService struct {
 	Addrs []string `json:"addrs,omitempty"`
 	// Comment is an optional text string for display in the admin panel.
 	Comment string `json:"comment,omitempty"`
+	// Annotations are optional key-value pairs that can be used to store arbitrary metadata.
+	Annotations map[string]string `json:"annotations,omitempty"`
 	// Ports are the ports of a VIPService that will be configured via Tailscale serve config.
 	// If set, any node wishing to advertise this VIPService must have this port configured via Tailscale serve.
 	Ports []string `json:"ports,omitempty"`