From c2327e9c135abecfb07385f454eb020e1d0803d4 Mon Sep 17 00:00:00 2001 From: Sekar Saravanan Date: Wed, 3 Jul 2024 09:41:51 +0530 Subject: [PATCH 01/13] issue-5714 - incorrect _cache_key generation fixed Signed-off-by: Sekar Saravanan (cherry picked from commit e8ca65046a853e5aeed32fa125748387c0039813) --- CHANGELOG.md | 6 ++++++ python/ambassador/ir/irhttpmapping.py | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ae3e266c2d..073112bc32 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -135,6 +135,12 @@ it will be removed; but as it won't be user-visible this isn't considered a brea starting. This will help address some of the intermittent issues seen during install and upgrades. +- Bugfix: _cache_key is getting generated incorrectly for mappings in ir.json, when using header + with regex in mapping. Always, It should be in the format of {kind}-{version}-{name}-{namespace}. + But header name is getting updated in the place of mapping name, if we created mapping with regex + header. + + ## [3.8.0] August 29, 2023 [3.8.0]: https://github.com/emissary-ingress/emissary/compare/v3.7.2...v3.8.0 diff --git a/python/ambassador/ir/irhttpmapping.py b/python/ambassador/ir/irhttpmapping.py index 700fe071ae..7eea649cc5 100644 --- a/python/ambassador/ir/irhttpmapping.py +++ b/python/ambassador/ir/irhttpmapping.py @@ -241,8 +241,8 @@ def __init__( if "regex_headers" in kwargs: # DON'T do anything special with a regex :authority match: we can't # do host-based filtering within the IR for it anyway. - for name, value in kwargs.get("regex_headers", {}).items(): - hdrs.append(KeyValueDecorator(name, value, regex=True)) + for hdr_name, hdr_value in kwargs.get("regex_headers", {}).items(): + hdrs.append(KeyValueDecorator(hdr_name, hdr_value, regex=True)) if "host" in kwargs: # It's deliberate that we'll allow kwargs['host'] to silently override an exact :authority From 1a8a18980d7572339da1eeb74cc9a82cf82cbbfb Mon Sep 17 00:00:00 2001 From: Flynn Date: Fri, 5 Jul 2024 14:52:09 -0400 Subject: [PATCH 02/13] Move the changelog comment into `releaseNotes.yml` as required by the build process. Signed-off-by: Flynn (cherry picked from commit d04280e3a8da03e4a28f3756c361097c2dc2555e) --- CHANGELOG.md | 6 ++++++ docs/releaseNotes.yml | 10 ++++++++++ 2 files changed, 16 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 073112bc32..eb4b28b7a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -103,6 +103,12 @@ it will be removed; but as it won't be user-visible this isn't considered a brea - Change: Upgraded Emissary-ingress to the latest release of Golang as part of our general dependency upgrade process. +- Bugfix: Emissary-ingress was incorrectly caching Mappings with regex headers using the header name + instead of the Mapping name, which could reduce the cache's effectiveness. This has been fixed so + that the correct key is used. ([Incorrect Cache Key for Mapping]) + +[Incorrect Cache Key for Mapping]: https://github.com/emissary-ingress/emissary/issues/5714 + ## [3.9.0] November 13, 2023 [3.9.0]: https://github.com/emissary-ingress/emissary/compare/v3.8.0...v3.9.0 diff --git a/docs/releaseNotes.yml b/docs/releaseNotes.yml index 32da6bfed0..47c6dac18b 100644 --- a/docs/releaseNotes.yml +++ b/docs/releaseNotes.yml @@ -58,6 +58,16 @@ items: Upgraded $productName$ to the latest release of Golang as part of our general dependency upgrade process. + - title: Fix internal keying for regex Mappings + type: bugfix + body: >- + $productName$ was incorrectly caching Mappings with regex headers + using the header name instead of the Mapping name, which could + reduce the cache's effectiveness. This has been fixed so that the + correct key is used. + github: + - title: "Incorrect Cache Key for Mapping" + link: https://github.com/emissary-ingress/emissary/issues/5714 - version: 3.9.0 prevVersion: 3.8.0 From 398ef94f50f9743c27eb6979b5cdc38cb78a2223 Mon Sep 17 00:00:00 2001 From: Flynn Date: Fri, 5 Jul 2024 14:52:37 -0400 Subject: [PATCH 03/13] Whitespace changes from my editor cleaning up YAML files... Signed-off-by: Flynn (cherry picked from commit 642c78428c53b9502063cb878f5982b522ad483f) --- CHANGELOG.md | 31 ++++++++++++------------------- docs/releaseNotes.yml | 37 ++++++++++++++++++------------------- 2 files changed, 30 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eb4b28b7a4..69ca6ac350 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -91,7 +91,7 @@ it will be removed; but as it won't be user-visible this isn't considered a brea ### Emissary-ingress and Ambassador Edge Stack - Feature: This upgrades Emissary-ingress to be built on Envoy v1.28.0 which provides security, - performance and feature enhancements. You can read more about them here: Envoy Proxy 1.28.0 Release Notes @@ -115,37 +115,30 @@ it will be removed; but as it won't be user-visible this isn't considered a brea ### Emissary-ingress and Ambassador Edge Stack - Feature: This upgrades Emissary-ingress to be built on Envoy v1.27.2 which provides security, - performance and feature enhancements. You can read more about them here: Envoy Proxy 1.27.2 Release Notes -- Feature: By default, Emissary-ingress will return an `UNAVAILABLE` code when a request using gRPC +- Feature: By default, Emissary-ingress will return an `UNAVAILABLE` code when a request using gRPC is rate limited. The `RateLimitService` resource now exposes a new - `grpc.use_resource_exhausted_code` field that when set to `true`, Emissary-ingress will return a - `RESOURCE_EXHAUSTED` gRPC code instead. Thanks to Jerome + `grpc.use_resource_exhausted_code` field that when set to `true`, Emissary-ingress will return a + `RESOURCE_EXHAUSTED` gRPC code instead. Thanks to Jerome Froelich for contributing this feature! - Feature: Envoy runtime fields that were provided to mitigate the recent HTTP/2 rapid reset - vulnerability can now be configured via the Module resource so the configuration will persist - between restarts. This configuration is added to the Envoy bootstrap config, so restarting - Emissary is necessary after changing these fields for the configuration to take effect. + vulnerability can now be configured via the Module resource so the configuration will persist + between restarts. This configuration is added to the Envoy bootstrap config, so restarting + Emissary is necessary after changing these fields for the configuration to take effect. - Change: APIExt would previously allow for TLS 1.0 connections. We have updated it to now only use - a minimum TLS version of 1.3 to resolve security concerns. + a minimum TLS version of 1.3 to resolve security concerns. - Change: - Update default image to Emissary-ingress v3.9.0.
- Bugfix: The APIExt server provides CRD conversion between the stored version v2 and the version - watched for by Emissary-ingress v3alpha1. Since this component is required to operate - Emissary-ingress, we have introduced an init container that will ensure it is available before - starting. This will help address some of the intermittent issues seen during install and - upgrades. - -- Bugfix: _cache_key is getting generated incorrectly for mappings in ir.json, when using header - with regex in mapping. Always, It should be in the format of {kind}-{version}-{name}-{namespace}. - But header name is getting updated in the place of mapping name, if we created mapping with regex - header. - + watched for by Emissary-ingress v3alpha1. Since this component is required to operate + Emissary-ingress, we have introduced an init container that will ensure it is available before + starting. This will help address some of the intermittent issues seen during install and upgrades. ## [3.8.0] August 29, 2023 [3.8.0]: https://github.com/emissary-ingress/emissary/compare/v3.7.2...v3.8.0 diff --git a/docs/releaseNotes.yml b/docs/releaseNotes.yml index 47c6dac18b..74e66bfb68 100644 --- a/docs/releaseNotes.yml +++ b/docs/releaseNotes.yml @@ -35,15 +35,15 @@ items: - version: 3.10.0 prevVersion: 3.9.0 date: 'TBD' - notes: + notes: - title: Upgrade to Envoy 1.28.0 type: feature body: >- - This upgrades $productName$ to be built on Envoy v1.28.0 which provides security, performance - and feature enhancements. You can read more about them here: + This upgrades $productName$ to be built on Envoy v1.28.0 which provides security, performance + and feature enhancements. You can read more about them here: Envoy Proxy 1.28.0 Release Notes docs: https://www.envoyproxy.io/docs/envoy/v1.28.0/version_history/version_history - + - title: Remove Ambassador Agent from published YAML Manifest type: change body: >- @@ -51,12 +51,11 @@ items: This is an optional component that provides additional features on top of $productName$ and we recommend installing it using the instructions found in the Ambassador Agent Repo. docs: https://github.com/datawire/ambassador-agent - + - title: Update to golang 1.21.5 type: change body: >- Upgraded $productName$ to the latest release of Golang as part of our general dependency upgrade process. - - title: Fix internal keying for regex Mappings type: bugfix @@ -76,34 +75,34 @@ items: - title: Upgrade to Envoy 1.27.2 type: feature body: >- - This upgrades $productName$ to be built on Envoy v1.27.2 which provides security, performance - and feature enhancements. You can read more about them here: + This upgrades $productName$ to be built on Envoy v1.27.2 which provides security, performance + and feature enhancements. You can read more about them here: Envoy Proxy 1.27.2 Release Notes docs: https://www.envoyproxy.io/docs/envoy/v1.27.2/version_history/version_history - title: Added support for RESOURCE_EXHAUSTED responses to grpc clients when rate limited type: feature body: >- - By default, $productName$ will return an UNAVAILABLE code when a request using gRPC - is rate limited. The RateLimitService resource now exposes a new grpc.use_resource_exhausted_code - field that when set to true, $productName$ will return a RESOURCE_EXHAUSTED gRPC code instead. + By default, $productName$ will return an UNAVAILABLE code when a request using gRPC + is rate limited. The RateLimitService resource now exposes a new grpc.use_resource_exhausted_code + field that when set to true, $productName$ will return a RESOURCE_EXHAUSTED gRPC code instead. Thanks to Jerome Froelich for contributing this feature! - title: Added support for setting specific Envoy runtime flags in the Module type: feature body: >- - Envoy runtime fields that were provided to mitigate the recent HTTP/2 rapid reset vulnerability - can now be configured via the Module resource so the configuration will persist between restarts. - This configuration is added to the Envoy bootstrap config, so restarting Emissary is necessary after + Envoy runtime fields that were provided to mitigate the recent HTTP/2 rapid reset vulnerability + can now be configured via the Module resource so the configuration will persist between restarts. + This configuration is added to the Envoy bootstrap config, so restarting Emissary is necessary after changing these fields for the configuration to take effect. - title: Update APIExt minimum TLS version type: change body: >- - APIExt would previously allow for TLS 1.0 connections. We have updated it to now only use a minimum + APIExt would previously allow for TLS 1.0 connections. We have updated it to now only use a minimum TLS version of 1.3 to resolve security concerns. docs: https://www.tenable.com/plugins/nessus/104743 - + - title: Shipped Helm chart v8.9.0 type: change body: >- @@ -113,9 +112,9 @@ items: - title: Ensure APIExt server is available before starting Emissary-ingress type: bugfix body: >- - The APIExt server provides CRD conversion between the stored version v2 and the version watched for - by $productName$ v3alpha1. Since this component is required to operate $productName$, we have - introduced an init container that will ensure it is available before starting. This will help address + The APIExt server provides CRD conversion between the stored version v2 and the version watched for + by $productName$ v3alpha1. Since this component is required to operate $productName$, we have + introduced an init container that will ensure it is available before starting. This will help address some of the intermittent issues seen during install and upgrades. docs: https://artifacthub.io/packages/helm/datawire/edge-stack/$emissaryChartVersion$ From d5216b3a8439fad92aa30e0754aafa4e03526918 Mon Sep 17 00:00:00 2001 From: Flynn Date: Sat, 20 Jul 2024 15:08:01 -0400 Subject: [PATCH 04/13] Updates to the Fake test harness so that we can test things in the IR Signed-off-by: Flynn --- cmd/entrypoint/irtype.go | 64 +++++++++++++++++++ cmd/entrypoint/testutil_fake_test.go | 96 ++++++++++++++++++++++++---- 2 files changed, 148 insertions(+), 12 deletions(-) create mode 100644 cmd/entrypoint/irtype.go diff --git a/cmd/entrypoint/irtype.go b/cmd/entrypoint/irtype.go new file mode 100644 index 0000000000..7458953f6a --- /dev/null +++ b/cmd/entrypoint/irtype.go @@ -0,0 +1,64 @@ +package entrypoint + +type IRMapping struct { + Active bool `json:"_active"` + CacheKey string `json:"_cache_key"` + Errored bool `json:"_errored"` + ReferencedBy []string `json:"_referenced_by"` + RKey string `json:"_rkey"` + Weight int `json:"_weight"` + // Cluster IRCluster `json:"cluster"` + ClusterKey string `json:"cluster_key"` + DefaultClass string `json:"default_class"` + GroupID string `json:"group_id"` + Headers []IRHeader `json:"headers"` + Host string `json:"host"` + Kind string `json:"kind"` + Location string `json:"location"` + Mappings []IRMapping `json:"mappings"` + Name string `json:"name"` + Namespace string `json:"namespace"` + Precedence int `json:"precedence"` + Prefix string `json:"prefix"` + // QueryParameters []IRQueryParameter `json:"query_parameters"` + // RegexRewrite IRRegexRewrite `json:"regex_rewrite"` + Resolver string `json:"resolver"` + Rewrite string `json:"rewrite"` + // RouteWeight IRRouteWeight `json:"route_weight"` + Service string `json:"service"` + TimeoutMS int `json:"timeout_ms"` +} + +type IRHeader struct { + Name string `json:"name"` + Regex bool `json:"regex"` + Value string `json:"value"` +} + +type IRGroup struct { + Active bool `json:"_active"` + CacheKey string `json:"_cache_key"` + DefaultClass string `json:"default_class"` + Errored bool `json:"_errored"` + ReferencedBy []string `json:"_referenced_by"` + RKey string `json:"_rkey"` + GroupID string `json:"group_id"` + Headers []IRHeader `json:"headers"` + Host string `json:"host"` + Kind string `json:"kind"` + Location string `json:"location"` + Mappings []IRMapping `json:"mappings"` + Name string `json:"name"` + Namespace string `json:"namespace"` + Precedence int `json:"precedence"` + Prefix string `json:"prefix"` + // QueryParameters []IRQueryParameter `json:"query_parameters"` + // RegexRewrite IRRegexRewrite `json:"regex_rewrite"` + Rewrite string `json:"rewrite"` + // RouteWeight IRRouteWeight `json:"route_weight"` + TimeoutMS int `json:"timeout_ms"` +} + +type IR struct { + Groups []IRGroup `json:"groups"` +} diff --git a/cmd/entrypoint/testutil_fake_test.go b/cmd/entrypoint/testutil_fake_test.go index 6569cb67e7..8f50870cea 100644 --- a/cmd/entrypoint/testutil_fake_test.go +++ b/cmd/entrypoint/testutil_fake_test.go @@ -4,9 +4,10 @@ import ( "context" "encoding/json" "fmt" - "io/ioutil" + "io" "net/http" "os" + "path/filepath" "reflect" "sync" "sync/atomic" @@ -74,6 +75,7 @@ type Fake struct { fastpath *testqueue.Queue // All fastpath snapshots that have been produced. snapshots *testqueue.Queue // All snapshots that have been produced. + irs *testqueue.Queue // All IRs that have been produced envoyConfigs *testqueue.Queue // All envoyConfigs that have been produced. // This is used to make Teardown idempotent. @@ -89,12 +91,17 @@ type FakeConfig struct { EnvoyConfig bool // If true then the Fake will produce envoy configs in addition to Snapshots. DiagdDebug bool // If true then diagd will have debugging enabled Timeout time.Duration // How long to wait for snapshots and/or envoy configs to become available. + OutputDir string // Directory for Emissary's output files } func (fc *FakeConfig) fillDefaults() { if fc.Timeout == 0 { fc.Timeout = 10 * time.Second } + + if fc.OutputDir == "" { + fc.OutputDir = "/tmp/mock" + } } // NewFake will construct a new Fake object. See RunFake for a convenient way to handle construct, @@ -120,6 +127,7 @@ func NewFake(t *testing.T, config FakeConfig) *Fake { fastpath: testqueue.NewQueue(t, config.Timeout), snapshots: testqueue.NewQueue(t, config.Timeout), + irs: testqueue.NewQueue(t, config.Timeout), envoyConfigs: testqueue.NewQueue(t, config.Timeout), } @@ -157,12 +165,18 @@ func (f *Fake) Setup() { f.DiagdBindPort = GetDiagdBindPort() + err = os.MkdirAll(f.config.OutputDir, os.ModePerm) + + if err != nil { + f.T.Fatalf("failed to create directory: %v", err) + } + f.group.Go("diagd", func(ctx context.Context) error { args := []string{ "diagd", - "/tmp", - "/tmp/bootstrap-ads.json", - "/tmp/envoy.json", + f.config.OutputDir, + filepath.Join(f.config.OutputDir, "bootstrap-ads.json"), + filepath.Join(f.config.OutputDir, "envoy.json"), "--no-envoy", "--host", "127.0.0.1", "--port", f.DiagdBindPort, @@ -230,7 +244,7 @@ func (f *Fake) GetFeatures(ctx context.Context, features interface{}) error { return fmt.Errorf("unexpected status code: %d", resp.StatusCode) } - body, err := ioutil.ReadAll(resp.Body) + body, err := io.ReadAll(resp.Body) if err != nil { return err @@ -299,9 +313,18 @@ func (f *Fake) AssertEndpointsEmpty(timeout time.Duration) { f.fastpath.AssertEmpty(f.T, timeout, "endpoints queue not empty") } +// SnapshotEntry contains a snapshot (the result of discovery), its +// corresponding IR and Envoy configs (if available, see below), and the +// disposition of the snapshot. +// +// The IR and Envoy configs will be available _only_ if the Fake harness was +// configured with EnvoyConfig set to true, and they are likely to be +// meaningful only if the disposition is SnapshotReady. type SnapshotEntry struct { - Disposition SnapshotDisposition - Snapshot *snapshot.Snapshot + Disposition SnapshotDisposition + Snapshot *snapshot.Snapshot + IRText []byte + EnvoyConfigText []byte } func (entry SnapshotEntry) String() string { @@ -314,20 +337,28 @@ func (entry SnapshotEntry) String() string { // We pass this into the watcher loop to get notified when a snapshot is produced. func (f *Fake) notifySnapshot(ctx context.Context, disp SnapshotDisposition, snapJSON []byte) error { + var envoyConfigText []byte + var irText []byte + if disp == SnapshotReady && f.config.EnvoyConfig { if err := notifyReconfigWebhooks(ctx, &noopNotable{}); err != nil { return err } - f.appendEnvoyConfig(ctx) + + irText = f.appendIR() + envoyConfigText = f.appendEnvoyConfig(ctx) } var snap *snapshot.Snapshot err := json.Unmarshal(snapJSON, &snap) if err != nil { - f.T.Fatalf("error decoding snapshot: %+v", err) + f.T.Errorf("error decoding snapshot: %+v", err) + return err } - f.snapshots.Add(f.T, SnapshotEntry{disp, snap}) + entry := SnapshotEntry{disp, snap, irText, envoyConfigText} + f.snapshots.Add(f.T, entry) + return nil } @@ -356,13 +387,54 @@ func (f *Fake) GetSnapshot(predicate func(*snapshot.Snapshot) bool) (*snapshot.S return entry.Snapshot, nil } -func (f *Fake) appendEnvoyConfig(ctx context.Context) { - msg, err := ambex.Decode(ctx, "/tmp/envoy.json") +func (f *Fake) appendIR() []byte { + irJSON, err := os.ReadFile(filepath.Join(f.config.OutputDir, "ir.json")) + if err != nil { + f.T.Fatalf("error reading ir.json: %+v", err) + } + + var ir IR + err = json.Unmarshal(irJSON, &ir) + if err != nil { + f.T.Fatalf("error unmarshaling ir.json: %+v", err) + } + + f.irs.Add(f.T, &ir) + + // Save the text of the IR, too, in with the snapshot. + return irJSON +} + +// GetIR will return the next IR that satisfies the supplied predicate. +func (f *Fake) GetIR(predicate func(*IR) bool) (*IR, error) { + f.T.Helper() + untyped, err := f.irs.Get(f.T, func(obj interface{}) bool { + ir := obj.(*IR) + return predicate(ir) + }) + if err != nil { + return nil, err + } + return untyped.(*IR), nil +} + +func (f *Fake) appendEnvoyConfig(ctx context.Context) []byte { + msg, err := ambex.Decode(ctx, filepath.Join(f.config.OutputDir, "envoy.json")) if err != nil { f.T.Fatalf("error decoding envoy.json after sending snapshot to python: %+v", err) } + bs := msg.(*v3bootstrap.Bootstrap) f.envoyConfigs.Add(f.T, bs) + + // Save the text of the Envoy config, too, in with the snapshot. + envoyConfig, err := os.ReadFile(filepath.Join(f.config.OutputDir, "envoy.json")) + + if err != nil { + f.T.Fatalf("error reading envoy.json: %+v", err) + } + + return envoyConfig } // GetEnvoyConfig will return the next envoy config that satisfies the supplied predicate. From 4878a9890e1b299b462f15c9da5b0106ecb24a99 Mon Sep 17 00:00:00 2001 From: Flynn Date: Sat, 13 Jul 2024 23:18:22 -0400 Subject: [PATCH 05/13] More work on irtype.go. Have irtype_test.go, too. Signed-off-by: Flynn --- cmd/entrypoint/irtype.go | 207 ++++++++++++++++++++++++++-------- cmd/entrypoint/irtype_test.go | 153 +++++++++++++++++++++++++ 2 files changed, 313 insertions(+), 47 deletions(-) create mode 100644 cmd/entrypoint/irtype_test.go diff --git a/cmd/entrypoint/irtype.go b/cmd/entrypoint/irtype.go index 7458953f6a..90e57cfcb8 100644 --- a/cmd/entrypoint/irtype.go +++ b/cmd/entrypoint/irtype.go @@ -1,32 +1,151 @@ package entrypoint -type IRMapping struct { +import ( + "encoding/json" + "fmt" +) + +type IRResource struct { Active bool `json:"_active"` - CacheKey string `json:"_cache_key"` + CacheKey string `json:"_cache_key,omitempty"` Errored bool `json:"_errored"` - ReferencedBy []string `json:"_referenced_by"` - RKey string `json:"_rkey"` - Weight int `json:"_weight"` - // Cluster IRCluster `json:"cluster"` - ClusterKey string `json:"cluster_key"` - DefaultClass string `json:"default_class"` - GroupID string `json:"group_id"` - Headers []IRHeader `json:"headers"` - Host string `json:"host"` - Kind string `json:"kind"` - Location string `json:"location"` - Mappings []IRMapping `json:"mappings"` - Name string `json:"name"` - Namespace string `json:"namespace"` - Precedence int `json:"precedence"` - Prefix string `json:"prefix"` - // QueryParameters []IRQueryParameter `json:"query_parameters"` - // RegexRewrite IRRegexRewrite `json:"regex_rewrite"` - Resolver string `json:"resolver"` - Rewrite string `json:"rewrite"` - // RouteWeight IRRouteWeight `json:"route_weight"` - Service string `json:"service"` - TimeoutMS int `json:"timeout_ms"` + ReferencedBy []string `json:"_referenced_by,omitempty"` + RKey string `json:"_rkey,omitempty"` + Location string `json:"location,omitempty"` + Kind string `json:"kind"` + Name string `json:"name"` + Namespace string `json:"namespace,omitempty"` +} + +type IRClusterHealthCheck struct { + IRResource +} + +type IRClusterTarget struct { + IP string `json:"ip"` + Port int `json:"port"` + TargetKind string `json:"target_kind"` +} + +type IRCluster struct { + IRResource + BarHostname string `json:"_hostname"` // Why this _and_ hostname? + BarNamespace string `json:"_namespace"` // Why this _and_ namespace? + Port int `json:"_port"` + Resolver string `json:"_resolver"` + ConnectTimeoutMs int `json:"connect_timeout_ms"` + EnableEndpoints bool `json:"enable_endpoints"` + EnableIPv4 bool `json:"enable_ipv4"` + EnableIPv6 bool `json:"enable_ipv6"` + EnvoyName string `json:"envoy_name"` + HealthChecks IRClusterHealthCheck `json:"health_checks,omitempty"` + IgnoreCluster bool `json:"ignore_cluster"` + LBType string `json:"lb_type"` + RespectDNSTTL bool `json:"respect_dns_ttl"` + Service string `json:"service"` + StatsName string `json:"stats_name"` + Targets []IRClusterTarget `json:"targets"` + Type string `json:"type"` + URLs []string `json:"urls"` +} + +type IRQueryParameter struct { + Name string `json:"name,omitempty"` + Value string `json:"value,omitempty"` + Regex bool `json:"regex,omitempty"` +} + +type IRRegexRewrite struct { + Pattern string `json:"pattern,omitempty"` + Substitution string `json:"substitution,omitempty"` +} + +// Route weights are really annoying: in Python they're a +// List[Union[str, int]], which is a pain to represent in Go. + +type IRRouteWeightElement struct { + Int int + Str string +} + +type IRRouteWeight []IRRouteWeightElement + +func (rw IRRouteWeight) MarshalJSON() ([]byte, error) { + arr := make([]interface{}, len(rw)) + + for i, elem := range rw { + if elem.Str != "" { + arr[i] = elem.Str + } else { + arr[i] = elem.Int + } + } + + return json.Marshal(arr) +} + +func (rw *IRRouteWeight) UnmarshalJSON(data []byte) error { + var arr []interface{} + + if err := json.Unmarshal(data, &arr); err != nil { + return err + } + + *rw = make([]IRRouteWeightElement, len(arr)) + + for i, elem := range arr { + switch v := elem.(type) { + case string: + (*rw)[i] = IRRouteWeightElement{Str: v} + case float64: + (*rw)[i] = IRRouteWeightElement{Int: int(v)} + default: + return fmt.Errorf("unexpected type in IRRouteWeight: %T", elem) + } + } + + return nil +} + +type IRMapping struct { + IRResource + // CumulativeWeight is the _computed_ cumulative weight assigned to a + // Mapping within its groups; Weight is the value specified in the + // Mapping's spec.weight (it's a pointer because that value is optional). + // + // If you have mappings in a Group with weights 10, 40, and unset, the + // Weights will be pointer-to-10, pointer-to-40, and nil, and the + // CumulativeWeights will be 10, 50, and 100. + CumulativeWeight int `json:"_weight"` + Weight *int `json:"weight"` + Cluster IRCluster `json:"cluster"` + ClusterKey string `json:"cluster_key"` + DefaultClass string `json:"default_class"` + GroupID string `json:"group_id"` + Headers []IRHeader `json:"headers"` + Host string `json:"host"` + Precedence int `json:"precedence"` + Prefix string `json:"prefix"` + QueryParameters []IRQueryParameter `json:"query_parameters,omitempty"` + RegexRewrite IRRegexRewrite `json:"regex_rewrite,omitempty"` + Resolver string `json:"resolver"` + Rewrite string `json:"rewrite"` + RouteWeight IRRouteWeight `json:"route_weight"` + Service string `json:"service"` + TimeoutMS int `json:"timeout_ms"` +} + +type IRRequestPolicy struct { + Action string `json:"action"` +} + +type IRHost struct { + IRResource + Hostname string `json:"hostname"` + InsecureAction string `json:"insecure_action"` + RequestPolicy map[string]IRRequestPolicy `json:"requestPolicy"` // Yes, really. + SecureAction string `json:"secure_action"` + SNI string `json:"sni"` } type IRHeader struct { @@ -36,29 +155,23 @@ type IRHeader struct { } type IRGroup struct { - Active bool `json:"_active"` - CacheKey string `json:"_cache_key"` - DefaultClass string `json:"default_class"` - Errored bool `json:"_errored"` - ReferencedBy []string `json:"_referenced_by"` - RKey string `json:"_rkey"` - GroupID string `json:"group_id"` - Headers []IRHeader `json:"headers"` - Host string `json:"host"` - Kind string `json:"kind"` - Location string `json:"location"` - Mappings []IRMapping `json:"mappings"` - Name string `json:"name"` - Namespace string `json:"namespace"` - Precedence int `json:"precedence"` - Prefix string `json:"prefix"` - // QueryParameters []IRQueryParameter `json:"query_parameters"` - // RegexRewrite IRRegexRewrite `json:"regex_rewrite"` - Rewrite string `json:"rewrite"` - // RouteWeight IRRouteWeight `json:"route_weight"` - TimeoutMS int `json:"timeout_ms"` + IRResource + DefaultClass string `json:"default_class"` + GroupID string `json:"group_id"` + GroupWeight IRRouteWeight `json:"group_weight"` + Headers []IRHeader `json:"headers"` + Host string `json:"host"` + Mappings []IRMapping `json:"mappings"` + Precedence int `json:"precedence"` + Prefix string `json:"prefix"` + QueryParameters []IRQueryParameter `json:"query_parameters"` + RegexRewrite IRRegexRewrite `json:"regex_rewrite"` + Rewrite string `json:"rewrite"` + TimeoutMS int `json:"timeout_ms"` } type IR struct { - Groups []IRGroup `json:"groups"` + Clusters map[string]IRCluster `json:"clusters"` + Groups []IRGroup `json:"groups"` + Hosts []IRHost `json:"hosts"` } diff --git a/cmd/entrypoint/irtype_test.go b/cmd/entrypoint/irtype_test.go new file mode 100644 index 0000000000..6488a7e9b5 --- /dev/null +++ b/cmd/entrypoint/irtype_test.go @@ -0,0 +1,153 @@ +package entrypoint_test + +import ( + "encoding/json" + "testing" + + "github.com/emissary-ingress/emissary/v3/cmd/entrypoint" + "github.com/stretchr/testify/assert" +) + +func TestIRRouteWeight(t *testing.T) { + rw := entrypoint.IRRouteWeight{ + {Int: 1}, + {Str: "foo"}, + {Int: 2}, + } + + // MarshalJSON + expectedJSON := `[1,"foo",2]` + actualJSON, err := json.Marshal(rw) + assert.Nil(t, err) + assert.Equal(t, expectedJSON, string(actualJSON)) + + var check entrypoint.IRRouteWeight + err = json.Unmarshal(actualJSON, &check) + assert.Nil(t, err) + assert.Equal(t, rw, check) + + // UnmarshalJSON + jsonData := []byte(`[1,"bar",3]`) + expectedRW := entrypoint.IRRouteWeight{ + {Int: 1}, + {Str: "bar"}, + {Int: 3}, + } + var actualRW entrypoint.IRRouteWeight + err = json.Unmarshal(jsonData, &actualRW) + assert.Nil(t, err) + assert.Equal(t, expectedRW, actualRW) +} + +func TestIRCluster(t *testing.T) { + clusterJSON := `{ + "_active": true, + "_cache_key": "Cluster-cluster_127_0_0_1_8877_default", + "_errored": false, + "_hostname": "127.0.0.1", + "_namespace": "default", + "_port": 8877, + "_referenced_by": [ + "--internal--" + ], + "_resolver": "kubernetes-service", + "_rkey": "cluster_127_0_0_1_8877_default", + "connect_timeout_ms": 3000, + "enable_endpoints": false, + "enable_ipv4": true, + "enable_ipv6": false, + "envoy_name": "cluster_127_0_0_1_8877_default", + "health_checks": { + "_active": true, + "_errored": false, + "_rkey": "ir.health_checks", + "kind": "IRHealthChecks", + "location": "--internal--", + "name": "health_checks", + "namespace": "default" + }, + "ignore_cluster": false, + "kind": "IRCluster", + "lb_type": "round_robin", + "location": "--internal--", + "name": "cluster_127_0_0_1_8877_default", + "namespace": "default", + "respect_dns_ttl": false, + "service": "127.0.0.1:8877", + "stats_name": "127_0_0_1_8877", + "targets": [ + { + "ip": "127.0.0.1", + "port": 8877, + "target_kind": "IPaddr" + } + ], + "type": "strict_dns", + "urls": [ + "tcp://127.0.0.1:8877" + ] + }` + + expectedCluster := entrypoint.IRCluster{ + IRResource: entrypoint.IRResource{ + Active: true, + CacheKey: "Cluster-cluster_127_0_0_1_8877_default", + Errored: false, + ReferencedBy: []string{"--internal--"}, + RKey: "cluster_127_0_0_1_8877_default", + Location: "--internal--", + Kind: "IRCluster", + Name: "cluster_127_0_0_1_8877_default", + Namespace: "default", + }, + BarHostname: "127.0.0.1", + BarNamespace: "default", + Port: 8877, + Resolver: "kubernetes-service", + ConnectTimeoutMs: 3000, + EnableEndpoints: false, + EnableIPv4: true, + EnableIPv6: false, + EnvoyName: "cluster_127_0_0_1_8877_default", + HealthChecks: entrypoint.IRClusterHealthCheck{ + IRResource: entrypoint.IRResource{ + Active: true, + Errored: false, + RKey: "ir.health_checks", + Kind: "IRHealthChecks", + Location: "--internal--", + Name: "health_checks", + Namespace: "default", + }, + }, + IgnoreCluster: false, + LBType: "round_robin", + RespectDNSTTL: false, + Service: "127.0.0.1:8877", + StatsName: "127_0_0_1_8877", + Targets: []entrypoint.IRClusterTarget{ + { + IP: "127.0.0.1", + Port: 8877, + TargetKind: "IPaddr", + }, + }, + Type: "strict_dns", + URLs: []string{ + "tcp://127.0.0.1:8877", + }, + } + + var unmarshaledCluster entrypoint.IRCluster + err := json.Unmarshal([]byte(clusterJSON), &unmarshaledCluster) + assert.Nil(t, err) + assert.Equal(t, expectedCluster, unmarshaledCluster) + + remarshaledJSON, err := json.Marshal(unmarshaledCluster) + assert.Nil(t, err) + + var unmarshaledCluster2 entrypoint.IRCluster + err = json.Unmarshal(remarshaledJSON, &unmarshaledCluster2) + assert.Nil(t, err) + assert.Equal(t, expectedCluster, unmarshaledCluster2) +} From 4458a71013fb08f8bbfb417354d4ad510889f670 Mon Sep 17 00:00:00 2001 From: Sekar Saravanan Date: Tue, 16 Jul 2024 12:21:38 +0530 Subject: [PATCH 06/13] issue-5702 - Multiple duplicate mappings generated for single mapping resource fixed Signed-off-by: Sekar Saravanan --- python/ambassador/ir/ir.py | 8 +++++++- python/ambassador_diag/diagd.py | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/python/ambassador/ir/ir.py b/python/ambassador/ir/ir.py index d0996a626d..bfb8b8a062 100644 --- a/python/ambassador/ir/ir.py +++ b/python/ambassador/ir/ir.py @@ -817,7 +817,13 @@ def add_mapping(self, aconf: Config, mapping: IRBaseMapping) -> Optional[IRBaseM else: self.logger.debug(f"IR: already have group for {mapping.name}") group = self.groups[mapping.group_id] - group.add_mapping(aconf, mapping) + + # Add mapping into the group only if the _cache_key doesn't exist in a group. + existing_mapping_cache_keys = [ group_mapping["_cache_key"] for group_mapping in group["mappings"] if "_cache_key" in group_mapping ] + if mapping["_cache_key"] in existing_mapping_cache_keys: + self.logger.debug(f"IR: _cache_key for {mapping.name} is {mapping['_cache_key']} already exists in a group.") + else: + group.add_mapping(aconf, mapping) self.cache_add(mapping) self.cache_add(group) diff --git a/python/ambassador_diag/diagd.py b/python/ambassador_diag/diagd.py index c93b30534f..316a49ade8 100644 --- a/python/ambassador_diag/diagd.py +++ b/python/ambassador_diag/diagd.py @@ -551,7 +551,7 @@ def check_cache(self) -> bool: result = False self.logger.error("CACHE: ENVOY CONFIG MISMATCH") errors += "econf diffs:\n" - errors += self.json_diff("econf", i1, i2) + errors += self.json_diff("econf", e1, e2) if not result: err_path = os.path.join(self.snapshot_path, "diff-tmp.txt") From 70c3ecbb783a3cddc381bc87857013de64661202 Mon Sep 17 00:00:00 2001 From: Flynn Date: Sat, 13 Jul 2024 17:49:35 -0400 Subject: [PATCH 07/13] Test for the issue 5702 unrelated-mappings bug. Huge props to @sekar-saravanan for finding this bug in the first place. Signed-off-by: Flynn --- .../testdata/unrelated-mappings/host.yaml | 12 +++ .../testdata/unrelated-mappings/mapping.yaml | 33 +++++++ .../testdata/unrelated-mappings/service.yaml | 37 ++++++++ .../unrelated-mappings/unrelated.yaml | 16 ++++ cmd/entrypoint/unrelated_mappings_test.go | 93 +++++++++++++++++++ 5 files changed, 191 insertions(+) create mode 100644 cmd/entrypoint/testdata/unrelated-mappings/host.yaml create mode 100644 cmd/entrypoint/testdata/unrelated-mappings/mapping.yaml create mode 100644 cmd/entrypoint/testdata/unrelated-mappings/service.yaml create mode 100644 cmd/entrypoint/testdata/unrelated-mappings/unrelated.yaml create mode 100644 cmd/entrypoint/unrelated_mappings_test.go diff --git a/cmd/entrypoint/testdata/unrelated-mappings/host.yaml b/cmd/entrypoint/testdata/unrelated-mappings/host.yaml new file mode 100644 index 0000000000..2356c0c662 --- /dev/null +++ b/cmd/entrypoint/testdata/unrelated-mappings/host.yaml @@ -0,0 +1,12 @@ +apiVersion: getambassador.io/v2 +kind: Host +metadata: + name: origin-test-app-emissariy.spike-pp.test-labs.com + namespace: infrastructure +spec: + ambassador_id: + - --apiVersion-v3alpha1-only--default + hostname: origin-test-app-emissariy.spike-pp.test-labs.com + requestPolicy: + insecure: + action: Route diff --git a/cmd/entrypoint/testdata/unrelated-mappings/mapping.yaml b/cmd/entrypoint/testdata/unrelated-mappings/mapping.yaml new file mode 100644 index 0000000000..842e97a9d9 --- /dev/null +++ b/cmd/entrypoint/testdata/unrelated-mappings/mapping.yaml @@ -0,0 +1,33 @@ + +apiVersion: getambassador.io/v2 +kind: Mapping +metadata: + labels: + app: workload1 + name: workload1-mapping + namespace: infrastructure +spec: + bypass_auth: true + host: test.example.com + prefix: / + rewrite: "" + service: workload1 + timeout_ms: 3000 + +--- + +apiVersion: getambassador.io/v2 +kind: Mapping +metadata: + labels: + app: workload2 + name: workload2-mapping + namespace: infrastructure +spec: + bypass_auth: true + host: test.example.com + prefix: / + rewrite: "" + service: workload2 + timeout_ms: 3000 + weight: 10 diff --git a/cmd/entrypoint/testdata/unrelated-mappings/service.yaml b/cmd/entrypoint/testdata/unrelated-mappings/service.yaml new file mode 100644 index 0000000000..bcd10c0ddb --- /dev/null +++ b/cmd/entrypoint/testdata/unrelated-mappings/service.yaml @@ -0,0 +1,37 @@ +apiVersion: v1 +kind: Service +metadata: + labels: + app: test-app-emissariy + name: test-app-emissariy-default + name: test-app-emissariy-default + namespace: infrastructure +spec: + ports: + - name: "80" + port: 80 + protocol: TCP + targetPort: 80 + selector: + app: test-app-emissariy + type: ClusterIP + +--- + +apiVersion: v1 +kind: Service +metadata: + labels: + app: canary-test-app-emissariy + name: canary-test-app-emissariy-default + name: canary-test-app-emissariy-default + namespace: infrastructure +spec: + ports: + - name: "80" + port: 80 + protocol: TCP + targetPort: 80 + selector: + app: canary-test-app-emissariy + type: ClusterIP \ No newline at end of file diff --git a/cmd/entrypoint/testdata/unrelated-mappings/unrelated.yaml b/cmd/entrypoint/testdata/unrelated-mappings/unrelated.yaml new file mode 100644 index 0000000000..b9b022668f --- /dev/null +++ b/cmd/entrypoint/testdata/unrelated-mappings/unrelated.yaml @@ -0,0 +1,16 @@ +--- +apiVersion: getambassador.io/v2 +kind: Mapping +metadata: + labels: + app: unrelated + name: unrelated + namespace: infrastructure +spec: + bypass_auth: true + host: unrelated.spike-pp.test-labs.com + prefix: / + resolver: endpoint + rewrite: "" + service: unrelated + timeout_ms: 3000 diff --git a/cmd/entrypoint/unrelated_mappings_test.go b/cmd/entrypoint/unrelated_mappings_test.go new file mode 100644 index 0000000000..fed7cafdd9 --- /dev/null +++ b/cmd/entrypoint/unrelated_mappings_test.go @@ -0,0 +1,93 @@ +package entrypoint_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/emissary-ingress/emissary/v3/cmd/entrypoint" +) + +func getWorkload1MappingGroup(ir *entrypoint.IR) (*entrypoint.IRGroup, bool) { + for _, group := range ir.Groups { + if group.Name == "GROUP: workload1-mapping" { + return &group, true + } + } + + return nil, false +} + +func predicate(ir *entrypoint.IR) bool { + _, ok := getWorkload1MappingGroup(ir) + + return ok +} + +// The Fake struct is a test harness for Emissary. See testutil_fake_test.go +// for details. Note that this test depends on diagd being in your path. If +// diagd is not available, the test will be skipped. + +func TestUnrelatedMappings(t *testing.T) { + // Use RunFake() to spin up the fake control plane, and note that we + // _must_ set EnvoyConfig true to do anything with IR. We need the IR + // for this test, so... + f := entrypoint.RunFake(t, entrypoint.FakeConfig{EnvoyConfig: true}, nil) + + // Next up, upsert our test data. + assert.NoError(t, f.UpsertFile("testdata/unrelated-mappings/service.yaml")) + assert.NoError(t, f.UpsertFile("testdata/unrelated-mappings/host.yaml")) + assert.NoError(t, f.UpsertFile("testdata/unrelated-mappings/mapping.yaml")) + + // Flush the Fake harness so that we get a configuration. + f.Flush() + + // We need the IR from that configuration. + ir, err := f.GetIR(predicate) + require.NoError(t, err) + + // Now we can check the IR. + checkIR(ir, t) + + // Next up, upsert a completely unrelated mapping. This mustn't affect + // our existing group. + assert.NoError(t, f.UpsertFile("testdata/unrelated-mappings/unrelated.yaml")) + + // Flush the Fake harness and repeat our IR check. + f.Flush() + ir, err = f.GetIR(predicate) + require.NoError(t, err) + checkIR(ir, t) +} + +func checkIR(ir *entrypoint.IR, t *testing.T) { + // In the IR, we should find a group called "workload1-mapping". + group, ok := getWorkload1MappingGroup(ir) + require.True(t, ok) + + // That group should have two mappings. + require.Len(t, group.Mappings, 2) + + // One mapping should have a "name" of "workload1-mapping" and a + // cumulative weight of 100; the other should have a "name" of + // "workload2-mapping" and a cumulative weight of 10. + found1 := false + found2 := false + + for _, mapping := range group.Mappings { + switch mapping.Name { + case "workload1-mapping": + assert.Equal(t, 100, mapping.CumulativeWeight) + found1 = true + case "workload2-mapping": + assert.Equal(t, 10, mapping.CumulativeWeight) + found2 = true + default: + t.Fatalf("unexpected mapping: %#v", mapping) + } + } + + assert.True(t, found1) + assert.True(t, found2) +} From 0b7da69598b18e0f4682b2feac00b1972bba40f1 Mon Sep 17 00:00:00 2001 From: Flynn Date: Fri, 19 Jul 2024 12:10:55 -0400 Subject: [PATCH 08/13] Move checkIR Signed-off-by: Flynn --- cmd/entrypoint/unrelated_mappings_test.go | 62 +++++++++++------------ 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/cmd/entrypoint/unrelated_mappings_test.go b/cmd/entrypoint/unrelated_mappings_test.go index fed7cafdd9..ad3e8477cf 100644 --- a/cmd/entrypoint/unrelated_mappings_test.go +++ b/cmd/entrypoint/unrelated_mappings_test.go @@ -25,6 +25,37 @@ func predicate(ir *entrypoint.IR) bool { return ok } +func checkIR(ir *entrypoint.IR, t *testing.T) { + // In the IR, we should find a group called "workload1-mapping". + group, ok := getWorkload1MappingGroup(ir) + require.True(t, ok) + + // That group should have two mappings. + require.Len(t, group.Mappings, 2) + + // One mapping should have a "name" of "workload1-mapping" and a + // cumulative weight of 100; the other should have a "name" of + // "workload2-mapping" and a cumulative weight of 10. + found1 := false + found2 := false + + for _, mapping := range group.Mappings { + switch mapping.Name { + case "workload1-mapping": + assert.Equal(t, 100, mapping.CumulativeWeight) + found1 = true + case "workload2-mapping": + assert.Equal(t, 10, mapping.CumulativeWeight) + found2 = true + default: + t.Fatalf("unexpected mapping: %#v", mapping) + } + } + + assert.True(t, found1) + assert.True(t, found2) +} + // The Fake struct is a test harness for Emissary. See testutil_fake_test.go // for details. Note that this test depends on diagd being in your path. If // diagd is not available, the test will be skipped. @@ -60,34 +91,3 @@ func TestUnrelatedMappings(t *testing.T) { require.NoError(t, err) checkIR(ir, t) } - -func checkIR(ir *entrypoint.IR, t *testing.T) { - // In the IR, we should find a group called "workload1-mapping". - group, ok := getWorkload1MappingGroup(ir) - require.True(t, ok) - - // That group should have two mappings. - require.Len(t, group.Mappings, 2) - - // One mapping should have a "name" of "workload1-mapping" and a - // cumulative weight of 100; the other should have a "name" of - // "workload2-mapping" and a cumulative weight of 10. - found1 := false - found2 := false - - for _, mapping := range group.Mappings { - switch mapping.Name { - case "workload1-mapping": - assert.Equal(t, 100, mapping.CumulativeWeight) - found1 = true - case "workload2-mapping": - assert.Equal(t, 10, mapping.CumulativeWeight) - found2 = true - default: - t.Fatalf("unexpected mapping: %#v", mapping) - } - } - - assert.True(t, found1) - assert.True(t, found2) -} From 98a7cd0ab98ee4fa639e2b5728a0553224482710 Mon Sep 17 00:00:00 2001 From: Flynn Date: Sat, 20 Jul 2024 14:06:19 -0400 Subject: [PATCH 09/13] Move Flush and GetIR into checkIR for ease of use Signed-off-by: Flynn --- cmd/entrypoint/unrelated_mappings_test.go | 37 +++++++++++------------ 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/cmd/entrypoint/unrelated_mappings_test.go b/cmd/entrypoint/unrelated_mappings_test.go index ad3e8477cf..e98ba93710 100644 --- a/cmd/entrypoint/unrelated_mappings_test.go +++ b/cmd/entrypoint/unrelated_mappings_test.go @@ -25,13 +25,20 @@ func predicate(ir *entrypoint.IR) bool { return ok } -func checkIR(ir *entrypoint.IR, t *testing.T) { +func checkIR(f *entrypoint.Fake) { + // Flush the Fake harness so that we get a configuration. + f.Flush() + + // We need the IR from that configuration. + ir, err := f.GetIR(predicate) + require.NoError(f.T, err) + // In the IR, we should find a group called "workload1-mapping". group, ok := getWorkload1MappingGroup(ir) - require.True(t, ok) + require.True(f.T, ok) // That group should have two mappings. - require.Len(t, group.Mappings, 2) + require.Len(f.T, group.Mappings, 2) // One mapping should have a "name" of "workload1-mapping" and a // cumulative weight of 100; the other should have a "name" of @@ -42,18 +49,18 @@ func checkIR(ir *entrypoint.IR, t *testing.T) { for _, mapping := range group.Mappings { switch mapping.Name { case "workload1-mapping": - assert.Equal(t, 100, mapping.CumulativeWeight) + assert.Equal(f.T, 100, mapping.CumulativeWeight) found1 = true case "workload2-mapping": - assert.Equal(t, 10, mapping.CumulativeWeight) + assert.Equal(f.T, 10, mapping.CumulativeWeight) found2 = true default: - t.Fatalf("unexpected mapping: %#v", mapping) + f.T.Fatalf("unexpected mapping: %#v", mapping) } } - assert.True(t, found1) - assert.True(t, found2) + assert.True(f.T, found1) + assert.True(f.T, found2) } // The Fake struct is a test harness for Emissary. See testutil_fake_test.go @@ -71,23 +78,13 @@ func TestUnrelatedMappings(t *testing.T) { assert.NoError(t, f.UpsertFile("testdata/unrelated-mappings/host.yaml")) assert.NoError(t, f.UpsertFile("testdata/unrelated-mappings/mapping.yaml")) - // Flush the Fake harness so that we get a configuration. - f.Flush() - - // We need the IR from that configuration. - ir, err := f.GetIR(predicate) - require.NoError(t, err) - // Now we can check the IR. - checkIR(ir, t) + checkIR(f) // Next up, upsert a completely unrelated mapping. This mustn't affect // our existing group. assert.NoError(t, f.UpsertFile("testdata/unrelated-mappings/unrelated.yaml")) // Flush the Fake harness and repeat our IR check. - f.Flush() - ir, err = f.GetIR(predicate) - require.NoError(t, err) - checkIR(ir, t) + checkIR(f) } From 443a1388c030ba68fb735e28b3d118b32301cf94 Mon Sep 17 00:00:00 2001 From: Flynn Date: Sat, 20 Jul 2024 14:12:13 -0400 Subject: [PATCH 10/13] Make checkIR check both kinds of weights, and improve its error messages Signed-off-by: Flynn --- cmd/entrypoint/unrelated_mappings_test.go | 116 +++++++++++++++++----- 1 file changed, 93 insertions(+), 23 deletions(-) diff --git a/cmd/entrypoint/unrelated_mappings_test.go b/cmd/entrypoint/unrelated_mappings_test.go index e98ba93710..ae3fa96cea 100644 --- a/cmd/entrypoint/unrelated_mappings_test.go +++ b/cmd/entrypoint/unrelated_mappings_test.go @@ -1,6 +1,7 @@ package entrypoint_test import ( + "fmt" "testing" "github.com/stretchr/testify/assert" @@ -25,7 +26,26 @@ func predicate(ir *entrypoint.IR) bool { return ok } -func checkIR(f *entrypoint.Fake) { +type WeightCheck struct { + weight *int + cumulative int +} + +func NewWeightCheck(weight int, cumulative int) WeightCheck { + weightPtr := &weight + + if weight < 0 { + weightPtr = nil + } + + return WeightCheck{weight: weightPtr, cumulative: cumulative} +} + +// checkIR is a helper function that flushes the world, gets an IR, then +// checks the IR for the expected state. It'll find the "workload1-mapping" +// group and check that it has mappings for each entry in the weights map, +// with the correct weights. +func checkIR(f *entrypoint.Fake, what string, weights map[string]WeightCheck) { // Flush the Fake harness so that we get a configuration. f.Flush() @@ -37,30 +57,75 @@ func checkIR(f *entrypoint.Fake) { group, ok := getWorkload1MappingGroup(ir) require.True(f.T, ok) - // That group should have two mappings. - require.Len(f.T, group.Mappings, 2) - - // One mapping should have a "name" of "workload1-mapping" and a - // cumulative weight of 100; the other should have a "name" of - // "workload2-mapping" and a cumulative weight of 10. - found1 := false - found2 := false + // That group should have the same number of mappings as we have entries + // in the weights map. + require.Len(f.T, group.Mappings, len(weights)) + + // Now we can check each mapping. Since we need all of them to be present + // in the group, we'll start with a set of all the mappings defined in the + // weights map, and remove them as we find them in the mapping. Any left + // over at the end were missing from the group. + missingMappings := make(map[string]struct{}) + for name := range weights { + missingMappings[name] = struct{}{} + } + // Next, walk over the group's mappings and check against the weights map. for _, mapping := range group.Mappings { - switch mapping.Name { - case "workload1-mapping": - assert.Equal(f.T, 100, mapping.CumulativeWeight) - found1 = true - case "workload2-mapping": - assert.Equal(f.T, 10, mapping.CumulativeWeight) - found2 = true - default: - f.T.Fatalf("unexpected mapping: %#v", mapping) + check, ok := weights[mapping.Name] + + if ok { + // It's present; remove it from the leftovers. + delete(missingMappings, mapping.Name) + + // Next, make sure the weights match. + var msg string + + if check.weight == nil { + if mapping.Weight != nil { + msg = fmt.Sprintf("%s: weight for %s should not be present but is %d", what, mapping.Name, *mapping.Weight) + } + } else if mapping.Weight == nil { + msg = fmt.Sprintf("%s: weight for %s should be %d but is not present", what, mapping.Name, *check.weight) + } else if *check.weight != *mapping.Weight { + msg = fmt.Sprintf("%s: unexpected weight for mapping %s: wanted %d, got %d", what, mapping.Name, *check.weight, mapping.Weight) + } + + if msg != "" { + for _, m := range group.Mappings { + msg += "\n" + + if m.Weight == nil { + msg += fmt.Sprintf(" - %s: weight unset", m.Name) + } else { + msg += fmt.Sprintf(" - %s: weight %d", m.Name, *m.Weight) + } + } + + f.T.Fatal(msg) + } + + // Finally, check the cumulative weight. + if check.cumulative != mapping.CumulativeWeight { + f.T.Fatalf("%s: unexpected cumulative weight for mapping %s: wanted %d, got %d", what, mapping.Name, check.cumulative, mapping.CumulativeWeight) + } + } else { + // It's not present; this is a problem. + f.T.Fatalf("%s: unexpected mapping: %#v", what, mapping.Name) } } - assert.True(f.T, found1) - assert.True(f.T, found2) + // Finally, we should have no leftovers. + if len(missingMappings) > 0 { + msg := fmt.Sprintf("%s: missing mappings:", what) + + for name := range missingMappings { + msg += "\n" + msg += fmt.Sprintf(" - %s", name) + } + + f.T.Fatal(msg) + } } // The Fake struct is a test harness for Emissary. See testutil_fake_test.go @@ -79,12 +144,17 @@ func TestUnrelatedMappings(t *testing.T) { assert.NoError(t, f.UpsertFile("testdata/unrelated-mappings/mapping.yaml")) // Now we can check the IR. - checkIR(f) + checkIR(f, "initial", map[string]WeightCheck{ + "workload1-mapping": NewWeightCheck(-1, 100), + "workload2-mapping": NewWeightCheck(10, 10), + }) // Next up, upsert a completely unrelated mapping. This mustn't affect // our existing group. assert.NoError(t, f.UpsertFile("testdata/unrelated-mappings/unrelated.yaml")) - // Flush the Fake harness and repeat our IR check. - checkIR(f) + checkIR(f, "upsert unrelated", map[string]WeightCheck{ + "workload1-mapping": NewWeightCheck(-1, 100), + "workload2-mapping": NewWeightCheck(10, 10), + }) } From 9ec26d740c7653295709890bbca9a4fdccb75c75 Mon Sep 17 00:00:00 2001 From: Flynn Date: Fri, 19 Jul 2024 12:08:30 -0400 Subject: [PATCH 11/13] rename mapping.yaml -> mapping1.yaml Signed-off-by: Flynn --- .../testdata/unrelated-mappings/{mapping.yaml => mapping1.yaml} | 0 cmd/entrypoint/unrelated_mappings_test.go | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename cmd/entrypoint/testdata/unrelated-mappings/{mapping.yaml => mapping1.yaml} (100%) diff --git a/cmd/entrypoint/testdata/unrelated-mappings/mapping.yaml b/cmd/entrypoint/testdata/unrelated-mappings/mapping1.yaml similarity index 100% rename from cmd/entrypoint/testdata/unrelated-mappings/mapping.yaml rename to cmd/entrypoint/testdata/unrelated-mappings/mapping1.yaml diff --git a/cmd/entrypoint/unrelated_mappings_test.go b/cmd/entrypoint/unrelated_mappings_test.go index ae3fa96cea..d353543087 100644 --- a/cmd/entrypoint/unrelated_mappings_test.go +++ b/cmd/entrypoint/unrelated_mappings_test.go @@ -141,7 +141,7 @@ func TestUnrelatedMappings(t *testing.T) { // Next up, upsert our test data. assert.NoError(t, f.UpsertFile("testdata/unrelated-mappings/service.yaml")) assert.NoError(t, f.UpsertFile("testdata/unrelated-mappings/host.yaml")) - assert.NoError(t, f.UpsertFile("testdata/unrelated-mappings/mapping.yaml")) + assert.NoError(t, f.UpsertFile("testdata/unrelated-mappings/mapping1.yaml")) // Now we can check the IR. checkIR(f, "initial", map[string]WeightCheck{ From 2004d63f8b4927d4c976acfb112a459c159bb095 Mon Sep 17 00:00:00 2001 From: Flynn Date: Fri, 19 Jul 2024 12:39:34 -0400 Subject: [PATCH 12/13] Make the unrelated test more obnoxious. Signed-off-by: Flynn --- .../testdata/unrelated-mappings/mapping2.yaml | 16 ++++++++++ cmd/entrypoint/unrelated_mappings_test.go | 32 +++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 cmd/entrypoint/testdata/unrelated-mappings/mapping2.yaml diff --git a/cmd/entrypoint/testdata/unrelated-mappings/mapping2.yaml b/cmd/entrypoint/testdata/unrelated-mappings/mapping2.yaml new file mode 100644 index 0000000000..031fc985aa --- /dev/null +++ b/cmd/entrypoint/testdata/unrelated-mappings/mapping2.yaml @@ -0,0 +1,16 @@ +--- +apiVersion: getambassador.io/v2 +kind: Mapping +metadata: + labels: + app: workload2 + name: workload2-mapping + namespace: infrastructure +spec: + bypass_auth: true + host: test.example.com + prefix: / + rewrite: "" + service: workload2 + timeout_ms: 3000 + weight: 50 diff --git a/cmd/entrypoint/unrelated_mappings_test.go b/cmd/entrypoint/unrelated_mappings_test.go index d353543087..15b1c8f24e 100644 --- a/cmd/entrypoint/unrelated_mappings_test.go +++ b/cmd/entrypoint/unrelated_mappings_test.go @@ -157,4 +157,36 @@ func TestUnrelatedMappings(t *testing.T) { "workload1-mapping": NewWeightCheck(-1, 100), "workload2-mapping": NewWeightCheck(10, 10), }) + + // Next, try updating the weight of workload2-mapping. + assert.NoError(t, f.UpsertFile("testdata/unrelated-mappings/mapping2.yaml")) + + checkIR(f, "update workload2-mapping weight", map[string]WeightCheck{ + "workload1-mapping": NewWeightCheck(-1, 100), + "workload2-mapping": NewWeightCheck(50, 50), + }) + + // Next up, delete our completely unrelated mapping. This mustn't affect + // our existing group. + assert.NoError(t, f.Delete("Mapping", "infrastructure", "unrelated")) + + checkIR(f, "delete unrelated", map[string]WeightCheck{ + "workload1-mapping": NewWeightCheck(-1, 100), + "workload2-mapping": NewWeightCheck(50, 50), + }) + + // Repeat that upsert-and-repeat cycle. + assert.NoError(t, f.UpsertFile("testdata/unrelated-mappings/unrelated.yaml")) + + checkIR(f, "re-upsert unrelated", map[string]WeightCheck{ + "workload1-mapping": NewWeightCheck(-1, 100), + "workload2-mapping": NewWeightCheck(50, 50), + }) + + assert.NoError(t, f.Delete("Mapping", "infrastructure", "unrelated")) + + checkIR(f, "re-delete unrelated", map[string]WeightCheck{ + "workload1-mapping": NewWeightCheck(-1, 100), + "workload2-mapping": NewWeightCheck(50, 50), + }) } From 2a2780981a2e94a0e5e6f0817bed3f7fc717796e Mon Sep 17 00:00:00 2001 From: Flynn Date: Sat, 20 Jul 2024 14:01:00 -0400 Subject: [PATCH 13/13] More obnoxiousness (change an existing mapping and add.a new related mapping in one step) Signed-off-by: Flynn --- .../testdata/unrelated-mappings/mapping3.yaml | 31 +++++++++++++++++++ cmd/entrypoint/unrelated_mappings_test.go | 11 +++++++ 2 files changed, 42 insertions(+) create mode 100644 cmd/entrypoint/testdata/unrelated-mappings/mapping3.yaml diff --git a/cmd/entrypoint/testdata/unrelated-mappings/mapping3.yaml b/cmd/entrypoint/testdata/unrelated-mappings/mapping3.yaml new file mode 100644 index 0000000000..ec193a83b0 --- /dev/null +++ b/cmd/entrypoint/testdata/unrelated-mappings/mapping3.yaml @@ -0,0 +1,31 @@ + +apiVersion: getambassador.io/v2 +kind: Mapping +metadata: + labels: + app: workload1 + name: workload1-mapping + namespace: infrastructure +spec: + bypass_auth: true + host: test.example.com + prefix: / + rewrite: "" + service: workload1 + timeout_ms: 3000 + weight: 20 +--- +apiVersion: getambassador.io/v2 +kind: Mapping +metadata: + labels: + app: workload3 + name: workload3-mapping + namespace: infrastructure +spec: + bypass_auth: true + host: test.example.com + prefix: / + rewrite: "" + service: workload3 + timeout_ms: 3000 diff --git a/cmd/entrypoint/unrelated_mappings_test.go b/cmd/entrypoint/unrelated_mappings_test.go index 15b1c8f24e..db7bdf625a 100644 --- a/cmd/entrypoint/unrelated_mappings_test.go +++ b/cmd/entrypoint/unrelated_mappings_test.go @@ -189,4 +189,15 @@ func TestUnrelatedMappings(t *testing.T) { "workload1-mapping": NewWeightCheck(-1, 100), "workload2-mapping": NewWeightCheck(50, 50), }) + + // Finally, do something complex: update the weight of workload1-mapping, + // add a workload3-mapping, and reintroduce the unrelated mapping. + assert.NoError(t, f.UpsertFile("testdata/unrelated-mappings/mapping3.yaml")) + assert.NoError(t, f.UpsertFile("testdata/unrelated-mappings/unrelated.yaml")) + + checkIR(f, "complex 1", map[string]WeightCheck{ + "workload1-mapping": NewWeightCheck(20, 20), + "workload2-mapping": NewWeightCheck(50, 70), + "workload3-mapping": NewWeightCheck(-1, 100), + }) }