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

HTTP Basic Auth filter #30079

Merged
merged 27 commits into from
Oct 30, 2023
Merged

Conversation

zhaohuabing
Copy link
Member

@zhaohuabing zhaohuabing commented Oct 11, 2023

The implementation for HTTP Basic Auth filter.

Description: HTTP Basic Authn is a frequently requested feature from users, I'd like to have Envoy to support it.

Related issue: #15365

Sponsor: wbpcode

Risk Level: low
Testing: Yes
Docs Changes: Yes
Release Notes: Yes
Platform Specific Features: No
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #30079 was opened by zhaohuabing.

see: more, trace.

@zhaohuabing zhaohuabing force-pushed the basic-auth-filter branch 3 times, most recently from d51179b to 947f186 Compare October 11, 2023 05:28
Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

Sounds a reasonable contribution, thanks. But I think at lease one maintainer sponsor and two reviewers for this new core extension.

All extensions must be sponsored by an existing maintainer. Sponsorship means that the maintainer will shepherd the extension through design/code reviews. Maintainers can self-sponsor extensions if they are going to write them, shepherd them, and maintain them.

Each extension must have two reviewers proposed for reviewing PRs to the extension. Neither of the reviewers must be a senior maintainer. Existing maintainers (including the sponsor) and other contributors can count towards this number. The initial reviewers will be codified in the CODEOWNERS file for long term maintenance. These reviewers can be swapped out as needed.

See https://github.com/envoyproxy/envoy/blob/main/EXTENSION_POLICY.md for more detail.

If you are willing, I can help to review or even sponsor it (this extension should be simple and won't take too much bandwidth), but we still need another reviewer to ensure the quality of extension.

Or you can add this extension as contrib extension. contrib extension have a relatively loose requirement.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Seems OK to me to add if @wbpcode is willing to review.

@zhaohuabing zhaohuabing changed the title API for the HTTP Basic Auth filter HTTP Basic Auth filter Oct 12, 2023
@zhaohuabing
Copy link
Member Author

zhaohuabing commented Oct 12, 2023

@wbpcode Since we agreed on the API, I converted this PR to draft and proceed with implementation.

@zhaohuabing zhaohuabing marked this pull request as draft October 12, 2023 02:30
@zhaohuabing zhaohuabing force-pushed the basic-auth-filter branch 7 times, most recently from 750e8b2 to ac45d4c Compare October 17, 2023 12:15
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
@zhaohuabing zhaohuabing marked this pull request as ready for review October 17, 2023 13:17
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
@wbpcode wbpcode self-assigned this Oct 18, 2023
CODEOWNERS Outdated Show resolved Hide resolved
@wbpcode
Copy link
Member

wbpcode commented Oct 24, 2023

Both is OK for this function (because this only be called once when configuration is loaded), although string view still be better to reduce unnecessary sub-string copy/creation.

I changed it back to std::string because the compiler was not happy about this line:

users->emplace(name, User{std::string(name), std::string(hash)});

SGTM, we can optimize it in the future anyway, if we have free time. Just merge main and add a release note. :)

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
wbpcode
wbpcode previously approved these changes Oct 24, 2023
Copy link
Member

@wbpcode wbpcode 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.

Hi, cc @mattklein123 could you also give a quick check in case I am missing something?

Copy link
Contributor

@cpakulski cpakulski left a comment

Choose a reason for hiding this comment

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

Few typos.
Should integration test be added to make sure that it works e-2-e?

@wbpcode
Copy link
Member

wbpcode commented Oct 25, 2023

Should integration test be added to make sure that it works e-2-e?

SGTM. But I have no strong opinion on this because this extension is pretty simple.

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
@zhaohuabing
Copy link
Member Author

Should integration test be added to make sure that it works e-2-e?

SGTM. But I have no strong opinion on this because this extension is pretty simple.

Should I add an e-2-e test? Looks like there are no e-2-e tests for HTTP filters in the test/integration directory, or I looked into the wrong place?

@wbpcode
Copy link
Member

wbpcode commented Oct 25, 2023

Integration tests are placed in sub directory of filter itself.

You can check test/extensions/filters/http/fault/fault_filter_integration_test.cc as a example to see how to write a integration test of http filter.

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
@zhaohuabing
Copy link
Member Author

@wbpcode @cpakulski Integration test has been added. PTAL.

@zhaohuabing
Copy link
Member Author

/retest

Copy link
Member

@wbpcode wbpcode 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.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Nice!

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.

4 participants