Fix Addresses Used in IamAwsProvider #1460
Conversation
| path="/latest/meta-data/iam/security-credentials/", | ||
| ), | ||
| ), | ||
| url+"/latest/meta-data/iam/security-credentials/", |
There was a problem hiding this comment.
This breaks if custom_endpoint is passed. I think you would need to leave it as is.
There was a problem hiding this comment.
Thanks for the feedback!
I noticed the same logic is already applied on
minio-py/minio/credentials/providers.py
Line 501 in 759a5f3
which follows the same pattern used in the Go implementation
https://github.com/minio/minio-go/blob/ab1c2fe050a4dd502c863178696a54c44610ca82/pkg/credentials/iam_aws.go#L334
However, the Go implementation has an issue: it doesn’t account for cases where a custom endpoint contains a path. This leads to the assumption that the custom_endpoint doesn’t have a path, which is why I addressed it this way on line 511.
To resolve this, we have two options when a custom_endpoint is passed:
- Assume it does not contain a path and use simple string concatenation (
+) in all instances where we manipulate the URL. (essentially what this PR did) - Assume it can contain a path and ensure proper handling by using urllib.parse.SplitResult in all usages to safely override the path.
Given that the Go implementation itself is inconsistent—sometimes appending directly to the URL and other times parsing the URL—it would be helpful to align on one approach to avoid further confusion and potential issues.
Which approach do you think would be best to adopt moving forward?
| headers=headers, | ||
| ) | ||
| role_names = res.data.decode("utf-8").split("\n") | ||
| if not role_names: | ||
| raise ValueError(f"no IAM roles attached to EC2 service {url}") | ||
| url += "/" + role_names[0].strip("\r") | ||
| url += "/latest/meta-data/iam/security-credentials/" + role_names[0].strip("\r") |
There was a problem hiding this comment.
May be you would need to do to same logic as in line #511
sam-mosleh
force-pushed
the
fix-iam-credentials-provider
branch
from
b5630b1
to
39c2f51
Compare
|
@balamurugana I totally get that these commits might get rewritten completely depending on the approach you choose, but my OCD brain just couldn’t resist not fixing these linting errors! 😅 |
Fixes #1459