From dc32293294f29f9de89b7fc312f2fdea71be4e71 Mon Sep 17 00:00:00 2001 From: Ricardo Katz Date: Thu, 4 Jun 2026 15:50:56 -0400 Subject: [PATCH] OCPBUGS-87166: Prevent SSRF via FQDN-typed EndpointSlices The OpenShift Router previously did not validate backend destinations resolved from FQDN-typed EndpointSlices. This allowed the usage of invalid EndpointSlices to target hostnames that resolves to restricted IPs (like the cloud metadata service at 169.254.169.254). This commit disables the usage of EndpointSlices of type FQDN, and add validations on Endpoints to check if a restricted IP is being used before adding them to HAProxy endpoints Backend. The implementation and disabling the usage of FQDN-backed endpoints is based on the following: * RFE-2832 to implement support for ExternalName was considered and rejected due to security concerns, so OpenShift today does not support officially the usage of FQDN-based endpoints on router * OCPBUGS-55506 relates to a mistake caused by the customer that caused unavailability and not the need to support FQDN-based names * The behavior of an endpointslice of type FQDN is deprecated on Kubernetes * The behavior of an endpointslice using an address that is not IPv4 or IPv6 (eg.: hostname) is unespecified by Kubernetes and has no usage. * The hostname field on endpointslice is not used on router This way, there is a common understanding that the usage of FQDN based addresses on Router was a mistake, and disabling it is the right fix. Additional validations of the IP address on the generated endpointnt array is added to guarantee that no invalid nor restricted IP is used. --- pkg/cmd/infra/router/template.go | 4 +- .../controller/endpointsubset/converter.go | 9 + .../endpointsubset/converter_test.go | 188 +++++ pkg/router/controller/extended_validator.go | 150 +++- .../controller/extended_validator_test.go | 676 ++++++++++++++++++ .../factory/factory_endpointslices_test.go | 1 + pkg/router/router_test.go | 1 + pkg/router/template/plugin_test.go | 2 +- 8 files changed, 1013 insertions(+), 18 deletions(-) create mode 100644 pkg/router/controller/extended_validator_test.go diff --git a/pkg/cmd/infra/router/template.go b/pkg/cmd/infra/router/template.go index 2505e43ca..4db7a2fd9 100644 --- a/pkg/cmd/infra/router/template.go +++ b/pkg/cmd/infra/router/template.go @@ -812,9 +812,7 @@ func (o *TemplateRouterOptions) Run(stopCh <-chan struct{}) error { if o.UpgradeValidation { plugin = controller.NewUpgradeValidation(plugin, recorder, o.UpgradeValidationForceAddCondition, o.UpgradeValidationForceRemoveCondition) } - if o.ExtendedValidation { - plugin = controller.NewExtendedValidator(plugin, recorder) - } + plugin = controller.NewExtendedValidator(plugin, recorder, o.ExtendedValidation) if o.AllowExternalCertificates { plugin = controller.NewRouteSecretManager(plugin, recorder, secretManager, o.RouterName, kc.CoreV1(), routeLister, authorizationClient.SubjectAccessReviews()) } diff --git a/pkg/router/controller/endpointsubset/converter.go b/pkg/router/controller/endpointsubset/converter.go index 46027e5ed..147ee81eb 100644 --- a/pkg/router/controller/endpointsubset/converter.go +++ b/pkg/router/controller/endpointsubset/converter.go @@ -3,13 +3,22 @@ package endpointsubset import ( corev1 "k8s.io/api/core/v1" discoveryv1 "k8s.io/api/discovery/v1" + + logf "github.com/openshift/router/log" ) +var log = logf.Logger.WithName("endpointsubset") + // ConvertEndpointSlice converts items to a slice of EndpointSubset's. func ConvertEndpointSlice(items []discoveryv1.EndpointSlice, addressOrderByFuncs []EndpointAddressLessFunc, portOrderByFuncs []EndpointPortLessFunc) []corev1.EndpointSubset { var subsets []corev1.EndpointSubset for i := range items { + if items[i].AddressType != discoveryv1.AddressTypeIPv4 && items[i].AddressType != discoveryv1.AddressTypeIPv6 { + log.Info("Skipping EndpointSlice with unsupported address type", "namespace", items[i].Namespace, "name", items[i].Name, "addressType", items[i].AddressType) + continue + } + var ports []corev1.EndpointPort var addresses []corev1.EndpointAddress var notReadyAddresses []corev1.EndpointAddress diff --git a/pkg/router/controller/endpointsubset/converter_test.go b/pkg/router/controller/endpointsubset/converter_test.go index aff478ee8..65b749c92 100644 --- a/pkg/router/controller/endpointsubset/converter_test.go +++ b/pkg/router/controller/endpointsubset/converter_test.go @@ -104,3 +104,191 @@ func TestConvertEndpointSlice(t *testing.T) { }) } } + +func TestConvertEndpointSlice_addressTypes(t *testing.T) { + serviceLabels := map[string]string{ + discoveryv1.LabelServiceName: "service-a", + } + sliceMeta := metav1.TypeMeta{ + Kind: "EndpointSlice", + APIVersion: "discovery.k8s.io/v1", + } + + tests := []struct { + name string + items []discoveryv1.EndpointSlice + want []v1.EndpointSubset + }{{ + name: "FQDN AddressType is skipped", + items: []discoveryv1.EndpointSlice{{ + TypeMeta: sliceMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "slice-fqdn", + Namespace: "namespace-a", + Labels: serviceLabels, + }, + AddressType: discoveryv1.AddressTypeFQDN, + Endpoints: []discoveryv1.Endpoint{{ + Addresses: []string{"metadata.google.internal"}, + }}, + Ports: []discoveryv1.EndpointPort{{ + Port: int32Ptr(8080), + }}, + }}, + }, { + name: "unknown AddressType is skipped", + items: []discoveryv1.EndpointSlice{{ + TypeMeta: sliceMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "slice-unknown", + Namespace: "namespace-a", + Labels: serviceLabels, + }, + AddressType: discoveryv1.AddressType("Unknown"), + Endpoints: []discoveryv1.Endpoint{{ + Addresses: []string{"10.0.0.2"}, + }}, + }}, + }, { + name: "empty AddressType is skipped", + items: []discoveryv1.EndpointSlice{{ + TypeMeta: sliceMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "slice-empty-type", + Namespace: "namespace-a", + Labels: serviceLabels, + }, + Endpoints: []discoveryv1.Endpoint{{ + Addresses: []string{"10.0.0.3"}, + }}, + }}, + }, { + name: "IPv6 AddressType is converted", + items: []discoveryv1.EndpointSlice{{ + TypeMeta: sliceMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "slice-ipv6", + Namespace: "namespace-a", + Labels: serviceLabels, + }, + AddressType: discoveryv1.AddressTypeIPv6, + Endpoints: []discoveryv1.Endpoint{{ + Addresses: []string{"2001:db8::1"}, + }}, + Ports: []discoveryv1.EndpointPort{{ + Port: int32Ptr(443), + }}, + }}, + want: []v1.EndpointSubset{{ + Addresses: []v1.EndpointAddress{{IP: "2001:db8::1"}}, + Ports: []v1.EndpointPort{{Port: 443}}, + }}, + }, { + name: "IPv4 slice with hostname in addresses passes through conversion", + items: []discoveryv1.EndpointSlice{{ + TypeMeta: sliceMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "slice-ipv4-hostname", + Namespace: "namespace-a", + Labels: serviceLabels, + }, + AddressType: discoveryv1.AddressTypeIPv4, + Endpoints: []discoveryv1.Endpoint{{ + Addresses: []string{"10.0.0.1", "metadata.google.internal"}, + }}, + Ports: []discoveryv1.EndpointPort{{ + Port: int32Ptr(8080), + }}, + }}, + want: []v1.EndpointSubset{{ + Addresses: []v1.EndpointAddress{ + {IP: "metadata.google.internal"}, + {IP: "10.0.0.1"}, + }, + Ports: []v1.EndpointPort{{Port: 8080}}, + }}, + }, { + name: "mixed supported and unsupported slices", + items: []discoveryv1.EndpointSlice{{ + TypeMeta: sliceMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "slice-fqdn", + Namespace: "namespace-a", + Labels: serviceLabels, + }, + AddressType: discoveryv1.AddressTypeFQDN, + Endpoints: []discoveryv1.Endpoint{{ + Addresses: []string{"metadata.google.internal"}, + }}, + }, { + TypeMeta: sliceMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "slice-unknown", + Namespace: "namespace-a", + Labels: serviceLabels, + }, + AddressType: discoveryv1.AddressType("Unknown"), + Endpoints: []discoveryv1.Endpoint{{ + Addresses: []string{"10.0.0.2"}, + }}, + }, { + TypeMeta: sliceMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "slice-empty-type", + Namespace: "namespace-a", + Labels: serviceLabels, + }, + Endpoints: []discoveryv1.Endpoint{{ + Addresses: []string{"10.0.0.3"}, + }}, + }, { + TypeMeta: sliceMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "slice-ipv4", + Namespace: "namespace-a", + Labels: serviceLabels, + }, + AddressType: discoveryv1.AddressTypeIPv4, + Endpoints: []discoveryv1.Endpoint{{ + Addresses: []string{"10.0.0.1"}, + }}, + Ports: []discoveryv1.EndpointPort{{ + Port: int32Ptr(80), + }}, + }, { + TypeMeta: sliceMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "slice-ipv6", + Namespace: "namespace-a", + Labels: serviceLabels, + }, + AddressType: discoveryv1.AddressTypeIPv6, + Endpoints: []discoveryv1.Endpoint{{ + Addresses: []string{"2001:db8::1"}, + }}, + Ports: []discoveryv1.EndpointPort{{ + Port: int32Ptr(443), + }}, + }}, + want: []v1.EndpointSubset{{ + Addresses: []v1.EndpointAddress{{IP: "10.0.0.1"}}, + Ports: []v1.EndpointPort{{Port: 80}}, + }, { + Addresses: []v1.EndpointAddress{{IP: "2001:db8::1"}}, + Ports: []v1.EndpointPort{{Port: 443}}, + }}, + }} + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := endpointsubset.ConvertEndpointSlice( + tc.items, + endpointsubset.DefaultEndpointAddressOrderByFuncs(), + endpointsubset.DefaultEndpointPortOrderByFuncs(), + ) + if diff := cmp.Diff(tc.want, got); len(diff) != 0 { + t.Errorf("ConvertEndpointSlice() failed (-want +got):\n%s", diff) + } + }) + } +} diff --git a/pkg/router/controller/extended_validator.go b/pkg/router/controller/extended_validator.go index a3112443f..38e948cfc 100644 --- a/pkg/router/controller/extended_validator.go +++ b/pkg/router/controller/extended_validator.go @@ -2,6 +2,8 @@ package controller import ( "fmt" + "net/netip" + "slices" kapi "k8s.io/api/core/v1" @@ -21,15 +23,20 @@ type ExtendedValidator struct { // recorder is an interface for indicating route status. recorder RouteStatusRecorder + + // extendedRouteValidation enables extended route validation checks. + extendedRouteValidation bool } -// NewExtendedValidator creates a plugin wrapper that ensures only routes that -// pass extended validation are relayed to the next plugin in the chain. -// Recorder is an interface for indicating route status updates. -func NewExtendedValidator(plugin router.Plugin, recorder RouteStatusRecorder) *ExtendedValidator { +// NewExtendedValidator creates a plugin wrapper that ensures only routes and +// endpoints that pass validation are relayed to the next plugin in the chain. +// Endpoint address validation is always enabled. Route validation is +// controlled by extendedRouteValidation. +func NewExtendedValidator(plugin router.Plugin, recorder RouteStatusRecorder, extendedRouteValidation bool) *ExtendedValidator { return &ExtendedValidator{ - plugin: plugin, - recorder: recorder, + plugin: plugin, + recorder: recorder, + extendedRouteValidation: extendedRouteValidation, } } @@ -39,21 +46,136 @@ func (p *ExtendedValidator) HandleNode(eventType watch.EventType, node *kapi.Nod } // HandleEndpoints processes watch events on the Endpoints resource. +// Addresses that fail IP validation are removed individually rather +// than rejecting the entire endpoint set. func (p *ExtendedValidator) HandleEndpoints(eventType watch.EventType, endpoints *kapi.Endpoints) error { + if eventType == watch.Deleted { + return p.plugin.HandleEndpoints(eventType, endpoints) + } + + // endpoints may be an object shared with (and owned by) an informer's + // cache, so it must be deep-copied before it is mutated below. + endpoints = endpoints.DeepCopy() + for i, subset := range endpoints.Subsets { + endpoints.Subsets[i].Addresses = filterValidAddresses(subset.Addresses) + endpoints.Subsets[i].NotReadyAddresses = filterValidAddresses(subset.NotReadyAddresses) + } return p.plugin.HandleEndpoints(eventType, endpoints) } +func filterValidAddresses(addrs []kapi.EndpointAddress) []kapi.EndpointAddress { + return slices.DeleteFunc(addrs, func(addr kapi.EndpointAddress) bool { + err := validateEndpointAddress(addr.IP) + if err != nil { + log.Error(err, "Skipping endpoint address with restricted or invalid IP", "address", addr.IP) + } + return err != nil + }) +} + +func validateEndpointAddress(address string) error { + addr, err := netip.ParseAddr(address) + if err != nil { + return fmt.Errorf("address %q is not a valid IP address", address) + } + // EndpointSlice addresses of type IPv4 or IPv6 are validated by the Kubernetes + // API, which requires canonical IP form and rejects hostnames, non-canonical + // encodings (such as ::127.0.0.1), and IPv4-mapped IPv6 literals. + return checkRestrictedIP(addr) +} + +var ( + azureMetadata = netip.MustParseAddr("168.63.129.16") + awsIPv6IMDS = netip.MustParseAddr("fd00:ec2::254") +) + +func ipv4CompatibleTo4(addr netip.Addr) (netip.Addr, bool) { + if !addr.Is6() || addr.Is4In6() { + return netip.Addr{}, false + } + b := addr.As16() + for i := 0; i < 12; i++ { + if b[i] != 0 { + return netip.Addr{}, false + } + } + return netip.AddrFrom4([4]byte{b[12], b[13], b[14], b[15]}), true +} + +func normalizeEmbeddedIPv4(addr netip.Addr) (netip.Addr, bool) { + if addr.Is4In6() { + return addr.Unmap(), true + } + if v4, ok := ipv4CompatibleTo4(addr); ok { + return v4, true + } + return addr, false +} + +func restrictedProperties(addr netip.Addr) error { + if addr.IsUnspecified() { + return fmt.Errorf("IP address %s is a restricted unspecified IP", addr) + } + if addr.IsLoopback() { + return fmt.Errorf("IP address %s is a restricted loopback IP", addr) + } + if addr.IsLinkLocalUnicast() { + return fmt.Errorf("IP address %s is a restricted link-local IP", addr) + } + if addr.IsMulticast() { + return fmt.Errorf("IP address %s is a restricted multicast IP", addr) + } + if addr == azureMetadata || addr == awsIPv6IMDS { + return fmt.Errorf("IP address %s is a restricted cloud metadata IP", addr) + } + return nil +} + +// checkRestrictedIP rejects addresses that must not be used as router backends. +// +// Validation runs in two phases against a single rule set (restrictedProperties): +// +// 1. Check the address as parsed. This catches native forms directly — plain IPv4 +// (10.0.0.1), native IPv6 loopback (::1), link-local (fe80::1), and cloud-specific +// IPv6 metadata (fd00:ec2::254). Most addresses, including essentially all plain +// IPv4 endpoints, are fully validated here and return immediately. +// +// 2. If the address embeds an IPv4 address inside an IPv6 encoding, extract it and +// check again. IPv4-mapped literals (::ffff:x.x.x.x) are unmapped; IPv4-compatible +// literals (::x.x.x.x / ::hh:hh:hh:hh, the deprecated ::/96 form) have their low +// 32 bits decoded. The second pass catches restricted destinations that do not look +// restricted in IPv6 form, such as ::a9fe:a9fe (169.254.169.254 metadata) or +// ::127.0.0.1 (loopback). +// +// The native check must run before extraction: ::1 also matches the ::/96 layout but +// is IPv6 loopback, not embedded 0.0.0.1; checking after extraction would misclassify +// it and allow it through. +// +// Typical cost: one pass for plain IPv4 and legitimate IPv6; two passes only for +// IPv4-mapped or IPv4-compatible encodings. +func checkRestrictedIP(addr netip.Addr) error { + if err := restrictedProperties(addr); err != nil { + return err + } + if normalized, ok := normalizeEmbeddedIPv4(addr); ok { + return restrictedProperties(normalized) + } + return nil +} + // HandleRoute processes watch events on the Route resource. func (p *ExtendedValidator) HandleRoute(eventType watch.EventType, route *routev1.Route) error { log.V(10).Info("HandleRoute: ExtendedValidator") - // Check if previously seen route and its Spec is unchanged. - routeName := routeNameKey(route) - if err := routeapihelpers.ExtendedValidateRoute(route).ToAggregate(); err != nil { - log.Error(err, "skipping route due to invalid configuration", "route", routeName) - - p.recorder.RecordRouteRejection(route, "ExtendedValidationFailed", err.Error()) - p.plugin.HandleRoute(watch.Deleted, route) - return fmt.Errorf("invalid route configuration") + + if p.extendedRouteValidation { + routeName := routeNameKey(route) + if err := routeapihelpers.ExtendedValidateRoute(route).ToAggregate(); err != nil { + log.Error(err, "skipping route due to invalid configuration", "route", routeName) + + p.recorder.RecordRouteRejection(route, "ExtendedValidationFailed", err.Error()) + p.plugin.HandleRoute(watch.Deleted, route) + return fmt.Errorf("invalid route configuration") + } } return p.plugin.HandleRoute(eventType, route) diff --git a/pkg/router/controller/extended_validator_test.go b/pkg/router/controller/extended_validator_test.go new file mode 100644 index 000000000..c5b11c082 --- /dev/null +++ b/pkg/router/controller/extended_validator_test.go @@ -0,0 +1,676 @@ +package controller + +import ( + "fmt" + "net/netip" + "testing" + + routev1 "github.com/openshift/api/route/v1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + kapi "k8s.io/api/core/v1" + discoveryv1 "k8s.io/api/discovery/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/watch" + + "github.com/openshift/router/pkg/router/controller/endpointsubset" +) + +type fakeTestPlugin struct { + endpoints []*kapi.Endpoints +} + +func (p *fakeTestPlugin) HandleRoute(watch.EventType, *routev1.Route) error { return nil } +func (p *fakeTestPlugin) HandleEndpoints(eventType watch.EventType, endpoints *kapi.Endpoints) error { + p.endpoints = append(p.endpoints, endpoints) + return nil +} +func (p *fakeTestPlugin) HandleNamespaces(sets.String) error { return nil } +func (p *fakeTestPlugin) HandleNode(watch.EventType, *kapi.Node) error { return nil } +func (p *fakeTestPlugin) Commit() error { return nil } + +type fakeTestRecorder struct { + rejections []string +} + +func (r *fakeTestRecorder) RecordRouteRejection(route *routev1.Route, reason, message string) { + r.rejections = append(r.rejections, fmt.Sprintf("%s: %s", reason, message)) +} +func (r *fakeTestRecorder) RecordRouteUpdate(route *routev1.Route, reason, message string) {} +func (r *fakeTestRecorder) RecordRouteUnservableInFutureVersions(route *routev1.Route, reason, message string) { +} +func (r *fakeTestRecorder) RecordRouteUnservableInFutureVersionsClear(route *routev1.Route) {} + +func Test_checkRestrictedIP(t *testing.T) { + tests := []struct { + name string + ip string + expectError bool + }{ + { + name: "valid public IPv4", + ip: "1.2.3.4", + expectError: false, + }, + { + name: "valid private IPv4", + ip: "10.0.0.1", + expectError: false, + }, + { + name: "loopback IPv4", + ip: "127.0.0.1", + expectError: true, + }, + { + name: "loopback IPv4 inside 127.0.0.0/8", + ip: "127.127.127.222", + expectError: true, + }, + { + name: "loopback IPv6", + ip: "::1", + expectError: true, + }, + { + name: "link-local IPv4 metadata", + ip: "169.254.169.254", + expectError: true, + }, + { + name: "link-local IPv4 other", + ip: "169.254.1.1", + expectError: true, + }, + { + name: "Azure metadata IP", + ip: "168.63.129.16", + expectError: true, + }, + { + name: "valid IPv6", + ip: "2001:db8::1", + expectError: false, + }, + { + name: "link-local IPv6", + ip: "fe80::1", + expectError: true, + }, + { + name: "unspecified IPv4", + ip: "0.0.0.0", + expectError: true, + }, + { + name: "unspecified IPv6", + ip: "::", + expectError: true, + }, + { + name: "AWS IPv6 IMDS", + ip: "fd00:ec2::254", + expectError: true, + }, + { + name: "link-local multicast IPv6", + ip: "ff02::1", + expectError: true, + }, + { + name: "link-local multicast IPv4", + ip: "224.0.0.1", + expectError: true, + }, + { + name: "non-link-local multicast IPv4", + ip: "239.255.255.250", + expectError: true, + }, + { + name: "IPv4-mapped IPv6 loopback", + ip: "::ffff:127.0.0.1", + expectError: true, + }, + { + name: "IPv4-mapped IPv6 metadata link-local", + ip: "::ffff:169.254.169.254", + expectError: true, + }, + { + name: "IPv4-compatible IPv6 metadata (canonical)", + ip: "::a9fe:a9fe", + expectError: true, + }, + { + name: "IPv4-compatible IPv6 metadata (dotted)", + ip: "::169.254.169.254", + expectError: true, + }, + { + name: "IPv4-compatible IPv6 loopback", + ip: "::127.0.0.1", + expectError: true, + }, + { + name: "IPv4-compatible IPv6 loopback (hex)", + ip: "::7f00:1", + expectError: true, + }, + { + name: "loopback IPv4 from within 127/8", + ip: "127.172.127.172", + expectError: true, + }, + { + name: "IPv4-compatible IPv6 loopback from within 127/8 (dotted)", + ip: "::127.172.127.172", + expectError: true, + }, + { + name: "IPv4-compatible IPv6 loopback from within 127/8 (hex)", + ip: "::7fac:7fac", + expectError: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + addr, err := netip.ParseAddr(tc.ip) + require.NoErrorf(t, err, "failed to parse IP address %s", tc.ip) + err = checkRestrictedIP(addr) + if tc.expectError { + require.Error(t, err) + } + if !tc.expectError { + require.NoError(t, err) + } + }) + } +} + +func Test_validateEndpointAddress(t *testing.T) { + tests := []struct { + name string + address string + expectError bool + }{ + { + name: "valid private network IPv4 address", + address: "10.0.0.1", + expectError: false, + }, + { + name: "valid private network IPv4 address", + address: "23.206.60.92", + expectError: false, + }, + { + name: "valid IPv6", + address: "2001:db8::1", + expectError: false, + }, + { + name: "restricted loopback IP", + address: "127.0.0.1", + expectError: true, + }, + { + name: "restricted loopback IP from within the range", + address: "127.0.127.1", + expectError: true, + }, { + name: "restricted link-local IP", + address: "169.254.169.254", + expectError: true, + }, + { + name: "restricted Azure metadata IP", + address: "168.63.129.16", + expectError: true, + }, + { + name: "unspecified IPv4", + address: "0.0.0.0", + expectError: true, + }, + { + name: "non-IP address rejected", + address: "evil.example.com", + expectError: true, + }, + { + name: "empty string rejected", + address: "", + expectError: true, + }, + { + name: "IPv4-mapped IPv6 loopback", + address: "::ffff:127.0.0.1", + expectError: true, + }, + { + name: "IPv4-mapped IPv6 metadata link-local", + address: "::ffff:169.254.169.254", + expectError: true, + }, + { + name: "IPv4-compatible IPv6 metadata (canonical)", + address: "::a9fe:a9fe", + expectError: true, + }, + { + name: "IPv4-compatible IPv6 loopback", + address: "::127.0.0.1", + expectError: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := validateEndpointAddress(tc.address) + if tc.expectError { + require.Error(t, err) + } + if !tc.expectError { + require.NoError(t, err) + } + }) + } +} + +func TestExtendedValidator_HandleEndpoints(t *testing.T) { + tests := []struct { + name string + endpoints *kapi.Endpoints + expectedEndpoints *kapi.Endpoints + }{ + { + name: "valid IP passes through", + endpoints: &kapi.Endpoints{ + Subsets: []kapi.EndpointSubset{ + { + Addresses: []kapi.EndpointAddress{{IP: "1.2.3.4"}}, + }, + }, + }, + expectedEndpoints: &kapi.Endpoints{ + Subsets: []kapi.EndpointSubset{ + { + Addresses: []kapi.EndpointAddress{{IP: "1.2.3.4"}}, + }, + }, + }, + }, + { + name: "restricted link-local IP filtered out", + endpoints: &kapi.Endpoints{ + Subsets: []kapi.EndpointSubset{ + { + Addresses: []kapi.EndpointAddress{{IP: "169.254.169.254"}}, + }, + }, + }, + expectedEndpoints: &kapi.Endpoints{ + Subsets: []kapi.EndpointSubset{ + { + Addresses: []kapi.EndpointAddress{}, + }, + }, + }, + }, + { + name: "restricted loopback IP filtered out", + endpoints: &kapi.Endpoints{ + Subsets: []kapi.EndpointSubset{ + { + Addresses: []kapi.EndpointAddress{{IP: "127.0.0.1"}}, + }, + }, + }, + expectedEndpoints: &kapi.Endpoints{ + Subsets: []kapi.EndpointSubset{ + { + Addresses: []kapi.EndpointAddress{}, + }, + }, + }, + }, + { + name: "restricted IP in NotReadyAddresses filtered out", + endpoints: &kapi.Endpoints{ + Subsets: []kapi.EndpointSubset{ + { + NotReadyAddresses: []kapi.EndpointAddress{{IP: "169.254.169.254"}}, + }, + }, + }, + expectedEndpoints: &kapi.Endpoints{ + Subsets: []kapi.EndpointSubset{ + { + NotReadyAddresses: []kapi.EndpointAddress{}, + }, + }, + }, + }, + { + name: "valid IP in NotReadyAddresses passes through", + endpoints: &kapi.Endpoints{ + Subsets: []kapi.EndpointSubset{ + { + NotReadyAddresses: []kapi.EndpointAddress{{IP: "10.0.0.5"}}, + }, + }, + }, + expectedEndpoints: &kapi.Endpoints{ + Subsets: []kapi.EndpointSubset{ + { + NotReadyAddresses: []kapi.EndpointAddress{{IP: "10.0.0.5"}}, + }, + }, + }, + }, + { + name: "mixed valid and restricted keeps only valid", + endpoints: &kapi.Endpoints{ + Subsets: []kapi.EndpointSubset{ + { + Addresses: []kapi.EndpointAddress{{IP: "10.0.0.1"}}, + }, + { + Addresses: []kapi.EndpointAddress{{IP: "127.0.0.1"}}, + }, + }, + }, + expectedEndpoints: &kapi.Endpoints{ + Subsets: []kapi.EndpointSubset{ + { + Addresses: []kapi.EndpointAddress{{IP: "10.0.0.1"}}, + }, + { + Addresses: []kapi.EndpointAddress{}, + }, + }, + }, + }, + { + name: "Azure metadata IP filtered out", + endpoints: &kapi.Endpoints{ + Subsets: []kapi.EndpointSubset{ + { + Addresses: []kapi.EndpointAddress{{IP: "168.63.129.16"}}, + }, + }, + }, + expectedEndpoints: &kapi.Endpoints{ + Subsets: []kapi.EndpointSubset{ + { + Addresses: []kapi.EndpointAddress{}, + }, + }, + }, + }, + { + name: "mixed valid and restricted in same subset", + endpoints: &kapi.Endpoints{ + Subsets: []kapi.EndpointSubset{ + { + Addresses: []kapi.EndpointAddress{ + {IP: "10.0.0.1"}, + {IP: "127.0.0.1"}, + {IP: "10.0.0.2"}, + }, + }, + }, + }, + expectedEndpoints: &kapi.Endpoints{ + Subsets: []kapi.EndpointSubset{ + { + Addresses: []kapi.EndpointAddress{ + {IP: "10.0.0.1"}, + {IP: "10.0.0.2"}, + }, + }, + }, + }, + }, + { + name: "non-IP hostname filtered out", + endpoints: &kapi.Endpoints{ + Subsets: []kapi.EndpointSubset{{ + Addresses: []kapi.EndpointAddress{ + {IP: "10.0.0.1"}, + {IP: "metadata.google.internal"}, + }, + }}, + }, + expectedEndpoints: &kapi.Endpoints{ + Subsets: []kapi.EndpointSubset{{ + Addresses: []kapi.EndpointAddress{{IP: "10.0.0.1"}}, + }}, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + inner := &fakeTestPlugin{} + recorder := &fakeTestRecorder{} + validator := NewExtendedValidator(inner, recorder, true) + + err := validator.HandleEndpoints(watch.Added, tc.endpoints) + require.NoError(t, err) + require.Len(t, inner.endpoints, 1) + + assert.Equal(t, tc.expectedEndpoints, inner.endpoints[0], "expected endpoints should match inner endpoints") + }) + } +} + +func TestExtendedValidator_EndpointValidationWithRouteValidationDisabled(t *testing.T) { + inner := &fakeTestPlugin{} + recorder := &fakeTestRecorder{} + validator := NewExtendedValidator(inner, recorder, false) + + endpoints := &kapi.Endpoints{ + Subsets: []kapi.EndpointSubset{{ + Addresses: []kapi.EndpointAddress{ + {IP: "10.0.0.1"}, + {IP: "127.0.0.1"}, + }, + }}, + } + + err := validator.HandleEndpoints(watch.Added, endpoints) + require.NoError(t, err) + require.Len(t, inner.endpoints, 1) + + expected := &kapi.Endpoints{ + Subsets: []kapi.EndpointSubset{{ + Addresses: []kapi.EndpointAddress{{IP: "10.0.0.1"}}, + }}, + } + assert.Equal(t, expected, inner.endpoints[0], "loopback should be filtered even with route validation disabled") +} + +func TestExtendedValidator_HandleEndpoints_DoesNotMutateInput(t *testing.T) { + inner := &fakeTestPlugin{} + recorder := &fakeTestRecorder{} + validator := NewExtendedValidator(inner, recorder, true) + + addresses := []kapi.EndpointAddress{ + {IP: "10.0.0.1"}, + {IP: "127.0.0.1"}, + {IP: "10.0.0.2"}, + } + notReadyAddresses := []kapi.EndpointAddress{ + {IP: "169.254.169.254"}, + } + endpoints := &kapi.Endpoints{ + Subsets: []kapi.EndpointSubset{ + { + Addresses: addresses, + NotReadyAddresses: notReadyAddresses, + }, + }, + } + + originalAddresses := append([]kapi.EndpointAddress{}, addresses...) + originalNotReadyAddresses := append([]kapi.EndpointAddress{}, notReadyAddresses...) + + err := validator.HandleEndpoints(watch.Added, endpoints) + require.NoError(t, err) + require.Len(t, inner.endpoints, 1) + + // The caller's Endpoints object, and the backing arrays of its address + // slices, must be left untouched: HandleEndpoints may be called with an + // object shared with (and owned by) an informer's cache. + assert.Equal(t, originalAddresses, endpoints.Subsets[0].Addresses, "input Addresses must not be mutated") + assert.Equal(t, originalNotReadyAddresses, endpoints.Subsets[0].NotReadyAddresses, "input NotReadyAddresses must not be mutated") + assert.Len(t, endpoints.Subsets[0].Addresses, 3, "input Addresses length must be unchanged") + + // The plugin further down the chain must still observe the filtered result. + assert.Equal(t, []kapi.EndpointAddress{{IP: "10.0.0.1"}, {IP: "10.0.0.2"}}, inner.endpoints[0].Subsets[0].Addresses) + assert.Equal(t, []kapi.EndpointAddress{}, inner.endpoints[0].Subsets[0].NotReadyAddresses) +} + +func TestExtendedValidator_HandleEndpoints_DeletedSkipsFiltering(t *testing.T) { + inner := &fakeTestPlugin{} + recorder := &fakeTestRecorder{} + validator := NewExtendedValidator(inner, recorder, true) + + endpoints := &kapi.Endpoints{ + Subsets: []kapi.EndpointSubset{{ + Addresses: []kapi.EndpointAddress{{IP: "127.0.0.1"}}, + }}, + } + + err := validator.HandleEndpoints(watch.Deleted, endpoints) + require.NoError(t, err) + require.Len(t, inner.endpoints, 1) + assert.Same(t, endpoints, inner.endpoints[0]) + assert.Equal(t, []kapi.EndpointAddress{{IP: "127.0.0.1"}}, inner.endpoints[0].Subsets[0].Addresses) +} + +func TestExtendedValidator_HandleEndpointSliceConversion(t *testing.T) { + serviceLabels := map[string]string{ + discoveryv1.LabelServiceName: "service-a", + } + endpointsFromSlices := func(namespace, name string, slices []discoveryv1.EndpointSlice) *kapi.Endpoints { + return &kapi.Endpoints{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Name: name, + }, + Subsets: endpointsubset.ConvertEndpointSlice( + slices, + endpointsubset.DefaultEndpointAddressOrderByFuncs(), + endpointsubset.DefaultEndpointPortOrderByFuncs(), + ), + } + } + + tests := []struct { + name string + slices []discoveryv1.EndpointSlice + expectedEndpoints *kapi.Endpoints + }{ + { + name: "FQDN EndpointSlice produces no backend addresses", + slices: []discoveryv1.EndpointSlice{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "slice-fqdn", + Namespace: "namespace-a", + Labels: serviceLabels, + }, + AddressType: discoveryv1.AddressTypeFQDN, + Endpoints: []discoveryv1.Endpoint{{ + Addresses: []string{"metadata.google.internal"}, + }}, + Ports: []discoveryv1.EndpointPort{{ + Port: int32Ptr(8080), + }}, + }}, + expectedEndpoints: &kapi.Endpoints{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace-a", + Name: "service-a", + }, + }, + }, + { + name: "IPv4 EndpointSlice with hostname keeps only valid IP after validation", + slices: []discoveryv1.EndpointSlice{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "slice-ipv4", + Namespace: "namespace-a", + Labels: serviceLabels, + }, + AddressType: discoveryv1.AddressTypeIPv4, + Endpoints: []discoveryv1.Endpoint{{ + Addresses: []string{"10.0.0.1", "metadata.google.internal"}, + }}, + Ports: []discoveryv1.EndpointPort{{ + Port: int32Ptr(8080), + }}, + }}, + expectedEndpoints: &kapi.Endpoints{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace-a", + Name: "service-a", + }, + Subsets: []kapi.EndpointSubset{{ + Addresses: []kapi.EndpointAddress{{IP: "10.0.0.1"}}, + Ports: []kapi.EndpointPort{{Port: 8080}}, + }}, + }, + }, + { + name: "IPv6 EndpointSlice with canonical metadata address filtered", + slices: []discoveryv1.EndpointSlice{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "slice-ipv6-metadata", + Namespace: "namespace-a", + Labels: serviceLabels, + }, + AddressType: discoveryv1.AddressTypeIPv6, + Endpoints: []discoveryv1.Endpoint{{ + Addresses: []string{"::a9fe:a9fe"}, + }}, + Ports: []discoveryv1.EndpointPort{{ + Port: int32Ptr(8080), + }}, + }}, + expectedEndpoints: &kapi.Endpoints{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace-a", + Name: "service-a", + }, + Subsets: []kapi.EndpointSubset{{ + Addresses: []kapi.EndpointAddress{}, + Ports: []kapi.EndpointPort{{Port: 8080}}, + }}, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + inner := &fakeTestPlugin{} + recorder := &fakeTestRecorder{} + validator := NewExtendedValidator(inner, recorder, true) + + endpoints := endpointsFromSlices("namespace-a", "service-a", tc.slices) + err := validator.HandleEndpoints(watch.Added, endpoints) + require.NoError(t, err) + require.Len(t, inner.endpoints, 1) + + assert.Equal(t, tc.expectedEndpoints, inner.endpoints[0]) + }) + } +} + +func int32Ptr(i int32) *int32 { + return &i +} diff --git a/pkg/router/controller/factory/factory_endpointslices_test.go b/pkg/router/controller/factory/factory_endpointslices_test.go index 2f57695f9..444e2c7e2 100644 --- a/pkg/router/controller/factory/factory_endpointslices_test.go +++ b/pkg/router/controller/factory/factory_endpointslices_test.go @@ -236,6 +236,7 @@ func TestEndpointSlicesAdd(t *testing.T) { discoveryv1.LabelServiceName: "service-b", }, }, + AddressType: discoveryv1.AddressTypeIPv4, }, expectedServiceName: "service-b", expectedEventType: watch.Modified, diff --git a/pkg/router/router_test.go b/pkg/router/router_test.go index d0f27bd8b..6a7b0c5a7 100644 --- a/pkg/router/router_test.go +++ b/pkg/router/router_test.go @@ -1228,6 +1228,7 @@ func (e mustCreateEndpointSlice) Apply(h *harness) error { }, UID: h.nextUID(), }, + AddressType: discoveryv1.AddressTypeIPv4, Endpoints: []discoveryv1.Endpoint{{ Addresses: addresses, }}, diff --git a/pkg/router/template/plugin_test.go b/pkg/router/template/plugin_test.go index 2e104024d..d59c5da9b 100644 --- a/pkg/router/template/plugin_test.go +++ b/pkg/router/template/plugin_test.go @@ -773,7 +773,7 @@ func (p *fakePlugin) Commit() error { func TestHandleRouteExtendedValidation(t *testing.T) { rejections := &fakeStatusRecorder{} fake := &fakePlugin{} - plugin := controller.NewExtendedValidator(fake, rejections) + plugin := controller.NewExtendedValidator(fake, rejections, true) original := metav1.Time{Time: time.Now()}