-
Notifications
You must be signed in to change notification settings - Fork 2
[CXP-524] feat: sync AWS resource tags behind --sync-resource-tags #158
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -145,6 +145,10 @@ type accountResourceType struct { | |
| // this so accounts never point at a Root/OU resource that this run never syncs, which | ||
| // would otherwise leave a dangling "MISSING RESOURCE" parent. | ||
| hierarchySync HierarchySyncFlags | ||
|
|
||
| // syncResourceTags gates the per-account organizations:ListTagsForResource call. | ||
| // See tags.go for why this is opt-in. | ||
| syncResourceTags bool | ||
| } | ||
|
|
||
| func (o *accountResourceType) ResourceType(_ context.Context) *v2.ResourceType { | ||
|
|
@@ -194,6 +198,15 @@ func (o *accountResourceType) List(ctx context.Context, _ *v2.ResourceId, opts r | |
| l.Debug("baton-aws: account found", zap.String("name", name), zap.String("account_id", accountId), zap.String("account_status", string(status))) | ||
|
|
||
| profile := accountProfile(ctx, account) | ||
|
|
||
| if o.syncResourceTags { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won't fix. We avoid changing existing profile fields, so |
||
| tags, err := fetchAccountTags(ctx, o.orgClient, accountId) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| profile[tagsProfileField] = tags | ||
| } | ||
|
|
||
| resourceOpts := []resourceSdk.ResourceOption{ | ||
| resourceSdk.WithAnnotation(annos), | ||
| // Sparse ACLs: advertise the scope-binding type as a child so the SDK | ||
|
|
@@ -973,6 +986,7 @@ func accountBuilder( | |
| region string, | ||
| identityClient client.IdentityStoreClient, | ||
| hierarchySync HierarchySyncFlags, | ||
| syncResourceTags bool, | ||
| ) *accountResourceType { | ||
| return &accountResourceType{ | ||
| resourceType: resourceTypeAccount, | ||
|
|
@@ -983,6 +997,7 @@ func accountBuilder( | |
| identityInstance: identityInstance, | ||
| region: region, | ||
| hierarchySync: hierarchySync, | ||
| syncResourceTags: syncResourceTags, | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,6 +75,15 @@ func (o *accountIAMResourceType) List(ctx context.Context, _ *v2.ResourceId, opt | |
| Id: awsSdk.ToString(account.Id), | ||
| } | ||
| profile := accountProfile(ctx, account) | ||
|
|
||
| if o.aws != nil && o.aws.syncResourceTags { | ||
| tags, err := fetchAccountTags(ctx, o.orgClient, awsSdk.ToString(account.Id)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: Unlike |
||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| profile[tagsProfileField] = tags | ||
| } | ||
|
|
||
| userResource, err := resourceSdk.NewAppResource( | ||
| awsSdk.ToString(account.Name), | ||
| resourceTypeAccountIam, | ||
|
|
||
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.
🟡 Suggestion: three places that document every other optional sync flag were not given the tags equivalent — the self-hosted env-var example (~line 789, which lists
BATON_SYNC_SECRETS,BATON_SYNC_IAM_USER_CONSOLE_ACCESS,BATON_SYNC_SSO_USER_LAST_LOGINbut notBATON_SYNC_RESOURCE_TAGS), the cloud-hosted setup Steps (~line 698, which has an "Optional. Enable Sync IAM User Console Access" step), and the "Section 4: Other permissions" bullet list (~line 647) that explains each optional permission in the policy JSON this PR edited at lines 499-501. Following theiam:GetLoginProfileprecedent in each spot would keep the flag discoverable from both install paths.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.
Addressed in 06a9318.