diff --git a/pkg/controllers/lballoc/lballoc.go b/pkg/controllers/lballoc/lballoc.go index 9abeb87e5..e5cafcb20 100644 --- a/pkg/controllers/lballoc/lballoc.go +++ b/pkg/controllers/lballoc/lballoc.go @@ -3,8 +3,10 @@ package lballoc import ( "context" "errors" + "fmt" "net" "os" + "strings" "sync" "time" @@ -391,16 +393,25 @@ func (lbc *LoadBalancerController) allocateService(svc *v1core.Service) error { if want6 && !have6 { ipv6, err6 = lbc.ipv6Ranges.getNextFreeIP(allocated6) } - err := err6 + + // If no allocation was needed, the service already has all requested IPs + if ipv4 == nil && ipv6 == nil && err4 == nil && err6 == nil { + return nil + } + + var errMsgs []string if err4 != nil { - err = err4 + errMsgs = append(errMsgs, fmt.Sprintf("IPv4: %v", err4)) + } + if err6 != nil { + errMsgs = append(errMsgs, fmt.Sprintf("IPv6: %v", err6)) } if ipv4 == nil && ipv6 == nil { - return errors.New("unable to allocate address: " + err.Error()) + return fmt.Errorf("unable to allocate address: %s", strings.Join(errMsgs, "; ")) } if (ipv4 == nil || ipv6 == nil) && requireDual { - return errors.New("unable to allocate dual-stack addresses: " + err.Error()) + return fmt.Errorf("unable to allocate dual-stack addresses: %s", strings.Join(errMsgs, "; ")) } // This is only non-nil during certain unit tests that need to understand when this goroutine is finished to remove diff --git a/pkg/controllers/lballoc/lballoc_test.go b/pkg/controllers/lballoc/lballoc_test.go index a39782961..585df578e 100644 --- a/pkg/controllers/lballoc/lballoc_test.go +++ b/pkg/controllers/lballoc/lballoc_test.go @@ -596,7 +596,7 @@ func TestAllocateService(t *testing.T) { fp := v1core.IPFamilyPolicyRequireDualStack svc.Spec.IPFamilyPolicy = &fp err = mlbc.allocateService(&svc) - errExp := "unable to allocate dual-stack addresses: no IPs left to allocate" + errExp := "unable to allocate dual-stack addresses: IPv4: no IPs left to allocate" if errExp != err.Error() { t.Fatalf("expected %s, got %s", errExp, err) } @@ -604,6 +604,7 @@ func TestAllocateService(t *testing.T) { mlbc.ipv4Ranges = ir4 mlbc.ipv6Ranges = newipRanges(nil) err = mlbc.allocateService(&svc) + errExp = "unable to allocate dual-stack addresses: IPv6: no IPs left to allocate" if errExp != err.Error() { t.Fatalf("expected %s, got %s", errExp, err) } @@ -612,7 +613,7 @@ func TestAllocateService(t *testing.T) { fp = v1core.IPFamilyPolicyPreferDualStack svc.Spec.IPFamilyPolicy = &fp err = mlbc.allocateService(&svc) - errExp = "unable to allocate address: no IPs left to allocate" + errExp = "unable to allocate address: IPv4: no IPs left to allocate; IPv6: no IPs left to allocate" if errExp != err.Error() { t.Fatalf("expected %s, got %s", errExp, err) } diff --git a/pkg/controllers/netpol/pod.go b/pkg/controllers/netpol/pod.go index a52d2209f..968024f28 100644 --- a/pkg/controllers/netpol/pod.go +++ b/pkg/controllers/netpol/pod.go @@ -4,12 +4,27 @@ import ( "crypto/sha256" "encoding/base32" "strings" + "unicode" api "k8s.io/api/core/v1" "k8s.io/client-go/tools/cache" "k8s.io/klog/v2" ) +// sanitizeForComment removes control characters (newlines, tabs, etc.) from a +// string before it is embedded into an iptables-restore comment. This is a +// defense-in-depth measure: Kubernetes API server validates pod/namespace names +// to DNS label format, but we should not rely solely on external validation when +// constructing input for iptables-restore. +func sanitizeForComment(s string) string { + return strings.Map(func(r rune) rune { + if unicode.IsControl(r) { + return -1 + } + return r + }, s) +} + func (npc *NetworkPolicyController) newPodEventHandler() cache.ResourceEventHandler { return cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { @@ -88,7 +103,7 @@ func (npc *NetworkPolicyController) syncPodFirewallChains(networkPoliciesInfo [] } // add rule to log the packets that will be dropped due to network policy enforcement - comment := "\"rule to log dropped traffic POD name:" + pod.name + " namespace: " + pod.namespace + "\"" + comment := "\"rule to log dropped traffic POD name:" + sanitizeForComment(pod.name) + " namespace: " + sanitizeForComment(pod.namespace) + "\"" args := []string{"-A", podFwChainName, "-m", "comment", "--comment", comment, "-m", "mark", "!", "--mark", "0x10000/0x10000", "-j", "NFLOG", "--nflog-group", "100", "-m", "limit", "--limit", "10/minute", "--limit-burst", "10", "\n"} @@ -100,8 +115,8 @@ func (npc *NetworkPolicyController) syncPodFirewallChains(networkPoliciesInfo [] filterTableRules.WriteString(strings.Join(args, " ")) // add rule to DROP if no applicable network policy permits the traffic - comment = "\"rule to REJECT traffic destined for POD name:" + pod.name + " namespace: " + - pod.namespace + "\"" + comment = "\"rule to REJECT traffic destined for POD name:" + sanitizeForComment(pod.name) + " namespace: " + + sanitizeForComment(pod.namespace) + "\"" args = []string{"-A", podFwChainName, "-m", "comment", "--comment", comment, "-m", "mark", "!", "--mark", "0x10000/0x10000", "-j", "REJECT", "\n"} filterTableRules.WriteString(strings.Join(args, " ")) @@ -188,7 +203,7 @@ func (npc *NetworkPolicyController) setupPodNetpolRules(pod podInfo, podFwChainN if _, ok := policy.targetPods[pod.ip]; !ok { continue } - comment := "\"run through nw policy " + policy.name + "\"" + comment := "\"run through nw policy " + sanitizeForComment(policy.name) + "\"" policyChainName := networkPolicyChainName(policy.namespace, policy.name, version, ipFamily) var args []string switch policy.policyType { @@ -255,7 +270,7 @@ func (npc *NetworkPolicyController) interceptPodInboundTraffic(pod podInfo, podF // ensure there is rule in filter table and FORWARD chain to jump to pod specific firewall chain // this rule applies to the traffic getting routed (coming for other node pods) - comment := "\"rule to jump traffic destined to POD name:" + pod.name + " namespace: " + pod.namespace + + comment := "\"rule to jump traffic destined to POD name:" + sanitizeForComment(pod.name) + " namespace: " + sanitizeForComment(pod.namespace) + " to chain " + podFwChainName + "\"" args := []string{"-A", kubeForwardChainName, "-m", "comment", "--comment", comment, "-d", ip, "-j", podFwChainName + "\n"} @@ -269,7 +284,7 @@ func (npc *NetworkPolicyController) interceptPodInboundTraffic(pod podInfo, podF // ensure there is rule in filter table and forward chain to jump to pod specific firewall chain // this rule applies to the traffic getting switched (coming for same node pods) - comment = "\"rule to jump traffic destined to POD name:" + pod.name + " namespace: " + pod.namespace + + comment = "\"rule to jump traffic destined to POD name:" + sanitizeForComment(pod.name) + " namespace: " + sanitizeForComment(pod.namespace) + " to chain " + podFwChainName + "\"" args = []string{"-A", kubeForwardChainName, "-m", "physdev", "--physdev-is-bridged", "-m", "comment", "--comment", comment, @@ -294,7 +309,7 @@ func (npc *NetworkPolicyController) interceptPodOutboundTraffic(pod podInfo, pod // ensure there is rule in filter table and FORWARD chain to jump to pod specific firewall chain // this rule applies to the traffic getting forwarded/routed (traffic from the pod destined // to pod on a different node) - comment := "\"rule to jump traffic from POD name:" + pod.name + " namespace: " + pod.namespace + + comment := "\"rule to jump traffic from POD name:" + sanitizeForComment(pod.name) + " namespace: " + sanitizeForComment(pod.namespace) + " to chain " + podFwChainName + "\"" args := []string{"-A", chain, "-m", "comment", "--comment", comment, "-s", ip, "-j", podFwChainName, "\n"} filterTableRules.WriteString(strings.Join(args, " ")) @@ -302,7 +317,7 @@ func (npc *NetworkPolicyController) interceptPodOutboundTraffic(pod podInfo, pod // ensure there is rule in filter table and forward chain to jump to pod specific firewall chain // this rule applies to the traffic getting switched (coming for same node pods) - comment := "\"rule to jump traffic from POD name:" + pod.name + " namespace: " + pod.namespace + + comment := "\"rule to jump traffic from POD name:" + sanitizeForComment(pod.name) + " namespace: " + sanitizeForComment(pod.namespace) + " to chain " + podFwChainName + "\"" args := []string{"-A", kubeForwardChainName, "-m", "physdev", "--physdev-is-bridged", "-m", "comment", "--comment", comment, diff --git a/pkg/controllers/netpol/sanitize_test.go b/pkg/controllers/netpol/sanitize_test.go new file mode 100644 index 000000000..5d86f4412 --- /dev/null +++ b/pkg/controllers/netpol/sanitize_test.go @@ -0,0 +1,58 @@ +package netpol + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestSanitizeForComment(t *testing.T) { + tests := []struct { + name string + input string + want string + }{ + { + name: "clean string passes through", + input: "my-pod-name", + want: "my-pod-name", + }, + { + name: "newlines stripped", + input: "my-pod\n-A INJECT -j DROP\n", + want: "my-pod-A INJECT -j DROP", + }, + { + name: "tabs stripped", + input: "my-pod\tinjected", + want: "my-podinjected", + }, + { + name: "carriage return stripped", + input: "my-pod\rinjected", + want: "my-podinjected", + }, + { + name: "null bytes stripped", + input: "my-pod\x00injected", + want: "my-podinjected", + }, + { + name: "normal characters preserved", + input: "nginx-deployment-7c79f4c9b8-abc12", + want: "nginx-deployment-7c79f4c9b8-abc12", + }, + { + name: "empty string", + input: "", + want: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := sanitizeForComment(tt.input) + assert.Equal(t, tt.want, got) + }) + } +}