diff --git a/cmd/k8s-operator/egress-services.go b/cmd/k8s-operator/egress-services.go index cf218ba4f..e997e5884 100644 --- a/cmd/k8s-operator/egress-services.go +++ b/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 { diff --git a/cmd/k8s-operator/ingress-for-pg.go b/cmd/k8s-operator/ingress-for-pg.go index 4fa0af2a2..1fa12eb59 100644 --- a/cmd/k8s-operator/ingress-for-pg.go +++ b/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 { - delete(cfg.Services, vipServiceName) - serveConfigChanged = true - continue + if ok := errors.As(err, errResp); !ok || errResp.Status != http.StatusNotFound { + return err } - 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 +} diff --git a/cmd/k8s-operator/ingress-for-pg_test.go b/cmd/k8s-operator/ingress-for-pg_test.go index c432eb7e1..8c4ffb691 100644 --- a/cmd/k8s-operator/ingress-for-pg_test.go +++ b/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() diff --git a/cmd/k8s-operator/proxygroup.go b/cmd/k8s-operator/proxygroup.go index 4b17d3470..463d29249 100644 --- a/cmd/k8s-operator/proxygroup.go +++ b/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 } diff --git a/cmd/k8s-operator/proxygroup_specs.go b/cmd/k8s-operator/proxygroup_specs.go index 1ea91004b..40bbaec17 100644 --- a/cmd/k8s-operator/proxygroup_specs.go +++ b/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,8 +236,8 @@ 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. - fmt.Sprintf("%s-%d", pg.Name, i), // State. + pgConfigSecretName(pg.Name, i), // Config with auth key. + fmt.Sprintf("%s-%d", pg.Name, i), // State. ) } return secrets @@ -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) } diff --git a/cmd/k8s-operator/proxygroup_test.go b/cmd/k8s-operator/proxygroup_test.go index 29100de1d..6829b3929 100644 --- a/cmd/k8s-operator/proxygroup_test.go +++ b/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 +}