Skip to content
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

aws-batch-alpha: Unable to specify ECS container env vars containing secrets #25559

Closed
Malanius opened this issue May 12, 2023 · 3 comments · Fixed by #26126
Closed

aws-batch-alpha: Unable to specify ECS container env vars containing secrets #25559

Malanius opened this issue May 12, 2023 · 3 comments · Fixed by #26126
Labels
@aws-cdk/aws-batch Related to AWS Batch bug This issue is a bug. effort/medium Medium work item – several days of effort p1

Comments

@Malanius
Copy link

Describe the bug

Since the changes in the batch-alpha module in 2.74.0, it is impossible to define environment variables holding values from secrets. This is due to the changed interface between the old JobDefinitionContainer and new IEcsContainerDefinition.
The old interface correctly uses key: value from ECS secret with control of which env variable the secret is assigned to and also which secret field is used. The new one just expects an array of Secrets Manager secrets that don't provide any way to specify these things.

Expected Behavior

To be able to define and use ECS secrets in the same way as with previous constructs.

Current Behavior

The new one just expects an array of Secrets Manager secrets that don't provide any way to assign the secret value and field to the container env variable.
I didn't even try to synthesise and deploy this to see what would happen as this clearly doesn't do what's needed.

Reproduction Steps

This is working pre 2.74.0:

import * as batch from '@aws-cdk/aws-batch-alpha';
import * as ecr from 'aws-cdk-lib/aws-ecr';
import * as esc from 'aws-cdk-lib/aws-ecs';
import * as sm from 'aws-cdk-lib/aws-secretsmanager';
import { Construct } from 'constructs';

declare const ecrRepo: ecr.IRepository;
declare const secret: sm.ISecret;

export class JobDefBug extends Construct {
  constructor(scope: Construct, id: string, _props: {}) {
    super(scope, id);

    new batch.JobDefinition(this, 'JobDefinition', {
      container: {
        image: esc.ContainerImage.fromEcrRepository(ecrRepo),
        secrets: {
          SOME_SECRET: esc.Secret.fromSecretsManager(secret, 'secret-field'),
        },
      },
    });
  }
}

Trying to update to the new, post 2.74.0 constructs, the secrets property is incompatible:

import * as batch from '@aws-cdk/aws-batch-alpha';
import * as ecr from 'aws-cdk-lib/aws-ecr';
import * as esc from 'aws-cdk-lib/aws-ecs';
import * as sm from 'aws-cdk-lib/aws-secretsmanager';
import { Construct } from 'constructs';

declare const ecrRepo: ecr.IRepository;
declare const secret: sm.ISecret;

export class JobDefBug extends Construct {
  constructor(scope: Construct, id: string, _props: {}) {
    super(scope, id);

    new batch.EcsJobDefinition(this, 'JobDefinition', {
      container: {
        image: esc.ContainerImage.fromEcrRepository(ecrRepo),
        secrets: {
          SOME_SECRET: esc.Secret.fromSecretsManager(secret, 'secret-field'),
        },
      },
    });
  }
}

The new interface also requires some properties that were optional and had default values in the old one like CPU and memory resources.

Possible Solution

Using the correct interface for secrets property in order to be able to use ECS secrets as before and be able to upgrade existing job definitions is probably the way.

Additional Information/Context

No response

CDK CLI Version

2.79.1

Framework Version

2.74.0

Node.js Version

16.20.0

OS

Linux

Language

Typescript

Language Version

Typescript (4.9.5)

Other information

No response

@Malanius Malanius added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 12, 2023
@github-actions github-actions bot added the @aws-cdk/aws-batch Related to AWS Batch label May 12, 2023
@pahud pahud self-assigned this May 12, 2023
@pahud
Copy link
Contributor

pahud commented May 12, 2023

Yeah I noticed the type is secretsmanager.ISecret[]; now. Thank you for your feedback.

@pahud pahud added p1 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels May 12, 2023
@pahud pahud removed their assignment May 12, 2023
@kellertobias
Copy link

Our hotifx for this issue was:

	const job = new batchAlpha.EcsJobDefinition(..., {container: {secrets: Object.values(secretsMap)}} /* Define as usual */)
	const cfnContainerDef = job.node.defaultChild as CfnJobDefinition;
	const secrets: { Name: string; ValueFrom: string }[] = [];
	Object.entries(secretsMap).forEach(([Name, secret]) => {
		secrets.push({ Name, ValueFrom: secret.secretArn });
	});
	cfnContainerDef.addPropertyOverride('ContainerProperties.Secrets', secrets);

@mergify mergify bot closed this as completed in #26126 Jun 29, 2023
mergify bot pushed a commit that referenced this issue Jun 29, 2023
…ironment Variables & Enable Kubernetes Secret Volumes (#26126)

Changes the type of `secrets` from `ISecret[]` to `{ [key: string]: ISecret }`. The `key` is the name of the environment variable to expose to the container. 

Also enables the specification of EKS Kubernetes volumes, which our README documented but wasn't actually supported because of a CFN issue that has since been fixed.

Closes #25559.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

lukey-aleios pushed a commit to lukey-aleios/aws-cdk that referenced this issue Jun 30, 2023
…ironment Variables & Enable Kubernetes Secret Volumes (aws#26126)

Changes the type of `secrets` from `ISecret[]` to `{ [key: string]: ISecret }`. The `key` is the name of the environment variable to expose to the container. 

Also enables the specification of EKS Kubernetes volumes, which our README documented but wasn't actually supported because of a CFN issue that has since been fixed.

Closes aws#25559.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
lukey-aleios pushed a commit to lukey-aleios/aws-cdk that referenced this issue Jun 30, 2023
…ironment Variables & Enable Kubernetes Secret Volumes (aws#26126)

Changes the type of `secrets` from `ISecret[]` to `{ [key: string]: ISecret }`. The `key` is the name of the environment variable to expose to the container. 

Also enables the specification of EKS Kubernetes volumes, which our README documented but wasn't actually supported because of a CFN issue that has since been fixed.

Closes aws#25559.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-batch Related to AWS Batch bug This issue is a bug. effort/medium Medium work item – several days of effort p1
Projects
None yet
3 participants