Update datadog tracer config proto and preserve remote_config default behavior for envoy v1.31 (#10145) #10191
Conversation
… behavior for envoy v1.31 (#10145)
github-actions
bot
added
keep pr updated
|
Visit the preview URL for this PR (updated for commit 7a5e4e1): https://gloo-edge--pr10191-andyfong-datadog-pro-odct0tv2.web.app (expires Fri, 18 Oct 2024 17:34:16 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 77c2b86e287749579b7ff9cadb81e099042ef677 |
|
Issues linked to changelog: |
…der from envoy 1.31
| @@ -57,14 +57,13 @@ var _ = Describe("Local Rate Limit", func() { | |||
| expectNotRateLimitedWithOutXRateLimitHeader = func() { | |||
| response := expectNotRateLimited() | |||
| // Since the values of the x-rate-limit headers change with time, we only check the presence of these header keys and not match their value | |||
| ExpectWithOffset(1, response).ToNot(matchers.ContainHeaderKeys([]string{"x-ratelimit-reset", "x-ratelimit-limit", "x-ratelimit-remaining"}), | |||
| ExpectWithOffset(1, response).ToNot(matchers.ContainHeaderKeys([]string{"x-ratelimit-limit", "x-ratelimit-remaining"}), | |||
There was a problem hiding this comment.
envoy 1.31 switch the local ratelimit implementation to AtomicBucket which is no longer timer based, so the x-ratelimit-reset is no longer a thing and will not send out that header anymore. Will call this out in docs upgrade faq and the way to revert back to the timer-based bucket implementation.
|
|
||
| remoteConfig := glooDatadogConfig.GetRemoteConfig() | ||
| if remoteConfig == nil { | ||
| // An empty RemoteConfig object on the envoy side means enabling RemoteConfig |
There was a problem hiding this comment.
This is a super helpful comment!
| google.protobuf.Duration polling_interval = 1; | ||
|
|
||
| // Disabled remote config. | ||
| // This field does not exist in envoy's config but allow us to preserve the default behavior |
There was a problem hiding this comment.
nit: Since this is our API, and we are exposing this specifically for backwards compatibility, should we clarify what the default behavior is?
There was a problem hiding this comment.
If have other updates I need to push, I will clarify that. However, in general, because all the proto field default to false, 0 or "". The implied default behavior is disabled=false. That's the reason to pick disabled as the field instead of enabled (In general, I really don't like anything that's double negative to mean positive coming from the C++ world but I was taught that in go and protobuf, rely on the default value instead.)
| "x-ratelimit headers should not be present for non rate limited requests") | ||
| } | ||
|
|
||
| expectNotRateLimitedWithXRateLimitHeader = func() { | ||
| response := expectNotRateLimited() | ||
| // Since the x-ratelimit-reset header value changes with time, we only check the presence of this header key and not match its value | ||
| ExpectWithOffset(2, response).To(matchers.ContainHeaderKeys([]string{"x-ratelimit-reset", "x-ratelimit-limit", "x-ratelimit-remaining"}), | ||
| ExpectWithOffset(2, response).To(matchers.ContainHeaderKeys([]string{"x-ratelimit-limit", "x-ratelimit-remaining"}), |
There was a problem hiding this comment.
I think the above comment about why the reset header will not be present is really useful, and I wonder if it could be even more effective if it were placed in code. By that I mean, what if we added an assertion to confirm that the reset header is not contained in the response headers?
There was a problem hiding this comment.
I should put the comment back but just remove the x-ratelimit-reset part. In general, do we ever do tests with the runtime guard option because we will call out in the docs if customer needs this back, they can set the runtime guard setting (for the next 6 months before envoy remove it). So, technically, we are not testing a path that customer might need be using but that might be a whole new can of worms. It's the right things to do but not sure it's worth it as they will be throwaway code in 6 months.
There was a problem hiding this comment.
Since I got 2 approval and the Enterprise side tests a mostly passing (re-running some flakes), I will probably add the comment in another PR without possibly a changelog only update to call out the runtime guard.
andy-fong
removed
the
work in progress
Description
x-ratelimit-resetheader as it's no longer set in envoy v1.31 (reversible with runtime guard envoy.reloadable_features.no_timer_based_rate_limit_token_bucketAPI changes
Added fields from envoy 1.31:
For remote_config, we also added a
disabledfield which is not on the envoy sideCode changes
CI changes
None because this plugin only has unit test and does not have e2e test, the tests would pass even with old envoy (because we already imported the new go-control-plane some time ago that already has the new fields)
Docs changes
Context
Before envoy 1.31, the remote_config functionality is on by default and cannot be turned off. In order to maintain the behavior for potential customers that might rely on this, we enable the feature by default if it's
not explicitly disable. Customers who are using datadog tracer would not need to change anything when upgrading to gloo/gloo-ee 1.18.
slack-conversation: https://solo-io-corp.slack.com/archives/G01EERAK3KJ/p1728052383874909
Interesting decisions
Testing steps
Only rely on unit test on this one because we do not have a datadog setup to actually test this e2e.
Notes for reviewers
Checklist: