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

Refactoring for external auth. Vol II #66

Merged
merged 2 commits into from
Aug 20, 2024
Merged

Refactoring for external auth. Vol II #66

merged 2 commits into from
Aug 20, 2024

Conversation

didierofrivia
Copy link
Member

@didierofrivia didierofrivia commented Aug 12, 2024

Part of the work needed for #58
This PR follows the work done in #64, it aims to extract the grpc service logic.

With this refactor we now can build different implementations of Service in order to call external GRPC services to validate messages.

Base automatically changed from refactoring-1-bis to main August 13, 2024 09:39
src/service.rs Outdated Show resolved Hide resolved
@didierofrivia didierofrivia force-pushed the refactoring-2 branch 2 times, most recently from fde0bb8 to 989f04f Compare August 14, 2024 08:34
@didierofrivia didierofrivia marked this pull request as ready for review August 14, 2024 09:21
upstream_name,
RATELIMIT_SERVICE_NAME,
RATELIMIT_METHOD_NAME,
initial_metadata,
Copy link
Member

Choose a reason for hiding this comment

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

I guess this should be self.metadata and initial_metadata disappears from that method signature, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've made the grpc_call outside the impl block so we can mock it in the tests, that's why it doesn't take a self param. Should it be include it within the RateLimitService impl?


const RATELIMIT_SERVICE_NAME: &str = "envoy.service.ratelimit.v3.RateLimitService";
const RATELIMIT_METHOD_NAME: &str = "ShouldRateLimit";
pub struct RateLimitService<'a> {
Copy link
Member

Choose a reason for hiding this comment

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

I think, if metadata is the "headers", and in our case the tracing ones, you can just claim ownership of these str and get rid of <'a>

@didierofrivia didierofrivia self-assigned this Aug 20, 2024
@didierofrivia didierofrivia reopened this Aug 20, 2024
@didierofrivia didierofrivia merged commit cebb46d into main Aug 20, 2024
13 checks passed
@didierofrivia didierofrivia deleted the refactoring-2 branch August 20, 2024 15:21
@didierofrivia didierofrivia mentioned this pull request Oct 1, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants