-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
opensearch: aoss support - 1.9 #6448
Conversation
648bbd2
to
d5186e7
Compare
plugins/out_es/es.c
Outdated
ctx->aws_region, ctx->aws_service_name, | ||
S3_MODE_SIGNED_PAYLOAD, |
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.
@matthewfala We need to test this with opensearch without using the new option to ensure there is no regression.
Since this applies to all requests, even if the new option is not set. Need to check if opensearch always allows S3 signing mode.
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.
Actually I'm pretty sure you're always allowed to send this header, since it increases the security of the request
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.
https://docs.aws.amazon.com/opensearch-service/latest/developerguide/request-signing.html
This suggests all of opensearch supports it.
plugins/out_es/es.c
Outdated
@@ -862,6 +862,7 @@ static void cb_es_flush(struct flb_event_chunk *event_chunk, | |||
} | |||
else { | |||
/* The request was issued successfully, validate the 'error' field */ | |||
flb_plg_error(ctx->ins, "Headers: \n%s", c->resp.data); |
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.
I think this was added for debugging, let's remove it for prod release?
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.
Great. Good catch. Updated.
plugins/out_opensearch/opensearch.c
Outdated
@@ -879,6 +879,7 @@ static void cb_opensearch_flush(struct flb_event_chunk *event_chunk, | |||
} | |||
else { | |||
/* The request was issued successfully, validate the 'error' field */ | |||
flb_plg_error(ctx->ins, "Headers: \n%s", c->resp.data); |
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.
see above comment
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.
updated
d5186e7
to
218ea0b
Compare
@@ -62,8 +62,8 @@ static flb_sds_t add_aws_auth(struct flb_http_client *c, | |||
flb_http_add_header(c, "User-Agent", 10, "aws-fluent-bit-plugin", 21); | |||
|
|||
signature = flb_signv4_do(c, FLB_TRUE, FLB_TRUE, time(NULL), |
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.
So I've heard that:
You can't include Content-Length as a signed header, otherwise you'll get an invalid signature error.
Currently, content-length is added in the http client add_host_and_content_length
function which is called when you create a client- do we need to add some code here to ignore that header? I think I remember that when we worked on this before we did...
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.
I guess if we test it and we don't get that error response then it works
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.
resolved.
I'm wondering if aoss needs content-type
to also be excluded from the signed headers. This PR does not exclude content-type
from the signed headers
plugins/out_es/es.c
Outdated
flb_http_add_header(c, "Content-Type", 12, "application/x-ndjson", 20); | ||
|
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 moved this line down so that its not part of the signed headers?
I think this is not a long term maintainable solution if that's the case... what if another maintainer goes and switches it back?
May be we need something like an option to pass a list of headers to flb_signv4_do that should not be signed?
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.
Also where is the fix for Content-Length not being signed?
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.
Noted. This was actually a part of Zhonghui's commits. No change on my part. The new "unsigned_headers" parameter will omit what we want. Following up on whether Content-Type should be omitted.
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.
Need to move this back up
This pr needs more testing. |
0, ctx->aws_provider); | ||
0, NULL, ctx->aws_provider); |
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.
@matthewfala Do you know why we have AWS Auth in the bigquery output here?? Isn't that a google only product??
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.
Supposedly, AWS clients can use google cloud products with AWS credentials via Workload Identity Federation
I see someone mention this usecase for fluent bit in the following comment: #5084
plugins/out_es/es.c
Outdated
ctx->aws_region, ctx->aws_service_name, | ||
S3_MODE_SIGNED_PAYLOAD, NULL, | ||
ctx->aws_provider); |
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.
Don't we want to add the AWS unsigned headers in es
as well? Otherwise there is no point in adding the AWS_Service_Name field to this plugin I think?
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.
Goood point
@@ -40,6 +40,7 @@ flb_sds_t flb_signv4_do(struct flb_http_client *c, int normalize_uri, | |||
time_t t_now, | |||
char *region, char *service, | |||
int s3_mode, | |||
struct mk_list *unsigned_headers, /* flb_slist */ |
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.
looks like this broke the signv4 unit tests...
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.
That's odd
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.
resolved
b552e14
to
1a98580
Compare
9c2a50a
to
b4d38b9
Compare
ce52152
to
f7be9ba
Compare
AWS-for-fluent-bit test imageTest ImageRepo 1: DocumentationCherry-picked AWS-for-fluent-bit test image.
Output AWS-for-fluent-bit cherrypick file
Out
|
@@ -40,6 +40,7 @@ flb_sds_t flb_signv4_do(struct flb_http_client *c, int normalize_uri, | |||
time_t t_now, | |||
char *region, char *service, | |||
int s3_mode, | |||
struct mk_list *unsigned_headers, /* flb_slist */ |
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.
resolved
@@ -62,8 +62,8 @@ static flb_sds_t add_aws_auth(struct flb_http_client *c, | |||
flb_http_add_header(c, "User-Agent", 10, "aws-fluent-bit-plugin", 21); | |||
|
|||
signature = flb_signv4_do(c, FLB_TRUE, FLB_TRUE, time(NULL), |
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.
resolved.
I'm wondering if aoss needs content-type
to also be excluded from the signed headers. This PR does not exclude content-type
from the signed headers
ctx->aws_region, "es", | ||
0, | ||
ctx->aws_region, ctx->aws_service_name, | ||
S3_MODE_SIGNED_PAYLOAD, ctx->aws_unsigned_headers, |
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.
This is the magic line of code.
@@ -61,6 +61,8 @@ struct flb_elasticsearch { | |||
/* one for the standard chain provider, one for sts assume role */ | |||
struct flb_tls *aws_sts_tls; | |||
char *aws_session_name; | |||
char *aws_service_name; | |||
struct mk_list *aws_unsigned_headers; |
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.
typically in code in FLB that I've seen we have just struct mk_list
no pointer on context that way we don't have to alloc it and its simpler I think...
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.
I had it like that at first, however there is an issue with just having a struct directly nested:
When you run the clean up logic, you need to know whether the slist has been initialized so that you can call:
flb_slist_destroy(ctx->aws_unsigned_headers);
accordingly.
aws_unsigned_headers might not be initialized depending on when the initialization script fails.
Thus using a pointer serves as a double purpose:
- Let us know if the slist is created
- store the slist
I could also have made a boolean to track this, but I think it is more straight forward and conforms better with the other code to just allocate memory.
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.
I see. I think that's fine but its not the only way to do it.
There is:
static inline int mk_list_is_set(struct mk_list *head)
{
if (head->next && head->prev) {
return 0;
}
return -1;
}
If you just embed the mk_list struct on the context and alloc it with calloc then you can use this function because the next and prev pointers will be initialized to NULL.
@@ -73,6 +73,8 @@ struct flb_opensearch { | |||
/* one for the standard chain provider, one for sts assume role */ | |||
struct flb_tls *aws_sts_tls; | |||
char *aws_session_name; | |||
char *aws_service_name; | |||
struct mk_list *aws_unsigned_headers; |
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.
Same comment as on es
Signed-off-by: Matthew Fala <falamatt@amazon.com>
Signed-off-by: Matthew Fala <falamatt@amazon.com>
f7be9ba
to
1917a77
Compare
master pr: #6612
OpenSearch and ElasticSearch improvements:
Several minor changes:
Testing
Configuration Files
Tested by FireLens team on Amazon AWS Opensearch with the following plugin configurations
Test Results
No error messages were detected. Logs are successfully received at the Opensearch cluster.
Test Valgrind Results
valgrind --leak-check=full ./fluent-bit -c ../../.vscode/fluent-bit-config/fluent-bit.conf
Opensearch
ElasticSearch
Unfortunately, es on both 1.9 and aoss-1.9 (this pr's branch) have numerous networking invalid read errors. This may need to be further investigated. There are no memory leaks introduced though:
es 1.9
es aoss-1.9
Interpretation of Valgrind Results
It's not clear why the invalid reads only appear in es and not the opensearch plugin which have nearly identical code.
CloudWatch_logs Plugin Regression Testing
CloudWatch_logs plugin is also tested with this PR to ensure that aws auth still works for other plugins.
CloudWatch receives logs successfully.
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.