-
Notifications
You must be signed in to change notification settings - Fork 687
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 support for initial_metadata to AuthService #3639
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Ryan Burn <[email protected]>
Signed-off-by: Ryan Burn <[email protected]>
Signed-off-by: Ryan Burn <[email protected]>
Signed-off-by: Ryan Burn <[email protected]>
Signed-off-by: Ryan Burn <[email protected]>
Signed-off-by: Ryan Burn <[email protected]>
Signed-off-by: Ryan Burn <[email protected]>
Signed-off-by: Ryan Burn <[email protected]>
…itial-metadata Signed-off-by: Ryan Burn <[email protected]>
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.
One quick change, if you could -- thanks very much!! 🙂
@@ -51,6 +51,7 @@ type AuthServiceSpec struct { | |||
AllowedRequestHeaders []string `json:"allowed_request_headers,omitempty"` | |||
AllowedAuthorizationHeaders []string `json:"allowed_authorization_headers,omitempty"` | |||
AddAuthHeaders map[string]BoolOrString `json:"add_auth_headers,omitempty"` | |||
InitialMetadata map[string]BoolOrString `json:"initial_metadata,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.
Would it make sense to just make this a map[string]string
? AddAuthHeaders
uses the bool
option in a very specific way, and we generally would rather not add more of those...
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.
done
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.
@kflynn - were there any other changes you wanted?
Signed-off-by: Ryan Burn <[email protected]>
2b79046
to
c0c61cd
Compare
I also need to add |
Description
Support specifying initial_metadata in AuthService.
When specified, envoy will add the initial_metadata to its ext auth request.
From Envoy's docs:
Testing
Added a kat test case to verify the auth service receives the metadata.
Checklist
[ x] I made sure to update
CHANGELOG.md
.Remember, the CHANGELOG needs to mention:
[ x] This is unlikely to impact how Ambassador performs at scale.
Remember, things that might have an impact at scale include:
My change is adequately tested.
Remember when considering testing:
to run legacy-mode tests manually (set
AMBASSADOR_LEGACY_MODE=true
and run the tests).(This will be fixed soon.)
I updated
DEVELOPING.md
with any any special dev tricks I had to use to work on this code efficiently.