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: ManagedComputeEnvironment L2's cannot handle CfnParameters due to validations #32905

Open
1 task
bdoyle0182 opened this issue Jan 14, 2025 · 3 comments · May be fixed by #32954
Open
1 task

aws-batch: ManagedComputeEnvironment L2's cannot handle CfnParameters due to validations #32905

bdoyle0182 opened this issue Jan 14, 2025 · 3 comments · May be fixed by #32954
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

@bdoyle0182
Copy link

Describe the bug

When attempting to instantiate one of the new L2's for compute environments, synthesis will fail if attempting to use cloud formation parameters for certain configurations. I can work around this with the L1, however it would be nice to be able to use the L2 and skip the client side validation the cdk does for these params. L1 is very tedious and cumbersome for compute environments with lots of properties that can and cannot be specified conditionally depending on others being specified.

I want to be able to do something like this:
maxvCpus: this.SPOT_MAX_VCPU_PARAMETER.valueAsNumber,

but at synthesis the L2 does validations that require the field to be resolved as it does validation that the number is valid.

It would be nice if the validation functions in ManagedComputeEnvironment could check if the values are tokens and if so skip the validation if the construct doesn't need them to be resolved elsewhere. This is done elsewhere in the cdk as there's many issue reports of L2 property validation functions not accounting for the value being a token and some L2's have accepted fixes to bypass if a token.

My use case is such that I have an organization where I'm attempting to build a stack set that creates compute environments in every account, but I want certain configs of that compute environment that created for members of my organization like min and max vcpu.

Regression Issue

  • Select this option if this issue appears to be a regression.

Last Known Working CDK Version

N/A

Expected Behavior

Not fail synthesis when passing a CfnParameter token to a ManagedComputeEnvironment L2.

Current Behavior

Synthesis fails due to client side validation of property values passed to the L2 expecting to be resolved.

Reproduction Steps

instantiate a managed compute environment L2 where the minvCpus field takes in a cfn parameter and converts it to number with .valueAsNumber()

Possible Solution

Potential fix would be to pass the L2's internal validation if the property is a Token. This pattern has been done elsewhere in the cdk in L2 validations so it shouldn't be breaking any design principle if the construct does not depend on it being resolved at synth time. See a similar bug and fix commit for a kinesis config bypassing validation if property is token:

3749c2a

#9038

Additional Information/Context

No response

CDK CLI Version

2.175.1

Framework Version

No response

Node.js Version

22

OS

macOS

Language

TypeScript

Language Version

No response

Other information

No response

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

pahud commented Jan 14, 2025

Apparently a bug.

function validateVCpus(id: string, minvCpus: number, maxvCpus: number): void {
if (minvCpus < 0) {
throw new Error(`Managed ComputeEnvironment '${id}' has 'minvCpus' = ${minvCpus} < 0; 'minvCpus' cannot be less than zero`);
}
if (minvCpus > maxvCpus) {
throw new Error(`Managed ComputeEnvironment '${id}' has 'minvCpus' = ${minvCpus} > 'maxvCpus' = ${maxvCpus}; 'minvCpus' cannot be greater than 'maxvCpus'`);
}
}

This function should check if it's resolvable token before validating its value. I'm making it a p1 but we welcome PRs as well.

@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 Jan 14, 2025
@pahud pahud removed their assignment Jan 14, 2025
@bdoyle0182
Copy link
Author

Yes I'll try to submit a PR myself in the next day or two when I get a chance since it's just a couple lines of code to skip over tokens. There's a couple other numeric based config validations in the L2 we can do a skip over token check

@bdoyle0182
Copy link
Author

@pahud I opened a PR here which should cover this! #32954

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
2 participants