diff --git a/CHANGELOG.md b/CHANGELOG.md index 6cec4ba9ac..8669fe9fc7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,8 +68,9 @@ Please see the [Envoy documentation](https://www.envoyproxy.io/docs/envoy/latest ### Emissary Ingress and Ambassador Edge Stack -- Change: Envoy-configuration snapshots get saved (as ambex-#.json) in /ambassador/snapshots. The number of snapshots is +- Change: Envoy-configuration snapshots get saved (as ambex-#.json) in /ambassador/snapshots. The number of snapshots is controlled by the `AMBASSADOR_AMBEX_SNAPSHOT_COUNT` environment variable; set it to 0 to disable. The default is 30. +- Bugfix: Fixed a regression when specifying a comma separated string for `cors.origins` on the `Mapping` resource ### Ambassador Edge Stack diff --git a/builder/builder.mk b/builder/builder.mk index 8f4bbabe8c..c7760de545 100644 --- a/builder/builder.mk +++ b/builder/builder.mk @@ -595,7 +595,7 @@ setup-venv: pip install orjson==3.3.1; \ rm -f venv/lib/python3.8/site-packages/_manylinux.py; \ else \ - pip install orjson==3.3.1; \ + pip install orjson; \ fi; \ pip install -r $(OSS_HOME)/builder/requirements.txt; \ pip install -e $(OSS_HOME)/python; \ diff --git a/charts/ambassador/crds/getambassador.io_mappings.yaml b/charts/ambassador/crds/getambassador.io_mappings.yaml index c61eefb5fd..dec2af4616 100644 --- a/charts/ambassador/crds/getambassador.io_mappings.yaml +++ b/charts/ambassador/crds/getambassador.io_mappings.yaml @@ -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 diff --git a/cmd/entrypoint/fake_mapping_cors_test.go b/cmd/entrypoint/fake_mapping_cors_test.go new file mode 100644 index 0000000000..afab124fb4 --- /dev/null +++ b/cmd/entrypoint/fake_mapping_cors_test.go @@ -0,0 +1,139 @@ +package entrypoint_test + +import ( + "testing" + + "github.com/datawire/ambassador/cmd/entrypoint" + envoy "github.com/datawire/ambassador/pkg/api/envoy/api/v2" + route "github.com/datawire/ambassador/pkg/api/envoy/api/v2/route" + bootstrap "github.com/datawire/ambassador/pkg/api/envoy/config/bootstrap/v2" + http "github.com/datawire/ambassador/pkg/api/envoy/config/filter/network/http_connection_manager/v2" + "github.com/datawire/ambassador/pkg/envoy-control-plane/resource/v2" + "github.com/datawire/ambassador/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 +} diff --git a/docs/yaml/ambassador/ambassador-crds.yaml b/docs/yaml/ambassador/ambassador-crds.yaml index da4babe13b..076aacc157 100644 --- a/docs/yaml/ambassador/ambassador-crds.yaml +++ b/docs/yaml/ambassador/ambassador-crds.yaml @@ -852,9 +852,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 diff --git a/docs/yaml/ambassador/ambassador-rbac-prometheus.yaml b/docs/yaml/ambassador/ambassador-rbac-prometheus.yaml index 48cd8d5073..0c13b6f701 100644 --- a/docs/yaml/ambassador/ambassador-rbac-prometheus.yaml +++ b/docs/yaml/ambassador/ambassador-rbac-prometheus.yaml @@ -916,9 +916,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 diff --git a/manifests/ambassador/ambassador-crds.yaml b/manifests/ambassador/ambassador-crds.yaml index da4babe13b..076aacc157 100644 --- a/manifests/ambassador/ambassador-crds.yaml +++ b/manifests/ambassador/ambassador-crds.yaml @@ -852,9 +852,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 diff --git a/pkg/api/getambassador.io/v2/common.go b/pkg/api/getambassador.io/v2/common.go index 98741045a7..e9e22846c1 100644 --- a/pkg/api/getambassador.io/v2/common.go +++ b/pkg/api/getambassador.io/v2/common.go @@ -264,6 +264,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" diff --git a/pkg/api/getambassador.io/v2/mapping_types.go b/pkg/api/getambassador.io/v2/mapping_types.go index f296335fcf..c884dbc88d 100644 --- a/pkg/api/getambassador.io/v2/mapping_types.go +++ b/pkg/api/getambassador.io/v2/mapping_types.go @@ -337,12 +337,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 { diff --git a/pkg/api/getambassador.io/v2/testdata/mappings.json b/pkg/api/getambassador.io/v2/testdata/mappings.json index bf8f7683fe..5757b41eaf 100644 --- a/pkg/api/getambassador.io/v2/testdata/mappings.json +++ b/pkg/api/getambassador.io/v2/testdata/mappings.json @@ -624,6 +624,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", @@ -795,7 +819,7 @@ "service": "https://a", "timeout_ms": 10000 } - }, + }, { "apiVersion": "getambassador.io/v2", "kind": "Mapping", diff --git a/pkg/api/getambassador.io/v2/zz_generated.deepcopy.go b/pkg/api/getambassador.io/v2/zz_generated.deepcopy.go index c64a1e596a..cc1184813a 100644 --- a/pkg/api/getambassador.io/v2/zz_generated.deepcopy.go +++ b/pkg/api/getambassador.io/v2/zz_generated.deepcopy.go @@ -377,8 +377,8 @@ func (in *CORS) DeepCopyInto(out *CORS) { *out = *in if in.Origins != nil { in, out := &in.Origins, &out.Origins - *out = make(StringOrStringList, len(*in)) - copy(*out, *in) + *out = new(StringLiteralOrStringList) + (*in).DeepCopyInto(*out) } if in.Methods != nil { in, out := &in.Methods, &out.Methods @@ -2152,6 +2152,35 @@ func (in *RetryPolicy) DeepCopy() *RetryPolicy { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *StringLiteralOrStringList) DeepCopyInto(out *StringLiteralOrStringList) { + *out = *in + if in.String != nil { + in, out := &in.String, &out.String + *out = new(string) + **out = **in + } + if in.ListOfStrings != nil { + in, out := &in.ListOfStrings, &out.ListOfStrings + *out = new([]string) + if **in != nil { + in, out := *in, *out + *out = make([]string, len(*in)) + copy(*out, *in) + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new StringLiteralOrStringList. +func (in *StringLiteralOrStringList) DeepCopy() *StringLiteralOrStringList { + if in == nil { + return nil + } + out := new(StringLiteralOrStringList) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in StringOrStringList) DeepCopyInto(out *StringOrStringList) { {