From 4d306fc68eee31ce697e04c6342f90cdf0ce499c Mon Sep 17 00:00:00 2001 From: Vinayak Mohanty Date: Fri, 5 Jun 2026 15:09:03 +0530 Subject: [PATCH] fix: truncate service comments in nftables to prevent length limit violations refactor: rename svcPortNameString to svcPortComment and update test validation in nftables proxier --- pkg/proxy/nftables/proxier.go | 19 +++++-- pkg/proxy/nftables/proxier_test.go | 79 ++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 5 deletions(-) diff --git a/pkg/proxy/nftables/proxier.go b/pkg/proxy/nftables/proxier.go index 3775858812a..65e22866d2f 100644 --- a/pkg/proxy/nftables/proxier.go +++ b/pkg/proxy/nftables/proxier.go @@ -855,6 +855,15 @@ func hashAndTruncate(name string) string { return name } +// truncateNFTablesComment truncates s to the maximum byte length accepted by +// nftables. Comments are cosmetic, so truncation does not affect routing. +func truncateNFTablesComment(s string) string { + if len(s) > knftables.CommentLengthMax { + return s[:knftables.CommentLengthMax] + } + return s +} + // servicePortChainNameBase returns the base name for a chain for the given ServicePort. // This is something like "HASH-namespace/serviceName/protocol/portName", e.g, // "ULMVA6XW-ns1/svc1/tcp/p80". @@ -1188,7 +1197,7 @@ func (proxier *Proxier) syncProxyRules() (retryError error) { } protocol := strings.ToLower(string(svcInfo.Protocol())) - svcPortNameString := svcInfo.nameString + svcPortComment := truncateNFTablesComment(svcInfo.nameString) // Figure out the endpoints for Cluster and Local traffic policy. // allLocallyReachableEndpoints is the set of all endpoints that can be routed to @@ -1346,7 +1355,7 @@ func (proxier *Proxier) syncProxyRules() (retryError error) { Value: []string{ internalTrafficFilterVerdict, }, - Comment: &svcPortNameString, + Comment: &svcPortComment, }) } @@ -1381,7 +1390,7 @@ func (proxier *Proxier) syncProxyRules() (retryError error) { Value: []string{ externalTrafficFilterVerdict, }, - Comment: &svcPortNameString, + Comment: &svcPortComment, }) } } @@ -1437,7 +1446,7 @@ func (proxier *Proxier) syncProxyRules() (retryError error) { Value: []string{ externalTrafficFilterVerdict, }, - Comment: &svcPortNameString, + Comment: &svcPortComment, }) } } @@ -1472,7 +1481,7 @@ func (proxier *Proxier) syncProxyRules() (retryError error) { Value: []string{ externalTrafficFilterVerdict, }, - Comment: &svcPortNameString, + Comment: &svcPortComment, }) } } diff --git a/pkg/proxy/nftables/proxier_test.go b/pkg/proxy/nftables/proxier_test.go index 1a591114026..78a2a79bb0d 100644 --- a/pkg/proxy/nftables/proxier_test.go +++ b/pkg/proxy/nftables/proxier_test.go @@ -22,6 +22,7 @@ import ( "fmt" "net" "reflect" + "strings" "testing" "time" @@ -451,6 +452,84 @@ func TestOverallNFTablesRules(t *testing.T) { assertNFTablesTransactionEqual(t, getLine(), expected, nft.Dump()) } +func TestTruncateNFTablesComment(t *testing.T) { + tests := []struct { + name string + input string + want string + }{ + { + name: "short comment unchanged", + input: "kube-system/kube-dns:dns", + want: "kube-system/kube-dns:dns", + }, + { + name: "exactly max length unchanged", + input: strings.Repeat("a", knftables.CommentLengthMax), + want: strings.Repeat("a", knftables.CommentLengthMax), + }, + { + name: "too long gets truncated", + input: strings.Repeat("a", knftables.CommentLengthMax+1), + want: strings.Repeat("a", knftables.CommentLengthMax), + }, + { + name: "realistic long service name truncated", + input: "loft-acme-platform-services-team-dev-v-shared-workloads-ns-001/my-api-svc-x-my-api-namespace-x-acme-shared-vcluster--a1b2c3d4e:http", + want: "loft-acme-platform-services-team-dev-v-shared-workloads-ns-001/my-api-svc-x-my-api-namespace-x-acme-shared-vcluster--a1b2c3d4e:h", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := truncateNFTablesComment(tc.input) + if got != tc.want { + t.Errorf("truncateNFTablesComment(%q) = %q, want %q", tc.input, got, tc.want) + } + if len(got) > knftables.CommentLengthMax { + t.Errorf("result length %d exceeds max %d", len(got), knftables.CommentLengthMax) + } + }) + } +} + +func TestSyncProxyRulesTruncatesNoEndpointServiceComment(t *testing.T) { + nft, fp := NewFakeProxier(v1.IPv4Protocol) + serviceNamespace := "loft-acme-platform-services-team-dev-v-shared-workloads-ns-001" + serviceName := "my-api-svc-x-my-api-namespace-x-acme-shared-vcluster--a1b2c3d4e" + portName := "http" + serviceComment := fmt.Sprintf("%s/%s:%s", serviceNamespace, serviceName, portName) + truncatedComment := truncateNFTablesComment(serviceComment) + if len(serviceComment) <= knftables.CommentLengthMax { + t.Fatalf("test service comment length %d should exceed max %d", len(serviceComment), knftables.CommentLengthMax) + } + + makeServiceMap(fp, + makeTestService(serviceNamespace, serviceName, func(svc *v1.Service) { + svc.Spec.Type = v1.ServiceTypeClusterIP + svc.Spec.ClusterIP = "172.30.0.47" + svc.Spec.Ports = []v1.ServicePort{{ + Name: portName, + Port: 80, + Protocol: v1.ProtocolTCP, + }} + }), + ) + + if err := fp.syncProxyRules(); err != nil { + t.Fatalf("syncProxyRules failed: %v", err) + } + + expected := truncatedComment + elem := nft.Table.Maps["no-endpoint-services"].FindElement("172.30.0.47", "tcp", "80") + if elem == nil || elem.Comment == nil || *elem.Comment != expected { + t.Fatalf("expected nftables transaction to contain truncated comment %q, got: %+v", expected, elem) + } + if strings.Contains(nft.Dump(), serviceComment) { + t.Fatalf("nftables transaction contains untruncated comment %q", serviceComment) + } +} + // TestNoEndpointsReject tests that a service with no endpoints rejects connections to // its ClusterIP, ExternalIPs, NodePort, and LoadBalancer IP. func TestNoEndpointsReject(t *testing.T) {