pr feedback

Author: ojproductions

packages/base/src/commands/ecs-fargate/instrument.ts

@@ -25,6 +25,30 @@ export type EcsFargateConfigOptions = Partial<{
   agentImage: string
 }>
 
+/**
+ * The family a `--task-definition` names, whether it is given as a family, a `family:revision`, or
+ * a full task definition ARN.
+ */
+const familyFromTaskDefinition = (taskDefinition: string): string =>
+  (taskDefinition.split('/').pop() ?? taskDefinition).split(':')[0]
+
+/**
+ * The families named more than once, which the run cannot act on: a family has one instrumented
+ * revision
+ */
+const duplicateFamilies = (taskDefinitions: string[]): string[] => {
+  const seen = new Set<string>()
+  const duplicates = new Set<string>()
+  for (const family of taskDefinitions.map(familyFromTaskDefinition)) {
+    if (seen.has(family)) {
+      duplicates.add(family)
+    }
+    seen.add(family)
+  }
+
+  return [...duplicates]
+}
+
 /**
  * Derive the cluster from an ECS service ARN
  */
@@ -77,7 +101,7 @@ export class EcsFargateInstrumentCommand extends BaseCommand {
   })
 
   protected dryRun = Option.Boolean('-d,--dry,--dry-run', false, {
-    description: 'Preview changes running command would apply',
+    description: 'Preview the changes the command would apply',
   })
 
   private taskDefinitions = Option.Array('--task-definition,--taskDefinition', {
@@ -156,6 +180,16 @@ export class EcsFargateInstrumentCommand extends BaseCommand {
     if (!config.taskDefinitions?.length) {
       errors.push('No task definitions specified to instrument. Use --task-definition.')
     }
+
+    const duplicates = duplicateFamilies(config.taskDefinitions ?? [])
+    if (duplicates.length > 0) {
+      errors.push(
+        `--task-definition names the same task definition family more than once (${duplicates.join(
+          ', '
+        )}). A run instruments one revision per family, so name the revision to instrument once.`
+      )
+    }
+
     if (config.cluster && !config.ecsServices?.length) {
       errors.push('--cluster names the cluster of the services to update, so it only applies with --ecs-service.')
     }

packages/datadog-ci/README.md

@@ -113,12 +113,6 @@ The following `<scope>` and `<command>` values are available.
 
 - `upload`: Upload iOS dSYM files for Error Tracking (macOS only).
 
-#### `ecs-fargate`
-
-<sub>**README:** [📚](/packages/plugin-ecs-fargate) | **Documentation:** [🔗](https://docs.datadoghq.com/integrations/ecs_fargate/) | **Plugin:** `@datadog/datadog-ci-plugin-ecs-fargate`</sub>
-
-- `instrument`: Apply Datadog instrumentation to an AWS ECS Fargate Task Definition.
-
 #### `flutter-symbols`
 
 <sub>**README:** [📚](/packages/base/src/commands/flutter-symbols) | **Documentation:** [🔗](https://docs.datadoghq.com/real_user_monitoring/error_tracking/flutter/)</sub>
@@ -247,6 +241,12 @@ The following are **beta** commands, you can enable them with with `DD_BETA_COMM
 - `correlate-image`: Correlate an image from a CD provider with its source commit. [🔗](https://docs.datadoghq.com/continuous_delivery/deployments/argocd#correlate-images-with-source-code)
 - `gate`: Evaluate a Deployment Gate. [🔗](https://docs.datadoghq.com/deployment_gates/)
 
+#### `ecs-fargate`
+
+<sub>**README:** [📚](/packages/plugin-ecs-fargate) | **Documentation:** [🔗](https://docs.datadoghq.com/integrations/aws-fargate/) | **Plugin:** `@datadog/datadog-ci-plugin-ecs-fargate`</sub>
+
+- `instrument`: Apply Datadog instrumentation to an AWS ECS Fargate Task Definition.
+
 #### `elf-symbols`
 
 <sub>**README:** [📚](/packages/base/src/commands/elf-symbols) | **Documentation:** [🔗](https://docs.datadoghq.com/profiler/enabling/ddprof/)</sub>

packages/datadog-ci/src/cli.ts

@@ -15,7 +15,7 @@ export * as gitMetadata from '@datadog/datadog-ci-base/commands/git-metadata/lib
 export * as utils from '@datadog/datadog-ci-base/helpers/utils'
 export {cliVersion, printVersion} from '@datadog/datadog-ci-base/version'
 
-export const BETA_COMMANDS = new Set(['deployment', 'elf-symbols', 'wasm-symbols'])
+export const BETA_COMMANDS = new Set(['deployment', 'ecs-fargate', 'elf-symbols', 'wasm-symbols'])
 
 const betaCommandsEnabled =
   process.env.DD_BETA_COMMANDS_ENABLED === '1' || process.env.DD_BETA_COMMANDS_ENABLED === 'true'

packages/plugin-ecs-fargate/README.md

@@ -4,9 +4,13 @@ You can use the CLI to instrument your AWS ECS Fargate task definitions with Dat
 
 ### `instrument`
 
+**Warning:** The `ecs-fargate instrument` command is in beta. It requires you to set `DD_BETA_COMMANDS_ENABLED=1`.
+
 Run `datadog-ci ecs-fargate instrument` to add the Datadog Agent sidecar to an ECS Fargate task definition. The command reads the task definitions you name, adds the `datadog-agent` container to each of them, and registers the result as a new revision. Nothing that is running changes until the new revision is deployed, which you can leave to the command with `--ecs-service`.
 
 ```bash
+export DD_BETA_COMMANDS_ENABLED=1
+
 # Instrument a task definition, reading the API key from an AWS Secrets Manager secret
 datadog-ci ecs-fargate instrument --task-definition my-app -r us-east-1 --api-key-secret-arn <secret-arn>
 
@@ -26,11 +30,15 @@ datadog-ci ecs-fargate instrument --task-definition my-app -r us-east-1 --api-ke
 
 Application containers are given `DD_TRACE_ENABLED` and `DD_LOGS_INJECTION`, so the tracers already installed in them send traces and tie your logs to those traces. Both are only filled in when the container does not set them itself, so a task definition that has already made a choice keeps it.
 
+The Agent sidecar accepts custom metrics over DogStatsD: `DD_DOGSTATSD_ORIGIN_DETECTION` and `DD_DOGSTATSD_ORIGIN_DETECTION_CLIENT` are turned on and `DD_DOGSTATSD_TAG_CARDINALITY` is set to `orchestrator`, so your metrics are tagged with the task that submitted them. These are filled in the same way, so a task definition that already sets them keeps its own values.
+
 Running the command twice is safe: the Agent container is matched by name, so an already instrumented task definition is reported as such and no revision is registered. Each revision the command registers is tagged `dd_sls_ci` with the version of `datadog-ci` that created it; upgrading the CLI does not on its own produce a new revision, since that tag is not part of the comparison.
 
 #### Deploying the new revision
 
-Pass `--ecs-service` for each service that should run the revision the command just registered, and `--cluster` if those services are not in the `default` cluster. A service named by its full ARN already says which cluster it runs in, so `--cluster` can be left off; passing one that the ARN contradicts is an error rather than a silent choice between them. A run updates services in a single cluster, so ARNs naming more than one are reported too. Each service is matched to the task definition family it currently runs, so a run over several task definitions points each service at its own new revision, and a service already running the instrumented revision is left alone rather than redeployed. Updating a service starts an ECS deployment: the command returns as soon as ECS accepts it, and the rollout follows your service's deployment configuration.
+Pass `--ecs-service` for each service that should run the revision the command just registered, and `--cluster` if those services are not in the `default` cluster. A service named by its full ARN already says which cluster it runs in, so `--cluster` can be left off; passing a `--cluster` that contradicts the ARN's cluster is an error, not a silent override. A run updates services in a single cluster, so ARNs naming more than one are reported too. Each service is matched to the task definition family it currently runs, so a run over several task definitions points each service at its own new revision, and a service already running the instrumented revision is left alone rather than redeployed. Updating a service starts an ECS deployment: the command returns as soon as ECS accepts it, and the rollout follows your service's deployment configuration.
+
+Because a service is matched by family, a run instruments one revision per family: naming two revisions of the same family, as `--task-definition my-app:3 --task-definition my-app:4` does, is reported rather than leaving the choice of which one to deploy to the order they were passed in.
 
 Services are only updated once every task definition in the run is instrumented, so a run that fails part way through does not leave half of your services on a new revision. Tasks you start yourself with `RunTask`, and services you do not name, keep running the revision they were on.
 
@@ -40,14 +48,21 @@ Services are only updated once every task definition in the run is instrumented,
 
 You must have valid [AWS credentials](https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-envvars.html#envvars-list) configured with access to the ECS actions `ecs:DescribeTaskDefinition`, `ecs:RegisterTaskDefinition`, and `ecs:TagResource`. The last one is required because the new revision is registered with tags: the ones the task definition already had, plus `dd_sls_ci`. Deploying with `--ecs-service` also needs `ecs:DescribeServices` and `ecs:UpdateService`.
 
+`--profile` uses a named profile from your AWS configuration instead. A profile with an `mfa_serial` is supported: the command asks for the code when it loads the profile.
+
 #### Datadog API key
 
-Pass `--api-key-secret-arn` with the ARN of an AWS Secrets Manager secret holding your [Datadog API key](https://app.datadoghq.com/organization-settings/api-keys). The Agent reads the key from the secret at runtime, which keeps it out of the task definition. The task's execution role needs `secretsmanager:GetSecretValue` on that secret.
+Pass `--api-key-secret-arn` with the ARN of an AWS Secrets Manager secret holding your [Datadog API key](https://app.datadoghq.com/organization-settings/api-keys). The Agent reads the key from the secret at runtime, which keeps it out of the task definition. The task's execution role needs `secretsmanager:GetSecretValue` on that secret. ECS resolves secrets through that role, so a task definition with no `executionRoleArn` is reported and left alone rather than turned into a revision whose tasks cannot start.
+
+If you do not pass `--api-key-secret-arn`, the command falls back to the `DD_API_KEY` environment variable and writes its value into the task definition in plain text, which it warns about. A key given this way is validated against your Datadog site before anything is registered, and is masked in the diff the command prints.
+
+#### Task role
 
-If you do not pass `--api-key-secret-arn`, the command falls back to the `DD_API_KEY` environment variable and writes its value into the task definition in plain text, which it warns about. A key given this way is validated against your Datadog site before anything is registered.
+The Agent collects ECS task metadata, which is what tags your telemetry with the task, container, and image it came from. It reads that from the ECS API as the task role, so give the task definition a `taskRoleArn` whose policy allows `ecs:ListClusters`, `ecs:ListContainerInstances`, and `ecs:DescribeContainerInstances`. The command does not change your task role, so it reports a task definition that has none.
 
 #### Environment variables
 
+- `DD_BETA_COMMANDS_ENABLED`: set to `1` to enable this command while it is in beta.
 - `DD_API_KEY` (or `DATADOG_API_KEY`): the Datadog API key to write into the task definition, used only when `--api-key-secret-arn` is not passed.
 - `DD_SITE` (or `DATADOG_SITE`): the [Datadog site](https://docs.datadoghq.com/getting_started/site/) to send data to. Defaults to `datadoghq.com`.
 - `AWS_REGION` (or `AWS_DEFAULT_REGION`): the region to use when `--region` is not passed.
@@ -59,7 +74,7 @@ You can pass the following arguments to `instrument` to specify its behavior. `-
 <!-- BEGIN_USAGE:instrument -->
 | Argument | Shorthand | Description | Default |
 | -------- | --------- | ----------- | ------- |
-| `--dry` or `--dry-run` | `-d` | Preview changes running command would apply | `false` |
+| `--dry` or `--dry-run` | `-d` | Preview the changes the command would apply | `false` |
 | `--task-definition` or `--taskDefinition` |  | The family, family:revision, or ARN of the task definition to instrument. Can be specified multiple times. |  |
 | `--region` | `-r` | The AWS region the task definition lives in |  |
 | `--profile` |  | Specify the AWS named profile credentials to use to instrument. Learn more about AWS named profiles here: https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-profiles.html#using-profiles |  |

packages/plugin-ecs-fargate/package.json

@@ -73,6 +73,7 @@
     "@aws-sdk/credential-providers": "3.1045.0",
     "@aws-sdk/types": "3.973.8",
     "@datadog/datadog-ci-base": "workspace:*",
+    "@inquirer/prompts": "7.10.1",
     "@smithy/property-provider": "2.2.0",
     "@smithy/util-retry": "2.2.0",
     "aws-sdk-client-mock": "4.1.0",

packages/plugin-ecs-fargate/src/__tests__/__snapshots__/instrument.test.ts.snap

@@ -21,7 +21,7 @@ exports[`ecs-fargate instrument prints the diff and registers nothing on a dry r
         "essential": true,
         "image": "my-app:latest",
         "logConfiguration": {
-@@ -16,18 +24,74 @@
+@@ -16,19 +24,87 @@
             "awslogs-region": "us-east-1",
             "awslogs-stream-prefix": "ecs"
           }
@@ -31,6 +31,18 @@ exports[`ecs-fargate instrument prints the diff and registers nothing on a dry r
 +     {
 +       "environment": [
 +         {
++           "name": "DD_DOGSTATSD_ORIGIN_DETECTION",
++           "value": "true"
++         },
++         {
++           "name": "DD_DOGSTATSD_ORIGIN_DETECTION_CLIENT",
++           "value": "true"
++         },
++         {
++           "name": "DD_DOGSTATSD_TAG_CARDINALITY",
++           "value": "orchestrator"
++         },
++         {
 +           "name": "ECS_FARGATE",
 +           "value": "true"
 +         },
@@ -95,6 +107,7 @@ exports[`ecs-fargate instrument prints the diff and registers nothing on a dry r
 +       "value": "vXXXX"
 +     }
 +   ],
+    "taskRoleArn": "arn:aws:iam::123456789012:role/my-app-task-role",
     "volumes": []
   }
 "

packages/plugin-ecs-fargate/src/__tests__/fixtures.ts

@@ -61,6 +61,7 @@ export const fargateTaskDefinition = ({
   cpu: '512',
   memory: '1024',
   executionRoleArn: 'arn:aws:iam::123456789012:role/ecsTaskExecutionRole',
+  taskRoleArn: 'arn:aws:iam::123456789012:role/my-app-task-role',
   containerDefinitions: [{...APP_CONTAINER}],
   volumes: [],
   ...overrides,

packages/plugin-ecs-fargate/src/__tests__/instrument.test.ts

@@ -15,7 +15,11 @@ jest.mock('@aws-sdk/credential-providers', () => ({
   ),
 }))
 
+const promptInput = jest.fn().mockResolvedValue('123456')
+jest.mock('@inquirer/prompts', () => ({input: promptInput}))
+
 import type {ContainerDefinition} from '@aws-sdk/client-ecs'
+import type {FromIniInit} from '@aws-sdk/credential-provider-ini'
 
 import {
   DescribeServicesCommand,
@@ -77,6 +81,7 @@ describe('ecs-fargate instrument', () => {
     ecsMock.on(DescribeServicesCommand).resolves({services: [fargateService()], failures: []})
     ecsMock.on(UpdateServiceCommand).resolves({})
     validateApiKey.mockClear().mockResolvedValue(true)
+    promptInput.mockClear()
     fromIni
       .mockClear()
       .mockImplementation(() => () => Promise.resolve({accessKeyId: 'access-key', secretAccessKey: 'secret-key'}))
@@ -257,6 +262,33 @@ describe('ecs-fargate instrument', () => {
       expect(ecsMock.commandCalls(DescribeTaskDefinitionCommand)).toHaveLength(0)
     })
 
+    test('reports task definitions that resolve to the same family', async () => {
+      const {code, context} = await runCLI([
+        '--api-key-secret-arn',
+        MOCK_API_KEY_SECRET_ARN,
+        '--task-definition',
+        'my-app:3',
+      ])
+
+      expect(code).toBe(1)
+      expect(context.stdout.toString()).toContain(
+        '--task-definition names the same task definition family more than once (my-app)'
+      )
+      expect(ecsMock.commandCalls(DescribeTaskDefinitionCommand)).toHaveLength(0)
+    })
+
+    test('accepts task definitions of different families named in different formats', async () => {
+      const {code} = await runCLI([
+        '--api-key-secret-arn',
+        MOCK_API_KEY_SECRET_ARN,
+        '--task-definition',
+        taskDefinitionArn('my-worker', 4),
+      ])
+
+      expect(code).toBe(0)
+      expect(ecsMock.commandCalls(RegisterTaskDefinitionCommand)).toHaveLength(2)
+    })
+
     test('reports a cluster given without a service to update', async () => {
       const {code, context} = await runCLI(['--api-key-secret-arn', MOCK_API_KEY_SECRET_ARN, '--cluster', MOCK_CLUSTER])
 
@@ -267,7 +299,18 @@ describe('ecs-fargate instrument', () => {
     test('uses the named AWS profile it is given', async () => {
       await runCLI(['--api-key-secret-arn', MOCK_API_KEY_SECRET_ARN, '--profile', 'my-profile'])
 
-      expect(fromIni).toHaveBeenCalledWith({profile: 'my-profile'})
+      expect(fromIni).toHaveBeenCalledWith(expect.objectContaining({profile: 'my-profile'}))
+    })
+
+    test('asks for a code when the named profile is backed by MFA', async () => {
+      await runCLI(['--api-key-secret-arn', MOCK_API_KEY_SECRET_ARN, '--profile', 'my-profile'])
+
+      const {mfaCodeProvider} = fromIni.mock.calls[0][0] as FromIniInit
+      expect(mfaCodeProvider).toBeDefined()
+      await mfaCodeProvider?.('arn:aws:iam::123456789012:mfa/someone')
+      expect(promptInput).toHaveBeenCalledWith(
+        expect.objectContaining({message: expect.stringContaining('arn:aws:iam::123456789012:mfa/someone')})
+      )
     })
 
     test('reports a named profile it cannot read', async () => {
@@ -551,6 +594,15 @@ describe('ecs-fargate instrument', () => {
       expect(envVarsOf(registeredContainers(), AGENT_CONTAINER_NAME)).toMatchObject({DD_API_KEY: MOCK_API_KEY})
     })
 
+    test('keeps a plaintext API key out of the diff it prints', async () => {
+      const {code, context} = await runCLI(['--dry-run'], {DATADOG_API_KEY: '', DD_API_KEY: MOCK_API_KEY})
+
+      expect(code).toBe(0)
+      const output = context.stdout.toString()
+      expect(output).toContain('DD_API_KEY')
+      expect(output).not.toContain(MOCK_API_KEY)
+    })
+
     test('fails when no API key is available', async () => {
       const {code, context} = await runCLI([], {DD_API_KEY: '', DATADOG_API_KEY: ''})
 
@@ -589,6 +641,24 @@ describe('ecs-fargate instrument', () => {
       expect(ecsMock.commandCalls(RegisterTaskDefinitionCommand)).toHaveLength(0)
     })
 
+    test('registers and deploys nothing when no execution role can read the API key secret', async () => {
+      ecsMock
+        .on(DescribeTaskDefinitionCommand)
+        .resolves({taskDefinition: fargateTaskDefinition({executionRoleArn: undefined}), tags: []})
+
+      const {code, context} = await runCLI([
+        '--api-key-secret-arn',
+        MOCK_API_KEY_SECRET_ARN,
+        '--ecs-service',
+        MOCK_SERVICE,
+      ])
+
+      expect(code).toBe(1)
+      expect(context.stdout.toString()).toContain('has no executionRoleArn')
+      expect(ecsMock.commandCalls(RegisterTaskDefinitionCommand)).toHaveLength(0)
+      expect(ecsMock.commandCalls(UpdateServiceCommand)).toHaveLength(0)
+    })
+
     test('reports a task definition that does not exist', async () => {
       ecsMock.on(DescribeTaskDefinitionCommand).resolves({})