-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Reduce number of parameters used by assets #3463
Comments
Hi @alexdilley, Thank you for reaching out! |
Hi @NGL321, Thanks for your response. We, Just started using CDK and ended up with this issue. We have 21 lambdas and the parameter count for it comes to more than 63. This makes it impossible for us to use CDK right now to deploy as cloudformation doesn't allow it. Anyway to make CDK inline the parameters (bucket/key/version hash) within the template directly instead of passing these as parameters ? |
Current workound that I have been using is
Code snippet incase anyone else needs to use it: |
The long-term solution should be to have only a single parameter per CDK asset. It will still require a parameter per asset since asset locations change based on their source hash but it will reduce the number of parameters by a factor of x3 |
I do not see why all parameters cannot be placed into a map parameter. They must have unique keys/ids already since parameters cannot have duplicated names, correct? I do not think the community would consider a stack limited to 30 assets to be the long term solution. 30 assets, assuming the users needed no parameters of their own (a pretty faulty assumption imho). |
@sublimemm I think that would also be the best way; however I'm not sure how it could be a map. CloudFormation doesn't seem to support pulling out values from a map unless it's specifically a What I've seen is having a long string as the parameter (joining all the parameters with a special character such as |
@eladb You said the one parameter per asset would reduce the number of parameters by a factor of 3, but I think it will actually do much more. Our stack has 5 assets and between them 63 parameters. So reducing it to 5 would be a much more palatable solution. It's unclear to me when the CDK decides something is a new asset or not... but I thought your original suggestion was one parameter per construct/lambda/etc. One per asset seems tenable, assuming the cdk is diligent with splitting the stack into assets. |
@sublimemm I just looked at my template and it looks like it follows the 3 parameters per asset. Maybe you have something else adding parameters? |
After some digging, it's all about the lambdas. Its adding 3 per lambda, we have tons of lambdas, there are many per asset. |
We are looking into improving this as part of our work on CI/CD. The current thinking is to actually reduce the number of asset parameters to zero by using a well-known convention-based physical names for the bootstrapping resources (bucket/ECR repository) and the source hash as the key (S3 object key/docker image tag). This will basically mean that we don't need any degrees of freedom during deployment. I am curious about people's thoughts on this... |
I think that is a great solution. |
The `NestedStack` construct is a special kind of `Stack`. Any resource defined within its scope will be included in a separate template from the parent stack. The template for the nested stack is synthesized into the cloud assembly but not treated as a deployable unit but rather as a file asset. This will cause the CLI to upload it to S3 and wire it's coordinates to the parent stack so we can reference its S3 URL. To support references between the parent stack and the nested stack, we abstracted the concept of preparing cross references by inverting the control of `consumeReference` from the reference object itself to the `Stack` object. This allows us to override it at the `NestedStack` level (through `prepareCrossReference`) and mutate the token accordingly. When an outside resource is referenced within the nested stack, it is wired through a synthesized CloudFormation parameter. When a resource inside the nested stack is referenced from outside, it will be wired through a synthesized CloudFormation output. This works for arbitrarily deep nesting. When an asset is referenced within a nested stack, it will be added to the top-level stack and wired through the asset parameter reference (like any other reference). Fixes #239 Fixes #395 Related #3437 Related #1439 Related #3463
To add to this... |
I'm attempting to migrate from raw CF YAML templates to the CDK, immediately hit this issue. It generated 120 parameters first attempt. Immediately blocked by this. I just spent 3 days converting templates thinking "surely cdk handles maximum limits". Very very discouraging to see this happen before ever being able to deploy our structure. We have 10s of thousands of lines of config and I want to move the team away from that. Would love to see some progress made toward this fix. In the meantime, anyone have a workaround aside from the one above? Would re-structuring the hierarchy in some way help? I've tried to create extra layers but they then just pass the parameters through the layer and the problem remains. |
@tekdroid yep. I tried restructuring too. But didn't work out. So I'm still using the workaround I told about above. (Inlining all the asset paths into the template) and using Updated script to do the inlining in case you are interested: https://gist.github.com/niranjan94/92f2636a29f09bd6cc53085951e78046 |
@niranjan94 thanks for the updated script, I appreciate that! I'll check it out and see if I can use this method for now. Cheers! |
@niranjan94 Your script is not working as expected for me. Would you mind helping me out? It removed all of the parameters, but didn't replace their usages. Is there another medium by which we could chat for a moment? If you can't that's fine, I can reverse engineer this script. Mainly I just wanted to see if I'm running the synthesis of the cdk output the same way you are. Looks like you're looking for Resource -> Properties -> Content, but I don't have a node at that path. I have Resource -> Properties -> TemplateURL, which is a join function for the s3 bucket/key parameters that were removed. EDIT: No worries, after second thought I really don't want to go down this road. I'll continue our teams conversion when this is officially fixed inside the CDK. |
@eladb I really like reducing the number to zero and using asset hashes. For the toolkit bucket, I would suggest an |
We can't use Copy @rix0rrr |
This seems like a bug. Can you please raise a separate issue? |
Mind raising a separate issue for that? |
@rix0rrr will the new asset system be the default system ever or will we always need to adjust |
After running the new bootstrap command, I'm getting the following error when doing
Any idea what I'm doing wrong? EDIT: It was due to using root credentials in our CLI. Creating a IAM User resolved the issue. |
@rix0rrr Is this documented in the CDK pipelines docs? |
@rix0rrr I am having a similar issue:
So the first time I bootstrap and deploy, it works. But any sub-sequential deployment I get this error. Only when I delete my S3 bucket and bootstrap again, can I deploy. So I can only deploy one time locally before I have to do this again. However this wouldn't work for us in deployment because it would be madness to remove the staging bucket after each deploy. My CDK version is 1.49.1 |
@NGL321 So we still have issues with deploying our stack (appsync api with lambda's, more than 20 = 60+ params) with the new CDK synthesis. The error is as mentioned in my prev post above. Our accounts are DEV, STAGING And PROD and we have SSO users with read/write permissions and we use the
So this becomes a bit hairy. Our permission set for the developers includes |
* Internal 9 fix workflow (#81) * trigger on prereleases as well * actions/checkout@v2 should checkout the correct branch anyway? * bash * fix tagged builds * add graphql-endpoint to build scripts * fix graphql endpoint Dockerfile * docker cp * zip arguments reversed >_< * Add generic subgraph generator to build * Build generic-subgraph-generator * Add Dockerfile for generic-subgraph-generator * Internal 9 fix workflow (#83) * trigger on prereleases as well * actions/checkout@v2 should checkout the correct branch anyway? * bash * fix tagged builds * add graphql-endpoint to build scripts * fix graphql endpoint Dockerfile * docker cp * zip arguments reversed >_< * python lambda.zips are located at /home/grapl * Internal 9 fix workflow (#84) * trigger on prereleases as well * actions/checkout@v2 should checkout the correct branch anyway? * bash * fix tagged builds * add graphql-endpoint to build scripts * fix graphql endpoint Dockerfile * docker cp * zip arguments reversed >_< * python lambda.zips are located at /home/grapl * use step outputs for artifact names * New CDK. * sneaky entry in parent .gitignore left the most important subdir out! * Add grapl-notebook * Restoring edge_ux - oops. * Clean and rebuild of the edge_ux, as well as fixing graphql endpoint string replacement. * prefer arrow function. * Bump service queue size from 1 to 10. * Fixing perms granted by `allowReadWrite` to also grant write perms. * Reduce console logs in schema.js * Community docs (#85) * initial attempt at community docs * document branching strategy * add link to Dobi issue * Add script to download release zips by tag. * Update to CDK 1.41.0. * Update deploy_all.sh to reflect new stack name for UX. * add grapl_analyzerlib to monorepo and fix rust containers * only rename the binaries in the lambdas * Add S3 encryption. Make fetch_Zips a bit more resilient. * Move JWT to secretsmanager * Remove unused import * Publish grapl_graph_descriptions and grapl_analyzerlib to PyPI (#91) * publish grapl_graph_descriptions and grapl_analyzerlib to PyPI * fix a few mistakes * remove some stuff that was erroneously brought in with grapl_analyzerlib * run mypy in lint workflow * fix a bug in the release workflow * Fix release workflow (#92) * publish grapl_graph_descriptions and grapl_analyzerlib to PyPI * fix a few mistakes * remove some stuff that was erroneously brought in with grapl_analyzerlib * run mypy in lint workflow * fix a bug in the release workflow * fix workflow syntax * whitespace * Fix the grapl-release workflow (#93) * publish grapl_graph_descriptions and grapl_analyzerlib to PyPI * fix a few mistakes * remove some stuff that was erroneously brought in with grapl_analyzerlib * run mypy in lint workflow * fix a bug in the release workflow * fix workflow syntax * whitespace * what? * whoops (#94) * Internal 18 fix release workflow (#95) * whoops * >_< * Fix pypa/gh-action-pypi-publish version (#96) * whoops * >_< * use correct version for pypa/gh-action-pypi-publish@v1.1.0 * Add rate limiting * Remove unused import * Valid emails for PyPI (#97) * valid emails for PyPI * update grapl-build and grapl-lint workflows to run on PR updates, run cargo update * run lints and builds on the staging branch * listen to synchronized events instead of edited * set the working-directory before attempting cargo audit * synchronize, not synchronized >_< * update working-directory * apparently working-directory does not work with uses * make cargo audit actually work * make cargo audit its own job s.t. it shows up as a separate check * attempt to fix the grapl-cargo-audit job * Internal 18 more release workflow fixes (#99) * valid emails for PyPI * update grapl-build and grapl-lint workflows to run on PR updates, run cargo update * run lints and builds on the staging branch * listen to synchronized events instead of edited * set the working-directory before attempting cargo audit * synchronize, not synchronized >_< * update working-directory * apparently working-directory does not work with uses * make cargo audit actually work * make cargo audit its own job s.t. it shows up as a separate check * attempt to fix the grapl-cargo-audit job * fix utility service builds * add another depends_on (#100) * Don't create a stack just for the secret. Use arn, not name. * Fixing path join in fetch script. * Remove engagement-graph from grapl_analyzerlib * Bump versions for grapl_analyzerlib * Use bump2version for python library releases (#101) * version bumps for python libraries * remove unreachable exceptions * python3 (#102) * Configure python correctly (#103) * python3 * configure python correctly * Check whether versions need to be bumped before building (#104) * python3 * configure python correctly * WIP so close * exclude yanked packages * some updates and fixes * Remove .bumpversion.cfg and increment versions (#105) * python3 * configure python correctly * WIP so close * exclude yanked packages * some updates and fixes * bump versions, remove .bumpversion.cfgs * Complete removal of EngagementGraph * Fix merge conflict * Add caching to Dockerfile * Black and rustfmt (#106) * black * rustfmt * add a newline to trigger git checks * cargo update * run black once last time * build in debian images instead of alpine (#108) * bump python library versions (#109) * build in debian images instead of alpine * bump python lib versions * add PyPI checks to lint workflow * - Adding version info as description to Lambdas. - Naming cleanup. - Pinning dgraph version. This should have been in a previous commit. * DGraph - don't use standalone version in AWS. * Fix rust builds (#110) * build in debian images instead of alpine * bump python lib versions * add PyPI checks to lint workflow * try to reduce peak disk usage for 10GB Github Actions limit * Removing a file created from merge that shouldn't have made it in previous commit. * Move docs * - Cleanup SageMaker Notebook name. * - Remove encryption on UX bucket - this needs to be public. * build in debian images instead of alpine (#108) * bump python library versions (#109) * build in debian images instead of alpine * bump python lib versions * add PyPI checks to lint workflow * Fix rust builds (#110) * build in debian images instead of alpine * bump python lib versions * add PyPI checks to lint workflow * try to reduce peak disk usage for 10GB Github Actions limit * Renaming and syncing queue names in cdk with rest of the codebase. * Fixing Python builds to work in AWS. * Python and Rust formatting. * Fix LGTM alert, unused import. * Update sqs-lambda * Add waiting for s3 and sqs * Add aws_region to events * Add ip_address to OutboundConnection * - Adding version info as description to Lambdas. - Naming cleanup. - Pinning dgraph version. This should have been in a previous commit. * DGraph - don't use standalone version in AWS. * Removing a file created from merge that shouldn't have made it in previous commit. * - Cleanup SageMaker Notebook name. * - Remove encryption on UX bucket - this needs to be public. * Renaming and syncing queue names in cdk with rest of the codebase. * Fixing Python builds to work in AWS. * Python and Rust formatting. * Fix LGTM alert, unused import. * Remove engagement graph from model plugin deployer. * Bump graph generator lib version * Bump graph generator lib version * Move grapl-config and grapl-graph-descriptions to crates.io * Add license and description * Add license and description * Update Cargo.toml * Update Cargo.toml * Slip in a change to fix logging * Update dependencies * Syncing queue names. * Fix unused import reported by LGTM. * Attach reverse edges in DGraph (#114) * implement reverse edges * some bug fixes * remove unused local * remove some more engagement graph stuff * add some debug logging * Set up reverse edges, remove types * fix logging * put EngagementClient and EngagementView back in * update for review comments Co-authored-by: colin-grapl <62314572+colin-grapl@users.noreply.github.com> Co-authored-by: colin <colin@graplsecurity.com> * Adding ability for generator lib s3 client to assume AWS STS role. * internal-13-ui-updates (#118) * fixed compiler warnings * Start dashboard * completed dashboard, page not found, extracted header, & routing * Add file handler * able to upload & delete plugins with UX changes * reformatted using black * lgtm changes * Remove hanging reference to engagement_graph * update version, fix encoding error Co-authored-by: colin <colin@graplsecurity.com> * Implement DGraph TTL cleanup job (#119) * basic ttl job structure * small fix * small fixes * code cleanup * write batch query * updates for review comments * WIP but close now * some updates from local testing * updates from local testing * make debug logs a little less spammy * add grapl-dgraph-ttl to local grapl * some more updates from local testing * CDK for grapl-dgraph-ttl (#121) * add --delay and --batch-size options to upload-sysmon-logs.py * attempt to CDK * add schedule rule * updates for review comments * address LGTM lint * cdk README * add extract-grapl-deployment-artifacts.sh * WIP build is broken * fix aws-cdk versions * some updates from local testing * WIP build still borked * README update * make dgraph-ttl a Construct instead of a NestedStack * fix an issue with the handler reference * some more updates from testing in sandbox * more fixes * normalize dgraph dns name handling (#126) * Upgrade aws cdk to 1.46 to take advantage of fix for aws/aws-cdk#3463 * - Adding deploy name to support deploying multiple instances of Grapl to a single AWS account. - All Constructs now use an interface for constructor params beyond scope and id. This essentially provides named params, making it clearer what the arguments are for. * Remove .env from .gitignore, env vars no longer used. * Fix Grapl prefix from stackName issue. * Fix EngagementUX Stack name. * unused imports cleanup. * Fix test code from template to new Grapl Stack params. * Fixing some hard-coded names to take a prefix. * Fix name for retry handler. * Fixing hard-coded table names, which now take from env vars. Also gonna mix up our naming convention. For fun. * jkz colin * Add asset_id_mappings table and remove node_id_retry_table. * Add new env vars to docker-compose.yml * Remove swp file that should not be there. * Moving dynamodb table name fetches for node-identifier to grapl-config. * Adding MG_ALPHAS to docker-compose for dgraph-ttl. * Integrate cdk-watchful (#124) * whitespace * add cdk-watchful dep * vendor cdk-watchful 0.5.1 * get vendored cdk-watchful to build * add watchful to the Grapl stack * some fixes * remove cdk-watchful DynamoDB stuff because we use on-demand pricing * remove unused import * instrument engagement and graphql * updates from local testing * make everything a NestedStack * internal-74 fix destroy_all.sh * remove dangerous S3 stuff * Updating CDK README to match changes from previous PR. * Routing (#131) * added paging for graphQL endpoint * added front-end pagination to plugins * graphQL paging * fixed duplicate pagination bug for lenses * added styling for paging * formatting * Revert extra lenses * Styling * fixed any types * revisions * refactored custom routing to use react-router-dom HashRouter * remove redirectTo function * added logic to check if logged in * removed unneeded props * Improve performance of plugin page and fix warnings * Update sqs-lambda * Update sqs-lambda * Bump versions for tokio, tokio-compat, other dependencies * Making the use of Watchful conditional on spec of email for watchful alerts. * Let's not forget to pass this new param to the Grapl Stack. * Update graph-generator-lib * Glob deps * Properly pass in UX Bucket URL to graphql endpoint * Fixing previous watcher commit. * Move ux bucket to grapl-cdk-stack * Fix import * Fix .gitignore in CDK to not ignore the JS in our UX. * modelPluginDeployer cdk hack * Lowercase the origin in engagement edge * Reformat * Fix endpoint URL * Use regional bucket domain with https * Fix some more issues related to cors, env vars, etc * Format engagement_edge * Only use one client per service * Reuse asset id db * Formatting via Prettier. Adding .prettierrc.toml. * Fixing bucket policies, which had a few issues: - ActionGetObject isn't a thing - resources for s3:GebObject action need path wildcard - extra perms in TODO note * no sids. * policy statement cleanup. * Create the Lmabda execution role so we can name it. Also rename publishesToBucket to writesToBucket. * Adding explicit lambda execution roles with leigble names to ModelPluginDeployer and DGraphTTL. * Add description text to CDK stacks. * Use dobi for builds (#146) * WIP * tag build images latest * replace docker-compose build with dobi * mark dobi-linux binary executable * split builds into separate jobs * small fix for release workflow * RefactorBugfix * Fixes a number of issues with localgrapl, and improves logging * Temporary fix for cors in python services. grapl_analyzerlib plugin retriever fix. Notebook ACL fix. * Fix acl for list of s3 * Improve error in dispatcher. Fix ACL * Use addResources * Fix a couple things: - pass new arg - change params to match scheme of others (extending Props) * Don't unwrap a failed DGraph upsert * Discard transaction on failure * Remove commented out code * Reformat python * fix py formatting issues. * Reformat python * Reformat python * Handle redeployment of plugins more gracefully * Update Dockerfile to cd before zip * It's a - not a _ * Fix underscores * Give Engagement Notebook role a ligble name and a description. * Fix a number of issues with the analyzer-executor * Revert prefix * Bump the grapl_analyzerlib VERSION * Add paging to get_lenses (#125) * added paging for graphQL endpoint * added front-end pagination to plugins * graphQL paging * fixed duplicate pagination bug for lenses * added styling for paging * formatting * Revert extra lenses * Styling * fixed any types * revisions * Reload page when login is successful and history changes. (#148) * added location.reload() when history changes * removed console.logs * removed console.log * Internal 135 error handling (#164) * added validation * added extra rows to plugin listing to prevent UI from jumping * added yup package * add back local_handlers for graph-merger and analyzer-dispatcher * staging changes * Tests in CI (#153) * move assetdb and sessiondb tests under integration feature flag * run rust tests in CI * fix dobi configs for rust deploy images * run python unit tests in builds * run tests in js builds * fix js tests * WIP tests run but they fail * WIP * python integration tests pass! * make rust integration tests run * make rust integration tests work and wire tests into github workflows * workflow improvements * use cached build artifacts in integration tests * remove bogus hypothesis examples * Internal 37 fix workflows (#168) * move assetdb and sessiondb tests under integration feature flag * run rust tests in CI * fix dobi configs for rust deploy images * run python unit tests in builds * run tests in js builds * fix js tests * WIP tests run but they fail * WIP * python integration tests pass! * make rust integration tests run * make rust integration tests work and wire tests into github workflows * workflow improvements * use cached build artifacts in integration tests * remove bogus hypothesis examples * fix failing release build * fix build * some fixes (#171) * some fixes * π * Fix debug builds * Remove test build step * another release workflow fix (#175) * Internal 82 local auth (#173) * fixed bugs with local secretsmanager * fixed dashboard buttons, bug fixes for local auth * added UX_BUCKET_URL * added missing parameter to GraphQLEndpoint for ux_bucket per Colin * added dynamodb tables & service to graplprovision for local-grapl-user_auth * removed links to documentation * added welcome back * debugging for local auth * removed print statement * check for print statement in graph-merger * merge with staging * cargo formatting * python formatting * run black for python formatting * PR comment edits * update documentation with localgrapl creds, python formatting * reformatting * fml (#177) * Internal 37 moar workflow fixes (#178) * fml * another fix * Bump sqs-lambda to 0.20.20 * Internal 172 canvas bug (#180) * graphql query fixes * extended DGraph TTL to 13 months * remove console.log * cargo update * revert cargo update. * Implement Github Actions caching (#170) * WIP -- begin implementing gh actions caching * WIP * WIP * run unit tests in dobi * WIP workflows * update workflows for new jobs * add caching to build workflow * fix workflow * yaml syntax * fix build workflow * chicken or egg? * copy pasta * apparently artifacts are per-workflow * remove upload/download * fix build workflow * another fix Co-authored-by: inickles-grapl <64668260+inickles-grapl@users.noreply.github.com> * Fix release workflow (#182) * WIP -- begin implementing gh actions caching * WIP * WIP * run unit tests in dobi * WIP workflows * update workflows for new jobs * add caching to build workflow * fix workflow * yaml syntax * fix build workflow * chicken or egg? * copy pasta * apparently artifacts are per-workflow * remove upload/download * fix build workflow * another fix * release workflow updates Co-authored-by: inickles-grapl <64668260+inickles-grapl@users.noreply.github.com> * fix release flow (#185) * fix release flow * wat * >_< * Fix merge conflict (#187) * Update grapl_analyzerlib * Create CODE_OF_CONDUCT.md * version bumps * add newline for teh lulz Co-authored-by: colin <colin@graplsecurity.com> Co-authored-by: andrea-grapl <64504029+andrea-grapl@users.noreply.github.com> Co-authored-by: colin-grapl <62314572+colin-grapl@users.noreply.github.com> * add check-pypi job to build workflow (#188) * V0.2.0 docs update (#189) * update CONTRIBUTING.md for v0.2.0 release * README.md reformat and some updates * add Grapl DFIR Slack invite link to CONTRIBUTING.md * move check-pypi job to the lint workflow * two more README.md updates Co-authored-by: colin <colin@graplsecurity.com> Co-authored-by: inickles-grapl <inickles@graplsecurity.com> Co-authored-by: colin-grapl <62314572+colin-grapl@users.noreply.github.com> Co-authored-by: inickles-grapl <64668260+inickles-grapl@users.noreply.github.com> Co-authored-by: andrea-grapl <64504029+andrea-grapl@users.noreply.github.com>
I'm using IAM user, but still has same error with you. Do I need new an IAM user in CLI? |
Updated the CDK to use the new bootstrap and synth still shows 63 parameters. I'm uploading a bunch of files Assets and referring it using s3_url. Does the new bootstrap work for S3 Asset class as well? |
For anyone who is struggling, here's how we made it work in our case: Doing so made it work perfectly. |
Nice, our team solved this the same way and ran into similar issues. It would be nice to have a list of the exact permissions a role would need to have least privilege access to make this work. |
I completely agree. We usually wait for deployments to break before modifying the trust relationships. Helps keep the permission boundary as intact as possible. |
I have used this solution in our projects, it's work! [CDK v1.92.0] @rix0rrr @eladb I have a maintenance question. |
CloudFormation increased the limit from 60 parameters to 200 parameteres in October (https://aws.amazon.com/about-aws/whats-new/2020/10/aws-cloudformation-now-supports-increased-limits-on-five-service-quotas/) |
This is solved with the new bootstrap stack which is now the default in v2 - I believe this can be closed. Feel free to add additional comments if anyone feels differently. |
|
I'm submitting a ...
What is the current behavior?
CloudFormation stacks are limited to 60 parameters; CDK produces a seemingly excessive number of parameters, thus easy to breach limitation.
To perhaps instead use mappings, as suggested in the CF docs.
To be able to define, for example, more than 20 Lambda functions in a stack: currently βΒ for each function βΒ CDK generates one parameter for its artifact hash, one for its S3 location, and one for its version.
The text was updated successfully, but these errors were encountered: