-
Notifications
You must be signed in to change notification settings - Fork 27
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
IMDSv1 and IMDSv2 abstraction for the retrieval of HANA DB Credentials from AWS Secrets Manager #112
Conversation
…t IMDSv1 and IMDSv2.
…t IMDSv1 and IMDSv2.
…t IMDSv1 and IMDSv2.
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.
Hey @schniber
I'm afraid we cannot include this change so easily. The package ec2_metadata
is not directly part of the repository where this package is published.
That's why we opted for the regular requests
package. Would be some way to continue using requests
?
PD:
In any case:
- I think that the tests would break
- The
requirements.txt
andspec
file should be updated
Hey @arbulu89, Thanks a lot for your feedback that I took into account in the latest commit. below is the executing of the tests with the makefile:
PR is ready for your review. Thanks a lot. Bests. |
…t IMDSv1 and IMDSv2.
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.
@schniber we should have tests on this new code.
That the old ones are passing is just a coincidence, as most probably the first requests return code is 200 in the tests.
We should mock with the unauthorized return code and see that the proper call are done
Besides that, I would've put some comment on that new if condition.
I guess that if it returns unauthorized, it means that we are using IMDSv2 version. It would be nice to comment that down
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.
Thank you @schniber ,
I think we could improve the test a bit, but in any case it looks good enough.
damn, now it looks like |
Hey @arbulu89, just to be sure, you ask @schniber do replace all |
Hi @wombelix and @schniber, only changing the https://github.com/SUSE/hanadb_exporter/blob/master/.github/workflows/exporter-ci.yml file to This is a temporary workaround. I have opened #113 for fixing this in the future. |
Thanks a lot @diegoakechi for the clarification, @schniber can you please replace |
Hello all, I have just pushed changes related to our last discussion. Please feel free to run a peer review and provide feedback. Thanks a lot ! Bests. |
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.
Thanks a lot @arbulu89, He's on vacation until next week, but we talked about the topic last week and he mentioned that some help in regards to the testing would be highly appreciated. So if you have some capacity to take his test cases and improve them, that would be really great. I'm also willing to contribute, feel free to let me know how I can help. |
@schniber added me as contributor to his fork, I will give it a shot to improve the tests a little |
Signed-off-by: Dominik Wombacher <domwom@amazon.de>
Based on feedback during PR review and pseudo code provided by arbulu89. Mocks for both get requests via side_effects and addition of mock for put. One assert added to verify if imdsv2 get included the test token. Signed-off-by: Dominik Wombacher <domwom@amazon.de>
Signed-off-by: Dominik Wombacher <domwom@amazon.de>
@arbulu89 I tried to brush up the tests, indentation and things like whitespaces and line width a little. Please let me know if I understood your recommendations correctly and if you think further improvements is required or if the PR can be merged. Result of local test run:
|
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.
Hey @wombelix
We are almost there!
2 things to polish the tests and we are ready to merge
IMDSv2 assert get assert extended to cover call to meta service with and without token header. Put assert added to verify submission of metadata token ttl header. Signed-off-by: Dominik Wombacher <domwom@amazon.de>
'side_effects' changed to 'side_effect', redundant return_value assignments removed. Signed-off-by: Dominik Wombacher <domwom@amazon.de>
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.
Thank you @wombelix
I appreciate your patience on this.
Let's merge!
Hello,
This is a proposed quickfix for #111 that allows the use of the python library ec2-metadata to read the region name as opposed to querying the instance metadata services directly with the requests library.
This will allow users running on EC2 instances that are configure to use IMDSv2 (as well as IMDSv1) to use the prometheus hana db exporter while safely storing the HANA DB Credentials in AWS Secrets Manager.
Thanks for your peer review.
Bests.