-
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
aws: Include IMDSv2 support for EC2 instances #4086
Conversation
0b39316
to
ad01ac8
Compare
/* | ||
* Create IMDS context | ||
* Returns NULL on error | ||
* Note: Setting the FLB_IO_ASYNC flag is the job of the client. | ||
* Flag Set Example: flags &= ~(FLB_IO_ASYNC) | ||
*/ |
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 love the code comments on each function and the thought put into overall organization here 👏
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.
+1
ad01ac8
to
69ad9ea
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.
First pass review, will do a second look tomorrow
struct flb_aws_imds_config { | ||
int use_imds_version; // FLB_AWS_IMDS_VERSION_EVALUATE for automatic detection | ||
}; | ||
|
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 like the constants, esp FLB_AWS_IMDS_VERSION_EVALUATE, and I am fine with this separate config structure, but I want to say tho that it feels a bit like over-kill to have a structure for a single option.
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.
Here is my motivation behind the config structure. A default config structure is declared in imds.c. This default config structure is used as a config template when our IMDS struct is initialized. That way in the future, if we want to add some more options to customize the IMDS' behavior (e.g. fallback behavior or number of retries) we can add a default configuration in one place and not have to worry about changing IMDS initialization code in other places.
Use
You can see the use of the default config structure used as a configuration template in create_ec2_provider() (and anywhere else IMDS is initialized future).
If you think that this ease of maintenance motivation is not strong enough to warrant the additional code, I can take the parameter in the create function. That's not a problem. Lmk.
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.
Most looks code. Good work! As it does refactor many work, be sure to test everything is fine - not only the feature support.
/* | ||
* Create IMDS context | ||
* Returns NULL on error | ||
* Note: Setting the FLB_IO_ASYNC flag is the job of the client. | ||
* Flag Set Example: flags &= ~(FLB_IO_ASYNC) | ||
*/ |
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.
+1
} | ||
|
||
if (c->resp.status != 200) { | ||
if (c->resp.payload_size > 0) { |
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.
More debug information is good but should we add more checks just for debug log? Just want to discuss this a little bit,
2a2f31f
to
c90a41f
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.
Good work! Mostly LGTM. Left some small comments. Also, you might want to rebase your commits. Like one for supporting IMDSv2 and the other one is for adding unit tests.
fa844ec
to
d9b623c
Compare
@matthewfala may be we should call out IMDSv2 support here: https://github.com/fluent/fluent-bit-docs/blob/master/administration/aws-credentials.md |
c7e97e3
d9b623c
to
c7e97e3
Compare
Here's the diff from the last approved commit to after the forced push:
|
Here's the diff from the second forced push where c7e97e3 is the last commit before the forced push
|
Signed-off-by: Matthew Fala <[email protected]>
…truct Signed-off-by: Matthew Fala <[email protected]>
c7e97e3
to
8a0c6e0
Compare
Add IMDSv2 Support to AWS Fluent Bit plugins
Instances with only IMDSv2 enabled should now have access to Fluent Bit.
Enter
[N/A]
in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
Documentation
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.
Config File
Debug Log
Disabled IMDS -> Enabled IMDS
aws ec2 modify-instance-metadata-options --instance-id i-001234567 --http-endpoint disabled
aws ec2 modify-instance-metadata-options --instance-id i-004f8a124a6f1a9eb --http-endpoint enabled
Result:
Valgrind
System Operation
Unit Tests
Design Motivation
Motivation
Here is my motivation behind creating a new structure for IMDS. The plugins/filters_aws/aws.c file has essentially the same routines listed here, but it takes the specialized filter context for each routine. The previous version of IMDSv1 that was here, had some of the same code, copy pasted, but this code took a flb_aws_client as context. I could have continued the copy paste pattern, but though that code duplication would result in greater overhead in maintenance in the future.
aws.c Compatability
The IMDS struct is designed to be fully compatible with plugins/aws_filters/aws.c with only about 5 lines of code changes. Moreover, much of aws.c's code can be deleted. Wiring the IMDS struct to the aws filter is out of the scope of this feature (since I am not going to retest the aws plugin), but also list the 5 code changes in aws.c that need to take place.
I expect that in the future, as changes are needed in aws.c and aws.c is being actively tested again, we can wire up the IMDS struct to aws.c and delete all the duplicated code.