-
Notifications
You must be signed in to change notification settings - Fork 73
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
feat: Multi bucket support #1742
Changes from 13 commits
f48bb2d
3c49e6f
a3baa32
b1f2c85
c20f71d
df34ec0
ed78564
8b365c1
423f6ba
2ab396b
87f2fd9
0bf1075
9d28f1a
4b1a471
421fd59
9c27c2c
17cae19
85130e3
8729889
0913ad5
62b2a36
e8548cf
bc4d412
f6a1c12
cddcd97
2c03a6d
3c1c486
77df31d
9739169
ea59529
1b08c09
107bcf9
2f932ec
d67854e
4dc5553
bacfe86
14ddd73
cb21531
b776969
0ded8f7
c1b09b4
8079d28
ec113d0
3380bd9
92c6299
88ea17d
96bc326
07194c3
50ebda5
6ced7be
98567de
036862b
c8a36f7
9843705
35527d3
b797249
7b326b6
ac7e04b
5bf1cff
37f4e44
f208b87
440de9e
8647309
bc33e54
e9f6859
9bc296b
4ce8db5
cba6cec
5429861
3a2dd2b
865b947
ca0d12d
cff7306
db34c50
98414b0
99b483f
179a1bb
16999af
084bd2c
9cde018
3473440
d9bb2c2
53db426
5792d10
cf0d456
0e07a9f
436df66
3fffaba
cb7cb4f
e042ab1
87a35ca
336a517
9ade76a
f0d993e
f27e98b
e329d26
a66fe37
0940ed6
1a7f095
13f20cf
a825efe
079fd73
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
--- | ||
'@aws-amplify/deployed-backend-client': minor | ||
'@aws-amplify/backend-output-schemas': minor | ||
'@aws-amplify/backend-output-storage': minor | ||
'@aws-amplify/backend-storage': minor | ||
'@aws-amplify/client-config': minor | ||
'@aws-amplify/plugin-types': minor | ||
--- | ||
|
||
add support for multi-buckets |
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 |
---|---|---|
@@ -1,9 +1,12 @@ | ||
import { z } from 'zod'; | ||
|
||
const bucketSchema = z.record(z.string(), z.string()); | ||
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. should we type this stronger as well? 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. Done d67854e |
||
|
||
export const storageOutputSchema = z.object({ | ||
version: z.literal('1'), | ||
payload: z.object({ | ||
bucketName: z.string(), | ||
storageRegion: z.string(), | ||
buckets: z.array(bucketSchema).optional(), | ||
}), | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,9 +36,19 @@ export class StackMetadataBackendOutputStorageStrategy | |
new CfnOutput(this.stack, key, { value }); | ||
}); | ||
|
||
const metadataValue = | ||
this.stack.templateOptions.metadata && | ||
this.stack.templateOptions.metadata[keyName] | ||
? this.stack.templateOptions.metadata[keyName] | ||
: { stackOutputs: [] }; | ||
|
||
this.stack.addMetadata(keyName, { | ||
...metadataValue, | ||
version: backendOutputEntry.version, | ||
stackOutputs: Object.keys(backendOutputEntry.payload), | ||
stackOutputs: [ | ||
...metadataValue.stackOutputs, | ||
...Object.keys(backendOutputEntry.payload), | ||
], | ||
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. Could you explain this change? Edit: after reading rest of the code I'm guessing this is to solve a problem of "multiple defineStorage contributing to same stack metadata/output key". 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. 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. Good point @rtpascual! I somehow didn't see 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. Done e8548cf |
||
}); | ||
}; | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -14,7 +14,6 @@ import { IBucket } from 'aws-cdk-lib/aws-s3'; | |||||||||||||||||
import { ResourceAccessAcceptor } from '@aws-amplify/plugin-types'; | ||||||||||||||||||
import { ResourceAccessAcceptorFactory } from '@aws-amplify/plugin-types'; | ||||||||||||||||||
import { ResourceProvider } from '@aws-amplify/plugin-types'; | ||||||||||||||||||
import { StorageOutput } from '@aws-amplify/backend-output-schemas'; | ||||||||||||||||||
|
||||||||||||||||||
// @public (undocumented) | ||||||||||||||||||
export type AmplifyStorageFactoryProps = Omit<AmplifyStorageProps, 'outputStorageStrategy'> & { | ||||||||||||||||||
|
@@ -23,9 +22,13 @@ export type AmplifyStorageFactoryProps = Omit<AmplifyStorageProps, 'outputStorag | |||||||||||||||||
|
||||||||||||||||||
// @public (undocumented) | ||||||||||||||||||
export type AmplifyStorageProps = { | ||||||||||||||||||
isDefault?: boolean; | ||||||||||||||||||
name: string; | ||||||||||||||||||
versioned?: boolean; | ||||||||||||||||||
outputStorageStrategy?: BackendOutputStorageStrategy<StorageOutput>; | ||||||||||||||||||
outputStorageStrategy?: BackendOutputStorageStrategy<{ | ||||||||||||||||||
version: '1'; | ||||||||||||||||||
payload: Record<string, string>; | ||||||||||||||||||
}>; | ||||||||||||||||||
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. why this change? 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. StorageOutput is from schema, which is the final output, but the initial output we stored in stack metadata is different, so I introduced a new type, 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. See how the new commit f6a1c12 looks? 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. I don't understand why do we need type Auth and Functions are just using their respective outputs, see:
and
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.
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. There would be some mis-match after
So we need to change the type, and modify the output when parsing to Is there a way to avoid the change? 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. tl;dr -- When we store output, we store the individual bucket (code) (which has There might be a way to avoid the complex, and I missed it. 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. The way we should evolve this is
To
And then in order for this to be backwards compatible we should populate bucketName and storageRegion with the one that's 'isDefault = true'. 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. And then use this trick amplify-backend/packages/backend-function/src/factory.ts Lines 393 to 398 in 890ce86
buckets .
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. Done d67854e |
||||||||||||||||||
triggers?: Partial<Record<AmplifyStorageTriggerEvent, ConstructFactory<ResourceProvider<FunctionResources>>>>; | ||||||||||||||||||
}; | ||||||||||||||||||
|
||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -13,10 +13,7 @@ import { | |||||
FunctionResources, | ||||||
ResourceProvider, | ||||||
} from '@aws-amplify/plugin-types'; | ||||||
import { | ||||||
StorageOutput, | ||||||
storageOutputKey, | ||||||
} from '@aws-amplify/backend-output-schemas'; | ||||||
import { storageOutputKey } from '@aws-amplify/backend-output-schemas'; | ||||||
import { RemovalPolicy, Stack } from 'aws-cdk-lib'; | ||||||
import { | ||||||
AttributionMetadataStorage, | ||||||
|
@@ -32,6 +29,11 @@ const storageStackType = 'storage-S3'; | |||||
export type AmplifyStorageTriggerEvent = 'onDelete' | 'onUpload'; | ||||||
|
||||||
export type AmplifyStorageProps = { | ||||||
/** | ||||||
* Whether this storage resource is the default storage resource for the backend | ||||||
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. nit: also add some wording here explaining that it's required if there are multiple storage definitions in the backend 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. Done |
||||||
* @default false | ||||||
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. isn't the default true? ie if I just have one storage instance and I don't define this field, then it's the default 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. Good point. I don't know if the default should be 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. Done |
||||||
*/ | ||||||
isDefault?: boolean; | ||||||
/** | ||||||
* Friendly name that will be used to derive the S3 Bucket name | ||||||
*/ | ||||||
|
@@ -42,7 +44,10 @@ export type AmplifyStorageProps = { | |||||
* @default false | ||||||
*/ | ||||||
versioned?: boolean; | ||||||
outputStorageStrategy?: BackendOutputStorageStrategy<StorageOutput>; | ||||||
outputStorageStrategy?: BackendOutputStorageStrategy<{ | ||||||
version: '1'; | ||||||
payload: Record<string, string>; | ||||||
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. I think @sobolk had a similar comment elsewhere, but we should keep this strongly typed, even if that means updating the type |
||||||
}>; | ||||||
/** | ||||||
* S3 event trigger configuration | ||||||
* @see https://docs.amplify.aws/gen2/build-a-backend/storage/#configure-storage-triggers | ||||||
|
@@ -122,7 +127,7 @@ export class AmplifyStorage | |||||
}, | ||||||
}; | ||||||
|
||||||
this.storeOutput(props.outputStorageStrategy); | ||||||
this.storeOutput(props.outputStorageStrategy, props.isDefault || false); | ||||||
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.
Suggested change
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.
|
||||||
|
||||||
new AttributionMetadataStorage().storeAttributionMetadata( | ||||||
Stack.of(this), | ||||||
|
@@ -146,15 +151,22 @@ export class AmplifyStorage | |||||
* Store storage outputs using provided strategy | ||||||
*/ | ||||||
private storeOutput = ( | ||||||
outputStorageStrategy: BackendOutputStorageStrategy<StorageOutput> = new StackMetadataBackendOutputStorageStrategy( | ||||||
Stack.of(this) | ||||||
) | ||||||
outputStorageStrategy: BackendOutputStorageStrategy<{ | ||||||
version: '1'; | ||||||
payload: Record<string, string>; | ||||||
}> = new StackMetadataBackendOutputStorageStrategy(Stack.of(this)), | ||||||
isDefault: boolean = false | ||||||
): void => { | ||||||
/* The following code can guarantee there's only one `isDefault` | ||||||
* because if we can only create one construct with the same `storageRegion` and `bucketName` name. | ||||||
* The default bucket takes the `storageRegion` and `bucketName` name without a number post-fix. | ||||||
*/ | ||||||
const num = isDefault ? '' : Math.floor(Math.random() * 100); | ||||||
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. We shouldn't use random generator here. This will cause re-deployments of storage as outputs are going to change every time impacting performance (e.g. this will cause sandbox never hotswapping). 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. Thanks. Yea, I'm looking for some suggestions to replace the random generator. I'll try those. 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. We should do the same thing that we do for auth and data:
keep a static counter in the factory class 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. Done cddcd97 |
||||||
outputStorageStrategy.addBackendOutputEntry(storageOutputKey, { | ||||||
version: '1', | ||||||
payload: { | ||||||
storageRegion: Stack.of(this).region, | ||||||
bucketName: this.resources.bucket.bucketName, | ||||||
[`storageRegion${num}`]: Stack.of(this).region, | ||||||
[`bucketName${num}`]: this.resources.bucket.bucketName, | ||||||
0618 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
}, | ||||||
}); | ||||||
}; | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,9 +63,15 @@ export class StorageAccessPolicyFactory { | |
}); | ||
} | ||
|
||
return new Policy(this.stack, `${this.namePrefix}${this.policyCount++}`, { | ||
0618 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
statements, | ||
}); | ||
return new Policy( | ||
this.stack, | ||
`${this.bucket.bucketName.slice(0, 6)}${Math.floor( | ||
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. same here, this needs consistent hashing or global counter. 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.
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.
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. Done. Using global counter ec113d0 |
||
Math.random() * 1000 | ||
)}`, | ||
{ | ||
statements, | ||
} | ||
); | ||
}; | ||
|
||
private getStatement = ( | ||
|
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.
why typing for
buckets
is different here than in other places ?additionally. should we use records here or should be come up with subtype with named properties? (this applies to all
buckets
introduced in this file)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.
Done d67854e