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()}