-
Notifications
You must be signed in to change notification settings - Fork 57
feat(config-bundle): add --kms-key for customer-managed encryption #1642
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ const ALL_STEPS: AddConfigBundleStep[] = [ | |
| 'addAnother', | ||
| 'branchName', | ||
| 'commitMessage', | ||
| 'kmsKey', | ||
| 'confirm', | ||
| ]; | ||
|
|
||
|
|
@@ -23,6 +24,7 @@ function getDefaultConfig(): AddConfigBundleConfig { | |
| componentsRaw: '', | ||
| branchName: 'mainline', | ||
| commitMessage: '', | ||
| kmsKeyArn: '', | ||
| }; | ||
| } | ||
|
|
||
|
|
@@ -112,6 +114,11 @@ export function useAddConfigBundleWizard() { | |
|
|
||
| const setCommitMessage = useCallback((commitMessage: string) => { | ||
| setConfig(c => ({ ...c, commitMessage })); | ||
| setStep('kmsKey'); | ||
|
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. Minor UX note (non-blocking): the wizard now unconditionally shows the KMS step to every user creating a config bundle, even though CMK is an advanced/opt-in feature. Given the prompt explicitly says "press Enter to skip", this is probably acceptable, but consider whether this should be gated behind a yes/no question ("Encrypt with a customer-managed KMS key?") so the common path doesn't grow an extra prompt. Feel free to ignore if this was a deliberate design choice. |
||
| }, []); | ||
|
|
||
| const setKmsKey = useCallback((kmsKeyArn: string) => { | ||
| setConfig(c => ({ ...c, kmsKeyArn })); | ||
| setStep('confirm'); | ||
| }, []); | ||
|
|
||
|
|
@@ -137,6 +144,7 @@ export function useAddConfigBundleWizard() { | |
| doneAddingComponents, | ||
| setBranchName, | ||
| setCommitMessage, | ||
| setKmsKey, | ||
| reset, | ||
| }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import { KmsKeyArnSchema } from './evaluator'; | ||
| import { z } from 'zod'; | ||
|
|
||
| // ============================================================================ | ||
|
|
@@ -44,6 +45,8 @@ export const ConfigBundleSchema = z.object({ | |
| branchName: z.string().max(128).optional(), | ||
| /** Optional commit message for this version. */ | ||
| commitMessage: z.string().max(500).optional(), | ||
| /** Optional KMS key ARN for customer-managed encryption of the bundle. */ | ||
| kmsKeyArn: KmsKeyArnSchema.optional(), | ||
|
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. Reusing
|
||
| }); | ||
|
|
||
| export type ConfigBundle = z.infer<typeof ConfigBundleSchema>; | ||
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.
Inconsistent up-front validation + missing telemetry on this handler.
(1)
EvaluatorPrimitive.ts:298validateskmsKeyArnwithisValidKmsKeyArnright before callingthis.add(...)and fails with a clear, flag-named message. Here, an invalid ARN falls through towriteProjectSpec's Zod validation, producing a generic schema error. The integ test (integ-tests/add-remove-config-bundle.test.ts:243) only assertsjson.erroris defined, so the worse message wouldn't be caught. Either:EvaluatorPrimitiveand add an explicitisValidKmsKeyArncheck just before thisthis.add({...})call that callsfail('--kms-key must be a valid KMS key ARN ...').(2) Missing telemetry instrumentation. Per
src/cli/telemetry/README.md, new features should be instrumented.add.config-bundleisn't inCOMMAND_SCHEMAS(src/cli/telemetry/schemas/command-run.ts) at all today, and this PR is a natural opportunity to add it since we now have an interesting attribute to track (CMK adoption). Suggested:Then wrap this non-interactive handler in
runCliCommand('add.config-bundle', !!cliOptions.json, async () => { ...; return { has_kms_key: !!cliOptions.kmsKey, component_count: Object.keys(components).length }; }).