Skip to content

Commit

Permalink
cors origins roundtrip fix (#3612)
Browse files Browse the repository at this point in the history
* cors mapping fix

Signed-off-by: Alix Cook <[email protected]>

* fake test

Signed-off-by: Alix Cook <[email protected]>

* changelog

Signed-off-by: Alix Cook <[email protected]>

Signed-off-by: Luke Shumaker <[email protected]>
  • Loading branch information
acookin authored and LukeShu committed Sep 28, 2021
1 parent 3f88fa1 commit 766c355
Show file tree
Hide file tree
Showing 9 changed files with 251 additions and 21 deletions.
4 changes: 1 addition & 3 deletions charts/emissary-ingress/crds/getambassador.io_mappings.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -148,9 +148,7 @@ spec:
- type: string
- type: array
origins:
description: StringOrStringList is just what it says on the tin, but note that it will always marshal as a list of strings right now.
items:
type: string
description: StringLiteralOrStringList is mostly like StringOrStringList, but instead of always forcing a list of strings, it will marshal a string literal as a string.
oneOf:
- type: string
- type: array
Expand Down
139 changes: 139 additions & 0 deletions cmd/entrypoint/fake_mapping_cors_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
package entrypoint_test

import (
"testing"

"github.com/datawire/ambassador/v2/cmd/entrypoint"
envoy "github.com/datawire/ambassador/v2/pkg/api/envoy/api/v2"
route "github.com/datawire/ambassador/v2/pkg/api/envoy/api/v2/route"
bootstrap "github.com/datawire/ambassador/v2/pkg/api/envoy/config/bootstrap/v2"
http "github.com/datawire/ambassador/v2/pkg/api/envoy/config/filter/network/http_connection_manager/v2"
"github.com/datawire/ambassador/v2/pkg/envoy-control-plane/resource/v2"
"github.com/datawire/ambassador/v2/pkg/envoy-control-plane/wellknown"

"github.com/stretchr/testify/assert"
)

func TestMappingCORSOriginsSlice(t *testing.T) {
f := entrypoint.RunFake(t, entrypoint.FakeConfig{EnvoyConfig: true}, nil)
f.UpsertYAML(`
---
apiVersion: getambassador.io/v2
kind: Mapping
metadata:
name: foo
namespace: default
spec:
prefix: /foo
service: foo.default
cors:
origins:
- foo.example.com
- bar.example.com
`)
f.Upsert(makeService("default", "foo"))
f.Flush()
snap := f.GetSnapshot(HasMapping("default", "foo"))
assert.NotNil(t, snap)

config := f.GetEnvoyConfig(func(config *bootstrap.Bootstrap) bool {
return FindCluster(config, ClusterNameContains("cluster_foo_default_default")) != nil
})

listener := findListener(config, func(l *envoy.Listener) bool {
return l.Name == "ambassador-listener-8080"
})

assert.NotNil(t, listener)

routeAction := findVirtualHostRoute(listener, func(r *route.RouteAction) bool {
return r.GetCluster() == "cluster_foo_default_default"
})
assert.NotNil(t, routeAction)
assert.NotNil(t, routeAction.Cors)
assert.Equal(t, len(routeAction.Cors.AllowOriginStringMatch), 2)
for _, m := range routeAction.Cors.AllowOriginStringMatch {
assert.Contains(t, []string{"bar.example.com", "foo.example.com"}, m.GetExact())

}
}

func TestMappingCORSOriginsString(t *testing.T) {
f := entrypoint.RunFake(t, entrypoint.FakeConfig{EnvoyConfig: true}, nil)
f.UpsertYAML(`
---
apiVersion: getambassador.io/v2
kind: Mapping
metadata:
name: foo
namespace: default
spec:
prefix: /foo
service: foo.default
cors:
origins: "foo.example.com,bar.example.com"
`)
f.Upsert(makeService("default", "foo"))
f.Flush()
snap := f.GetSnapshot(HasMapping("default", "foo"))
assert.NotNil(t, snap)

config := f.GetEnvoyConfig(func(config *bootstrap.Bootstrap) bool {
return FindCluster(config, ClusterNameContains("cluster_foo_default_default")) != nil
})

listener := findListener(config, func(l *envoy.Listener) bool {
return l.Name == "ambassador-listener-8080"
})

assert.NotNil(t, listener)

routeAction := findVirtualHostRoute(listener, func(r *route.RouteAction) bool {
return r.GetCluster() == "cluster_foo_default_default"
})
assert.NotNil(t, routeAction)
assert.NotNil(t, routeAction.Cors)
assert.Equal(t, len(routeAction.Cors.AllowOriginStringMatch), 2)
for _, m := range routeAction.Cors.AllowOriginStringMatch {
assert.Contains(t, []string{"bar.example.com", "foo.example.com"}, m.GetExact())

}
}

func findVirtualHostRoute(listener *envoy.Listener, predicate func(*route.RouteAction) bool) *route.RouteAction {
for _, fc := range listener.FilterChains {
for _, filter := range fc.Filters {
if filter.Name != wellknown.HTTPConnectionManager {
continue
}
hcm := resource.GetHTTPConnectionManager(filter)
if hcm != nil {
rs, ok := hcm.RouteSpecifier.(*http.HttpConnectionManager_RouteConfig)
if ok {
for _, vh := range rs.RouteConfig.VirtualHosts {
for _, vhr := range vh.Routes {
routeAction, ok := vhr.Action.(*route.Route_Route)
if ok {
if predicate(routeAction.Route) {
return routeAction.Route
}
}
}
}
}
}
}

}
return nil

}

func findListener(envoyConfig *bootstrap.Bootstrap, predicate func(*envoy.Listener) bool) *envoy.Listener {
for _, listener := range envoyConfig.StaticResources.Listeners {
if predicate(listener) {
return listener
}
}
return nil
}
4 changes: 1 addition & 3 deletions docs/yaml/ambassador/ambassador-rbac-prometheus.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1058,9 +1058,7 @@ spec:
- type: string
- type: array
origins:
description: StringOrStringList is just what it says on the tin, but note that it will always marshal as a list of strings right now.
items:
type: string
description: StringLiteralOrStringList is mostly like StringOrStringList, but instead of always forcing a list of strings, it will marshal a string literal as a string.
oneOf:
- type: string
- type: array
Expand Down
4 changes: 1 addition & 3 deletions manifests/emissary/ambassador-crds.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -844,9 +844,7 @@ spec:
- type: string
- type: array
origins:
description: StringOrStringList is just what it says on the tin, but note that it will always marshal as a list of strings right now.
items:
type: string
description: StringLiteralOrStringList is mostly like StringOrStringList, but instead of always forcing a list of strings, it will marshal a string literal as a string.
oneOf:
- type: string
- type: array
Expand Down
4 changes: 1 addition & 3 deletions manifests/emissary/emissary-crds.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -844,9 +844,7 @@ spec:
- type: string
- type: array
origins:
description: StringOrStringList is just what it says on the tin, but note that it will always marshal as a list of strings right now.
items:
type: string
description: StringLiteralOrStringList is mostly like StringOrStringList, but instead of always forcing a list of strings, it will marshal a string literal as a string.
oneOf:
- type: string
- type: array
Expand Down
46 changes: 46 additions & 0 deletions pkg/api/getambassador.io/v2/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,52 @@ func (sl *StringOrStringList) UnmarshalJSON(data []byte) error {
return err
}

// StringLiteralOrStringList is mostly like StringOrStringList,
// but instead of always forcing a list of strings, it will
// marshal a string literal as a string.
// +kubebuilder:validation:Type="d6e-union:string,array"
type StringLiteralOrStringList struct {
String *string `json:"-"`
ListOfStrings *[]string `json:"-"`
}

func (sl *StringLiteralOrStringList) UnmarshalJSON(data []byte) error {
if string(data) == "null" {
*sl = StringLiteralOrStringList{}
return nil
}

var err error
var list []string
var single string

if err = json.Unmarshal(data, &single); err == nil {
*sl = StringLiteralOrStringList{String: &single}
return nil
}

if err = json.Unmarshal(data, &list); err == nil {
*sl = StringLiteralOrStringList{ListOfStrings: &list}
return nil
}

return err
}

func (sl *StringLiteralOrStringList) MarshalJSON() ([]byte, error) {
switch {
case sl.String == nil && sl.ListOfStrings == nil:
return json.Marshal(nil)
case sl.String == nil && sl.ListOfStrings != nil:
return json.Marshal(sl.ListOfStrings)
case sl.String != nil && sl.ListOfStrings == nil:
return json.Marshal(sl.String)
case sl.String != nil && sl.ListOfStrings != nil:
panic("invalid StringLiteralOrStringList")
}
panic("not reached")
}

// BoolOrString is a type that can hold a Boolean or a string.
//
// +kubebuilder:validation:Type="d6e-union:string,boolean"
Expand Down
12 changes: 6 additions & 6 deletions pkg/api/getambassador.io/v2/mapping_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -357,12 +357,12 @@ type KeepAlive struct {
}

type CORS struct {
Origins StringOrStringList `json:"origins,omitempty"`
Methods StringOrStringList `json:"methods,omitempty"`
Headers StringOrStringList `json:"headers,omitempty"`
Credentials *bool `json:"credentials,omitempty"`
ExposedHeaders StringOrStringList `json:"exposed_headers,omitempty"`
MaxAge string `json:"max_age,omitempty"`
Origins *StringLiteralOrStringList `json:"origins,omitempty"`
Methods StringOrStringList `json:"methods,omitempty"`
Headers StringOrStringList `json:"headers,omitempty"`
Credentials *bool `json:"credentials,omitempty"`
ExposedHeaders StringOrStringList `json:"exposed_headers,omitempty"`
MaxAge string `json:"max_age,omitempty"`
}

type RetryPolicy struct {
Expand Down
26 changes: 25 additions & 1 deletion pkg/api/getambassador.io/v2/testdata/mappings.json
Original file line number Diff line number Diff line change
Expand Up @@ -653,6 +653,30 @@
}
}
},
{
"apiVersion": "getambassador.io/v2",
"kind": "Mapping",
"metadata": {
"annotations": {
"kubectl.kubernetes.io/last-applied-configuration": "{\"apiVersion\":\"getambassador.io/v2\",\"kind\":\"Mapping\",\"metadata\":{\"annotations\":{},\"name\":\"load-testing-base\",\"namespace\":\"ambassador\"},\"spec\":{\"prefix\":\"/load-testing/\",\"service\":\"load-http-echo.default\"}}\n"
},
"creationTimestamp": "2020-08-11T20:54:27Z",
"generation": 1,
"name": "cors-origins-string",
"namespace": "ambassador",
"resourceVersion": "4462591",
"selfLink": "/apis/getambassador.io/v2/namespaces/ambassador/mappings/load-testing-base",
"uid": "d5ef2932-a6a3-439b-bca8-d80f195cd9f6"
},
"spec": {
"prefix": "/load-testing/",
"service": "load-http-echo.default",
"cors": {
"origins": "ffs,ffs2",
"credentials": true
}
}
},
{
"apiVersion": "getambassador.io/v2",
"kind": "Mapping",
Expand Down Expand Up @@ -824,7 +848,7 @@
"service": "https://a",
"timeout_ms": 10000
}
},
},
{
"apiVersion": "getambassador.io/v2",
"kind": "Mapping",
Expand Down
33 changes: 31 additions & 2 deletions pkg/api/getambassador.io/v2/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 766c355

Please sign in to comment.