-
-
Notifications
You must be signed in to change notification settings - Fork 559
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
influxdb inlet (auth proxy) #8430
Conversation
4a04b36
to
00e944f
Compare
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.
The implementing looks good!
There is some room for simplification around the CLI commands and on the endpoint.
Also, this way of creating inlet+interceptor is pretty new (kafka is the only user of interceptor but has many services and multiple inlets).
Ideally, we would like to keep inlet creation independent of interceptors while avoiding the re-creation of CRUD for every inlet type.
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.
Non-blocking, but I would appreciate it if you could generalize this code, to intercept HTTPs header, other uses for it are already appearing.
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.
For example, @metaclips is already modifying the Host
header on the client side to make services work
} | ||
let out = guard | ||
.state | ||
.process_http_buffer(buffer, &token.unwrap_or_default())?; |
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.
In this case, I think it would be safer to close the portal by returning an error.
That should be easier to debug.
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.
you mean returning an error for the intercept() function, would that close the portal?.
Right now this case just log a warn/err on the logs.
implementations/rust/ockam/ockam_api/src/influxdb_token_lease/token_lease_refresher.rs
Outdated
Show resolved
Hide resolved
implementations/rust/ockam/ockam_api/src/influxdb_token_lease/token_lease_refresher.rs
Outdated
Show resolved
Hide resolved
implementations/rust/ockam/ockam_api/src/influxdb_token_lease/token_lease_refresher.rs
Show resolved
Hide resolved
} = create_inlet; | ||
|
||
let prefix_route = if is_http_auth_inlet { |
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.
You can avoid modifying the TCP creation by creating an endpoint which handles both the creation of the http_auth_interceptor
and the inlet creation (by passing a proper prefix route).
This helps keep the TCP inlet creation code straightforward, regardless of the number of special cases. You can also add any number of custom parameters you need.
Kafka handles the interceptor similarly (but with some extra complexity) using services:
pub(super) async fn start_kafka_inlet_service( |
(Post, ["node", "services", DefaultAddress::KAFKA_INLET]) => encode_response( |
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.
it's significant more boilerplate 🤷 . Agree there should be some better way, but we can think on generalization once there are more than one use of it
implementations/rust/ockam/ockam_api/src/nodes/service/tcp_inlets/node_manager_worker.rs
Outdated
Show resolved
Hide resolved
|
||
/// Create TCP Inlets | ||
#[derive(Clone, Debug, Args)] | ||
pub struct CreateCommand { |
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.
You should be able to ""inherit"" fields by declaring a field containing the inlet Create
command with the #[command(flatten)]
attribute. From the clap point of view would be as-if the fields were locally declared.
Other than InfluxDb, I can't think of other use cases where the |
implementations/rust/ockam/ockam_api/src/influxdb_token_lease/token_lease_refresher.rs
Show resolved
Hide resolved
c714860
to
32748f5
Compare
add an http inlet (tcp inlet + http interceptor) that modify http requests passing through it, attaching an Authorization token retrieved from a token lease manager service. Inteded to be used with influxdb for now, from that the name.
81ff6c6
to
86e0d63
Compare
closing in favor of #8461 |
Assuming the identity is enrolled in a project that has influxdb addon enabled, like:
An http authentication inlet is started as:
Then sending influxdb' request (writes, queries) to
localhost:8087
will cause the request to be sent to upstream influxdb instance (the outlet would be pointing to influxdb), but with the request modified to include anAuthentication
header with a token obtained from the project' token lease manager. This token is rotated as needed, without clients having to worry about that.The same can be done through a node' config file as well:
Notes: