From 2fd606d3da7123c71e930f6f39fe165eb38da183 Mon Sep 17 00:00:00 2001 From: Flynn Date: Wed, 14 Jul 2021 18:03:18 -0400 Subject: [PATCH 01/18] Update for v1.13.10 Signed-off-by: Flynn --- CHANGELOG.md | 7 +++++++ docs/yaml/versions.yml | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b5aa50317f..76af145084 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,6 +63,13 @@ Please see the [Envoy documentation](https://www.envoyproxy.io/docs/envoy/latest ## RELEASE NOTES +## [1.13.10] (TBD) +[1.13.10]: https://github.com/emissary-ingress/emissary/compare/v1.13.9...v1.13.10 + +### Emissary Ingress and Ambassador Edge Stack + +(no changes yet) + ## [1.13.9] June 30, 2021 [1.13.9]: https://github.com/emissary-ingress/emissary/compare/v1.13.8...v1.13.9 diff --git a/docs/yaml/versions.yml b/docs/yaml/versions.yml index 0acee71b29..6a29763019 100644 --- a/docs/yaml/versions.yml +++ b/docs/yaml/versions.yml @@ -1,2 +1,2 @@ -version: 1.13.9 +version: 1.13.10 quoteVersion: 0.4.1 From 0c901468ac148d7245609750af13836139f895c6 Mon Sep 17 00:00:00 2001 From: Alix Cook Date: Wed, 14 Jul 2021 18:33:18 -0400 Subject: [PATCH 02/18] promote to passed works better Signed-off-by: Flynn --- .circleci/config.yml | 1 + .circleci/config.yml.d/amb_oss.yml | 1 + 2 files changed, 2 insertions(+) diff --git a/.circleci/config.yml b/.circleci/config.yml index c54e9d403e..49271babb2 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -1107,6 +1107,7 @@ workflows: - oss-dev-gotest-fastreconfigure - oss-dev-gotest - oss-dev-generate + - oss-dev-images name: "oss-dev-promote-to-passed" filters: branches: diff --git a/.circleci/config.yml.d/amb_oss.yml b/.circleci/config.yml.d/amb_oss.yml index 8be078e478..30c49b3fef 100644 --- a/.circleci/config.yml.d/amb_oss.yml +++ b/.circleci/config.yml.d/amb_oss.yml @@ -334,6 +334,7 @@ workflows: - oss-dev-gotest-fastreconfigure - oss-dev-gotest - oss-dev-generate + - oss-dev-images name: "oss-dev-promote-to-passed" <<: *filter-master-release-branches-only From e07109ebf58e6654355f84eec0558a38ff0c5948 Mon Sep 17 00:00:00 2001 From: Flynn Date: Tue, 13 Jul 2021 12:10:57 -0400 Subject: [PATCH 03/18] Allow "make deploy" to work. Signed-off-by: Flynn --- Makefile | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/Makefile b/Makefile index 74906b6908..a1a43647c4 100644 --- a/Makefile +++ b/Makefile @@ -49,3 +49,32 @@ SHELL = bash ln -s $(OSS_HOME)/tools/hooks/prepare-commit-msg $(OSS_HOME)/.git/hooks/prepare-commit-msg githooks: .git/hooks/prepare-commit-msg + +preflight-dev-kubeconfig: + @if [ -z "$(DEV_KUBECONFIG)" ] ; then \ + echo "DEV_KUBECONFIG must be set"; \ + exit 1; \ + fi +.PHONY: preflight-dev-kubeconfig + +deploy: push preflight-cluster + $(MAKE) deploy-only +.PHONY: deploy + +deploy-only: preflight-dev-kubeconfig + mkdir -p $(OSS_HOME)/build/helm/ && \ + (kubectl --kubeconfig $(DEV_KUBECONFIG) create ns ambassador || true) && \ + helm template ambassador --include-crds --output-dir $(OSS_HOME)/build/helm -n ambassador charts/ambassador/ \ + --set createNamespace=true \ + --set service.selector.service=ambassador \ + --set replicaCount=1 \ + --set enableAES=false \ + --set image.fullImageOverride=$$(sed -n 2p docker/ambassador.docker.push.remote) && \ + kubectl --kubeconfig $(DEV_KUBECONFIG) apply -f $(OSS_HOME)/build/helm/ambassador/crds/ && \ + kubectl --kubeconfig $(DEV_KUBECONFIG) apply -f $(OSS_HOME)/build/helm/ambassador/templates && \ + rm -rf $(OSS_HOME)/build/helm + kubectl --kubeconfig $(DEV_KUBECONFIG) -n ambassador wait --for condition=available --timeout=90s deploy --all + @printf "$(GRN)Your ambassador service IP:$(END) $(BLD)$$(kubectl --kubeconfig $(DEV_KUBECONFIG) get -n ambassador service ambassador -o 'go-template={{range .status.loadBalancer.ingress}}{{print .ip "\n"}}{{end}}')$(END)\n" + @printf "$(GRN)Your ambassador image:$(END) $(BLD)$$(kubectl --kubeconfig $(DEV_KUBECONFIG) get -n ambassador deploy ambassador -o 'go-template={{(index .spec.template.spec.containers 0).image}}')$(END)\n" + @printf "$(GRN)Your built image:$(END) $(BLD)$$(sed -n 2p docker/ambassador.docker.push.remote)$(END)\n" +.PHONY: deploy-only \ No newline at end of file From 7115d19f11e5dc68f5c1ce2c02f19fd22d60afed Mon Sep 17 00:00:00 2001 From: Flynn Date: Tue, 13 Jul 2021 12:11:37 -0400 Subject: [PATCH 04/18] Basic Ambex visibility: save a certain number of Ambex snapshots to disk for debugging Signed-off-by: Flynn --- cmd/ambex/main.go | 231 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 196 insertions(+), 35 deletions(-) diff --git a/cmd/ambex/main.go b/cmd/ambex/main.go index 3c081c0d20..9ceb1fa310 100644 --- a/cmd/ambex/main.go +++ b/cmd/ambex/main.go @@ -47,8 +47,10 @@ import ( "net" "os" "os/signal" + "path" "path/filepath" "reflect" + "strconv" "strings" "syscall" @@ -71,8 +73,9 @@ import ( v3server "github.com/datawire/ambassador/pkg/envoy-control-plane/server/v3" "github.com/datawire/ambassador/pkg/memory" - // envoy protobuf v2 -- Be sure to import the package of any types that the Python emits a - // "@type" of in the generated config, even if that package is otherwise not used by ambex. + // Envoy API v2 + // Be sure to import the package of any types that're referenced with "@type" in our + // generated Envoy config, even if that package is otherwise not used by ambex. v2 "github.com/datawire/ambassador/pkg/api/envoy/api/v2" _ "github.com/datawire/ambassador/pkg/api/envoy/api/v2/auth" v2core "github.com/datawire/ambassador/pkg/api/envoy/api/v2/core" @@ -90,12 +93,8 @@ import ( v2discovery "github.com/datawire/ambassador/pkg/api/envoy/service/discovery/v2" // Envoy API v3 - // - // XXX TODO Is this actually true and necessary? - // yes it is true and necessary. need to import all types so they can be decoded by `decode` - // Be sure to import the package of any types that the Python - // emits a "@type" of in the generated config, even if that package is otherwise - // not used by ambex. + // Be sure to import the package of any types that're referenced with "@type" in our + // generated Envoy config, even if that package is otherwise not used by ambex. _ "github.com/datawire/ambassador/pkg/api/envoy/config/accesslog/v3" v3bootstrap "github.com/datawire/ambassador/pkg/api/envoy/config/bootstrap/v3" v3clusterconfig "github.com/datawire/ambassador/pkg/api/envoy/config/cluster/v3" @@ -321,7 +320,113 @@ func Clone(src proto.Message) proto.Message { return dst } -func update(ctx context.Context, config v2cache.SnapshotCache, configv3 v3cache.SnapshotCache, generation *int, dirs []string, edsEndpoints map[string]*v2.ClusterLoadAssignment, edsEndpointsV3 map[string]*v3endpointconfig.ClusterLoadAssignment, fastpathSnapshot *FastpathSnapshot, updates chan<- Update) { +// Observability: +// +// These "expanded" snapshots make the snapshots we log easier to read: basically, +// instead of just indexing by Golang types, make the JSON marshal with real names. +type v2ExpandedSnapshot struct { + Endpoints v2cache.Resources `json:"endpoints"` + Clusters v2cache.Resources `json:"clusters"` + Routes v2cache.Resources `json:"routes"` + Listeners v2cache.Resources `json:"listeners"` + Runtimes v2cache.Resources `json:"runtimes"` +} + +func NewV2ExpandedSnapshot(v2snap *v2cache.Snapshot) v2ExpandedSnapshot { + return v2ExpandedSnapshot{ + Endpoints: v2snap.Resources[types.Endpoint], + Clusters: v2snap.Resources[types.Cluster], + Routes: v2snap.Resources[types.Route], + Listeners: v2snap.Resources[types.Listener], + Runtimes: v2snap.Resources[types.Runtime], + } +} + +type v3ExpandedSnapshot struct { + Endpoints v3cache.Resources `json:"endpoints"` + Clusters v3cache.Resources `json:"clusters"` + Routes v3cache.Resources `json:"routes"` + Listeners v3cache.Resources `json:"listeners"` + Runtimes v3cache.Resources `json:"runtimes"` +} + +func NewV3ExpandedSnapshot(v3snap *v3cache.Snapshot) v3ExpandedSnapshot { + return v3ExpandedSnapshot{ + Endpoints: v3snap.Resources[types.Endpoint], + Clusters: v3snap.Resources[types.Cluster], + Routes: v3snap.Resources[types.Route], + Listeners: v3snap.Resources[types.Listener], + Runtimes: v3snap.Resources[types.Runtime], + } +} + +// A combinedSnapshot has both a V2 and V3 snapshot, for logging. +type combinedSnapshot struct { + Version string `json:"version"` + V2 v2ExpandedSnapshot `json:"v2"` + V3 v3ExpandedSnapshot `json:"v3"` +} + +// csDump creates a combinedSnapshot from a V2 snapshot and a V3 snapshot, then +// dumps the combinedSnapshot to disk. Only numsnaps snapshots are kept: ambex-1.json +// is the newest, then ambex-2.json, etc., so ambex-$numsnaps.json is the oldest. +// Every time we write a new one, we rename all the older ones, ditching the oldest +// after we've written numsnaps snapshots. +func csDump(snapdirPath string, numsnaps int, generation int, v2snap *v2cache.Snapshot, v3snap *v3cache.Snapshot) { + if numsnaps <= 0 { + // Don't do snapshotting at all. + return + } + + // OK, they want snapshots. Make a proper version string... + version := fmt.Sprintf("v%d", generation) + + // ...and a combinedSnapshot. + cs := combinedSnapshot{ + Version: version, + V2: NewV2ExpandedSnapshot(v2snap), + V3: NewV3ExpandedSnapshot(v3snap), + } + + // Next up, marshal as JSON and write to ambex-0.json. Note that we + // didn't say anything about a -0 file; that's because it's about to + // be renamed. + + bs, err := json.MarshalIndent(cs, "", " ") + + if err != nil { + log.Errorf("CSNAP: marshal failure: %s", err) + return + } + + csPath := path.Join(snapdirPath, "ambex-0.json") + + err = ioutil.WriteFile(csPath, bs, 0644) + + if err != nil { + log.Errorf("CSNAP: write failure: %s", err) + } else { + log.Infof("Saved snapshot %s", version) + } + + // Rotate everything one file down. This includes renaming the just-written + // ambex-0 to ambex-1. + for i := numsnaps; i > 0; i-- { + previous := i - 1 + + fromPath := path.Join(snapdirPath, fmt.Sprintf("ambex-%d.json", previous)) + toPath := path.Join(snapdirPath, fmt.Sprintf("ambex-%d.json", i)) + + err := os.Rename(fromPath, toPath) + + if (err != nil) && !os.IsNotExist(err) { + log.Infof("CSNAP: could not rename %s -> %s: %#v", fromPath, toPath, err) + } + } +} + +// Get an updated snapshot going. +func update(ctx context.Context, snapdirPath string, numsnaps int, config v2cache.SnapshotCache, configv3 v3cache.SnapshotCache, generation *int, dirs []string, edsEndpoints map[string]*v2.ClusterLoadAssignment, edsEndpointsV3 map[string]*v3endpointconfig.ClusterLoadAssignment, fastpathSnapshot *FastpathSnapshot, updates chan<- Update) { clusters := []ctypes.Resource{} // v2.Cluster routes := []ctypes.Resource{} // v2.RouteConfiguration listeners := []ctypes.Resource{} // v2.Listener @@ -473,8 +578,10 @@ func update(ctx context.Context, config v2cache.SnapshotCache, configv3 v3cache. endpointsv3 := JoinEdsClustersV3(ctx, clustersv3, edsEndpointsV3) // Create a new configuration snapshot from everything we have just loaded from disk. - version := fmt.Sprintf("v%d", *generation) + curgen := *generation *generation++ + + version := fmt.Sprintf("v%d", curgen) snapshot := v2cache.NewSnapshot( version, endpoints, @@ -506,12 +613,13 @@ func update(ctx context.Context, config v2cache.SnapshotCache, configv3 v3cache. // This used to just directly update envoy. Since we want ratelimiting, we now send an // Update object down the channel with a function that knows how to do the update if/when // the ratelimiting logic decides. - // - // We also need to pay attention to contexts here so we can shutdown properly. If we didn't - // have the context portion, the ratelimit goroutine could shutdown first and we could end - // up blocking here and never shutting down. - select { - case updates <- Update{version, func() error { + + log.Debugf("Created snapshot %s", version) + csDump(snapdirPath, numsnaps, curgen, &snapshot, &snapshotv3) + + update := Update{version, func() error { + log.Debugf("Accepting snapshot %s", version) + err := config.SetSnapshot("test-id", snapshot) if err != nil { return fmt.Errorf("V2 Snapshot error %q for %+v", err, snapshot) @@ -523,7 +631,13 @@ func update(ctx context.Context, config v2cache.SnapshotCache, configv3 v3cache. } return nil - }}: + }} + + // We also need to pay attention to contexts here so we can shutdown properly. If we didn't + // have the context portion, the ratelimit goroutine could shutdown first and we could end + // up blocking here and never shutting down. + select { + case updates <- update: case <-ctx.Done(): } } @@ -539,66 +653,66 @@ func warn(err error) bool { // OnStreamOpen is called once an xDS stream is open with a stream ID and the type URL (or "" for ADS). func (l loggerv2) OnStreamOpen(_ context.Context, sid int64, stype string) error { - l.Infof("V2 Stream open[%v]: %v", sid, stype) + l.Debugf("V2 Stream open[%v]: %v", sid, stype) return nil } func (l loggerv3) OnStreamOpen(_ context.Context, sid int64, stype string) error { - l.Infof("V3 Stream open[%v]: %v", sid, stype) + l.Debugf("V3 Stream open[%v]: %v", sid, stype) return nil } // OnStreamClosed is called immediately prior to closing an xDS stream with a stream ID. func (l loggerv2) OnStreamClosed(sid int64) { - l.Infof("V2 Stream closed[%v]", sid) + l.Debugf("V2 Stream closed[%v]", sid) } func (l loggerv3) OnStreamClosed(sid int64) { - l.Infof("V3 Stream closed[%v]", sid) + l.Debugf("V3 Stream closed[%v]", sid) } // OnStreamRequest is called once a request is received on a stream. func (l loggerv2) OnStreamRequest(sid int64, req *v2.DiscoveryRequest) error { - l.Infof("V2 Stream request[%v] for type %s: requesting %d resources", sid, req.TypeUrl, len(req.ResourceNames)) + l.Debugf("V2 Stream request[%v] for type %s: requesting %d resources", sid, req.TypeUrl, len(req.ResourceNames)) l.Debugf("V2 Stream request[%v] dump: %v", sid, req) return nil } func (l loggerv3) OnStreamRequest(sid int64, req *v3discovery.DiscoveryRequest) error { - l.Infof("V3 Stream request[%v] for type %s: requesting %d resources", sid, req.TypeUrl, len(req.ResourceNames)) + l.Debugf("V3 Stream request[%v] for type %s: requesting %d resources", sid, req.TypeUrl, len(req.ResourceNames)) l.Debugf("V3 Stream request[%v] dump: %v", sid, req) return nil } // OnStreamResponse is called immediately prior to sending a response on a stream. func (l loggerv2) OnStreamResponse(sid int64, req *v2.DiscoveryRequest, res *v2.DiscoveryResponse) { - l.Infof("V2 Stream response[%v] for type %s: returning %d resources", sid, res.TypeUrl, len(res.Resources)) + l.Debugf("V2 Stream response[%v] for type %s: returning %d resources", sid, res.TypeUrl, len(res.Resources)) l.Debugf("V2 Stream dump response[%v]: %v -> %v", sid, req, res) } func (l loggerv3) OnStreamResponse(sid int64, req *v3discovery.DiscoveryRequest, res *v3discovery.DiscoveryResponse) { - l.Infof("V3 Stream response[%v] for type %s: returning %d resources", sid, res.TypeUrl, len(res.Resources)) + l.Debugf("V3 Stream response[%v] for type %s: returning %d resources", sid, res.TypeUrl, len(res.Resources)) l.Debugf("V3 Stream dump response[%v]: %v -> %v", sid, req, res) } // OnFetchRequest is called for each Fetch request func (l loggerv2) OnFetchRequest(_ context.Context, r *v2.DiscoveryRequest) error { - l.Infof("V2 Fetch request: %v", r) + l.Debugf("V2 Fetch request: %v", r) return nil } func (l loggerv3) OnFetchRequest(_ context.Context, r *v3discovery.DiscoveryRequest) error { - l.Infof("V3 Fetch request: %v", r) + l.Debugf("V3 Fetch request: %v", r) return nil } // OnFetchResponse is called immediately prior to sending a response. func (l loggerv2) OnFetchResponse(req *v2.DiscoveryRequest, res *v2.DiscoveryResponse) { - l.Infof("V2 Fetch response: %v -> %v", req, res) + l.Debugf("V2 Fetch response: %v -> %v", req, res) } func (l loggerv3) OnFetchResponse(req *v3discovery.DiscoveryRequest, res *v3discovery.DiscoveryResponse) { - l.Infof("V3 Fetch response: %v -> %v", req, res) + l.Debugf("V3 Fetch response: %v -> %v", req, res) } func Main(ctx context.Context, Version string, rawArgs ...string) error { @@ -617,10 +731,44 @@ func Main2(ctx context.Context, Version string, getUsage MemoryGetter, fastpathC if args.debug { log.SetLevel(logrus.DebugLevel) } else { - log.SetLevel(logrus.WarnLevel) + log.SetLevel(logrus.InfoLevel) } - log.Infof("Ambex %s starting...", Version) + // ambex logs its own snapshots, separately from the ones provided by the Python + // side of the world, in $rootdir/snapshots/ambex-#.json, where rootdir is taken + // from $AMBASSADOR_CONFIG_BASE_DIR if set, else $ambassador_root if set, else + // whatever, set rootdir to /ambassador. + + snapdirPath := os.Getenv("AMBASSADOR_CONFIG_BASE_DIR") + + if snapdirPath == "" { + snapdirPath = os.Getenv("ambassador_root") + } + + if snapdirPath == "" { + snapdirPath = "/ambassador" + } + + snapdirPath = path.Join(snapdirPath, "snapshots") + + // We'll keep $AMBASSADOR_AMBEX_SNAPSHOT_COUNT snapshots. If unset, or set to + // something we can't treat as an int, use 30 (which Flynn just made up, so don't + // be afraid to change it if need be). + + numsnapStr := os.Getenv("AMBASSADOR_AMBEX_SNAPSHOT_COUNT") + + if numsnapStr == "" { + numsnapStr = "30" + } + + numsnaps, err := strconv.Atoi(numsnapStr) + + if (err != nil) || (numsnaps < 0) { + numsnaps = 30 + log.Errorf("Invalid AMBASSADOR_AMBEX_SNAPSHOT_COUNT: %s, using %d", numsnapStr, numsnaps) + } + + log.Infof("Ambex %s starting, snapdirPath %s", Version, snapdirPath) watcher, err := fsnotify.NewWatcher() if err != nil { @@ -677,29 +825,42 @@ func Main2(ctx context.Context, Version string, getUsage MemoryGetter, fastpathC var fastpathSnapshot *FastpathSnapshot edsEndpoints := map[string]*v2.ClusterLoadAssignment{} edsEndpointsV3 := map[string]*v3endpointconfig.ClusterLoadAssignment{} - update(ctx, config, configv3, &generation, args.dirs, edsEndpoints, edsEndpointsV3, fastpathSnapshot, updates) + // We always start by updating with a totally empty snapshot. + // + // XXX This seems questionable: why do we do this? Envoy isn't currently started until + // we have a real configuration... + update(ctx, snapdirPath, numsnaps, config, configv3, &generation, args.dirs, edsEndpoints, edsEndpointsV3, fastpathSnapshot, updates) + + // This is the main loop where the magic happens. The fact that it uses a label + // depresses me, though. OUTER: for { select { case sig := <-ch: + // Signal handling: reconfigure on HUP, bail on INT or TERM. + // + // XXX Y'know, redoing this with if would let us ditch that silly label. switch sig { case syscall.SIGHUP: - update(ctx, config, configv3, &generation, args.dirs, edsEndpoints, edsEndpointsV3, fastpathSnapshot, updates) + update(ctx, snapdirPath, numsnaps, config, configv3, &generation, args.dirs, edsEndpoints, edsEndpointsV3, fastpathSnapshot, updates) case os.Interrupt, syscall.SIGTERM: break OUTER } case fpSnap := <-fastpathCh: + // Fastpath update. Grab new endpoints and update. if fpSnap.Endpoints != nil { edsEndpoints = fpSnap.Endpoints.ToMap_v2() edsEndpointsV3 = fpSnap.Endpoints.ToMap_v3() } fastpathSnapshot = fpSnap - update(ctx, config, configv3, &generation, args.dirs, edsEndpoints, edsEndpointsV3, fastpathSnapshot, updates) + update(ctx, snapdirPath, numsnaps, config, configv3, &generation, args.dirs, edsEndpoints, edsEndpointsV3, fastpathSnapshot, updates) case <-watcher.Events: - update(ctx, config, configv3, &generation, args.dirs, edsEndpoints, edsEndpointsV3, fastpathSnapshot, updates) + // Non-fastpath update. Just update. + update(ctx, snapdirPath, numsnaps, config, configv3, &generation, args.dirs, edsEndpoints, edsEndpointsV3, fastpathSnapshot, updates) case err := <-watcher.Errors: + // Something went wrong, so scream about that. log.WithError(err).Warn("Watcher error") case <-ctx.Done(): break OUTER From 6f072efd2698be93b3cb90d4ae7a1b2125b6fffb Mon Sep 17 00:00:00 2001 From: Flynn Date: Wed, 14 Jul 2021 17:25:25 -0400 Subject: [PATCH 05/18] CHANGELOG Signed-off-by: Flynn --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 76af145084..46e969f9e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,7 +68,8 @@ Please see the [Envoy documentation](https://www.envoyproxy.io/docs/envoy/latest ### Emissary Ingress and Ambassador Edge Stack -(no changes yet) +- 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. ## [1.13.9] June 30, 2021 [1.13.9]: https://github.com/emissary-ingress/emissary/compare/v1.13.8...v1.13.9 From 4bfad6f04bd2fd7b6d2ab71aa37ddc15c4ee78c7 Mon Sep 17 00:00:00 2001 From: Flynn Date: Thu, 15 Jul 2021 11:53:08 -0400 Subject: [PATCH 06/18] Support hotfix tags in CI Signed-off-by: Flynn --- .circleci/config.yml | 12 ++++++++---- .circleci/config.yml.d/amb_oss.yml | 10 ++++++---- builder/builder.mk | 3 ++- 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 49271babb2..1e7f9678eb 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -919,10 +919,12 @@ jobs: - job-yaml-publish-s3: update-stable: false _anchors: - filter-rc-only: + filter-rc-hf-only: filters: tags: - only: /^v[0-9]+\.[0-9]+\.[0-9]+-rc\.[0-9]+$/ + only: + - /^v[0-9]+\.[0-9]+\.[0-9]+-rc\.[0-9]+$/ + - /^v[0-9]+\.[0-9]+\.[0-9]+-hf\.[0-9]+\+[0-9]+$/ branches: ignore: /.*/ filter-ga-only: @@ -953,13 +955,15 @@ workflows: only: /^v[0-9]+\.[0-9]+\.[0-9]+$/ branches: ignore: /.*/ - 'OSS: Release Candidate': + 'OSS: Release Candidate or Hotfix': jobs: - oss-promote-dev-to-rc: name: "oss-release-promote-dev-to-rc" filters: tags: - only: /^v[0-9]+\.[0-9]+\.[0-9]+-rc\.[0-9]+$/ + only: + - /^v[0-9]+\.[0-9]+\.[0-9]+-rc\.[0-9]+$/ + - /^v[0-9]+\.[0-9]+\.[0-9]+-hf\.[0-9]+\+[0-9]+$/ branches: ignore: /.*/ 'OSS: Dev': diff --git a/.circleci/config.yml.d/amb_oss.yml b/.circleci/config.yml.d/amb_oss.yml index 30c49b3fef..e428af3dd8 100644 --- a/.circleci/config.yml.d/amb_oss.yml +++ b/.circleci/config.yml.d/amb_oss.yml @@ -152,10 +152,12 @@ _anchors: # All of these filters assume that "Only build pull requests" is turned on at # https://app.circleci.com/settings/project/github/emissary-ingress/emissary/advanced - "filter-rc-only": &filter-rc-only + "filter-rc-hf-only": &filter-rc-hf-only filters: tags: - only: /^v[0-9]+\.[0-9]+\.[0-9]+-rc\.[0-9]+$/ + only: + - /^v[0-9]+\.[0-9]+\.[0-9]+-rc\.[0-9]+$/ + - /^v[0-9]+\.[0-9]+\.[0-9]+-hf\.[0-9]+\+[0-9]+$/ branches: ignore: /.*/ @@ -188,10 +190,10 @@ workflows: name: "oss-release-promote-to-ga" - "OSS: Release Candidate": + "OSS: Release Candidate or Hotfix": jobs: - "oss-promote-dev-to-rc": - <<: *filter-rc-only + <<: *filter-rc-hf-only name: "oss-release-promote-dev-to-rc" diff --git a/builder/builder.mk b/builder/builder.mk index 6b2820c176..4ab9543ef4 100644 --- a/builder/builder.mk +++ b/builder/builder.mk @@ -774,7 +774,8 @@ release/promote-oss/to-ea-latest: release/promote-oss/dev-to-rc: @test -n "$(RELEASE_REGISTRY)" || (printf "$${RELEASE_REGISTRY_ERR}\n"; exit 1) - @[[ "$(RELEASE_VERSION)" =~ ^[0-9]+\.[0-9]+\.[0-9]+-rc\.[0-9]+$$ ]] || (printf '$(RED)ERROR: RELEASE_VERSION=%s does not look like an RC tag\n' "$(RELEASE_VERSION)"; exit 1) + @[[ ( "$(RELEASE_VERSION)" =~ ^[0-9]+\.[0-9]+\.[0-9]+-rc\.[0-9]+$$ ) || \ + ( "$(RELEASE_VERSION)" =~ ^[0-9]+\.[0-9]+\.[0-9]+-hf\.[0-9]+\+[0-9]+$$ ) ]] || (printf '$(RED)ERROR: RELEASE_VERSION=%s does not look like an RC tag\n' "$(RELEASE_VERSION)"; exit 1) @set -e; { \ if [ -n "$(IS_DIRTY)" ]; then \ echo "release/promote-oss/dev-to-rc: tree must be clean" >&2 ;\ From 70577ca4d1746b913b1ed2a91e8cf6d11b22b949 Mon Sep 17 00:00:00 2001 From: Flynn Date: Thu, 15 Jul 2021 13:39:44 -0400 Subject: [PATCH 07/18] Fix the docker tag for hotfixes. Signed-off-by: Flynn --- builder/builder.mk | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/builder/builder.mk b/builder/builder.mk index 4ab9543ef4..8f4bbabe8c 100644 --- a/builder/builder.mk +++ b/builder/builder.mk @@ -789,10 +789,11 @@ release/promote-oss/dev-to-rc: fi ;\ printf "$(CYN)==> $(GRN)found version $(BLU)$$dev_version$(GRN) for $(BLU)$$commit$(GRN) in S3...$(END)\n" ;\ veroverride=$(RELEASE_VERSION) ; \ + tag=$$(echo $(RELEASE_VERSION) | tr '+' '-') ; \ $(MAKE) release/promote-oss/.main \ PROMOTE_FROM_VERSION="$$dev_version" \ PROMOTE_FROM_REPO=$(DEV_REGISTRY) \ - PROMOTE_TO_VERSION=$(RELEASE_VERSION) \ + PROMOTE_TO_VERSION="$$tag" \ PROMOTE_CHANNEL=test ; \ chartsuffix=$(RELEASE_VERSION) ; \ chartsuffix=$${chartsuffix#*-} ; \ From 8fc4b51eded05b5f2f5588fd73591644309c86f0 Mon Sep 17 00:00:00 2001 From: Flynn Date: Thu, 15 Jul 2021 15:35:29 -0400 Subject: [PATCH 08/18] Note Edge Stack Consul certificate-rotation logging changes in the CHANGELOG. Signed-off-by: Flynn --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 46e969f9e9..6cec4ba9ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -71,6 +71,11 @@ Please see the [Envoy documentation](https://www.envoyproxy.io/docs/envoy/latest - 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. +### Ambassador Edge Stack + +- Change: Consul certificate-rotation logging now includes the fingerprints and validity timestamps of certificates + being rotated. + ## [1.13.9] June 30, 2021 [1.13.9]: https://github.com/emissary-ingress/emissary/compare/v1.13.8...v1.13.9 From cd94125202efa92b7ca44ecffde04499bd4d8773 Mon Sep 17 00:00:00 2001 From: Alix Cook Date: Fri, 16 Jul 2021 16:24:31 -0400 Subject: [PATCH 09/18] cors origins roundtrip fix (#3612) * cors mapping fix Signed-off-by: Alix Cook * fake test Signed-off-by: Alix Cook * changelog Signed-off-by: Alix Cook --- CHANGELOG.md | 3 +- builder/builder.mk | 2 +- .../crds/getambassador.io_mappings.yaml | 4 +- cmd/entrypoint/fake_mapping_cors_test.go | 139 ++++++++++++++++++ docs/yaml/ambassador/ambassador-crds.yaml | 4 +- .../ambassador-rbac-prometheus.yaml | 4 +- manifests/ambassador/ambassador-crds.yaml | 4 +- pkg/api/getambassador.io/v2/common.go | 46 ++++++ pkg/api/getambassador.io/v2/mapping_types.go | 12 +- .../v2/testdata/mappings.json | 26 +++- .../v2/zz_generated.deepcopy.go | 33 ++++- 11 files changed, 254 insertions(+), 23 deletions(-) create mode 100644 cmd/entrypoint/fake_mapping_cors_test.go 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) { { From b2a223459e97090524109ac7120a4241b6fdb9ee Mon Sep 17 00:00:00 2001 From: Aidan Hahn Date: Tue, 20 Jul 2021 11:01:11 -0700 Subject: [PATCH 10/18] added timeout to the mapping->docs CRD --- charts/ambassador/crds/getambassador.io_devportals.yaml | 3 +++ charts/ambassador/crds/getambassador.io_mappings.yaml | 2 ++ docs/yaml/ambassador/ambassador-crds.yaml | 5 +++++ docs/yaml/ambassador/ambassador-rbac-prometheus.yaml | 5 +++++ manifests/ambassador/ambassador-crds.yaml | 5 +++++ pkg/api/getambassador.io/v2/devportal_types.go | 4 ++++ pkg/api/getambassador.io/v2/mapping_types.go | 1 + 7 files changed, 25 insertions(+) diff --git a/charts/ambassador/crds/getambassador.io_devportals.yaml b/charts/ambassador/crds/getambassador.io_devportals.yaml index 291d2a66e9..93a0ac1f11 100644 --- a/charts/ambassador/crds/getambassador.io_devportals.yaml +++ b/charts/ambassador/crds/getambassador.io_devportals.yaml @@ -63,6 +63,9 @@ spec: service: description: Service is the service being documented type: string + timeout_ms: + description: Timeout speifies the amount of time devportal will wait for the downstream service to report an openapi spec back + type: integer url: description: URL is the URL used for obtaining docs type: string diff --git a/charts/ambassador/crds/getambassador.io_mappings.yaml b/charts/ambassador/crds/getambassador.io_mappings.yaml index dec2af4616..0e8de7cb7f 100644 --- a/charts/ambassador/crds/getambassador.io_mappings.yaml +++ b/charts/ambassador/crds/getambassador.io_mappings.yaml @@ -162,6 +162,8 @@ spec: type: boolean path: type: string + timeout_ms: + type: integer url: type: string type: object diff --git a/docs/yaml/ambassador/ambassador-crds.yaml b/docs/yaml/ambassador/ambassador-crds.yaml index 076aacc157..af956314e2 100644 --- a/docs/yaml/ambassador/ambassador-crds.yaml +++ b/docs/yaml/ambassador/ambassador-crds.yaml @@ -231,6 +231,9 @@ spec: service: description: Service is the service being documented type: string + timeout_ms: + description: Timeout speifies the amount of time devportal will wait for the downstream service to report an openapi spec back + type: integer url: description: URL is the URL used for obtaining docs type: string @@ -866,6 +869,8 @@ spec: type: boolean path: type: string + timeout_ms: + type: integer url: type: string type: object diff --git a/docs/yaml/ambassador/ambassador-rbac-prometheus.yaml b/docs/yaml/ambassador/ambassador-rbac-prometheus.yaml index 0c13b6f701..01740e9ebb 100644 --- a/docs/yaml/ambassador/ambassador-rbac-prometheus.yaml +++ b/docs/yaml/ambassador/ambassador-rbac-prometheus.yaml @@ -295,6 +295,9 @@ spec: service: description: Service is the service being documented type: string + timeout_ms: + description: Timeout speifies the amount of time devportal will wait for the downstream service to report an openapi spec back + type: integer url: description: URL is the URL used for obtaining docs type: string @@ -930,6 +933,8 @@ spec: type: boolean path: type: string + timeout_ms: + type: integer url: type: string type: object diff --git a/manifests/ambassador/ambassador-crds.yaml b/manifests/ambassador/ambassador-crds.yaml index 076aacc157..af956314e2 100644 --- a/manifests/ambassador/ambassador-crds.yaml +++ b/manifests/ambassador/ambassador-crds.yaml @@ -231,6 +231,9 @@ spec: service: description: Service is the service being documented type: string + timeout_ms: + description: Timeout speifies the amount of time devportal will wait for the downstream service to report an openapi spec back + type: integer url: description: URL is the URL used for obtaining docs type: string @@ -866,6 +869,8 @@ spec: type: boolean path: type: string + timeout_ms: + type: integer url: type: string type: object diff --git a/pkg/api/getambassador.io/v2/devportal_types.go b/pkg/api/getambassador.io/v2/devportal_types.go index 09711e4110..3de7eff8e9 100644 --- a/pkg/api/getambassador.io/v2/devportal_types.go +++ b/pkg/api/getambassador.io/v2/devportal_types.go @@ -54,6 +54,10 @@ type DevPortalDocsSpec struct { // URL is the URL used for obtaining docs URL string `json:"url,omitempty"` + + // Timeout speifies the amount of time devportal will wait + // for the downstream service to report an openapi spec back + Timeout int `json:"timeout_ms,omitempty"` } // DevPortalSearchSpec allows configuration over search functionality for the DevPortal diff --git a/pkg/api/getambassador.io/v2/mapping_types.go b/pkg/api/getambassador.io/v2/mapping_types.go index c884dbc88d..31de6fb4d7 100644 --- a/pkg/api/getambassador.io/v2/mapping_types.go +++ b/pkg/api/getambassador.io/v2/mapping_types.go @@ -135,6 +135,7 @@ type DocsInfo struct { URL string `json:"url,omitempty"` Ignored *bool `json:"ignored,omitempty"` DisplayName string `json:"display_name,omitempty"` + Timeout int `json:"timeout_ms,omitempty"` } // These are separate types partly because it makes it easier to think about From e07dc75a44a729d0f81a9854acffda6eb1190c3d Mon Sep 17 00:00:00 2001 From: Aidan Hahn Date: Wed, 21 Jul 2021 17:03:27 -0700 Subject: [PATCH 11/18] add config generation test Signed-off-by: Aidan Hahn --- .../crds/getambassador.io_devportals.yaml | 2 +- docs/yaml/ambassador/ambassador-crds.yaml | 2 +- .../ambassador-rbac-prometheus.yaml | 2 +- manifests/ambassador/ambassador-crds.yaml | 2 +- .../getambassador.io/v2/devportal_types.go | 2 +- python/tests/test_irmapping.py | 57 +++++++++++++++++++ 6 files changed, 62 insertions(+), 5 deletions(-) create mode 100644 python/tests/test_irmapping.py diff --git a/charts/ambassador/crds/getambassador.io_devportals.yaml b/charts/ambassador/crds/getambassador.io_devportals.yaml index 93a0ac1f11..db7354fc49 100644 --- a/charts/ambassador/crds/getambassador.io_devportals.yaml +++ b/charts/ambassador/crds/getambassador.io_devportals.yaml @@ -64,7 +64,7 @@ spec: description: Service is the service being documented type: string timeout_ms: - description: Timeout speifies the amount of time devportal will wait for the downstream service to report an openapi spec back + description: Timeout specifies the amount of time devportal will wait for the downstream service to report an openapi spec back type: integer url: description: URL is the URL used for obtaining docs diff --git a/docs/yaml/ambassador/ambassador-crds.yaml b/docs/yaml/ambassador/ambassador-crds.yaml index af956314e2..f303605177 100644 --- a/docs/yaml/ambassador/ambassador-crds.yaml +++ b/docs/yaml/ambassador/ambassador-crds.yaml @@ -232,7 +232,7 @@ spec: description: Service is the service being documented type: string timeout_ms: - description: Timeout speifies the amount of time devportal will wait for the downstream service to report an openapi spec back + description: Timeout specifies the amount of time devportal will wait for the downstream service to report an openapi spec back type: integer url: description: URL is the URL used for obtaining docs diff --git a/docs/yaml/ambassador/ambassador-rbac-prometheus.yaml b/docs/yaml/ambassador/ambassador-rbac-prometheus.yaml index 01740e9ebb..dc70f9028f 100644 --- a/docs/yaml/ambassador/ambassador-rbac-prometheus.yaml +++ b/docs/yaml/ambassador/ambassador-rbac-prometheus.yaml @@ -296,7 +296,7 @@ spec: description: Service is the service being documented type: string timeout_ms: - description: Timeout speifies the amount of time devportal will wait for the downstream service to report an openapi spec back + description: Timeout specifies the amount of time devportal will wait for the downstream service to report an openapi spec back type: integer url: description: URL is the URL used for obtaining docs diff --git a/manifests/ambassador/ambassador-crds.yaml b/manifests/ambassador/ambassador-crds.yaml index af956314e2..f303605177 100644 --- a/manifests/ambassador/ambassador-crds.yaml +++ b/manifests/ambassador/ambassador-crds.yaml @@ -232,7 +232,7 @@ spec: description: Service is the service being documented type: string timeout_ms: - description: Timeout speifies the amount of time devportal will wait for the downstream service to report an openapi spec back + description: Timeout specifies the amount of time devportal will wait for the downstream service to report an openapi spec back type: integer url: description: URL is the URL used for obtaining docs diff --git a/pkg/api/getambassador.io/v2/devportal_types.go b/pkg/api/getambassador.io/v2/devportal_types.go index 3de7eff8e9..be22af6f15 100644 --- a/pkg/api/getambassador.io/v2/devportal_types.go +++ b/pkg/api/getambassador.io/v2/devportal_types.go @@ -55,7 +55,7 @@ type DevPortalDocsSpec struct { // URL is the URL used for obtaining docs URL string `json:"url,omitempty"` - // Timeout speifies the amount of time devportal will wait + // Timeout specifies the amount of time devportal will wait // for the downstream service to report an openapi spec back Timeout int `json:"timeout_ms,omitempty"` } diff --git a/python/tests/test_irmapping.py b/python/tests/test_irmapping.py new file mode 100644 index 0000000000..0d649369bd --- /dev/null +++ b/python/tests/test_irmapping.py @@ -0,0 +1,57 @@ +import copy +import logging +import sys + +import pytest + +logging.basicConfig( + level=logging.INFO, + format="%(asctime)s test %(levelname)s: %(message)s", + datefmt='%Y-%m-%d %H:%M:%S' +) + +logger = logging.getLogger("ambassador") + +from ambassador import Config, IR +from ambassador.fetch import ResourceFetcher +from ambassador.utils import NullSecretHandler + +def _get_ir_config(yaml): + aconf = Config() + fetcher = ResourceFetcher(logger, aconf) + fetcher.parse_yaml(yaml) + aconf.load_all(fetcher.sorted()) + + secret_handler = NullSecretHandler(logger, None, None, "0") + ir = IR(aconf, file_checker=lambda path: True, secret_handler=secret_handler) + + assert ir + return ir + + +@pytest.mark.compilertest +def test_ir_mapping(): + yaml = """ +apiVersion: getambassador.io/v2 +kind: Mapping +name: slowsvc-slow +namespace: ambassador +prefix: /slow/ +service: slowsvc +timeout_ms: 1000 +docs: + path: /endpoint + display_name: "slow service" + timeout_ms: 8000 +""" + + conf = _get_ir_config(yaml) + all_mappings = [] + for i in conf.groups.values(): + all_mappings = all_mappings + i.mappings + + slowsvc_mappings = [x for x in all_mappings if x['name'] == 'slowsvc-slow'] + assert(len(slowsvc_mappings) == 1) + print(slowsvc_mappings[0].as_dict()) + assert(slowsvc_mappings[0].docs['timeout_ms'] == 8000) + From 3c53df77971ee6ba38a7c41e9c2f2cf324412edb Mon Sep 17 00:00:00 2001 From: Flynn Date: Thu, 22 Jul 2021 15:54:57 -0400 Subject: [PATCH 12/18] Exclude hotfix tags when looking at versions. Signed-off-by: Flynn --- builder/builder.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builder/builder.sh b/builder/builder.sh index 92fbd181a1..05bb060766 100755 --- a/builder/builder.sh +++ b/builder/builder.sh @@ -315,8 +315,8 @@ module_version() { dirty="" fi # The _previous_ tag, plus a git delta, like 'v1.13.3-117-g2434c437f'... or, if we're _on_ - # a tag, just something like 'v1.13.3'. - GIT_DESCRIPTION=$(git describe --tags --match 'v*') + # a tag, just something like 'v1.13.3'. Don't let hotfix tags appear here. + GIT_DESCRIPTION=$(git describe --tags --match 'v*' --exclude '*-hf.*') echo GIT_DESCRIPTION="\"$GIT_DESCRIPTION\"" # Do we have a '-' in our GIT_DESCRIPTION? From 8da6ca12f92bf998c973f16ed3ea7010db71ee20 Mon Sep 17 00:00:00 2001 From: Aidan Hahn Date: Mon, 26 Jul 2021 15:43:40 -0700 Subject: [PATCH 13/18] fix pytest schema checks Signed-off-by: Aidan Hahn --- python/schemas/v2/Mapping.schema | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/python/schemas/v2/Mapping.schema b/python/schemas/v2/Mapping.schema index 87bb09aebd..85c99055e4 100644 --- a/python/schemas/v2/Mapping.schema +++ b/python/schemas/v2/Mapping.schema @@ -85,7 +85,9 @@ "properties": { "path": { "type": "string" }, "url": { "type": "string" }, - "ignored": { "type": "boolean" } + "ignored": { "type": "boolean" }, + "timeout_ms": { "type": "integer" }, + "display_name": { "type": "string" } }, "additionalProperties": false }, From 4f4ac2b2e668b09449c34a9fd5eb7b99b9ddc34c Mon Sep 17 00:00:00 2001 From: Flynn Date: Fri, 23 Jul 2021 18:10:39 -0400 Subject: [PATCH 14/18] Allow switching the ambex ratelimiter off. Signed-off-by: Flynn --- cmd/ambex/main.go | 2 ++ cmd/ambex/ratelimit.go | 36 +++++++++++++++++++++++++++--------- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/cmd/ambex/main.go b/cmd/ambex/main.go index 9ceb1fa310..30dcd5afba 100644 --- a/cmd/ambex/main.go +++ b/cmd/ambex/main.go @@ -715,6 +715,8 @@ func (l loggerv3) OnFetchResponse(req *v3discovery.DiscoveryRequest, res *v3disc l.Debugf("V3 Fetch response: %v -> %v", req, res) } +// NOTE WELL: this Main() does NOT RUN from entrypoint! This one is only relevant if you +// explicitly run Ambex by hand. func Main(ctx context.Context, Version string, rawArgs ...string) error { usage := memory.GetMemoryUsage() go usage.Watch(ctx) diff --git a/cmd/ambex/ratelimit.go b/cmd/ambex/ratelimit.go index 0404b2bff4..0c671acbf1 100644 --- a/cmd/ambex/ratelimit.go +++ b/cmd/ambex/ratelimit.go @@ -6,7 +6,8 @@ import ( "strconv" "time" - _debug "github.com/datawire/ambassador/pkg/debug" + "github.com/datawire/ambassador/pkg/debug" + "github.com/datawire/dlib/dlog" ) // An Update encapsulates everything needed to perform an update (of envoy configuration). The @@ -32,18 +33,30 @@ func Updater(ctx context.Context, updates <-chan Update, getUsage MemoryGetter) } type debugInfo struct { - Times []time.Time `json:"times"` - StaleCount int `json:"staleCount"` - StaleMax int `json:"staleMax"` - Synced bool `json:"synced"` + Times []time.Time `json:"times"` + StaleCount int `json:"staleCount"` + StaleMax int `json:"staleMax"` + Synced bool `json:"synced"` + DisableRatelimiter bool `json:"disableRatelimiter"` } func updaterWithTicker(ctx context.Context, updates <-chan Update, getUsage MemoryGetter, drainTime time.Duration, ticker *time.Ticker, clock func() time.Time) error { - dbg := _debug.FromContext(ctx) + dbg := debug.FromContext(ctx) info := dbg.Value("envoyReconfigs") + // Is the rate-limiter meant to be active at all? + disableRatelimiter, err := strconv.ParseBool(os.Getenv("AMBASSADOR_AMBEX_NO_RATELIMIT")) + + if err != nil { + disableRatelimiter = false + } + + if disableRatelimiter { + dlog.Info(ctx, "snapshot ratelimiter DISABLED") + } + // This slice holds the times of any updates we have made. This lets us compute how many stale // configs are being held in memory since we can filter this list down to just those times that // are between now - drain-time and now, i.e. we keep only the events that are more recent than @@ -57,7 +70,7 @@ func updaterWithTicker(ctx context.Context, updates <-chan Update, getUsage Memo for { // The basic idea here is that we wakeup whenever we either a) get a new snapshot to update, // or b) the timer ticks. In case a) we update the "latest" variable so that it always holds - // the most recent desired Ufpdate. In either case, we filter the list of updateTimes so we + // the most recent desired Update. In either case, we filter the list of updateTimes so we // know exactly how many updates are in memory, and then based on that we decide whether we // can do another reconfig or whether we should wait until the next (tick|update) whichever // happens first. @@ -83,6 +96,11 @@ func updaterWithTicker(ctx context.Context, updates <-chan Update, getUsage Memo updateTimes = gcUpdateTimes(updateTimes, now, drainTime) usagePercent := getUsage() + + if disableRatelimiter { + usagePercent = 0 + } + var maxStaleReconfigs int switch { case usagePercent >= 90: @@ -114,7 +132,7 @@ func updaterWithTicker(ctx context.Context, updates <-chan Update, getUsage Memo staleReconfigs := len(updateTimes) - info.Store(debugInfo{updateTimes, staleReconfigs, maxStaleReconfigs, pushed}) + info.Store(debugInfo{updateTimes, staleReconfigs, maxStaleReconfigs, pushed, disableRatelimiter}) // Decide if we have enough capacity left to perform a reconfig. if maxStaleReconfigs > 0 && staleReconfigs >= maxStaleReconfigs { @@ -141,7 +159,7 @@ func updaterWithTicker(ctx context.Context, updates <-chan Update, getUsage Memo log.Infof("Pushing snapshot %+v", latest.Version) pushed = true - info.Store(debugInfo{updateTimes, staleReconfigs, maxStaleReconfigs, pushed}) + info.Store(debugInfo{updateTimes, staleReconfigs, maxStaleReconfigs, pushed, disableRatelimiter}) } } From 25eede677c5b3d5a82c3b38c0cc56752e1999988 Mon Sep 17 00:00:00 2001 From: Flynn Date: Fri, 23 Jul 2021 21:32:46 -0400 Subject: [PATCH 15/18] CHANGELOG Signed-off-by: Flynn --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8669fe9fc7..5e67526cf7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,6 +70,10 @@ Please see the [Envoy documentation](https://www.envoyproxy.io/docs/envoy/latest - 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. +- Change: Set `AMBASSADOR_AMBEX_NO_RATELIMIT` to `true` to completely disable ratelimiting Envoy + reconfiguration under memory pressure. This can help performance with the endpoint or Consul resolvers, + but could make OOMkills more likely with large configurations. The default is `false`, meaning that + the rate limiter is active. - Bugfix: Fixed a regression when specifying a comma separated string for `cors.origins` on the `Mapping` resource ### Ambassador Edge Stack From 444a2b1be25fb153c07f7b0c62a4647f5dc9b1ff Mon Sep 17 00:00:00 2001 From: Aidan Hahn Date: Mon, 26 Jul 2021 16:47:17 -0700 Subject: [PATCH 16/18] add changelog notes for devportal timeout changes --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8669fe9fc7..52c1f5d0dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,12 +70,14 @@ Please see the [Envoy documentation](https://www.envoyproxy.io/docs/envoy/latest - 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. +- Change: A new field is added to the `Docs` field in the `Mapping` resource: `timeout_ms` is an optional integer that, when present, configures the number of milliseconds the Devportal HTTP client will wait to access the docs endpoint of the service mapping. - Bugfix: Fixed a regression when specifying a comma separated string for `cors.origins` on the `Mapping` resource ### Ambassador Edge Stack - Change: Consul certificate-rotation logging now includes the fingerprints and validity timestamps of certificates being rotated. +- Change: Devportal leverages the `timeout_ms` field in the `docs` field of the `Mapping` resource to configure the HTTP client it uses to fetch API specifications. ## [1.13.9] June 30, 2021 [1.13.9]: https://github.com/emissary-ingress/emissary/compare/v1.13.8...v1.13.9 From 405477bc19f91e62b1a1df8d32e3b6b4b74356cc Mon Sep 17 00:00:00 2001 From: Flynn Date: Tue, 27 Jul 2021 16:09:22 -0400 Subject: [PATCH 17/18] Update CHANGELOG for 1.13.10 Signed-off-by: Flynn --- CHANGELOG.md | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 51696776b7..182c863152 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,23 +68,26 @@ 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 - controlled by the `AMBASSADOR_AMBEX_SNAPSHOT_COUNT` environment variable; set it to 0 to disable. The default is 30. -- Change: A new field is added to the `Docs` field in the `Mapping` resource: `timeout_ms` is an optional integer that, - when present, configures the number of milliseconds the Devportal HTTP client will wait to access the docs endpoint - of the service mapping. +- Bugfix: Fixed a regression when specifying a comma separated string for `cors.origins` on the + `Mapping` resource. ([#3609]) +- 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. - Change: Set `AMBASSADOR_AMBEX_NO_RATELIMIT` to `true` to completely disable ratelimiting Envoy - reconfiguration under memory pressure. This can help performance with the endpoint or Consul resolvers, - but could make OOMkills more likely with large configurations. The default is `false`, meaning that - the rate limiter is active. -- Bugfix: Fixed a regression when specifying a comma separated string for `cors.origins` on the `Mapping` resource + reconfiguration under memory pressure. This can help performance with the endpoint or Consul + resolvers, but could make OOMkills more likely with large configurations. The default is `false`, + meaning that the rate limiter is active. ### Ambassador Edge Stack -- Change: Consul certificate-rotation logging now includes the fingerprints and validity timestamps of certificates - being rotated. -- Change: Devportal leverages the `timeout_ms` field in the `docs` field of the `Mapping` resource to configure the - HTTP client it uses to fetch API specifications. +- Bugfix: The `Mapping` resource can now specify `docs.timeout_ms` to set the timeout when the + Dev Portal is fetching API specifications. +- Bugfix: The Dev Portal will now strip HTML tags when displaying search results, showing just + the actual content of the search result. +- Change: Consul certificate-rotation logging now includes the fingerprints and validity + timestamps of certificates being rotated. + +[#3609]: https://github.com/emissary-ingress/emissary/issues/3609 ## [1.13.9] June 30, 2021 [1.13.9]: https://github.com/emissary-ingress/emissary/compare/v1.13.8...v1.13.9 From 17c6bb2c70c46c88305f966eecf875c80c658c21 Mon Sep 17 00:00:00 2001 From: Flynn Date: Tue, 27 Jul 2021 16:15:57 -0400 Subject: [PATCH 18/18] Another minor CHANGELOG tweak Signed-off-by: Flynn --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 182c863152..b4c5cf28e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -78,7 +78,7 @@ Please see the [Envoy documentation](https://www.envoyproxy.io/docs/envoy/latest resolvers, but could make OOMkills more likely with large configurations. The default is `false`, meaning that the rate limiter is active. -### Ambassador Edge Stack +### Ambassador Edge Stack only - Bugfix: The `Mapping` resource can now specify `docs.timeout_ms` to set the timeout when the Dev Portal is fetching API specifications.