opensearch: aoss support - 1.9 #6448
Conversation
matthewfala
force-pushed
the
aoss-1.9
branch
from
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.
@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.
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.
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.
I think this was added for debugging, let's remove it for prod release?
There was a problem hiding this comment.
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); | |||
matthewfala
force-pushed
the
aoss-1.9
branch
from
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.
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.
I guess if we test it and we don't get that error response then it works
There was a problem hiding this comment.
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.
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.
Also where is the fix for Content-Length not being signed?
There was a problem hiding this comment.
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.
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.
@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.
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.
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?
| @@ -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.
looks like this broke the signv4 unit tests...
matthewfala
force-pushed
the
aoss-1.9
branch
from
b552e14
to
1a98580
Compare
matthewfala
force-pushed
the
aoss-1.9
branch
from
9c2a50a
to
b4d38b9
Compare
matthewfala
force-pushed
the
aoss-1.9
branch
from
ce52152
to
f7be9ba
Compare
matthewfala
requested review from
edsiper,
leonardo-albertovich,
fujimotos and
koleini
as code owners
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 */ | |||
| @@ -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.
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.
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.
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.
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.
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.
Same comment as on es
Signed-off-by: Matthew Fala <falamatt@amazon.com>
Signed-off-by: Matthew Fala <falamatt@amazon.com>
matthewfala
force-pushed
the
aoss-1.9
branch
from
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.confOpensearch
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.