Skip to content
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

Add GetAllHeaders method to golang HTTP filter #33821

Merged
merged 16 commits into from
May 16, 2024

Conversation

willemveerman
Copy link
Contributor

@willemveerman willemveerman commented Apr 26, 2024

Commit Message: Add GetAllHeaders method to golang HTTP filter
Additional Description: Adds a new method which returns a copy of the underlying map[string][]string object which contains all the request headers.
Risk Level: Low
Testing: Unit test
Docs Changes: none required - doesn't change the configuration params of the plugin
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Copy link

Hi @willemveerman, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #33821 was opened by willemveerman.

see: more, trace.

Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #33821 was opened by willemveerman.

see: more, trace.

Signed-off-by: Willem Veerman <6502426+willemveerman@users.noreply.github.com>
Signed-off-by: Willem Veerman <6502426+willemveerman@users.noreply.github.com>
Signed-off-by: Willem Veerman <6502426+willemveerman@users.noreply.github.com>
Signed-off-by: Willem Veerman <6502426+willemveerman@users.noreply.github.com>
@willemveerman willemveerman changed the title initial WIP GetAllHeaders Add GetAllHeaders method to golang HTTP filter Apr 29, 2024
Signed-off-by: Willem Veerman <6502426+willemveerman@users.noreply.github.com>
@willemveerman willemveerman marked this pull request as ready for review May 6, 2024 14:45
Copy link
Member

@doujiang24 doujiang24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, we need some tests though the implementation is simple.

contrib/golang/filters/http/source/go/pkg/http/type.go Outdated Show resolved Hide resolved
@phlax phlax self-assigned this May 7, 2024
Signed-off-by: Willem Veerman <6502426+willemveerman@users.noreply.github.com>
@phlax
Copy link
Member

phlax commented May 10, 2024

@doujiang24 i think this may be waiting for further review

@willemveerman
Copy link
Contributor Author

@doujiang24 i think this may be waiting for further review

I still need to add the tests

Signed-off-by: Willem Veerman <6502426+willemveerman@users.noreply.github.com>
Signed-off-by: Willem Veerman <6502426+willemveerman@users.noreply.github.com>
@willemveerman
Copy link
Contributor Author

@doujiang24 I've added what I think is a test, but I'm not sure how to run the tests - can you shed some light?

@doujiang24
Copy link
Member

@willemveerman Here is a doc section: Testing Envoy with Bazel

For the current ci failure:

compilepkg: missing strict dependencies:
	/b/f/w/contrib/golang/filters/http/test/test_data/basic/filter.go: import of "golang.org/x/exp/slices"

https://dev.azure.com/cncf/envoy/_build/results?buildId=170352&view=logs&j=8c169225-0ae8-53bd-947f-07cb81846cb5&t=d1a98671-b7ba-5fbf-f06c-ff337c010df4&l=159

As the error message shows it missing dependency.
it would be a easier way to remove such dependency, it might be a bit complicated to introduce a golang dependency in bazel.

Signed-off-by: Willem Veerman <6502426+willemveerman@users.noreply.github.com>
Signed-off-by: Willem Veerman <6502426+willemveerman@users.noreply.github.com>
Signed-off-by: Willem Veerman <6502426+willemveerman@users.noreply.github.com>
Signed-off-by: Willem Veerman <6502426+willemveerman@users.noreply.github.com>
@willemveerman
Copy link
Contributor Author

willemveerman commented May 14, 2024

@doujiang24 OK, it's ready for review now

Actually, I'm just going to improve the tests a little

Signed-off-by: Willem Veerman <6502426+willemveerman@users.noreply.github.com>
Signed-off-by: Willem Veerman <6502426+willemveerman@users.noreply.github.com>
Signed-off-by: Willem Veerman <6502426+willemveerman@users.noreply.github.com>
@willemveerman
Copy link
Contributor Author

OK I've added one further test to cover the case of the headerMap being changed, in order to verify that the slices are being copied. Sorry for delay, I had a hard time building the tests on my m2 arm, but found a workaround. @doujiang24

Copy link
Member

@doujiang24 doujiang24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a small style issue, otherwise, lgtm, thanks~

contrib/golang/filters/http/source/go/pkg/http/type.go Outdated Show resolved Hide resolved
Signed-off-by: Willem Veerman <6502426+willemveerman@users.noreply.github.com>
Copy link
Member

@doujiang24 doujiang24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks

@phlax phlax merged commit 6e7370d into envoyproxy:main May 16, 2024
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants