From b0c3e6f6c5504bd9a67cfcdcfb49527c0a1c9b03 Mon Sep 17 00:00:00 2001 From: Irbe Krumina Date: Tue, 19 Mar 2024 14:54:17 +0000 Subject: [PATCH] cmd/k8s-operator,ipn/conf.go: fix --accept-routes for proxies (#11453) Fix a bug where all proxies got configured with --accept-routes set to true. The bug was introduced in https://github.com/tailscale/tailscale/pull/11238. Updates#cleanup Signed-off-by: Irbe Krumina --- cmd/k8s-operator/connector_test.go | 40 +++--- cmd/k8s-operator/ingress_test.go | 50 ++++---- cmd/k8s-operator/operator_test.go | 189 +++++++++++++++++----------- cmd/k8s-operator/proxyclass_test.go | 4 +- cmd/k8s-operator/sts.go | 9 +- cmd/k8s-operator/testutils_test.go | 39 ++++-- ipn/conf.go | 4 +- 7 files changed, 190 insertions(+), 145 deletions(-) diff --git a/cmd/k8s-operator/connector_test.go b/cmd/k8s-operator/connector_test.go index 7e8e9599f..07f0e2d97 100644 --- a/cmd/k8s-operator/connector_test.go +++ b/cmd/k8s-operator/connector_test.go @@ -73,38 +73,34 @@ func TestConnector(t *testing.T) { hostname: "test-connector", isExitNode: true, subnetRoutes: "10.40.0.0/14", - confFileHash: "9321660203effb80983eaecc7b5ac5a8c53934926f46e895b9fe295dcfc5a904", } - expectEqual(t, fc, expectedSecret(t, opts)) - expectEqual(t, fc, expectedSTS(t, fc, opts)) + expectEqual(t, fc, expectedSecret(t, opts), nil) + expectEqual(t, fc, expectedSTS(t, fc, opts), removeHashAnnotation) // Add another route to be advertised. mustUpdate[tsapi.Connector](t, fc, "", "test", func(conn *tsapi.Connector) { conn.Spec.SubnetRouter.AdvertiseRoutes = []tsapi.Route{"10.40.0.0/14", "10.44.0.0/20"} }) opts.subnetRoutes = "10.40.0.0/14,10.44.0.0/20" - opts.confFileHash = "fb6c4daf67425f983985750cd8d6f2beae77e614fcb34176604571f5623d6862" expectReconciled(t, cr, "", "test") - expectEqual(t, fc, expectedSTS(t, fc, opts)) + expectEqual(t, fc, expectedSTS(t, fc, opts), removeHashAnnotation) // Remove a route. mustUpdate[tsapi.Connector](t, fc, "", "test", func(conn *tsapi.Connector) { conn.Spec.SubnetRouter.AdvertiseRoutes = []tsapi.Route{"10.44.0.0/20"} }) opts.subnetRoutes = "10.44.0.0/20" - opts.confFileHash = "bacba177bcfe3849065cf6fee53d658a9bb4144197ac5b861727d69ea99742bb" expectReconciled(t, cr, "", "test") - expectEqual(t, fc, expectedSTS(t, fc, opts)) + expectEqual(t, fc, expectedSTS(t, fc, opts), removeHashAnnotation) // Remove the subnet router. mustUpdate[tsapi.Connector](t, fc, "", "test", func(conn *tsapi.Connector) { conn.Spec.SubnetRouter = nil }) opts.subnetRoutes = "" - opts.confFileHash = "7c421a99128eb80e79a285a82702f19f8f720615542a15bd794858a6275d8079" expectReconciled(t, cr, "", "test") - expectEqual(t, fc, expectedSTS(t, fc, opts)) + expectEqual(t, fc, expectedSTS(t, fc, opts), removeHashAnnotation) // Re-add the subnet router. mustUpdate[tsapi.Connector](t, fc, "", "test", func(conn *tsapi.Connector) { @@ -113,9 +109,8 @@ func TestConnector(t *testing.T) { } }) opts.subnetRoutes = "10.44.0.0/20" - opts.confFileHash = "bacba177bcfe3849065cf6fee53d658a9bb4144197ac5b861727d69ea99742bb" expectReconciled(t, cr, "", "test") - expectEqual(t, fc, expectedSTS(t, fc, opts)) + expectEqual(t, fc, expectedSTS(t, fc, opts), removeHashAnnotation) // Delete the Connector. if err = fc.Delete(context.Background(), cn); err != nil { @@ -156,19 +151,17 @@ func TestConnector(t *testing.T) { parentType: "connector", subnetRoutes: "10.40.0.0/14", hostname: "test-connector", - confFileHash: "57d922331890c9b1c8c6ae664394cb254334c551d9cd9db14537b5d9da9fb17e", } - expectEqual(t, fc, expectedSecret(t, opts)) - expectEqual(t, fc, expectedSTS(t, fc, opts)) + expectEqual(t, fc, expectedSecret(t, opts), nil) + expectEqual(t, fc, expectedSTS(t, fc, opts), removeHashAnnotation) // Add an exit node. mustUpdate[tsapi.Connector](t, fc, "", "test", func(conn *tsapi.Connector) { conn.Spec.ExitNode = true }) opts.isExitNode = true - opts.confFileHash = "1499b591fd97a50f0330db6ec09979792c49890cf31f5da5bb6a3f50dba1e77a" expectReconciled(t, cr, "", "test") - expectEqual(t, fc, expectedSTS(t, fc, opts)) + expectEqual(t, fc, expectedSTS(t, fc, opts), removeHashAnnotation) // Delete the Connector. if err = fc.Delete(context.Background(), cn); err != nil { @@ -243,10 +236,9 @@ func TestConnectorWithProxyClass(t *testing.T) { hostname: "test-connector", isExitNode: true, subnetRoutes: "10.40.0.0/14", - confFileHash: "9321660203effb80983eaecc7b5ac5a8c53934926f46e895b9fe295dcfc5a904", } - expectEqual(t, fc, expectedSecret(t, opts)) - expectEqual(t, fc, expectedSTS(t, fc, opts)) + expectEqual(t, fc, expectedSecret(t, opts), nil) + expectEqual(t, fc, expectedSTS(t, fc, opts), removeHashAnnotation) // 2. Update Connector to specify a ProxyClass. ProxyClass is not yet // ready, so its configuration is NOT applied to the Connector @@ -255,7 +247,7 @@ func TestConnectorWithProxyClass(t *testing.T) { conn.Spec.ProxyClass = "custom-metadata" }) expectReconciled(t, cr, "", "test") - expectEqual(t, fc, expectedSTS(t, fc, opts)) + expectEqual(t, fc, expectedSTS(t, fc, opts), removeHashAnnotation) // 3. ProxyClass is set to Ready by proxy-class reconciler. Connector // get reconciled and configuration from the ProxyClass is applied to @@ -269,12 +261,8 @@ func TestConnectorWithProxyClass(t *testing.T) { }}} }) opts.proxyClass = pc.Name - // We lose the auth key on second reconcile, because in code it's set to - // StringData, but is actually read from Data. This works with a real - // API server, but not with our test setup here. - opts.confFileHash = "1499b591fd97a50f0330db6ec09979792c49890cf31f5da5bb6a3f50dba1e77a" expectReconciled(t, cr, "", "test") - expectEqual(t, fc, expectedSTS(t, fc, opts)) + expectEqual(t, fc, expectedSTS(t, fc, opts), removeHashAnnotation) // 4. Connector.spec.proxyClass field is unset, Connector gets // reconciled and configuration from the ProxyClass is removed from the @@ -284,5 +272,5 @@ func TestConnectorWithProxyClass(t *testing.T) { }) opts.proxyClass = "" expectReconciled(t, cr, "", "test") - expectEqual(t, fc, expectedSTS(t, fc, opts)) + expectEqual(t, fc, expectedSTS(t, fc, opts), removeHashAnnotation) } diff --git a/cmd/k8s-operator/ingress_test.go b/cmd/k8s-operator/ingress_test.go index a7cec1a81..8fc68a8c7 100644 --- a/cmd/k8s-operator/ingress_test.go +++ b/cmd/k8s-operator/ingress_test.go @@ -88,12 +88,11 @@ func TestTailscaleIngress(t *testing.T) { fullName, shortName := findGenName(t, fc, "default", "test", "ingress") opts := configOpts{ - stsName: shortName, - secretName: fullName, - namespace: "default", - parentType: "ingress", - hostname: "default-test", - confFileHash: "6cceb342cd3e1c56cd1bd94c29df63df3653c35fe98a7e7afcdee0dcaa2ad549", + stsName: shortName, + secretName: fullName, + namespace: "default", + parentType: "ingress", + hostname: "default-test", } serveConfig := &ipn.ServeConfig{ TCP: map[uint16]*ipn.TCPPortHandler{443: {HTTPS: true}}, @@ -101,9 +100,9 @@ func TestTailscaleIngress(t *testing.T) { } opts.serveConfig = serveConfig - expectEqual(t, fc, expectedSecret(t, opts)) - expectEqual(t, fc, expectedHeadlessService(shortName, "ingress")) - expectEqual(t, fc, expectedSTSUserspace(t, fc, opts)) + expectEqual(t, fc, expectedSecret(t, opts), nil) + expectEqual(t, fc, expectedHeadlessService(shortName, "ingress"), nil) + expectEqual(t, fc, expectedSTSUserspace(t, fc, opts), removeHashAnnotation) // 2. Ingress status gets updated with ingress proxy's MagicDNS name // once that becomes available. @@ -118,7 +117,7 @@ func TestTailscaleIngress(t *testing.T) { {Hostname: "foo.tailnetxyz.ts.net", Ports: []networkingv1.IngressPortStatus{{Port: 443, Protocol: "TCP"}}}, }, } - expectEqual(t, fc, ing) + expectEqual(t, fc, ing, nil) // 3. Resources get created for Ingress that should allow forwarding // cluster traffic @@ -126,11 +125,8 @@ func TestTailscaleIngress(t *testing.T) { mak.Set(&ing.ObjectMeta.Annotations, AnnotationExperimentalForwardClusterTrafficViaL7IngresProxy, "true") }) opts.shouldEnableForwardingClusterTrafficViaIngress = true - // configfile hash changed at this point in test env only because we - // lost auth key due to how changes are applied in test client. - opts.confFileHash = "fb9006e30ecda75e88c29dcd0ca2dd28a2ae964d001c66e1be3efe159cc3821d" expectReconciled(t, ingR, "default", "test") - expectEqual(t, fc, expectedSTS(t, fc, opts)) + expectEqual(t, fc, expectedSTS(t, fc, opts), removeHashAnnotation) // 4. Resources get cleaned up when Ingress class is unset mustUpdate(t, fc, "default", "test", func(ing *networkingv1.Ingress) { @@ -223,12 +219,11 @@ func TestTailscaleIngressWithProxyClass(t *testing.T) { fullName, shortName := findGenName(t, fc, "default", "test", "ingress") opts := configOpts{ - stsName: shortName, - secretName: fullName, - namespace: "default", - parentType: "ingress", - hostname: "default-test", - confFileHash: "6cceb342cd3e1c56cd1bd94c29df63df3653c35fe98a7e7afcdee0dcaa2ad549", + stsName: shortName, + secretName: fullName, + namespace: "default", + parentType: "ingress", + hostname: "default-test", } serveConfig := &ipn.ServeConfig{ TCP: map[uint16]*ipn.TCPPortHandler{443: {HTTPS: true}}, @@ -236,9 +231,9 @@ func TestTailscaleIngressWithProxyClass(t *testing.T) { } opts.serveConfig = serveConfig - expectEqual(t, fc, expectedSecret(t, opts)) - expectEqual(t, fc, expectedHeadlessService(shortName, "ingress")) - expectEqual(t, fc, expectedSTSUserspace(t, fc, opts)) + expectEqual(t, fc, expectedSecret(t, opts), nil) + expectEqual(t, fc, expectedHeadlessService(shortName, "ingress"), nil) + expectEqual(t, fc, expectedSTSUserspace(t, fc, opts), removeHashAnnotation) // 2. Ingress is updated to specify a ProxyClass, ProxyClass is not yet // ready, so proxy resource configuration does not change. @@ -246,7 +241,7 @@ func TestTailscaleIngressWithProxyClass(t *testing.T) { mak.Set(&ing.ObjectMeta.Labels, LabelProxyClass, "custom-metadata") }) expectReconciled(t, ingR, "default", "test") - expectEqual(t, fc, expectedSTSUserspace(t, fc, opts)) + expectEqual(t, fc, expectedSTSUserspace(t, fc, opts), removeHashAnnotation) // 3. ProxyClass is set to Ready by proxy-class reconciler. Ingress get // reconciled and configuration from the ProxyClass is applied to the @@ -261,10 +256,7 @@ func TestTailscaleIngressWithProxyClass(t *testing.T) { }) expectReconciled(t, ingR, "default", "test") opts.proxyClass = pc.Name - // configfile hash changed at this point in test env only because we - // lost auth key due to how changes are applied in test client. - opts.confFileHash = "fb9006e30ecda75e88c29dcd0ca2dd28a2ae964d001c66e1be3efe159cc3821d" - expectEqual(t, fc, expectedSTSUserspace(t, fc, opts)) + expectEqual(t, fc, expectedSTSUserspace(t, fc, opts), removeHashAnnotation) // 4. tailscale.com/proxy-class label is removed from the Ingress, the // Ingress gets reconciled and the custom ProxyClass configuration is @@ -274,5 +266,5 @@ func TestTailscaleIngressWithProxyClass(t *testing.T) { }) expectReconciled(t, ingR, "default", "test") opts.proxyClass = "" - expectEqual(t, fc, expectedSTSUserspace(t, fc, opts)) + expectEqual(t, fc, expectedSTSUserspace(t, fc, opts), removeHashAnnotation) } diff --git a/cmd/k8s-operator/operator_test.go b/cmd/k8s-operator/operator_test.go index 3dba7325f..7188ce65b 100644 --- a/cmd/k8s-operator/operator_test.go +++ b/cmd/k8s-operator/operator_test.go @@ -71,12 +71,11 @@ func TestLoadBalancerClass(t *testing.T) { parentType: "svc", hostname: "default-test", clusterTargetIP: "10.20.30.40", - confFileHash: "6cceb342cd3e1c56cd1bd94c29df63df3653c35fe98a7e7afcdee0dcaa2ad549", } - expectEqual(t, fc, expectedSecret(t, opts)) - expectEqual(t, fc, expectedHeadlessService(shortName, "svc")) - expectEqual(t, fc, expectedSTS(t, fc, opts)) + expectEqual(t, fc, expectedSecret(t, opts), nil) + expectEqual(t, fc, expectedHeadlessService(shortName, "svc"), nil) + expectEqual(t, fc, expectedSTS(t, fc, opts), removeHashAnnotation) // Normally the Tailscale proxy pod would come up here and write its info // into the secret. Simulate that, then verify reconcile again and verify @@ -119,7 +118,7 @@ func TestLoadBalancerClass(t *testing.T) { }, }, } - expectEqual(t, fc, want) + expectEqual(t, fc, want, nil) // Turn the service back into a ClusterIP service, which should make the // operator clean up. @@ -158,7 +157,7 @@ func TestLoadBalancerClass(t *testing.T) { Type: corev1.ServiceTypeClusterIP, }, } - expectEqual(t, fc, want) + expectEqual(t, fc, want, nil) } func TestTailnetTargetFQDNAnnotation(t *testing.T) { @@ -213,12 +212,11 @@ func TestTailnetTargetFQDNAnnotation(t *testing.T) { parentType: "svc", tailnetTargetFQDN: tailnetTargetFQDN, hostname: "default-test", - confFileHash: "6cceb342cd3e1c56cd1bd94c29df63df3653c35fe98a7e7afcdee0dcaa2ad549", } - expectEqual(t, fc, expectedSecret(t, o)) - expectEqual(t, fc, expectedHeadlessService(shortName, "svc")) - expectEqual(t, fc, expectedSTS(t, fc, o)) + expectEqual(t, fc, expectedSecret(t, o), nil) + expectEqual(t, fc, expectedHeadlessService(shortName, "svc"), nil) + expectEqual(t, fc, expectedSTS(t, fc, o), removeHashAnnotation) want := &corev1.Service{ TypeMeta: metav1.TypeMeta{ Kind: "Service", @@ -239,10 +237,10 @@ func TestTailnetTargetFQDNAnnotation(t *testing.T) { Selector: nil, }, } - expectEqual(t, fc, want) - expectEqual(t, fc, expectedSecret(t, o)) - expectEqual(t, fc, expectedHeadlessService(shortName, "svc")) - expectEqual(t, fc, expectedSTS(t, fc, o)) + expectEqual(t, fc, want, nil) + expectEqual(t, fc, expectedSecret(t, o), nil) + expectEqual(t, fc, expectedHeadlessService(shortName, "svc"), nil) + expectEqual(t, fc, expectedSTS(t, fc, o), removeHashAnnotation) // Change the tailscale-target-fqdn annotation which should update the // StatefulSet @@ -324,12 +322,11 @@ func TestTailnetTargetIPAnnotation(t *testing.T) { parentType: "svc", tailnetTargetIP: tailnetTargetIP, hostname: "default-test", - confFileHash: "6cceb342cd3e1c56cd1bd94c29df63df3653c35fe98a7e7afcdee0dcaa2ad549", } - expectEqual(t, fc, expectedSecret(t, o)) - expectEqual(t, fc, expectedHeadlessService(shortName, "svc")) - expectEqual(t, fc, expectedSTS(t, fc, o)) + expectEqual(t, fc, expectedSecret(t, o), nil) + expectEqual(t, fc, expectedHeadlessService(shortName, "svc"), nil) + expectEqual(t, fc, expectedSTS(t, fc, o), removeHashAnnotation) want := &corev1.Service{ TypeMeta: metav1.TypeMeta{ Kind: "Service", @@ -350,10 +347,10 @@ func TestTailnetTargetIPAnnotation(t *testing.T) { Selector: nil, }, } - expectEqual(t, fc, want) - expectEqual(t, fc, expectedSecret(t, o)) - expectEqual(t, fc, expectedHeadlessService(shortName, "svc")) - expectEqual(t, fc, expectedSTS(t, fc, o)) + expectEqual(t, fc, want, nil) + expectEqual(t, fc, expectedSecret(t, o), nil) + expectEqual(t, fc, expectedHeadlessService(shortName, "svc"), nil) + expectEqual(t, fc, expectedSTS(t, fc, o), removeHashAnnotation) // Change the tailscale-target-ip annotation which should update the // StatefulSet @@ -432,12 +429,11 @@ func TestAnnotations(t *testing.T) { parentType: "svc", hostname: "default-test", clusterTargetIP: "10.20.30.40", - confFileHash: "6cceb342cd3e1c56cd1bd94c29df63df3653c35fe98a7e7afcdee0dcaa2ad549", } - expectEqual(t, fc, expectedSecret(t, o)) - expectEqual(t, fc, expectedHeadlessService(shortName, "svc")) - expectEqual(t, fc, expectedSTS(t, fc, o)) + expectEqual(t, fc, expectedSecret(t, o), nil) + expectEqual(t, fc, expectedHeadlessService(shortName, "svc"), nil) + expectEqual(t, fc, expectedSTS(t, fc, o), removeHashAnnotation) want := &corev1.Service{ TypeMeta: metav1.TypeMeta{ Kind: "Service", @@ -457,7 +453,7 @@ func TestAnnotations(t *testing.T) { Type: corev1.ServiceTypeClusterIP, }, } - expectEqual(t, fc, want) + expectEqual(t, fc, want, nil) // Turn the service back into a ClusterIP service, which should make the // operator clean up. @@ -489,7 +485,7 @@ func TestAnnotations(t *testing.T) { Type: corev1.ServiceTypeClusterIP, }, } - expectEqual(t, fc, want) + expectEqual(t, fc, want, nil) } func TestAnnotationIntoLB(t *testing.T) { @@ -541,12 +537,11 @@ func TestAnnotationIntoLB(t *testing.T) { parentType: "svc", hostname: "default-test", clusterTargetIP: "10.20.30.40", - confFileHash: "6cceb342cd3e1c56cd1bd94c29df63df3653c35fe98a7e7afcdee0dcaa2ad549", } - expectEqual(t, fc, expectedSecret(t, o)) - expectEqual(t, fc, expectedHeadlessService(shortName, "svc")) - expectEqual(t, fc, expectedSTS(t, fc, o)) + expectEqual(t, fc, expectedSecret(t, o), nil) + expectEqual(t, fc, expectedHeadlessService(shortName, "svc"), nil) + expectEqual(t, fc, expectedSTS(t, fc, o), removeHashAnnotation) // Normally the Tailscale proxy pod would come up here and write its info // into the secret. Simulate that, since it would have normally happened at @@ -579,7 +574,7 @@ func TestAnnotationIntoLB(t *testing.T) { Type: corev1.ServiceTypeClusterIP, }, } - expectEqual(t, fc, want) + expectEqual(t, fc, want, nil) // Remove Tailscale's annotation, and at the same time convert the service // into a tailscale LoadBalancer. @@ -590,10 +585,8 @@ func TestAnnotationIntoLB(t *testing.T) { }) expectReconciled(t, sr, "default", "test") // None of the proxy machinery should have changed... - // (although configfile hash will change in test env only because we lose auth key due to out test not syncing secret.StringData -> secret.Data) - o.confFileHash = "fb9006e30ecda75e88c29dcd0ca2dd28a2ae964d001c66e1be3efe159cc3821d" - expectEqual(t, fc, expectedHeadlessService(shortName, "svc")) - expectEqual(t, fc, expectedSTS(t, fc, o)) + expectEqual(t, fc, expectedHeadlessService(shortName, "svc"), nil) + expectEqual(t, fc, expectedSTS(t, fc, o), removeHashAnnotation) // ... but the service should have a LoadBalancer status. want = &corev1.Service{ @@ -625,7 +618,7 @@ func TestAnnotationIntoLB(t *testing.T) { }, }, } - expectEqual(t, fc, want) + expectEqual(t, fc, want, nil) } func TestLBIntoAnnotation(t *testing.T) { @@ -675,12 +668,11 @@ func TestLBIntoAnnotation(t *testing.T) { parentType: "svc", hostname: "default-test", clusterTargetIP: "10.20.30.40", - confFileHash: "6cceb342cd3e1c56cd1bd94c29df63df3653c35fe98a7e7afcdee0dcaa2ad549", } - expectEqual(t, fc, expectedSecret(t, o)) - expectEqual(t, fc, expectedHeadlessService(shortName, "svc")) - expectEqual(t, fc, expectedSTS(t, fc, o)) + expectEqual(t, fc, expectedSecret(t, o), nil) + expectEqual(t, fc, expectedHeadlessService(shortName, "svc"), nil) + expectEqual(t, fc, expectedSTS(t, fc, o), removeHashAnnotation) // Normally the Tailscale proxy pod would come up here and write its info // into the secret. Simulate that, then verify reconcile again and verify @@ -723,7 +715,7 @@ func TestLBIntoAnnotation(t *testing.T) { }, }, } - expectEqual(t, fc, want) + expectEqual(t, fc, want, nil) // Turn the service back into a ClusterIP service, but also add the // tailscale annotation. @@ -742,12 +734,8 @@ func TestLBIntoAnnotation(t *testing.T) { }) expectReconciled(t, sr, "default", "test") - // configfile hash changes on a re-apply in this case in tests only as - // we lose the auth key due to the test apply not syncing - // secret.StringData -> Data. - o.confFileHash = "fb9006e30ecda75e88c29dcd0ca2dd28a2ae964d001c66e1be3efe159cc3821d" - expectEqual(t, fc, expectedHeadlessService(shortName, "svc")) - expectEqual(t, fc, expectedSTS(t, fc, o)) + expectEqual(t, fc, expectedHeadlessService(shortName, "svc"), nil) + expectEqual(t, fc, expectedSTS(t, fc, o), removeHashAnnotation) want = &corev1.Service{ TypeMeta: metav1.TypeMeta{ @@ -768,7 +756,7 @@ func TestLBIntoAnnotation(t *testing.T) { Type: corev1.ServiceTypeClusterIP, }, } - expectEqual(t, fc, want) + expectEqual(t, fc, want, nil) } func TestCustomHostname(t *testing.T) { @@ -821,12 +809,11 @@ func TestCustomHostname(t *testing.T) { parentType: "svc", hostname: "reindeer-flotilla", clusterTargetIP: "10.20.30.40", - confFileHash: "42376226c7d76ed6d6318315dc6c402f7d993bc0b01a5b0e6c8a833106b7509e", } - expectEqual(t, fc, expectedSecret(t, o)) - expectEqual(t, fc, expectedHeadlessService(shortName, "svc")) - expectEqual(t, fc, expectedSTS(t, fc, o)) + expectEqual(t, fc, expectedSecret(t, o), nil) + expectEqual(t, fc, expectedHeadlessService(shortName, "svc"), nil) + expectEqual(t, fc, expectedSTS(t, fc, o), removeHashAnnotation) want := &corev1.Service{ TypeMeta: metav1.TypeMeta{ Kind: "Service", @@ -847,7 +834,7 @@ func TestCustomHostname(t *testing.T) { Type: corev1.ServiceTypeClusterIP, }, } - expectEqual(t, fc, want) + expectEqual(t, fc, want, nil) // Turn the service back into a ClusterIP service, which should make the // operator clean up. @@ -882,7 +869,7 @@ func TestCustomHostname(t *testing.T) { Type: corev1.ServiceTypeClusterIP, }, } - expectEqual(t, fc, want) + expectEqual(t, fc, want, nil) } func TestCustomPriorityClassName(t *testing.T) { @@ -937,10 +924,9 @@ func TestCustomPriorityClassName(t *testing.T) { hostname: "tailscale-critical", priorityClassName: "custom-priority-class-name", clusterTargetIP: "10.20.30.40", - confFileHash: "13cdef0d5f6f0f2406af028710ea1e0f99f65aba4021e4e70ac75a73cf141fd1", } - expectEqual(t, fc, expectedSTS(t, fc, o)) + expectEqual(t, fc, expectedSTS(t, fc, o), removeHashAnnotation) } func TestProxyClassForService(t *testing.T) { @@ -1000,11 +986,10 @@ func TestProxyClassForService(t *testing.T) { parentType: "svc", hostname: "default-test", clusterTargetIP: "10.20.30.40", - confFileHash: "6cceb342cd3e1c56cd1bd94c29df63df3653c35fe98a7e7afcdee0dcaa2ad549", } - expectEqual(t, fc, expectedSecret(t, opts)) - expectEqual(t, fc, expectedHeadlessService(shortName, "svc")) - expectEqual(t, fc, expectedSTS(t, fc, opts)) + expectEqual(t, fc, expectedSecret(t, opts), nil) + expectEqual(t, fc, expectedHeadlessService(shortName, "svc"), nil) + expectEqual(t, fc, expectedSTS(t, fc, opts), removeHashAnnotation) // 2. The Service gets updated with tailscale.com/proxy-class label // pointing at the 'custom-metadata' ProxyClass. The ProxyClass is not @@ -1013,7 +998,7 @@ func TestProxyClassForService(t *testing.T) { mak.Set(&svc.Labels, LabelProxyClass, "custom-metadata") }) expectReconciled(t, sr, "default", "test") - expectEqual(t, fc, expectedSTS(t, fc, opts)) + expectEqual(t, fc, expectedSTS(t, fc, opts), removeHashAnnotation) // 3. ProxyClass is set to Ready, the Service gets reconciled by the // services-reconciler and the customization from the ProxyClass is @@ -1027,12 +1012,8 @@ func TestProxyClassForService(t *testing.T) { }}} }) opts.proxyClass = pc.Name - // configfile hash changes on a second apply in test env only because we - // lose auth key due to out test not syncing secret.StringData -> - // secret.Data - opts.confFileHash = "fb9006e30ecda75e88c29dcd0ca2dd28a2ae964d001c66e1be3efe159cc3821d" expectReconciled(t, sr, "default", "test") - expectEqual(t, fc, expectedSTS(t, fc, opts)) + expectEqual(t, fc, expectedSTS(t, fc, opts), removeHashAnnotation) // 4. tailscale.com/proxy-class label is removed from the Service, the // configuration from the ProxyClass is removed from the cluster @@ -1042,7 +1023,7 @@ func TestProxyClassForService(t *testing.T) { }) opts.proxyClass = "" expectReconciled(t, sr, "default", "test") - expectEqual(t, fc, expectedSTS(t, fc, opts)) + expectEqual(t, fc, expectedSTS(t, fc, opts), removeHashAnnotation) } func TestDefaultLoadBalancer(t *testing.T) { @@ -1086,7 +1067,7 @@ func TestDefaultLoadBalancer(t *testing.T) { fullName, shortName := findGenName(t, fc, "default", "test", "svc") - expectEqual(t, fc, expectedHeadlessService(shortName, "svc")) + expectEqual(t, fc, expectedHeadlessService(shortName, "svc"), nil) o := configOpts{ stsName: shortName, secretName: fullName, @@ -1094,9 +1075,9 @@ func TestDefaultLoadBalancer(t *testing.T) { parentType: "svc", hostname: "default-test", clusterTargetIP: "10.20.30.40", - confFileHash: "6cceb342cd3e1c56cd1bd94c29df63df3653c35fe98a7e7afcdee0dcaa2ad549", } - expectEqual(t, fc, expectedSTS(t, fc, o)) + expectEqual(t, fc, expectedSTS(t, fc, o), removeHashAnnotation) + } func TestProxyFirewallMode(t *testing.T) { @@ -1148,10 +1129,70 @@ func TestProxyFirewallMode(t *testing.T) { hostname: "default-test", firewallMode: "nftables", clusterTargetIP: "10.20.30.40", - confFileHash: "6cceb342cd3e1c56cd1bd94c29df63df3653c35fe98a7e7afcdee0dcaa2ad549", } - expectEqual(t, fc, expectedSTS(t, fc, o)) + expectEqual(t, fc, expectedSTS(t, fc, o), removeHashAnnotation) +} +func TestTailscaledConfigfileHash(t *testing.T) { + fc := fake.NewFakeClient() + ft := &fakeTSClient{} + zl, err := zap.NewDevelopment() + if err != nil { + t.Fatal(err) + } + sr := &ServiceReconciler{ + Client: fc, + ssr: &tailscaleSTSReconciler{ + Client: fc, + tsClient: ft, + defaultTags: []string{"tag:k8s"}, + operatorNamespace: "operator-ns", + proxyImage: "tailscale/tailscale", + }, + logger: zl.Sugar(), + isDefaultLoadBalancer: true, + } + + // Create a service that we should manage, and check that the initial round + // of objects looks right. + mustCreate(t, fc, &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "default", + // The apiserver is supposed to set the UID, but the fake client + // doesn't. So, set it explicitly because other code later depends + // on it being set. + UID: types.UID("1234-UID"), + }, + Spec: corev1.ServiceSpec{ + ClusterIP: "10.20.30.40", + Type: corev1.ServiceTypeLoadBalancer, + }, + }) + + expectReconciled(t, sr, "default", "test") + + fullName, shortName := findGenName(t, fc, "default", "test", "svc") + o := configOpts{ + stsName: shortName, + secretName: fullName, + namespace: "default", + parentType: "svc", + hostname: "default-test", + clusterTargetIP: "10.20.30.40", + confFileHash: "705e5ffd0bd5326237efdf542c850a65a54101284d5daa30775420fcc64d89c1", + } + expectEqual(t, fc, expectedSTS(t, fc, o), nil) + + // 2. Hostname gets changed, configfile is updated and a new hash value + // is produced. + mustUpdate(t, fc, "default", "test", func(svc *corev1.Service) { + mak.Set(&svc.Annotations, AnnotationHostname, "another-test") + }) + o.hostname = "another-test" + o.confFileHash = "1a087f887825d2b75d3673c7c2b0131f8ec1f0b1cb761d33e236dd28350dfe23" + expectReconciled(t, sr, "default", "test") + expectEqual(t, fc, expectedSTS(t, fc, o), nil) } func Test_isMagicDNSName(t *testing.T) { diff --git a/cmd/k8s-operator/proxyclass_test.go b/cmd/k8s-operator/proxyclass_test.go index aada3b2cb..70b92f29d 100644 --- a/cmd/k8s-operator/proxyclass_test.go +++ b/cmd/k8s-operator/proxyclass_test.go @@ -69,7 +69,7 @@ func TestProxyClass(t *testing.T) { LastTransitionTime: &metav1.Time{Time: cl.Now().Truncate(time.Second)}, }) - expectEqual(t, fc, pc) + expectEqual(t, fc, pc, nil) // 2. An invalid ProxyClass resource gets its status updated to Invalid. pc.Spec.StatefulSet.Labels["foo"] = "?!someVal" @@ -79,5 +79,5 @@ func TestProxyClass(t *testing.T) { expectReconciled(t, pcr, "", "test") msg := `ProxyClass is not valid: .spec.statefulSet.labels: Invalid value: "?!someVal": a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?')` tsoperator.SetProxyClassCondition(pc, tsapi.ProxyClassready, metav1.ConditionFalse, reasonProxyClassInvalid, msg, 0, cl, zl.Sugar()) - expectEqual(t, fc, pc) + expectEqual(t, fc, pc, nil) } diff --git a/cmd/k8s-operator/sts.go b/cmd/k8s-operator/sts.go index 1c231b1f2..8534ef7d2 100644 --- a/cmd/k8s-operator/sts.go +++ b/cmd/k8s-operator/sts.go @@ -650,10 +650,11 @@ func applyProxyClassToStatefulSet(pc *tsapi.ProxyClass, ss *appsv1.StatefulSet) // produces returns tailscaled configuration and a hash of that configuration. func tailscaledConfig(stsC *tailscaleSTSConfig, newAuthkey string, oldSecret *corev1.Secret) ([]byte, string, error) { conf := ipn.ConfigVAlpha{ - Version: "alpha0", - AcceptDNS: "false", - Locked: "false", - Hostname: &stsC.Hostname, + Version: "alpha0", + AcceptDNS: "false", + AcceptRoutes: "false", // AcceptRoutes defaults to true + Locked: "false", + Hostname: &stsC.Hostname, } if stsC.Connector != nil { routes, err := netutil.CalcAdvertiseRoutes(stsC.Connector.routes, stsC.Connector.isExitNode) diff --git a/cmd/k8s-operator/testutils_test.go b/cmd/k8s-operator/testutils_test.go index 58c150e5d..767ab2e0c 100644 --- a/cmd/k8s-operator/testutils_test.go +++ b/cmd/k8s-operator/testutils_test.go @@ -97,7 +97,9 @@ func expectedSTS(t *testing.T, cl client.Client, opts configOpts) *appsv1.Statef ReadOnly: true, MountPath: "/etc/tsconfig", }} - annots["tailscale.com/operator-last-set-config-file-hash"] = opts.confFileHash + if opts.confFileHash != "" { + annots["tailscale.com/operator-last-set-config-file-hash"] = opts.confFileHash + } if opts.firewallMode != "" { tsContainer.Env = append(tsContainer.Env, corev1.EnvVar{ Name: "TS_DEBUG_FIREWALL_MODE", @@ -269,7 +271,6 @@ func expectedSTSUserspace(t *testing.T, cl client.Client, opts configOpts) *apps "tailscale.com/parent-resource-type": opts.parentType, "app": "1234-UID", }, - Annotations: map[string]string{"tailscale.com/operator-last-set-config-file-hash": opts.confFileHash}, }, Spec: corev1.PodSpec{ ServiceAccountName: "proxies", @@ -280,6 +281,10 @@ func expectedSTSUserspace(t *testing.T, cl client.Client, opts configOpts) *apps }, }, } + ss.Spec.Template.Annotations = map[string]string{} + if opts.confFileHash != "" { + ss.Spec.Template.Annotations["tailscale.com/operator-last-set-config-file-hash"] = opts.confFileHash + } // If opts.proxyClass is set, retrieve the ProxyClass and apply // configuration from that to the StatefulSet. if opts.proxyClass != "" { @@ -339,11 +344,12 @@ func expectedSecret(t *testing.T, opts configOpts) *corev1.Secret { mak.Set(&s.StringData, "serve-config", string(serveConfigBs)) } conf := &ipn.ConfigVAlpha{ - Version: "alpha0", - AcceptDNS: "false", - Hostname: &opts.hostname, - Locked: "false", - AuthKey: ptr.To("secret-authkey"), + Version: "alpha0", + AcceptDNS: "false", + Hostname: &opts.hostname, + Locked: "false", + AuthKey: ptr.To("secret-authkey"), + AcceptRoutes: "false", } var routes []netip.Prefix if opts.subnetRoutes != "" || opts.isExitNode { @@ -433,7 +439,13 @@ func mustUpdateStatus[T any, O ptrObject[T]](t *testing.T, client client.Client, } } -func expectEqual[T any, O ptrObject[T]](t *testing.T, client client.Client, want O) { +// expectEqual accepts a Kubernetes object and a Kubernetes client. It tests +// whether an object with equivalent contents can be retrieved by the passed +// client. If you want to NOT test some object fields for equality, ensure that +// they are not present in the passed object and use the modify func to remove +// them from the cluster object. If no such modifications are needed, you can +// pass nil in place of the modify function. +func expectEqual[T any, O ptrObject[T]](t *testing.T, client client.Client, want O, modify func(O)) { t.Helper() got := O(new(T)) if err := client.Get(context.Background(), types.NamespacedName{ @@ -447,6 +459,9 @@ func expectEqual[T any, O ptrObject[T]](t *testing.T, client client.Client, want // so just remove it from both got and want. got.SetResourceVersion("") want.SetResourceVersion("") + if modify != nil { + modify(got) + } if diff := cmp.Diff(got, want); diff != "" { t.Fatalf("unexpected object (-got +want):\n%s", diff) } @@ -543,3 +558,11 @@ func (c *fakeTSClient) Deleted() []string { defer c.Unlock() return c.deleted } + +// removeHashAnnotation can be used to remove declarative tailscaled config hash +// annotation from proxy StatefulSets to make the tests more maintainable (so +// that we don't have to change the annotation in each test case after any +// change to the configfile contents). +func removeHashAnnotation(sts *appsv1.StatefulSet) { + delete(sts.Spec.Template.Annotations, podAnnotationLastSetConfigFileHash) +} diff --git a/ipn/conf.go b/ipn/conf.go index f795e5205..ffd4c7269 100644 --- a/ipn/conf.go +++ b/ipn/conf.go @@ -23,8 +23,8 @@ type ConfigVAlpha struct { OperatorUser *string `json:",omitempty"` // local user name who is allowed to operate tailscaled without being root or using sudo Hostname *string `json:",omitempty"` - AcceptDNS opt.Bool `json:"acceptDNS,omitempty"` // --accept-dns - AcceptRoutes opt.Bool `json:"acceptRoutes,omitempty"` + AcceptDNS opt.Bool `json:"acceptDNS,omitempty"` // --accept-dns + AcceptRoutes opt.Bool `json:"acceptRoutes,omitempty"` // --accept-routes defaults to true ExitNode *string `json:"exitNode,omitempty"` // IP, StableID, or MagicDNS base name AllowLANWhileUsingExitNode opt.Bool `json:"allowLANWhileUsingExitNode,omitempty"`