- Notifications
You must be signed in to change notification settings - Fork686
feat: support multiple SSM parameters for large runner matcher configs (#4790)#4792
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
trigger ci with a minor change
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.
@edersonbrilhante nice contribution. I just tried the parameter split with the multi-runner example in this repo. By adding a few config and adding more labes. Result is I got this error, related to the addedforeach
││ on ../../modules/webhook/webhook.tf line 27, in resource "aws_ssm_parameter" "runner_matcher_config":│ 27: for_each = { for idx, val in local.matcher_chunks : idx => val }│ ├────────────────│ │ local.matcher_chunks will be known only after apply││ The "for_each" map includes keys derived from resource attributes that cannot be determined until apply, and so Terraform cannot determine the full set of keys that will│ identify the instances of this resource.││ When working with unknown values in for_each, it's better to define the map keys statically in your configuration and place apply-time results only in the map values.││ Alternatively, you could use the -target planning option to first apply only the resources that the for_each value depends on, and then apply a second time to fully│ converge.Wondering you you tested the change? I was not able to get it working right now.
I did test it, but I forgot to include a large configuration and only tested the normal case. My bad. I found the same bug now. |
@npalm I just pushed my fix. I tested a normal config and a tested a really big one with 7 ssm resources and it worked fine now. |
edersonbrilhante commentedOct 3, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Thx for the updates, will not be able to able to get the PR merged in the next weeks. I will catch up end of the month. Sorry for tthe delay. |
No problem. :) |
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.
Pull Request Overview
Implements support for splitting the runner matcher configuration across multiple SSM parameters to avoid size limits for large configurations. The primary change allowsPARAMETER_RUNNER_MATCHER_CONFIG_PATH to accept multiple parameter paths separated by a colon.
- Added logic to split large runner matcher configurations into chunks based on SSM parameter tier limits
- Updated infrastructure to create multiple SSM parameters when needed and pass them as a list
- Modified configuration loader to handle multiple parameter paths and combine them at runtime
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/webhook/webhook.tf | Implements chunking logic and creates multiple SSM parameters for large configs |
| modules/webhook/eventbridge/webhook.tf | Updates environment variable to join multiple parameter paths with colons |
| modules/webhook/eventbridge/variables.tf | Changes ssm_parameter_runner_matcher_config from object to list of objects |
| modules/webhook/eventbridge/dispatcher.tf | Updates IAM policy and environment variables to handle multiple parameters |
| modules/webhook/direct/webhook.tf | Updates environment variables to join multiple parameter paths with colons |
| modules/webhook/direct/variables.tf | Changes ssm_parameter_runner_matcher_config from object to list of objects |
| lambdas/functions/webhook/src/ConfigLoader.ts | Implements logic to load and combine configuration from multiple SSM parameters |
| lambdas/functions/webhook/src/ConfigLoader.test.ts | Adds test coverage for multi-parameter configuration loading |
Tip: Customize your code reviews with copilot-instructions.md.Create the file orlearn how to get started.
| if(!paramPathsEnv||paramPathsEnv==='undefined'||paramPathsEnv==='null'||!paramPathsEnv.includes(':')){ | ||
| awaitthis.loadParameter(paramPathsEnv,'matcherConfig'); | ||
| return; | ||
| } | ||
| constpaths=paramPathsEnv | ||
| .split(':') | ||
| .map((p)=>p.trim()) | ||
| .filter(Boolean); | ||
| letcombinedString=''; | ||
| for(constpathofpaths){ | ||
| awaitthis.loadParameter(path,'matcherConfig'); | ||
| combinedString+=this.matcherConfig; | ||
| } | ||
| try{ | ||
| this.matcherConfig=JSON.parse(combinedString); | ||
| }catch(error){ | ||
| this.configLoadingErrors.push(`Failed to parse combined matcher config:${(errorasError).message}`); | ||
| } |
CopilotAIOct 14, 2025
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.
The condition logic is incorrect. When there's no colon in the path (single parameter), the function should still load the parameter, but the condition will return true and callloadParameter with the full path. However, when there IS a colon (multiple parameters), the condition returns false and proceeds to the multi-path logic. The condition should be!paramPathsEnv.includes(':') to handle single parameters correctly.
| if(!paramPathsEnv||paramPathsEnv==='undefined'||paramPathsEnv==='null'||!paramPathsEnv.includes(':')){ | |
| awaitthis.loadParameter(paramPathsEnv,'matcherConfig'); | |
| return; | |
| } | |
| constpaths=paramPathsEnv | |
| .split(':') | |
| .map((p)=>p.trim()) | |
| .filter(Boolean); | |
| letcombinedString=''; | |
| for(constpathofpaths){ | |
| awaitthis.loadParameter(path,'matcherConfig'); | |
| combinedString+=this.matcherConfig; | |
| } | |
| try{ | |
| this.matcherConfig=JSON.parse(combinedString); | |
| }catch(error){ | |
| this.configLoadingErrors.push(`Failed to parse combined matcher config:${(errorasError).message}`); | |
| } | |
| if(!paramPathsEnv||paramPathsEnv==='undefined'||paramPathsEnv==='null'){ | |
| // Optionally, log or handle missing matcher config path | |
| return; | |
| }elseif(paramPathsEnv.includes(':')){ | |
| constpaths=paramPathsEnv | |
| .split(':') | |
| .map((p)=>p.trim()) | |
| .filter(Boolean); | |
| letcombinedString=''; | |
| for(constpathofpaths){ | |
| awaitthis.loadParameter(path,'matcherConfig'); | |
| combinedString+=this.matcherConfig; | |
| } | |
| try{ | |
| this.matcherConfig=JSON.parse(combinedString); | |
| }catch(error){ | |
| this.configLoadingErrors.push(`Failed to parse combined matcher config:${(errorasError).message}`); | |
| } | |
| }else{ | |
| awaitthis.loadParameter(paramPathsEnv,'matcherConfig'); | |
| } |
| letcombinedString=''; | ||
| for(constpathofpaths){ | ||
| awaitthis.loadParameter(path,'matcherConfig'); | ||
| combinedString+=this.matcherConfig; | ||
| } | ||
| try{ | ||
| this.matcherConfig=JSON.parse(combinedString); | ||
| }catch(error){ | ||
| this.configLoadingErrors.push(`Failed to parse combined matcher config:${(errorasError).message}`); | ||
| } |
CopilotAIOct 14, 2025
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.
The code concatenatesthis.matcherConfig as a string, butmatcherConfig is typed asRunnerMatcherConfig[] (an array). This will result in[object Object] being concatenated instead of the JSON string. Should concatenate the parameter value directly or store it in a temporary variable.
| letcombinedString=''; | |
| for(constpathofpaths){ | |
| awaitthis.loadParameter(path,'matcherConfig'); | |
| combinedString+=this.matcherConfig; | |
| } | |
| try{ | |
| this.matcherConfig=JSON.parse(combinedString); | |
| }catch(error){ | |
| this.configLoadingErrors.push(`Failed to parse combined matcher config:${(errorasError).message}`); | |
| } | |
| letcombinedMatcherConfig:RunnerMatcherConfig[]=[]; | |
| for(constpathofpaths){ | |
| awaitthis.loadParameter(path,'matcherConfig'); | |
| if(Array.isArray(this.matcherConfig)){ | |
| combinedMatcherConfig.push(...this.matcherConfig); | |
| }else{ | |
| this.configLoadingErrors.push(`Matcher config at path "${path}" is not an array`); | |
| } | |
| } | |
| this.matcherConfig=combinedMatcherConfig; |
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.
@edersonbrilhante took a while but tested and reviewed the PR.
- I left some small comments, one permission needs to be dropped
- can you have a look on the copilot suggestions as well
- I have tested incrasing the matching resulting in a split. and down size back to a single matcher variable. All works like a hcarm.
| resource_arns=jsonencode([var.config.ssm_parameter_runner_matcher_config.arn]) | ||
| resource_arns=jsonencode( | ||
| concat( | ||
| [var.config.github_app_parameters.webhook_secret.arn], |
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.
This results in the dispatcher can access the github webhok secret. Which is not needed. Only the webhook lambda is required to access this secret. Adding the webhook secret paramater is correct for the direct case but not when the eventbride is used. So here[var.config.github_app_parameters.webhook_secret.arn] can be removed.
~ resource "aws_iam_role_policy" "dispatcher_ssm" { id = "npalm-multi-dispatcher-lambda-72a4fc05:publish-ssm-policy" name = "publish-ssm-policy" ~ policy = jsonencode( ~ { ~ Statement = [ ~ { ~ Resource = [ + "arn:aws:ssm:eu-central-1:123:parameter/github-action-runners/npalm-multi/app/github_app_webhook_secret", "arn:aws:ssm:eu-central-1:123:parameter/github-action-runners/npalm-multi/webhook/runner-matcher-config", ] # (2 unchanged attributes hidden) }, ] # (1 unchanged attribute hidden) } ) # (2 unchanged attributes hidden) }| # sorted list | ||
| runner_matcher_config_sorted=[forkinsort(keys(local.runner_matcher_config)):local.runner_matcher_config[k]] | ||
| # Define worst-case dummy ARN/ID lengths |
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.
Would be helpfull to add a short description why thos word-case values are calculated. Now in context of the PR it is very clear. But once merged it woulde be helpfull to have a short text that this intermediate values are used to calculate the slices to split the matcher config cross multiple paramaters.
PR Description
Implements support for splitting the runner matcher configuration across multiple SSM parameters.
PARAMETER_RUNNER_MATCHER_CONFIG_PATHcan now accept multiple parameter paths separated by a colon (:).This avoids SSM size limits for large configurations and improves scalability for environments with many runner types and labels.
Closes#4790