-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[configgrpc, confighttp] Support lists of name/value pairs for headers #13996
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #13996 +/- ##
==========================================
- Coverage 92.27% 92.26% -0.01%
==========================================
Files 656 657 +1
Lines 41050 41093 +43
==========================================
+ Hits 37877 37914 +37
- Misses 2172 2176 +4
- Partials 1001 1003 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
codeboten
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this is the right approach 👍🏻
#### Description #43509 changed the `make for-all` Makefile target to access the `ALL_MODS` variable through environment variables instead of expanding it into the shell command, in order to bypass Windows' 8192 byte command line length limit. However, the environment variable was not defined, which led to `make for-all` becoming a no-op. This can be confirmed by running `make for-all CMD='echo test'`. (I discovered this issue because [this PR](open-telemetry/opentelemetry-collector#13996) in core repository, which should be failing `contrib-tests`, suddenly started passing them. This was because the job inserts `replace` statements in a copy of contrib using `for-all`; failing to do that caused the contrib tests to run against whatever version of core is imported here rather than against the PR's code.) This PR fixes that oversight, by adding the `export` keyword to the `ALL_MODS` variable.
|
Next steps:
|
#### Description open-telemetry#43509 changed the `make for-all` Makefile target to access the `ALL_MODS` variable through environment variables instead of expanding it into the shell command, in order to bypass Windows' 8192 byte command line length limit. However, the environment variable was not defined, which led to `make for-all` becoming a no-op. This can be confirmed by running `make for-all CMD='echo test'`. (I discovered this issue because [this PR](open-telemetry/opentelemetry-collector#13996) in core repository, which should be failing `contrib-tests`, suddenly started passing them. This was because the job inserts `replace` statements in a copy of contrib using `for-all`; failing to do that caused the contrib tests to run against whatever version of core is imported here rather than against the PR's code.) This PR fixes that oversight, by adding the `export` keyword to the `ALL_MODS` variable.
|
I pushed the corresponding changes in contrib on my fork here; all in all, about 150 small changes, mostly in tests. Along the way, I ended up modifying this PR's core API to make it match the native
I will mark this PR as ready for review, but as a reminder, we can still change course and go for the "backwards-compatible" approach if the benefits of this PR are not deemed to be worth the additional complexity. |
config/configgrpc/configgrpc.go
Outdated
|
|
||
| // The headers associated with gRPC requests. | ||
| Headers map[string]configopaque.String `mapstructure:"headers,omitempty"` | ||
| Headers *configopaque.MapList `mapstructure:"headers,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can do without the * that would be better
config/confighttp/client.go
Outdated
| // Existing header values are overwritten if collision happens. | ||
| // Header values are opaque since they may be sensitive. | ||
| Headers map[string]configopaque.String `mapstructure:"headers,omitempty"` | ||
| Headers *configopaque.MapList `mapstructure:"headers,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
|
I am going to mark this as draft until it is ready for another review, feel free to mark as ready whenever that happens! |
|
I think I've addressed all the previous comments, and I've filed a corresponding draft PR in contrib (open-telemetry/opentelemetry-collector-contrib#43701) which is almost passing tests, so I'll mark this as ready for another round of review. |
mx-psi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Since this introduces a new type on a 1.x module, I want to get at least one more approval before moving forward with this.
507664f
#### Description Fork of #43832 with fixes from #43701 to accommodate breaking changes from open-telemetry/opentelemetry-collector#13996 --------- Co-authored-by: Pablo Baeyens <[email protected]>
#### Description Fork of open-telemetry#43832 with fixes from open-telemetry#43701 to accommodate breaking changes from open-telemetry/opentelemetry-collector#13996 --------- Co-authored-by: Pablo Baeyens <[email protected]>
#### Description Fork of open-telemetry#43832 with fixes from open-telemetry#43701 to accommodate breaking changes from open-telemetry/opentelemetry-collector#13996 --------- Co-authored-by: Pablo Baeyens <[email protected]>
Description
This PR introduces:
MapList[T]type, which is equivalent to[]{ Name string, Value T }, but can be unmarshalled from amap[string]Tas well. This type is defined in a newinternal/maplistpackage.configopaque.MapListconfiggrpcandconfighttpare changed to use this type instead ofmap[string]configopaque.Stringfor storing header configuration, so as to be consistent with the SDK declarative config (see tracking issue).Note that while this is relatively elegant, it causes a number of breaking changes. An alternative would be to keep the current
map[string]configopaque.Stringtypes, and add an Unmarshal method on the surrounding struct: see this PoC.Link to tracking issue
Fixes #13930
Testing
I added a basic unit test in
internal/maplist.