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-cdk_aws-apprunner-alpha: Invalid cpu or memory should be validated #25872

Closed
1 of 2 tasks
go-to-k opened this issue Jun 6, 2023 · 10 comments · Fixed by #25877
Closed
1 of 2 tasks

@aws-cdk_aws-apprunner-alpha: Invalid cpu or memory should be validated #25872

go-to-k opened this issue Jun 6, 2023 · 10 comments · Fixed by #25877
Labels
@aws-cdk/aws-apprunner Related to the apprunner package effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2

Comments

@go-to-k
Copy link
Contributor

go-to-k commented Jun 6, 2023

Describe the feature

App Runner's CPU and Memory have a fixed input patterns in CloudFormation.
However, in CDK's L2 Construct (alpha), the input format is free (like Some vCPU). Tests are as follows (packages/@aws-cdk/aws-apprunner-alpha/test/service.test.ts).

test('custom cpu and memory units are allowed', () => {
  // GIVEN
  const app = new cdk.App();
  const stack = new cdk.Stack(app, 'demo-stack');
  // WHEN
  new apprunner.Service(stack, 'DemoService', {
    source: apprunner.Source.fromEcrPublic({
      imageIdentifier: 'public.ecr.aws/aws-containers/hello-app-runner:latest',
    }),
    cpu: apprunner.Cpu.of('Some vCPU'),
    memory: apprunner.Memory.of('Some GB'),
  });
  // THEN
  Template.fromStack(stack).hasResourceProperties('AWS::AppRunner::Service', {
    InstanceConfiguration: {
      Cpu: 'Some vCPU',
      Memory: 'Some GB',
    },
    NetworkConfiguration: {
      EgressConfiguration: {
        EgressType: 'DEFAULT',
      },
    },
  });
});

I would like to validate an invalid format before deploy.

Use Case

Incorrect values can be detected at the synth stage before deployment.

Proposed Solution

Create a private method for validation and call it at the constructor in the Service class.

Other Information

CFn output is the following message when deploy errors.

Properties validation failed for resource AppRunnerConstructAppRunnerService123456789 with message: #/InstanceConfiguration/Cpu: failed validation constraint for keyword [pattern]

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

v2.82.0

Environment details (OS name and version, etc.)

MacOS Ventura 13.2

@go-to-k go-to-k added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Jun 6, 2023
@github-actions github-actions bot added the @aws-cdk/aws-apprunner Related to the apprunner package label Jun 6, 2023
@pahud pahud self-assigned this Jun 6, 2023
@pahud
Copy link
Contributor

pahud commented Jun 6, 2023

I believe we should first consider using existing options in the enum-like class as below as they are all valid values. The of method is primarily for new values not defined in the class(and we should add it in). However, it's also good to have a validation for all values with the of() method but I wonder what is the value you are trying to use with of() instead of the pre-defined options in the class?

export class Cpu {
/**
* 0.25 vCPU
*/
public static readonly QUARTER_VCPU = Cpu.of('0.25 vCPU');
/**
* 0.5 vCPU
*/
public static readonly HALF_VCPU = Cpu.of('0.5 vCPU');
/**
* 1 vCPU
*/
public static readonly ONE_VCPU = Cpu.of('1 vCPU')
/**
* 2 vCPU
*/
public static readonly TWO_VCPU = Cpu.of('2 vCPU')
/**
* 4 vCPU
*/
public static readonly FOUR_VCPU = Cpu.of('4 vCPU')

@pahud pahud added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. p2 effort/medium Medium work item – several days of effort labels Jun 6, 2023
@pahud pahud removed their assignment Jun 6, 2023
@pahud pahud removed the needs-triage This issue or PR still needs to be triaged. label Jun 6, 2023
@go-to-k
Copy link
Contributor Author

go-to-k commented Jun 6, 2023

@pahud

I believe we should first consider using existing options in the enum-like class as below as they are all valid values.

and we should add it in

I agree.

I wonder what is the value you are trying to use with of() instead of the pre-defined options in the class?

Since only unit types (e.g. 1 vCPU) are defined in the enum-like class, I assume there is an of() method only for numeric types (e.g. 1024). In other words, if we define options for numeric type, there will be no more cases to use of().

If of() is removed, there is no need for validation. But in fact, I don't think we can remove the of() for backward compatibility. Therefore, it would be also good to validate for all values with the of() method.

I think what we can do now is one of the following

  1. Add number type to enum options.
  2. Validate all values with the of() method.
  3. Do both.

@go-to-k
Copy link
Contributor Author

go-to-k commented Jun 6, 2023

I just thought that, as the significance of existence of of(), there may be a case to pass numerical values to of() without using enum options, when specs are written in numerical values in an external parameter file, etc., and then read them in.

@go-to-k
Copy link
Contributor Author

go-to-k commented Jun 6, 2023

In that case, the enum options for numeric types may not be necessary; it is also difficult to know how to use them differently from the enum options for unit types. Then, what to do is Validate all values with the of() method..

@pahud
Copy link
Contributor

pahud commented Jun 6, 2023

Agree. We would love to review your PR when it's ready.

@go-to-k
Copy link
Contributor Author

go-to-k commented Jun 6, 2023

Thank you, I'll try it that way. (Validate all values with the of() method.)

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jun 6, 2023
@go-to-k
Copy link
Contributor Author

go-to-k commented Jun 7, 2023

@pahud

Hi! I have submitted a PR, please review.

@keyboSlice
Copy link

Sorry to jump into an issue that may only be tangentially related, but I'm currently receiving validation errors when trying to set resource allocation using the enum-like classes provided by the package - is that expected?

The following service creation:

new apprunner.Service(this, getResourceName(`apprunner-service`, props.stage), {
      cpu: apprunner.Cpu.TWO_VCPU,
      memory: apprunner.Memory.SIX_GB,
      source: apprunner.Source.fromAsset({
        asset: dockerImage,
        imageConfiguration: {
          port: 3000,
          environmentVariables: {
            // some extra config here
          },
        },
      }),
    });

Results in the following error from CloudFormation -

#/InstanceConfiguration/Memory: failed validation constraint for keyword [pattern] #/InstanceConfiguration/Cpu: failed validation constraint for keyword [pattern]

Is that expected from the way that I'm providing the values do you know please?

@go-to-k
Copy link
Contributor Author

go-to-k commented Jun 10, 2023

@keyboSlice

I was able to deploy with the above specs (Cpu.TWO_VCPU and Memory.SIX_GB) using the enum-like classes.
I think your version may be out of date. My version is following,

"aws-cdk": "2.83.1",
"@aws-cdk/aws-apprunner-alpha": "^2.83.1-alpha.0",

@mergify mergify bot closed this as completed in #25877 Jun 12, 2023
mergify bot pushed a commit that referenced this issue Jun 12, 2023
This PR adds the same validation for the App Runner's CPU and Memory values as [CloudFormation's input patterns](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-apprunner-service-instanceconfiguration.html#cfn-apprunner-service-instanceconfiguration-cpu).

Closes #25872

----

*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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-apprunner Related to the apprunner package effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants