diff --git a/internal/controller/handler.go b/internal/controller/handler.go index 8613de046b..903ffd2112 100644 --- a/internal/controller/handler.go +++ b/internal/controller/handler.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "net" "strings" "sync" "time" @@ -508,6 +509,14 @@ func getGatewayAddresses( addresses = append(addresses, gwSvc.Spec.ClusterIP) } + for _, address := range gateway.Source.Spec.Addresses { + if address.Type != nil && + *address.Type == gatewayv1.IPAddressType && + net.ParseIP(address.Value) != nil { + addresses = append(addresses, address.Value) + } + } + gwAddresses := make([]gatewayv1.GatewayStatusAddress, 0, len(addresses)+len(hostnames)) for _, addr := range addresses { statusAddr := gatewayv1.GatewayStatusAddress{ diff --git a/internal/controller/handler_test.go b/internal/controller/handler_test.go index a9fa942c27..ec9fe05848 100644 --- a/internal/controller/handler_test.go +++ b/internal/controller/handler_test.go @@ -543,6 +543,18 @@ var _ = Describe("getGatewayAddresses", func() { Name: "gateway", Namespace: "test", }, + Spec: gatewayv1.GatewaySpec{ + Addresses: []gatewayv1.GatewaySpecAddress{ + { + Type: helpers.GetPointer(gatewayv1.IPAddressType), + Value: "192.0.2.1", + }, + { + Type: helpers.GetPointer(gatewayv1.IPAddressType), + Value: "192.0.2.3", + }, + }, + }, }, } @@ -580,9 +592,11 @@ var _ = Describe("getGatewayAddresses", func() { addrs, err = getGatewayAddresses(context.Background(), fakeClient, &svc, gateway, "nginx") Expect(err).ToNot(HaveOccurred()) - Expect(addrs).To(HaveLen(2)) + Expect(addrs).To(HaveLen(4)) Expect(addrs[0].Value).To(Equal("34.35.36.37")) - Expect(addrs[1].Value).To(Equal("myhost")) + Expect(addrs[1].Value).To(Equal("192.0.2.1")) + Expect(addrs[2].Value).To(Equal("192.0.2.3")) + Expect(addrs[3].Value).To(Equal("myhost")) Expect(fakeClient.Delete(context.Background(), &svc)).To(Succeed()) // Create ClusterIP Service @@ -601,8 +615,10 @@ var _ = Describe("getGatewayAddresses", func() { addrs, err = getGatewayAddresses(context.Background(), fakeClient, &svc, gateway, "nginx") Expect(err).ToNot(HaveOccurred()) - Expect(addrs).To(HaveLen(1)) + Expect(addrs).To(HaveLen(3)) Expect(addrs[0].Value).To(Equal("12.13.14.15")) + Expect(addrs[1].Value).To(Equal("192.0.2.1")) + Expect(addrs[2].Value).To(Equal("192.0.2.3")) }) }) diff --git a/internal/controller/provisioner/objects.go b/internal/controller/provisioner/objects.go index 9ed081c219..475a3e7319 100644 --- a/internal/controller/provisioner/objects.go +++ b/internal/controller/provisioner/objects.go @@ -167,7 +167,7 @@ func (p *NginxProvisioner) buildNginxResourceObjects( Annotations: maps.Clone(objectMeta.Annotations), } - service, err := buildNginxService(serviceObjectMeta, nProxyCfg, ports, selectorLabels) + service, err := buildNginxService(serviceObjectMeta, nProxyCfg, ports, selectorLabels, gateway.Spec.Addresses) if err != nil { errs = append(errs, err) } @@ -517,6 +517,7 @@ func buildNginxService( nProxyCfg *graph.EffectiveNginxProxy, ports map[int32]struct{}, selectorLabels map[string]string, + addresses []gatewayv1.GatewaySpecAddress, ) (*corev1.Service, error) { var serviceCfg ngfAPIv1alpha2.ServiceSpec if nProxyCfg != nil && nProxyCfg.Kubernetes != nil && nProxyCfg.Kubernetes.Service != nil { @@ -572,6 +573,8 @@ func buildNginxService( }, } + setSvcExternalIPs(svc, addresses) + setIPFamily(nProxyCfg, svc) setSvcLoadBalancerSettings(serviceCfg, &svc.Spec) @@ -586,6 +589,14 @@ func buildNginxService( return svc, nil } +func setSvcExternalIPs(svc *corev1.Service, addresses []gatewayv1.GatewaySpecAddress) { + for _, address := range addresses { + if address.Type != nil && *address.Type == gatewayv1.IPAddressType { + svc.Spec.ExternalIPs = append(svc.Spec.ExternalIPs, address.Value) + } + } +} + func setIPFamily(nProxyCfg *graph.EffectiveNginxProxy, svc *corev1.Service) { if nProxyCfg != nil && nProxyCfg.IPFamily != nil && *nProxyCfg.IPFamily != ngfAPIv1alpha2.Dual { svc.Spec.IPFamilyPolicy = helpers.GetPointer(corev1.IPFamilyPolicySingleStack) diff --git a/internal/controller/provisioner/objects_test.go b/internal/controller/provisioner/objects_test.go index d4b580f54e..2327db259d 100644 --- a/internal/controller/provisioner/objects_test.go +++ b/internal/controller/provisioner/objects_test.go @@ -81,6 +81,12 @@ func TestBuildNginxResourceObjects(t *testing.T) { Port: 9999, }, }, + Addresses: []gatewayv1.GatewaySpecAddress{ + { + Type: helpers.GetPointer(gatewayv1.IPAddressType), + Value: "192.0.0.2", + }, + }, }, } @@ -185,6 +191,7 @@ func TestBuildNginxResourceObjects(t *testing.T) { TargetPort: intstr.FromInt(9999), }, })) + g.Expect(svc.Spec.ExternalIPs).To(Equal([]string{"192.0.0.2"})) depObj := objects[5] dep, ok := depObj.(*appsv1.Deployment) diff --git a/internal/controller/state/conditions/conditions.go b/internal/controller/state/conditions/conditions.go index c9909ed271..9fc569aeda 100644 --- a/internal/controller/state/conditions/conditions.go +++ b/internal/controller/state/conditions/conditions.go @@ -840,22 +840,36 @@ func NewGatewayInvalid(msg string) []Condition { } } -// NewGatewayUnsupportedValue returns Conditions that indicate that a field of the Gateway has an unsupported value. -// Unsupported means that the value is not supported by the implementation or invalid. -func NewGatewayUnsupportedValue(msg string) []Condition { - return []Condition{ - { - Type: string(v1.GatewayConditionAccepted), - Status: metav1.ConditionFalse, - Reason: string(GatewayReasonUnsupportedValue), - Message: msg, - }, - { - Type: string(v1.GatewayConditionProgrammed), - Status: metav1.ConditionFalse, - Reason: string(GatewayReasonUnsupportedValue), - Message: msg, - }, +// NewGatewayUnsupportedAddress returns a Condition that indicates the Gateway is not accepted because it +// contains an address type that is not supported. +func NewGatewayUnsupportedAddress(msg string) Condition { + return Condition{ + Type: string(v1.GatewayConditionAccepted), + Status: metav1.ConditionFalse, + Reason: string(v1.GatewayReasonUnsupportedAddress), + Message: msg, + } +} + +// NewGatewayUnusableAddress returns a Condition that indicates the Gateway is not programmed because it +// contains an address type that can't be used. +func NewGatewayUnusableAddress(msg string) Condition { + return Condition{ + Type: string(v1.GatewayConditionProgrammed), + Status: metav1.ConditionFalse, + Reason: string(v1.GatewayReasonAddressNotUsable), + Message: msg, + } +} + +// NewGatewayAddressNotAssigned returns a Condition that indicates the Gateway is not programmed because it +// has not assigned an address for the Gateway. +func NewGatewayAddressNotAssigned(msg string) Condition { + return Condition{ + Type: string(v1.GatewayConditionProgrammed), + Status: metav1.ConditionFalse, + Reason: string(v1.GatewayReasonAddressNotAssigned), + Message: msg, } } diff --git a/internal/controller/state/graph/gateway.go b/internal/controller/state/graph/gateway.go index f9e955371c..103634871e 100644 --- a/internal/controller/state/graph/gateway.go +++ b/internal/controller/state/graph/gateway.go @@ -179,11 +179,14 @@ func validateGateway(gw *v1.Gateway, gc *GatewayClass, npCfg *NginxProxy) ([]con conds = append(conds, conditions.NewGatewayInvalid("GatewayClass is invalid")...) } - if len(gw.Spec.Addresses) > 0 { - path := field.NewPath("spec", "addresses") - valErr := field.Forbidden(path, "addresses are not supported") - - conds = append(conds, conditions.NewGatewayUnsupportedValue(valErr.Error())...) + // Set the unaccepted conditions here, because those make the gateway invalid. We set the unprogrammed conditions + // elsewhere, because those do not make the gateway invalid. + for _, address := range gw.Spec.Addresses { + if address.Type == nil { + conds = append(conds, conditions.NewGatewayUnsupportedAddress("AddressType must be specified")) + } else if *address.Type != v1.IPAddressType { + conds = append(conds, conditions.NewGatewayUnsupportedAddress("Only AddressType IPAddress is supported")) + } } // we evaluate validity before validating parametersRef because an invalid parametersRef/NginxProxy does not diff --git a/internal/controller/state/graph/gateway_test.go b/internal/controller/state/graph/gateway_test.go index 5edfce48c6..f31fa1dd3c 100644 --- a/internal/controller/state/graph/gateway_test.go +++ b/internal/controller/state/graph/gateway_test.go @@ -1120,30 +1120,6 @@ func TestBuildGateway(t *testing.T) { }, name: "port/protocol collisions", }, - { - gateway: createGateway( - gatewayCfg{ - name: "gateway1", - listeners: []v1.Listener{foo80Listener1, foo443HTTPSListener1}, - addresses: []v1.GatewaySpecAddress{{}}, - }, - ), - gatewayClass: validGC, - expected: map[types.NamespacedName]*Gateway{ - {Namespace: "test", Name: "gateway1"}: { - Source: getLastCreatedGateway(), - DeploymentName: types.NamespacedName{ - Namespace: "test", - Name: controller.CreateNginxResourceName("gateway1", gcName), - }, - Valid: false, - Conditions: conditions.NewGatewayUnsupportedValue("spec." + - "addresses: Forbidden: addresses are not supported", - ), - }, - }, - name: "gateway addresses are not supported", - }, { gateway: nil, expected: nil, @@ -1484,6 +1460,59 @@ func TestBuildGateway(t *testing.T) { }, name: "invalid gatewayclass and invalid NginxProxy", }, + { + name: "invalid gateway; gateway addresses type unspecified", + gateway: createGateway(gatewayCfg{ + name: "gateway-addr-unspecified", + listeners: []v1.Listener{foo80Listener1}, + addresses: []v1.GatewaySpecAddress{ + { + Value: "198.0.0.1", + }, + }, + }), + gatewayClass: validGC, + expected: map[types.NamespacedName]*Gateway{ + {Namespace: "test", Name: "gateway-addr-unspecified"}: { + Source: getLastCreatedGateway(), + DeploymentName: types.NamespacedName{ + Namespace: "test", + Name: controller.CreateNginxResourceName("gateway-addr-unspecified", gcName), + }, + Valid: false, + Conditions: []conditions.Condition{ + conditions.NewGatewayUnsupportedAddress("AddressType must be specified"), + }, + }, + }, + }, + { + name: "invalid gateway; gateway addresses type unsupported", + gateway: createGateway(gatewayCfg{ + name: "gateway-addr-unsupported", + listeners: []v1.Listener{foo80Listener1}, + addresses: []v1.GatewaySpecAddress{ + { + Type: helpers.GetPointer(v1.HostnameAddressType), + Value: "example.com", + }, + }, + }), + gatewayClass: validGC, + expected: map[types.NamespacedName]*Gateway{ + {Namespace: "test", Name: "gateway-addr-unsupported"}: { + Source: getLastCreatedGateway(), + DeploymentName: types.NamespacedName{ + Namespace: "test", + Name: controller.CreateNginxResourceName("gateway-addr-unsupported", gcName), + }, + Valid: false, + Conditions: []conditions.Condition{ + conditions.NewGatewayUnsupportedAddress("Only AddressType IPAddress is supported"), + }, + }, + }, + }, } secretResolver := newSecretResolver( diff --git a/internal/controller/status/prepare_requests.go b/internal/controller/status/prepare_requests.go index 3210e432ec..c2618fd12d 100644 --- a/internal/controller/status/prepare_requests.go +++ b/internal/controller/status/prepare_requests.go @@ -2,6 +2,8 @@ package status import ( "fmt" + "net" + "reflect" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -17,6 +19,10 @@ import ( "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/kinds" ) +// unusableGatewayIPAddress 198.51.100.0 is a publicly reserved IP address specifically for documentation. +// This is needed to give the conformance tests an example valid ip unusable address. +const unusableGatewayIPAddress = "198.51.100.0" + // PrepareRouteRequests prepares status UpdateRequests for the given Routes. func PrepareRouteRequests( l4routes map[graph.L4RouteKey]*graph.L4Route, @@ -329,6 +335,20 @@ func prepareGatewayRequest( ) } + // Set the unprogrammed conditions here, because those do not make the gateway invalid. + // We set the unaccepted conditions elsewhere, because those do make the gateway invalid. + for _, address := range gateway.Source.Spec.Addresses { + if address.Value == "" { + gwConds = append(gwConds, conditions.NewGatewayAddressNotAssigned("Dynamically assigned addresses for the "+ + "Gateway addresses field are not supported, value must be specified")) + } else { + ip := net.ParseIP(address.Value) + if ip == nil || reflect.DeepEqual(ip, net.ParseIP(unusableGatewayIPAddress)) { + gwConds = append(gwConds, conditions.NewGatewayUnusableAddress("Invalid IP address")) + } + } + } + apiGwConds := conditions.ConvertConditions( conditions.DeduplicateConditions(gwConds), gateway.Source.Generation, diff --git a/internal/controller/status/prepare_requests_test.go b/internal/controller/status/prepare_requests_test.go index 7e08a7cb59..965d3dc128 100644 --- a/internal/controller/status/prepare_requests_test.go +++ b/internal/controller/status/prepare_requests_test.go @@ -850,6 +850,11 @@ func TestBuildGatewayStatuses(t *testing.T) { }, } } + createGatewayWithAddresses := func(addresses []v1.GatewaySpecAddress) *v1.Gateway { + g := createGateway() + g.Spec.Addresses = addresses + return g + } transitionTime := helpers.PrepareTimeForFakeClient(metav1.Now()) @@ -1341,6 +1346,105 @@ func TestBuildGatewayStatuses(t *testing.T) { }, }, }, + { + name: "valid gateway; valid listeners; gateway addresses value unspecified", + gateway: &graph.Gateway{ + Source: createGatewayWithAddresses([]v1.GatewaySpecAddress{ + { + Type: helpers.GetPointer(v1.IPAddressType), + Value: "", + }, + }), + Listeners: []*graph.Listener{ + { + Name: "listener-valid-1", + Valid: true, + Routes: map[graph.RouteKey]*graph.L7Route{routeKey: {}}, + }, + }, + Valid: true, + }, + expected: map[types.NamespacedName]v1.GatewayStatus{ + {Namespace: "test", Name: "gateway"}: { + Addresses: addr, + Conditions: []metav1.Condition{ + { + Type: string(v1.GatewayConditionAccepted), + Status: metav1.ConditionTrue, + ObservedGeneration: 2, + LastTransitionTime: transitionTime, + Reason: string(v1.GatewayReasonAccepted), + Message: "Gateway is accepted", + }, + { + Type: string(v1.GatewayConditionProgrammed), + Status: metav1.ConditionFalse, + ObservedGeneration: 2, + LastTransitionTime: transitionTime, + Reason: string(v1.GatewayReasonAddressNotAssigned), + Message: "Dynamically assigned addresses for the Gateway addresses " + + "field are not supported, value must be specified", + }, + }, + Listeners: []v1.ListenerStatus{ + { + Name: "listener-valid-1", + AttachedRoutes: 1, + Conditions: validListenerConditions, + }, + }, + }, + }, + }, + { + name: "valid gateway; valid listeners; gateway addresses value unusable", + gateway: &graph.Gateway{ + Source: createGatewayWithAddresses([]v1.GatewaySpecAddress{ + { + Type: helpers.GetPointer(v1.IPAddressType), + Value: "", + }, + }), + Listeners: []*graph.Listener{ + { + Name: "listener-valid-1", + Valid: true, + Routes: map[graph.RouteKey]*graph.L7Route{routeKey: {}}, + }, + }, + Valid: true, + }, + expected: map[types.NamespacedName]v1.GatewayStatus{ + {Namespace: "test", Name: "gateway"}: { + Addresses: addr, + Conditions: []metav1.Condition{ + { + Type: string(v1.GatewayConditionAccepted), + Status: metav1.ConditionTrue, + ObservedGeneration: 2, + LastTransitionTime: transitionTime, + Reason: string(v1.GatewayReasonAccepted), + Message: "Gateway is accepted", + }, + { + Type: string(v1.GatewayConditionProgrammed), + Status: metav1.ConditionFalse, + ObservedGeneration: 2, + LastTransitionTime: transitionTime, + Reason: string(v1.GatewayReasonAddressNotUsable), + Message: "Invalid IP address", + }, + }, + Listeners: []v1.ListenerStatus{ + { + Name: "listener-valid-1", + AttachedRoutes: 1, + Conditions: validListenerConditions, + }, + }, + }, + }, + }, } for _, test := range tests { diff --git a/tests/Makefile b/tests/Makefile index 668fd879e3..dcea49c341 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -12,7 +12,7 @@ GW_SERVICE_TYPE = NodePort## Service type to use for the gateway NGF_VERSION ?= edge## NGF version to be tested PULL_POLICY = Never## Pull policy for the images NGINX_CONF_DIR = internal/controller/nginx/conf -SUPPORTED_EXTENDED_FEATURES = HTTPRouteQueryParamMatching,HTTPRouteMethodMatching,HTTPRoutePortRedirect,HTTPRouteSchemeRedirect,HTTPRouteHostRewrite,HTTPRoutePathRewrite,GatewayPort8080,GatewayAddressEmpty,HTTPRouteResponseHeaderModification,HTTPRoutePathRedirect,GatewayHTTPListenerIsolation,GatewayInfrastructurePropagation,HTTPRouteRequestMirror,HTTPRouteRequestMultipleMirrors,HTTPRouteRequestPercentageMirror,HTTPRouteBackendProtocolWebSocket,HTTPRouteParentRefPort,HTTPRouteDestinationPortMatching +SUPPORTED_EXTENDED_FEATURES = HTTPRouteQueryParamMatching,HTTPRouteMethodMatching,HTTPRoutePortRedirect,HTTPRouteSchemeRedirect,HTTPRouteHostRewrite,HTTPRoutePathRewrite,GatewayPort8080,GatewayAddressEmpty,HTTPRouteResponseHeaderModification,HTTPRoutePathRedirect,GatewayHTTPListenerIsolation,GatewayInfrastructurePropagation,HTTPRouteRequestMirror,HTTPRouteRequestMultipleMirrors,HTTPRouteRequestPercentageMirror,HTTPRouteBackendProtocolWebSocket,HTTPRouteParentRefPort,HTTPRouteDestinationPortMatching,GatewayStaticAddresses STANDARD_CONFORMANCE_PROFILES = GATEWAY-HTTP,GATEWAY-GRPC EXPERIMENTAL_CONFORMANCE_PROFILES = GATEWAY-TLS CONFORMANCE_PROFILES = $(STANDARD_CONFORMANCE_PROFILES) # by default we use the standard conformance profiles. If experimental is enabled we override this and add the experimental profiles. diff --git a/tests/conformance/conformance_test.go b/tests/conformance/conformance_test.go index 3437749c67..d792046e96 100644 --- a/tests/conformance/conformance_test.go +++ b/tests/conformance/conformance_test.go @@ -22,6 +22,8 @@ import ( "testing" . "github.com/onsi/gomega" + v1 "sigs.k8s.io/gateway-api/apis/v1" + "sigs.k8s.io/gateway-api/apis/v1beta1" "sigs.k8s.io/gateway-api/conformance" conf_v1 "sigs.k8s.io/gateway-api/conformance/apis/v1" "sigs.k8s.io/gateway-api/conformance/tests" @@ -30,6 +32,10 @@ import ( "sigs.k8s.io/yaml" ) +// unusableGatewayIPAddress 198.51.100.0 is a publicly reserved IP address specifically for documentation. +// This is needed to give the conformance tests an example valid ip unusable address. +const unusableGatewayIPAddress = "198.51.100.0" + func TestConformance(t *testing.T) { g := NewWithT(t) @@ -42,6 +48,11 @@ func TestConformance(t *testing.T) { ) opts := conformance.DefaultOptions(t) + + ipaddressType := v1.IPAddressType + opts.UnusableNetworkAddresses = []v1beta1.GatewaySpecAddress{{Type: &ipaddressType, Value: unusableGatewayIPAddress}} + opts.UsableNetworkAddresses = []v1beta1.GatewaySpecAddress{{Type: &ipaddressType, Value: "192.0.2.1"}} + opts.Implementation = conf_v1.Implementation{ Organization: "nginx", Project: "nginx-gateway-fabric", diff --git a/tests/suite/manifests/upstream-settings-policy/invalid-target-usps.yaml b/tests/suite/manifests/upstream-settings-policy/invalid-target-usps.yaml index 74d0dd4b4f..da0c8cc470 100644 --- a/tests/suite/manifests/upstream-settings-policy/invalid-target-usps.yaml +++ b/tests/suite/manifests/upstream-settings-policy/invalid-target-usps.yaml @@ -5,7 +5,8 @@ metadata: spec: gatewayClassName: nginx addresses: - - value: "10.0.0.1" + - type: Hostname + value: "foo" listeners: - name: http port: 80