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

SNS Tag update #221

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ services:
- '127.0.0.1:4566:4566' # LocalStack Gateway
- '127.0.0.1:4510-4559:4510-4559' # external services port range
environment:
- SERVICES=sns,sqs,s3
- SERVICES=sns,sqs,s3,sts
- DEBUG=0
- DATA_DIR=${DATA_DIR-}
- DOCKER_HOST=unix:///var/run/docker.sock
Expand Down
2 changes: 1 addition & 1 deletion packages/sns/docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ services:
- '127.0.0.1:4566:4566' # LocalStack Gateway
- '127.0.0.1:4510-4559:4510-4559' # external services port range
environment:
- SERVICES=sns,sqs,s3
- SERVICES=sns,sqs,s3,sts
- DEBUG=0
- DATA_DIR=${DATA_DIR-}
- DOCKER_HOST=unix:///var/run/docker.sock
Expand Down
1 change: 1 addition & 0 deletions packages/sns/lib/sns/AbstractSnsService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export type SNSTopicConfig = {
export type ExtraSNSCreationParams = {
queueUrlsWithSubscribePermissionsPrefix?: string | readonly string[]
allowedSourceOwner?: string
forceTagUpdate?: boolean
}

export type SNSCreationConfig = {
Expand Down
2 changes: 2 additions & 0 deletions packages/sns/lib/utils/snsInitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ export async function initSnsSqs(
allowedSourceOwner: creationConfig.allowedSourceOwner,
topicArnsWithPublishPermissionsPrefix: creationConfig.topicArnsWithPublishPermissionsPrefix,
logger: extraParams?.logger,
forceTagUpdate: creationConfig.forceTagUpdate,
},
)
if (!subscriptionArn) {
Expand Down Expand Up @@ -229,6 +230,7 @@ export async function initSns(
const topicArn = await assertTopic(snsClient, creationConfig.topic!, {
queueUrlsWithSubscribePermissionsPrefix: creationConfig.queueUrlsWithSubscribePermissionsPrefix,
allowedSourceOwner: creationConfig.allowedSourceOwner,
forceTagUpdate: creationConfig.forceTagUpdate,
})
return {
topicArn,
Expand Down
1 change: 1 addition & 0 deletions packages/sns/lib/utils/snsSubscriber.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ export async function subscribeToTopic(
const { queueUrl, queueArn } = await assertQueue(sqsClient, queueConfiguration, {
topicArnsWithPublishPermissionsPrefix: extraParams?.topicArnsWithPublishPermissionsPrefix,
updateAttributesIfExists: extraParams?.updateAttributesIfExists,
forceTagUpdate: extraParams?.forceTagUpdate,
})

const subscribeCommand = new SubscribeCommand({
Expand Down
61 changes: 54 additions & 7 deletions packages/sns/lib/utils/snsUtils.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {
type CreateTopicCommandInput,
type SNSClient,
TagResourceCommand,
paginateListTopics,
} from '@aws-sdk/client-sns'
import {
Expand All @@ -12,11 +13,12 @@ import {
SetTopicAttributesCommand,
UnsubscribeCommand,
} from '@aws-sdk/client-sns'
import type { Either } from '@lokalise/node-core'
import { type Either, isError } from '@lokalise/node-core'
import { calculateOutgoingMessageSize as sqsCalculateOutgoingMessageSize } from '@message-queue-toolkit/sqs'

import type { ExtraSNSCreationParams } from '../sns/AbstractSnsService'

import { GetCallerIdentityCommand, STSClient } from '@aws-sdk/client-sts'
import { generateTopicSubscriptionPolicy } from './snsAttributeUtils'

type AttributesResult = {
Expand Down Expand Up @@ -82,13 +84,19 @@ export async function assertTopic(
topicOptions: CreateTopicCommandInput,
extraParams?: ExtraSNSCreationParams,
) {
const command = new CreateTopicCommand(topicOptions)
const response = await snsClient.send(command)

if (!response.TopicArn) {
throw new Error('No topic arn in response')
let topicArn: string
try {
const command = new CreateTopicCommand(topicOptions)
const response = await snsClient.send(command)
if (!response.TopicArn) throw new Error('No topic arn in response')
topicArn = response.TopicArn
} catch (err) {
// We only manually build ARN in case of tag update
if (!extraParams?.forceTagUpdate) throw err
// To build ARN we need topic name and error should be "topic already exist with different tags"
if (!topicOptions.Name || !isTopicAlreadyExistWithDifferentTagsError(err)) throw err
topicArn = await buildTopicArn(snsClient, topicOptions.Name)
}
const topicArn = response.TopicArn

if (extraParams?.queueUrlsWithSubscribePermissionsPrefix || extraParams?.allowedSourceOwner) {
const setTopicAttributesCommand = new SetTopicAttributesCommand({
Expand All @@ -102,6 +110,13 @@ export async function assertTopic(
})
await snsClient.send(setTopicAttributesCommand)
}
if (extraParams?.forceTagUpdate) {
const tagTopicCommand = new TagResourceCommand({
ResourceArn: topicArn,
Tags: topicOptions.Tags,
})
await snsClient.send(tagTopicCommand)
}

return topicArn
}
Expand Down Expand Up @@ -178,3 +193,35 @@ export async function getTopicArnByName(snsClient: SNSClient, topicName?: string
*/
export const calculateOutgoingMessageSize = (message: unknown) =>
sqsCalculateOutgoingMessageSize(message)

const isTopicAlreadyExistWithDifferentTagsError = (error: unknown) =>
!!error &&
isError(error) &&
'Error' in error &&
!!error.Error &&
typeof error.Error === 'object' &&
'Code' in error.Error &&
'Message' in error.Error &&
typeof error.Error.Message === 'string' &&
error.Error.Code === 'InvalidParameter' &&
error.Error.Message.includes('already exists with different tags')

/**
* Manually builds the ARN of a topic based on the current AWS account and the topic name.
* It follows the following pattern: arn:aws:sns:<region>:<account-id>:<topic-name>
* Doc -> https://docs.aws.amazon.com/IAM/latest/UserGuide/reference-arns.html
*/
const buildTopicArn = async (client: SNSClient, topicName: string) => {
Copy link
Owner

Choose a reason for hiding this comment

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

I would expose this from utils, this can be useful in a variety of different use-cases.

Would be nice to also write some tests for it, to ensure that the built arn matches the real arn created by SNS based on same parameters

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mmm, Okay! will expose it 🙏

const region =
typeof client.config.region === 'string' ? client.config.region : await client.config.region()

const stsClient = new STSClient({
Copy link
Owner

@kibertoad kibertoad Oct 18, 2024

Choose a reason for hiding this comment

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

recreating client on every request sounds like a lot of overhead, I'd suggest to pass it as a dependency instead

Copy link
Collaborator Author

@CarlosGamero CarlosGamero Oct 18, 2024

Choose a reason for hiding this comment

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

Do you mean as a dependency for users? the idea was to hide its usage by using SNS config parameters, although I am not really sure if it is a good idea 🤔.

What do you mean by recreating it in every request? I think it is done just once on init right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I got it, you mean to the method, would be fine to create it on publisher/consumer creation and pass it as parameter?

Copy link
Owner

Choose a reason for hiding this comment

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

wouldn't it be done once per buildTopicArn, which realistically means once for every topic?

Copy link
Owner

Choose a reason for hiding this comment

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

yeah, same as we pass sqsClient and snsClient

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But as a dependency passed by the user, or do you think it is a good idea to build it internally base on SNS config? Advantage of not requesting it is that the API won’t change

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(Btw sorry, I am already a bit slow 😓)

Copy link
Owner

Choose a reason for hiding this comment

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

I would pass by user, would be less surprising than globally cached dependency. but we can also make an optional parameter and build automatically if not passed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense to me! Will think about it and address the change in the new PR
Thanks Igor 🙌🏽

endpoint: client.config.endpoint,
region,
credentials: client.config.credentials,
endpointProvider: client.config.endpointProvider,
})
const identityResponse = await stsClient.send(new GetCallerIdentityCommand({}))

return `arn:aws:sns:${region}:${identityResponse.Account}:${topicName}`
}
5 changes: 3 additions & 2 deletions packages/sns/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
"peerDependencies": {
"@aws-sdk/client-sns": "^3.632.0",
"@aws-sdk/client-sqs": "^3.632.0",
"@aws-sdk/client-sts": "^3.632.0",
"@message-queue-toolkit/core": ">=15.0.0",
"@message-queue-toolkit/schemas": ">=2.0.0",
"@message-queue-toolkit/sqs": "^17.0.0"
Expand All @@ -40,11 +41,11 @@
"@aws-sdk/client-s3": "^3.670.0",
"@aws-sdk/client-sns": "^3.670.0",
"@aws-sdk/client-sqs": "^3.670.0",
"@biomejs/biome": "1.9.3",
"@kibertoad/biome-config": "^1.2.1",
"@message-queue-toolkit/core": "*",
"@message-queue-toolkit/s3-payload-store": "*",
"@message-queue-toolkit/sqs": "*",
"@biomejs/biome": "1.9.3",
"@kibertoad/biome-config": "^1.2.1",
"@types/node": "^22.7.5",
"@vitest/coverage-v8": "^2.1.2",
"awilix": "^12.0.1",
Expand Down
Loading
Loading