Skip to content
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

Merged
merged 11 commits into from
Aug 30, 2023

Conversation

achaurasiaConfluent
Copy link
Contributor

@achaurasiaConfluent achaurasiaConfluent commented Aug 10, 2023

Pull Request check list

  • Commit conforms to CONTRIBUTING.md?
  • Proper tests/regressions included?
  • Documentation updated?

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

pkg/server/plugin/nodeattestor/awsiid/iid.go Outdated Show resolved Hide resolved
pkg/server/plugin/nodeattestor/awsiid/iid.go Outdated Show resolved Hide resolved
pkg/server/plugin/nodeattestor/awsiid/iid_test.go Outdated Show resolved Hide resolved
pkg/server/plugin/nodeattestor/awsiid/iid_test.go Outdated Show resolved Hide resolved
doc/plugin_server_nodeattestor_aws_iid.md Outdated Show resolved Hide resolved
doc/plugin_server_nodeattestor_aws_iid.md Outdated Show resolved Hide resolved
@rturner3 rturner3 changed the title Chore/aws parition support Chore/aws partition support Aug 15, 2023
@achaurasiaConfluent achaurasiaConfluent force-pushed the chore/aws-parition-support branch 2 times, most recently from f48d671 to 4cf0898 Compare August 19, 2023 19:47
Copy link
Collaborator

@rturner3 rturner3 left a 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?

pkg/server/plugin/nodeattestor/awsiid/iid.go Outdated Show resolved Hide resolved
pkg/server/plugin/nodeattestor/awsiid/iid_test.go Outdated Show resolved Hide resolved
pkg/server/plugin/nodeattestor/awsiid/iid_test.go Outdated Show resolved Hide resolved
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>
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>
rturner3
rturner3 previously approved these changes Aug 24, 2023
Copy link
Collaborator

@rturner3 rturner3 left a 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))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
err := doConfig(t, coreConfig, fmt.Sprintf("partition = \"%s\"", partition))
err := doConfig(t, coreConfig, fmt.Sprintf("partition = %q", partition))

nit: one small simplification

Copy link
Contributor Author

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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"aws",
defaultPartition,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@azdagron azdagron added this to the 1.8.0 milestone Aug 29, 2023
azdagron and others added 2 commits August 29, 2023 16:58
Signed-off-by: Anil Chaurasia <achaurasia@confluent.io>
@azdagron azdagron merged commit 2f54e71 into spiffe:main Aug 30, 2023
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants