Repository: avenga/couper
Pull request #236 (2021-05-18 13:42:38)

use transport hostname instead for validation purposes
Skip relative url rewrite if variables are used `{...}`.

malud: #230 

Patch:
diff --git a/handler/transport/backend.go b/handler/transport/backend.go
index fab038f5..016f1242 100644
--- a/handler/transport/backend.go
+++ b/handler/transport/backend.go
@@ -157,7 +157,7 @@ func (b *Backend) RoundTrip(req *http.Request) (*http.Response, error) {
 }
 
 func (b *Backend) openAPIValidate(req *http.Request, tc *Config, deadlineErr <-chan error) (*http.Response, error) {
-	requestValidationInput, err := b.openAPIValidator.ValidateRequest(req, tc.hash(), tc.Scheme+"://"+tc.Origin)
+	requestValidationInput, err := b.openAPIValidator.ValidateRequest(req, tc.hash())
 	if err != nil {
 		return nil, errors.BackendValidation.Label(b.name).Kind("backend_request_validation").With(err)
 	}
diff --git a/handler/validation/openapi.go b/handler/validation/openapi.go
index 2e25ba14..e87f69b3 100644
--- a/handler/validation/openapi.go
+++ b/handler/validation/openapi.go
@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
+	"net"
 	"net/http"
 	"net/url"
 	"sync"
@@ -16,7 +17,7 @@ import (
 	"github.com/avenga/couper/eval"
 )
 
-var swaggers sync.Map
+var routers sync.Map
 
 type OpenAPI struct {
 	options *OpenAPIOptions
@@ -31,45 +32,66 @@ func NewOpenAPI(opts *OpenAPIOptions) *OpenAPI {
 	}
 }
 
-func (v *OpenAPI) getModifiedSwagger(key, origin string) (*openapi3.Swagger, error) {
-	swagger, exists := swaggers.Load(key)
+func (v *OpenAPI) getRouter(key, origin string) (*openapi3filter.Router, error) {
+	router, exists := routers.Load(key)
 	if !exists {
 		clonedSwagger := cloneSwagger(v.options.swagger)
 
 		var newServers []string
 		for _, s := range clonedSwagger.Servers {
+			// Do not touch a url if template variables are used.
+			if names, err := s.ParameterNames(); len(names) > 0 || err != nil {
+				continue
+			}
+
 			su, err := url.Parse(s.URL)
 			if err != nil {
 				return nil, err
 			}
+
 			if !su.IsAbs() {
 				newServers = append(newServers, origin+s.URL)
+				continue
 			}
+
+			if su.Port() == "" && (su.Scheme == "https" || su.Scheme == "http") {
+				su.Host = su.Hostname() + ":"
+				if su.Scheme == "https" {
+					su.Host += "443"
+				} else {
+					su.Host += "80"
+				}
+				s.URL = su.String()
+			}
+
 		}
+
 		for _, ns := range newServers {
 			clonedSwagger.AddServer(&openapi3.Server{URL: ns})
 		}
 
-		swaggers.Store(key, clonedSwagger)
-		swagger = clonedSwagger
-	}
+		r := openapi3filter.NewRouter()
+		if err := r.AddSwagger(clonedSwagger); err != nil {
+			return nil, err
+		}
 
-	if s, ok := swagger.(*openapi3.Swagger); ok {
-		return s, nil
+		routers.Store(key, r)
+		return r, nil
 	}
 
-	return nil, fmt.Errorf("swagger wrong type: %v", swagger)
+	return router.(*openapi3filter.Router), nil
 }
 
-func cloneSwagger(s *openapi3.Swagger) *openapi3.Swagger {
-	sw := *s
-	// this is not a deep clone; we only want to add servers
-	sw.Servers = s.Servers[:]
-	return &sw
-}
-
-func (v *OpenAPI) ValidateRequest(req *http.Request, key, origin string) (*openapi3filter.RequestValidationInput, error) {
-	swagger, err := v.getModifiedSwagger(key, origin)
+func (v *OpenAPI) ValidateRequest(req *http.Request, key string) (*openapi3filter.RequestValidationInput, error) {
+	// reqURL is modified due to origin transport configuration
+	serverURL := *req.URL
+	// possible hostname attribute override
+	if _, p, _ := net.SplitHostPort(req.Host); p != "" { // hostname could contain a port already
+		serverURL.Host = req.Host
+	} else {
+		serverURL.Host = req.Host + ":" + serverURL.Port()
+	}
+	router, err := v.getRouter(key, serverURL.Scheme+"://"+serverURL.Host)
 	if err != nil {
 		if ctx, ok := req.Context().Value(request.OpenAPI).(*OpenAPIContext); ok {
 			ctx.errors = append(ctx.errors, err)
@@ -80,12 +102,7 @@ func (v *OpenAPI) ValidateRequest(req *http.Request, key, origin string) (*opena
 		return nil, nil
 	}
 
-	router := openapi3filter.NewRouter()
-	if err = router.AddSwagger(swagger); err != nil {
-		return nil, err
-	}
-
-	route, pathParams, err := router.FindRoute(req.Method, req.URL)
+	route, pathParams, err := router.FindRoute(req.Method, &serverURL)
 	if err != nil {
 		err = fmt.Errorf("'%s %s': %w", req.Method, req.URL.Path, err)
 		if ctx, ok := req.Context().Value(request.OpenAPI).(*OpenAPIContext); ok {
@@ -168,3 +185,10 @@ func (v *OpenAPI) ValidateResponse(beresp *http.Response, requestValidationInput
 
 	return nil
 }
+
+func cloneSwagger(s *openapi3.Swagger) *openapi3.Swagger {
+	sw := *s
+	// this is not a deep clone; we only want to add servers
+	sw.Servers = s.Servers[:]
+	return &sw
+}
diff --git a/handler/validation/openapi_options.go b/handler/validation/openapi_options.go
index 0ffbdad5..c8cbff68 100644
--- a/handler/validation/openapi_options.go
+++ b/handler/validation/openapi_options.go
@@ -3,7 +3,6 @@ package validation
 import (
 	"fmt"
 	"io/ioutil"
-	"net/url"
 	"path/filepath"
 
 	"github.com/getkin/kin-openapi/openapi3"
@@ -41,26 +40,6 @@ func NewOpenAPIOptions(openapi *config.OpenAPI) (*OpenAPIOptions, error) {
 	return NewOpenAPIOptionsFromBytes(openapi, b)
 }
 
-func canonicalizeServerURLs(swagger *openapi3.Swagger) error {
-	for _, server := range swagger.Servers {
-		su, err := url.Parse(server.URL)
-		if err != nil {
-			return err
-		}
-
-		if su.IsAbs() && su.Port() == "" && (su.Scheme == "https" || su.Scheme == "http") {
-			su.Host = su.Hostname() + ":"
-			if su.Scheme == "https" {
-				su.Host += "443"
-			} else {
-				su.Host += "80"
-			}
-			server.URL = su.String()
-		}
-	}
-	return nil
-}
-
 func NewOpenAPIOptionsFromBytes(openapi *config.OpenAPI, bytes []byte) (*OpenAPIOptions, error) {
 	if openapi == nil || bytes == nil {
 		return nil, nil
@@ -71,10 +50,6 @@ func NewOpenAPIOptionsFromBytes(openapi *config.OpenAPI, bytes []byte) (*OpenAPI
 		return nil, fmt.Errorf("error loading openapi file: %w", err)
 	}
 
-	if err = canonicalizeServerURLs(swagger); err != nil {
-		return nil, err
-	}
-
 	// Always buffer if openAPI is active. Request buffering is handled by openapifilter too.
 	// Anyway adding request buffer option to let Couper check the body limits.
 	bufferBodies := eval.BufferRequest | eval.BufferResponse


Test patch:
diff --git a/handler/validation/openapi_test.go b/handler/validation/openapi_test.go
index 9540d9e7..8931d573 100644
--- a/handler/validation/openapi_test.go
+++ b/handler/validation/openapi_test.go
@@ -12,7 +12,6 @@ import (
 
 	"github.com/hashicorp/hcl/v2"
 	"github.com/hashicorp/hcl/v2/hcltest"
-	logrustest "github.com/sirupsen/logrus/hooks/test"
 	"github.com/zclconf/go-cty/cty"
 
 	"github.com/avenga/couper/config"
@@ -124,7 +123,7 @@ func TestOpenAPIValidator_ValidateRequest(t *testing.T) {
 func TestOpenAPIValidator_RelativeServerURL(t *testing.T) {
 	helper := test.New(t)
 
-	log, hook := logrustest.NewNullLogger()
+	log, hook := test.NewLogger()
 	logger := log.WithContext(context.Background())
 	beConf := &config.Backend{
 		Remain: body.New(&hcl.BodyContent{Attributes: hcl.Attributes{
@@ -144,66 +143,145 @@ func TestOpenAPIValidator_RelativeServerURL(t *testing.T) {
 		OpenAPI: openAPI,
 	}, logger)
 
-	t.Run("relative server URL", func(t *testing.T) {
-		req := httptest.NewRequest(http.MethodGet, "https://httpbin.org/anything", nil)
+	req := httptest.NewRequest(http.MethodGet, "https://httpbin.org/anything", nil)
 
-		hook.Reset()
-		_, err := backend.RoundTrip(req)
-		if err != nil {
-			helper.Must(err)
-		}
+	hook.Reset()
+	_, err = backend.RoundTrip(req)
+	if err != nil {
+		helper.Must(err)
+	}
 
-		if t.Failed() {
-			for _, entry := range hook.Entries {
-				t.Log(entry.String())
-			}
+	if t.Failed() {
+		for _, entry := range hook.Entries {
+			t.Log(entry.String())
 		}
-	})
+	}
 }
 
-func TestOpenAPIValidator_NonCanonicalServerURL(t *testing.T) {
+func TestOpenAPIValidator_TemplateVariables(t *testing.T) {
 	helper := test.New(t)
 
-	log, hook := logrustest.NewNullLogger()
+	origin := test.NewBackend()
+	defer origin.Close()
+
+	log, hook := test.NewLogger()
 	logger := log.WithContext(context.Background())
-	beConf := &config.Backend{
-		Remain: body.New(&hcl.BodyContent{Attributes: hcl.Attributes{
-			"origin": &hcl.Attribute{
-				Name: "origin",
-				Expr: hcltest.MockExprLiteral(cty.StringVal("https://httpbin.org")),
-			},
-		}}),
-		OpenAPI: &config.OpenAPI{
-			File: filepath.Join("testdata/backend_03_openapi.yaml"),
-		},
+
+	type testCase struct {
+		name, origin, hostname string
 	}
-	openAPI, err := validation.NewOpenAPIOptions(beConf.OpenAPI)
+
+	openAPI, err := validation.NewOpenAPIOptions(&config.OpenAPI{
+		File: filepath.Join("testdata/backend_04_openapi.yaml"),
+	})
 	helper.Must(err)
 
-	backend := transport.NewBackend(beConf.Remain, &transport.Config{}, &transport.BackendOptions{
-		OpenAPI: openAPI,
-	}, logger)
+	for _, tc := range []testCase{
+		{name: "tpl url", origin: "https://api.example.com"},
+		{name: "relative url", origin: origin.Addr()},
+	} {
+		t.Run(tc.name, func(subT *testing.T) {
+			beConf := &config.Backend{
+				Remain: body.New(&hcl.BodyContent{Attributes: hcl.Attributes{
+					"origin": &hcl.Attribute{
+						Name: "origin",
+						Expr: hcltest.MockExprLiteral(cty.StringVal(tc.origin)),
+					},
+					"hostname": &hcl.Attribute{
+						Name: "hostname",
+						Expr: hcltest.MockExprLiteral(cty.StringVal(tc.hostname)),
+					},
+					"proxy": &hcl.Attribute{
+						Name: "proxy",
+						Expr: hcltest.MockExprLiteral(cty.StringVal(origin.Addr())),
+					},
+					"path": &hcl.Attribute{
+						Name: "path",
+						Expr: hcltest.MockExprLiteral(cty.StringVal("/anything")),
+					},
+				}}),
+			}
+
+			backend := transport.NewBackend(beConf.Remain, &transport.Config{}, &transport.BackendOptions{
+				OpenAPI: openAPI,
+			}, logger)
+
+			req := httptest.NewRequest(http.MethodGet, "https://test.local/", nil)
+
+			hook.Reset()
+			_, err = backend.RoundTrip(req)
+			if err != nil && err.Error() != "backend error" {
+				subT.Error(err)
+			}
+
+			if subT.Failed() {
+				for _, entry := range hook.Entries {
+					subT.Log(entry.String())
+				}
+			}
+		})
+	}
+}
+
+func TestOpenAPIValidator_NonCanonicalServerURL(t *testing.T) {
+	helper := test.New(t)
+
+	origin := test.NewBackend()
+	defer origin.Close()
+
+	log, hook := test.NewLogger()
+	logger := log.WithContext(context.Background())
+
+	openAPI, err := validation.NewOpenAPIOptions(&config.OpenAPI{
+		File: filepath.Join("testdata/backend_03_openapi.yaml"),
+	})
+	helper.Must(err)
 
 	tests := []struct {
 		name, url string
 	}{
-		{"http", "http://httpbin.org/anything"},
-		{"https", "https://httpbin.org/anything"},
+		{"http", "http://api.example.com"},
+		{"http80", "http://api.example.com:80"},
+		{"http443", "http://api.example.com:443"},
+		{"https", "https://api.example.com"},
+		{"https443", "https://api.example.com:443"},
+		{"https80", "https://api.example.com:80"},
+		{"httpsHigh", "https://api.example.com:12345"},
+		{"httpHigh", "http://api.example.com:12345"},
 	}
 
 	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			req := httptest.NewRequest(http.MethodGet, tt.url, nil)
+		t.Run(tt.name, func(subT *testing.T) {
+			Remain := body.New(&hcl.BodyContent{Attributes: hcl.Attributes{
+				"origin": &hcl.Attribute{
+					Name: "origin",
+					Expr: hcltest.MockExprLiteral(cty.StringVal(tt.url)),
+				},
+				"path": &hcl.Attribute{
+					Name: "path",
+					Expr: hcltest.MockExprLiteral(cty.StringVal("/anything")),
+				},
+				"proxy": &hcl.Attribute{
+					Name: "proxy",
+					Expr: hcltest.MockExprLiteral(cty.StringVal(origin.Addr())),
+				},
+			}})
 
+			backend := transport.NewBackend(Remain, &transport.Config{}, &transport.BackendOptions{
+				OpenAPI: openAPI,
+			}, logger)
 			hook.Reset()
-			_, err := backend.RoundTrip(req)
-			if err != nil {
-				helper.Must(err)
+
+			req := httptest.NewRequest(http.MethodGet, tt.url, nil)
+
+			_, err = backend.RoundTrip(req)
+			if err != nil && err.Error() != "backend error" {
+				subT.Error(err)
 			}
 
-			if t.Failed() {
+			if subT.Failed() {
 				for _, entry := range hook.Entries {
-					t.Log(entry.String())
+					subT.Log(entry.String())
 				}
 			}
 		})
diff --git a/handler/validation/testdata/backend_03_openapi.yaml b/handler/validation/testdata/backend_03_openapi.yaml
index 94f8a419..88ac9bd8 100644
--- a/handler/validation/testdata/backend_03_openapi.yaml
+++ b/handler/validation/testdata/backend_03_openapi.yaml
@@ -3,8 +3,14 @@ info:
   title: 'Couper backend validation test: non-canonical server URL'
   version: 'v1.2.3'
 servers:
-  - url: https://httpbin.org/
-  - url: http://httpbin.org/
+  - url: 'http://api.example.com:12345'
+  - url: 'https://api.example.com:12345'
+  - url: 'https://api.example.com:443'
+  - url: 'https://api.example.com:80'
+  - url: 'http://api.example.com:443'
+  - url: 'http://api.example.com:80'
+  - url: 'http://api.example.com'
+  - url: 'https://api.example.com'
 paths:
   /anything:
     get:
diff --git a/handler/validation/testdata/backend_04_openapi.yaml b/handler/validation/testdata/backend_04_openapi.yaml
new file mode 100644
index 00000000..572554eb
--- /dev/null
+++ b/handler/validation/testdata/backend_04_openapi.yaml
@@ -0,0 +1,24 @@
+openapi: '3'
+info:
+  title: 'Couper backend validation test: template variables'
+  version: 'v1.2.3'
+servers:
+  - url: '/anything'
+  - url: '/{sub}/anything'
+    variables:
+      sub:
+        default: 'api'
+  - url: 'http://{broken/v1'
+    variables:
+      broken:
+        default: https://api.example.com
+  - url: 'https://{sub}.example.com/anything'
+    variables:
+      sub:
+        default: api
+paths:
+  /:
+    get:
+      responses:
+        200:
+          description: OK