-
Notifications
You must be signed in to change notification settings - Fork 469
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
Chore/aws partition support #4427
Chore/aws partition support #4427
Conversation
5d79df6
to
7686103
Compare
f48d671
to
4cf0898
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.
It looks like some unintended commits from main
got included into the branch associated with this PR. Could you clean this up to only include the commits you are proposing using git rebase -i main
followed by a git push -f <remote> achaurasiaConfluent:chore/aws-parition-support
?
8e0cb7e
to
c928cd1
Compare
Signed-off-by: Anil Chaurasia <achaurasia@confluent.io>
Signed-off-by: Anil Chaurasia <achaurasia@confluent.io>
Signed-off-by: Anil Chaurasia <achaurasia@confluent.io>
Signed-off-by: Anil Chaurasia <achaurasia@confluent.io>
c928cd1
to
75717a7
Compare
Signed-off-by: Anil Chaurasia <achaurasia@confluent.io>
Signed-off-by: Anil Chaurasia <achaurasia@confluent.io>
75717a7
to
a0078a0
Compare
a936ef8
to
fc8faf3
Compare
Signed-off-by: Anil Chaurasia <achaurasia@confluent.io>
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.
LGTM, just offered one small improvement if you get a chance
|
||
t.Run("success when valid partitions specified ", func(t *testing.T) { | ||
for _, partition := range partitions { | ||
err := doConfig(t, coreConfig, fmt.Sprintf("partition = \"%s\"", partition)) |
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.
err := doConfig(t, coreConfig, fmt.Sprintf("partition = \"%s\"", partition)) | |
err := doConfig(t, coreConfig, fmt.Sprintf("partition = %q", partition)) |
nit: one small simplification
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. Updated.
// No constant was found in the sdk, using the list of paritions defined on | ||
// the page https://docs.aws.amazon.com/IAM/latest/UserGuide/reference-arns.html | ||
partitions = []string{ | ||
"aws", |
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.
"aws", | |
defaultPartition, |
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.
Signed-off-by: Anil Chaurasia <achaurasia@confluent.io>
Pull Request check list
Affected functionality
SPIRE Server AWSIID node attestor
Description of change
Currently spire node attestation only works in default aws partition. This PR allow server to attest nodes in non default partition.
Note that server can not cross partition boundary.
Which issue this PR fixes
#4423
Signed-off-by: achaurasia@confluent.io