From e2a433f7ed3e9b42c7fd1d7aa653c91be13a5503 Mon Sep 17 00:00:00 2001 From: Paul Abel Date: Mon, 3 Feb 2025 14:37:31 +0000 Subject: [PATCH 1/3] create function to generate errorPageDetails struct --- internal/configs/virtualserver.go | 20 ++++---- internal/configs/virtualserver_test.go | 69 ++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 10 deletions(-) diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 8535467b87..dce95a032d 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -538,11 +538,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( // generates config for VirtualServer routes for _, r := range vsEx.VirtualServer.Spec.Routes { - errorPages := errorPageDetails{ - pages: r.ErrorPages, - index: len(errorPageLocations), - owner: vsEx.VirtualServer, - } + errorPages := generateErrorPageDetails(r.ErrorPages, errorPageLocations, vsEx.VirtualServer) errorPageLocations = append(errorPageLocations, generateErrorPageLocations(errorPages.index, errorPages.pages)...) // ignore routes that reference VirtualServerRoute @@ -674,11 +670,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( isVSR := true upstreamNamer := NewUpstreamNamerForVirtualServerRoute(vsEx.VirtualServer, vsr) for _, r := range vsr.Spec.Subroutes { - errorPages := errorPageDetails{ - pages: r.ErrorPages, - index: len(errorPageLocations), - owner: vsr, - } + errorPages := generateErrorPageDetails(r.ErrorPages, errorPageLocations, vsr) errorPageLocations = append(errorPageLocations, generateErrorPageLocations(errorPages.index, errorPages.pages)...) vsrNamespaceName := fmt.Sprintf("%v/%v", vsr.Namespace, vsr.Name) // use the VirtualServer error pages if the route does not define any @@ -2976,6 +2968,14 @@ func generateErrorPages(errPageIndex int, errorPages []conf_v1.ErrorPage) []vers return ePages } +func generateErrorPageDetails(errorPages []conf_v1.ErrorPage, errorPageLocations []version2.ErrorPageLocation, owner runtime.Object) errorPageDetails { + return errorPageDetails{ + pages: errorPages, + index: len(errorPageLocations), + owner: owner, + } +} + func generateErrorPageLocations(errPageIndex int, errorPages []conf_v1.ErrorPage) []version2.ErrorPageLocation { var errorPageLocations []version2.ErrorPageLocation for i, e := range errorPages { diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index 93af610053..75975f922f 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -14201,6 +14201,75 @@ func TestGenerateErrorPageLocations(t *testing.T) { } } +func TestGenerateErrorPageDetails(t *testing.T) { + t.Parallel() + tests := []struct { + errorPages []conf_v1.ErrorPage + errorLocations []version2.ErrorPageLocation + owner runtime.Object + expected errorPageDetails + }{ + {}, // empty + { + errorPages: []conf_v1.ErrorPage{ + { + Codes: []int{404, 405, 500, 502}, + Return: &conf_v1.ErrorPageReturn{ + ActionReturn: conf_v1.ActionReturn{ + Code: 200, + Headers: nil, + }, + }, + Redirect: nil, + }, + }, + errorLocations: []version2.ErrorPageLocation{ + { + Name: "@error_page_0_0", + DefaultType: "text/plain", + Return: &version2.Return{ + Text: "All Good", + }, + }, + }, + owner: &conf_v1.VirtualServer{ + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "namespace", + Name: "name", + }, + }, + expected: errorPageDetails{ + pages: []conf_v1.ErrorPage{ + { + Codes: []int{404, 405, 500, 502}, + Return: &conf_v1.ErrorPageReturn{ + ActionReturn: conf_v1.ActionReturn{ + Code: 200, + Headers: nil, + }, + }, + Redirect: nil, + }, + }, + index: 1, + owner: &conf_v1.VirtualServer{ + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "namespace", + Name: "name", + }, + }, + }, + }, + } + + for _, test := range tests { + result := generateErrorPageDetails(test.errorPages, test.errorLocations, test.owner) + if !reflect.DeepEqual(result, test.expected) { + t.Errorf("generateErrorPageDetails() returned %v but expected %v", result, test.expected) + } + } +} + func TestGenerateProxySSLName(t *testing.T) { t.Parallel() result := generateProxySSLName("coffee-v1", "default") From 980a835af49ac5209b6427be51fba6a6a93ccd3e Mon Sep 17 00:00:00 2001 From: Paul Abel Date: Mon, 3 Feb 2025 15:40:45 +0000 Subject: [PATCH 2/3] rename backup endpoint variable --- internal/configs/virtualserver.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index dce95a032d..ced27cf46c 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -466,13 +466,12 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( upstreamName := virtualServerUpstreamNamer.GetNameForUpstream(u.Name) upstreamNamespace := vsEx.VirtualServer.Namespace endpoints := vsc.generateEndpointsForUpstream(vsEx.VirtualServer, upstreamNamespace, u, vsEx) - backupEndpoints := vsc.generateBackupEndpointsForUpstream(vsEx.VirtualServer, upstreamNamespace, u, vsEx) + backup := vsc.generateBackupEndpointsForUpstream(vsEx.VirtualServer, upstreamNamespace, u, vsEx) // isExternalNameSvc is always false for OSS _, isExternalNameSvc := vsEx.ExternalNameSvcs[GenerateExternalNameSvcKey(upstreamNamespace, u.Service)] - ups := vsc.generateUpstream(vsEx.VirtualServer, upstreamName, u, isExternalNameSvc, endpoints, backupEndpoints) + ups := vsc.generateUpstream(vsEx.VirtualServer, upstreamName, u, isExternalNameSvc, endpoints, backup) upstreams = append(upstreams, ups) - u.TLS.Enable = isTLSEnabled(u, vsc.spiffeCerts, vsEx.VirtualServer.Spec.InternalRoute) crUpstreams[upstreamName] = u From cea91f2349b87552c7a9b116c0d560d1614f4128 Mon Sep 17 00:00:00 2001 From: Paul Abel Date: Tue, 4 Feb 2025 13:48:03 +0000 Subject: [PATCH 3/3] move similar generate upstreams to a function --- internal/configs/virtualserver.go | 117 +++++++++++++++++------------- 1 file changed, 66 insertions(+), 51 deletions(-) diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index ced27cf46c..f22ac98e49 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -458,62 +458,37 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( // generate upstreams for VirtualServer for _, u := range vsEx.VirtualServer.Spec.Upstreams { - - if (sslConfig == nil || !vsc.cfgParams.HTTP2) && isGRPC(u.Type) { - vsc.addWarningf(vsEx.VirtualServer, "gRPC cannot be configured for upstream %s. gRPC requires enabled HTTP/2 and TLS termination.", u.Name) - } - - upstreamName := virtualServerUpstreamNamer.GetNameForUpstream(u.Name) - upstreamNamespace := vsEx.VirtualServer.Namespace - endpoints := vsc.generateEndpointsForUpstream(vsEx.VirtualServer, upstreamNamespace, u, vsEx) - backup := vsc.generateBackupEndpointsForUpstream(vsEx.VirtualServer, upstreamNamespace, u, vsEx) - - // isExternalNameSvc is always false for OSS - _, isExternalNameSvc := vsEx.ExternalNameSvcs[GenerateExternalNameSvcKey(upstreamNamespace, u.Service)] - ups := vsc.generateUpstream(vsEx.VirtualServer, upstreamName, u, isExternalNameSvc, endpoints, backup) - upstreams = append(upstreams, ups) - u.TLS.Enable = isTLSEnabled(u, vsc.spiffeCerts, vsEx.VirtualServer.Spec.InternalRoute) - crUpstreams[upstreamName] = u - - if hc := generateHealthCheck(u, upstreamName, vsc.cfgParams); hc != nil { - healthChecks = append(healthChecks, *hc) - if u.HealthCheck.StatusMatch != "" { - statusMatches = append( - statusMatches, - generateUpstreamStatusMatch(upstreamName, u.HealthCheck.StatusMatch), - ) - } - } + upstreams, healthChecks, statusMatches = generateUpstreams( + sslConfig, + vsc, + u, + vsEx.VirtualServer, + vsEx.VirtualServer.Namespace, + virtualServerUpstreamNamer, + vsEx, + upstreams, + crUpstreams, + healthChecks, + statusMatches, + ) } // generate upstreams for each VirtualServerRoute for _, vsr := range vsEx.VirtualServerRoutes { upstreamNamer := NewUpstreamNamerForVirtualServerRoute(vsEx.VirtualServer, vsr) for _, u := range vsr.Spec.Upstreams { - if (sslConfig == nil || !vsc.cfgParams.HTTP2) && isGRPC(u.Type) { - vsc.addWarningf(vsr, "gRPC cannot be configured for upstream %s. gRPC requires enabled HTTP/2 and TLS termination", u.Name) - } - - upstreamName := upstreamNamer.GetNameForUpstream(u.Name) - upstreamNamespace := vsr.Namespace - endpoints := vsc.generateEndpointsForUpstream(vsr, upstreamNamespace, u, vsEx) - backup := vsc.generateBackupEndpointsForUpstream(vsEx.VirtualServer, upstreamNamespace, u, vsEx) - - // isExternalNameSvc is always false for OSS - _, isExternalNameSvc := vsEx.ExternalNameSvcs[GenerateExternalNameSvcKey(upstreamNamespace, u.Service)] - ups := vsc.generateUpstream(vsr, upstreamName, u, isExternalNameSvc, endpoints, backup) - upstreams = append(upstreams, ups) - u.TLS.Enable = isTLSEnabled(u, vsc.spiffeCerts, vsEx.VirtualServer.Spec.InternalRoute) - crUpstreams[upstreamName] = u - - if hc := generateHealthCheck(u, upstreamName, vsc.cfgParams); hc != nil { - healthChecks = append(healthChecks, *hc) - if u.HealthCheck.StatusMatch != "" { - statusMatches = append( - statusMatches, - generateUpstreamStatusMatch(upstreamName, u.HealthCheck.StatusMatch), - ) - } - } + upstreams, healthChecks, statusMatches = generateUpstreams( + sslConfig, + vsc, + u, + vsr, + vsr.Namespace, + upstreamNamer, + vsEx, + upstreams, + crUpstreams, + healthChecks, + statusMatches, + ) } } @@ -882,6 +857,46 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( return vsCfg, vsc.warnings } +func generateUpstreams( + sslConfig *version2.SSL, + vsc *virtualServerConfigurator, + u conf_v1.Upstream, + owner runtime.Object, + ownerNamespace string, + upstreamNamer *upstreamNamer, + vsEx *VirtualServerEx, + upstreams []version2.Upstream, + crUpstreams map[string]conf_v1.Upstream, + healthChecks []version2.HealthCheck, + statusMatches []version2.StatusMatch, +) ([]version2.Upstream, []version2.HealthCheck, []version2.StatusMatch) { + if (sslConfig == nil || !vsc.cfgParams.HTTP2) && isGRPC(u.Type) { + vsc.addWarningf(owner, "gRPC cannot be configured for upstream %s. gRPC requires enabled HTTP/2 and TLS termination", u.Name) + } + + upstreamName := upstreamNamer.GetNameForUpstream(u.Name) + endpoints := vsc.generateEndpointsForUpstream(owner, ownerNamespace, u, vsEx) + backup := vsc.generateBackupEndpointsForUpstream(owner, ownerNamespace, u, vsEx) + + // isExternalNameSvc is always false for OSS + _, isExternalNameSvc := vsEx.ExternalNameSvcs[GenerateExternalNameSvcKey(ownerNamespace, u.Service)] + ups := vsc.generateUpstream(owner, upstreamName, u, isExternalNameSvc, endpoints, backup) + upstreams = append(upstreams, ups) + u.TLS.Enable = isTLSEnabled(u, vsc.spiffeCerts, vsEx.VirtualServer.Spec.InternalRoute) + crUpstreams[upstreamName] = u + + if hc := generateHealthCheck(u, upstreamName, vsc.cfgParams); hc != nil { + healthChecks = append(healthChecks, *hc) + if u.HealthCheck.StatusMatch != "" { + statusMatches = append( + statusMatches, + generateUpstreamStatusMatch(upstreamName, u.HealthCheck.StatusMatch), + ) + } + } + return upstreams, healthChecks, statusMatches +} + // rateLimit hold the configuration for the ratelimiting Policy type rateLimit struct { Reqs []version2.LimitReq