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.

Sign up for GitHub

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

Jump to bottom

Extending restricted api to http #6435

Merged
merged 33 commits into from
Nov 3, 2023
Merged

Extending restricted api to http #6435

merged 33 commits into from
Nov 3, 2023

Conversation

nnshah1
Copy link
Contributor

@nnshah1 nnshah1 commented Oct 16, 2023 •
edited
Loading

Adding support for restricting access to http apis based on header key and value (similar to grpc restricted protocols)

  • Refactored command line parsing to create a map of features to header key and values
  • Added check to all Handle APIs

@nnshah1 nnshah1 marked this pull request as draft October 16, 2023 02:50
@nnshah1 nnshah1 changed the title Extending restricted api extension to http Extending restricted api to http Oct 16, 2023
@nnshah1 nnshah1 marked this pull request as ready for review October 24, 2023 20:52
nnshah1
nnshah1 commented Oct 24, 2023
View reviewed changes
src/http_server.cc Outdated Show resolved Hide resolved
nv-kmcgill53
nv-kmcgill53 reviewed Oct 25, 2023
View reviewed changes
Copy link
Contributor

@nv-kmcgill53 nv-kmcgill53 left a comment

Choose a reason for hiding this comment

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

Looks good for the most part. I think we do want to take 10 min and think about if we can sanely eliminate the check for the hot path.

src/http_server.cc Outdated Show resolved Hide resolved
src/http_server.cc Show resolved Hide resolved
src/http_server.cc Outdated Show resolved Hide resolved
src/http_server.cc Outdated Show resolved Hide resolved
GuanLuo
GuanLuo previously approved these changes Oct 25, 2023
View reviewed changes
src/common.h Outdated Show resolved Hide resolved
Tabrizian
Tabrizian reviewed Oct 25, 2023
View reviewed changes
src/grpc/grpc_server.cc Outdated Show resolved Hide resolved
qa/L0_http/test.sh Show resolved Hide resolved
src/http_server.cc Outdated Show resolved Hide resolved
rmccorm4
rmccorm4 reviewed Oct 26, 2023
View reviewed changes
src/http_server.cc Outdated Show resolved Hide resolved
nv-kmcgill53
nv-kmcgill53 reviewed Oct 27, 2023
View reviewed changes
src/http_server.cc Outdated Show resolved Hide resolved
Tabrizian
Tabrizian reviewed Oct 27, 2023
View reviewed changes
src/http_server.cc Outdated Show resolved Hide resolved
nv-kmcgill53
nv-kmcgill53 previously approved these changes Oct 27, 2023
View reviewed changes
rmccorm4
rmccorm4 reviewed Oct 31, 2023
View reviewed changes
src/command_line_parser.cc Outdated Show resolved Hide resolved
src/restricted_features.h Show resolved Hide resolved
Tabrizian
Tabrizian previously approved these changes Oct 31, 2023
View reviewed changes
nv-kmcgill53
nv-kmcgill53 previously approved these changes Oct 31, 2023
View reviewed changes
@nnshah1 nnshah1 dismissed stale reviews from nv-kmcgill53 and Tabrizian via 2d2e7e7 October 31, 2023 22:04
@nnshah1 nnshah1 force-pushed the nnshah1-restricted-http-api branch from 22f427b to 123515d Compare November 2, 2023 06:48
nnshah1
nnshah1 commented Nov 2, 2023
View reviewed changes
Dockerfile.win10.min Outdated Show resolved Hide resolved
@nnshah1 nnshah1 merged commit 4b481a6 into main Nov 3, 2023
3 checks passed
@nnshah1 nnshah1 deleted the nnshah1-restricted-http-api branch November 3, 2023 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rmccorm4 rmccorm4 rmccorm4 approved these changes

@GuanLuo GuanLuo Awaiting requested review from GuanLuo

@Tabrizian Tabrizian Awaiting requested review from Tabrizian

@nv-kmcgill53 nv-kmcgill53 Awaiting requested review from nv-kmcgill53

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nnshah1 @Tabrizian @rmccorm4 @GuanLuo @nv-kmcgill53