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

refactor(lambda): missing Lambda VPC and security validation #26528

Merged
merged 6 commits into from
Aug 22, 2023

Conversation

lpizzinidev
Copy link
Contributor

Specifying the securityGroups property requires a vpc.

This fix adds validation for the case when a vpc is not specified, but securityGroups is.
It also adds validation for the case when both securityGroups and allowAllOutbound are specified (allowAllOutbound should be configured in the SGs).

Question for the reviewers
How should we handle the case of an empty list in securityGroups? (eg securityGroups: [])

Closes #26508.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@aws-cdk-automation aws-cdk-automation requested a review from a team July 27, 2023 07:16
@github-actions github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p2 star-contributor [Pilot] contributed between 25-49 PRs to the CDK labels Jul 27, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@lpizzinidev
Copy link
Contributor Author

Exemption Request.
The unit tests should already cover all the validation cases for the related function.

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Jul 27, 2023
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 27, 2023
@aws-cdk-automation
Copy link
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@lpizzinidev lpizzinidev changed the title fix(lambda): validate securityGroups in VPC configuration fix(lambda): missing Lambda VPC and security validation Aug 18, 2023
@lpizzinidev lpizzinidev changed the title fix(lambda): missing Lambda VPC and security validation chore(lambda): missing Lambda VPC and security validation Aug 18, 2023
@lpizzinidev lpizzinidev changed the title chore(lambda): missing Lambda VPC and security validation fix(lambda): missing Lambda VPC and security validation Aug 18, 2023
@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Aug 18, 2023
@lpizzinidev lpizzinidev changed the title fix(lambda): missing Lambda VPC and security validation refactor(lambda): missing Lambda VPC and security validation Aug 18, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review August 18, 2023 09:43

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Aug 18, 2023
rix0rrr
rix0rrr previously approved these changes Aug 22, 2023
@mergify
Copy link
Contributor

mergify bot commented Aug 22, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Aug 22, 2023
@mergify
Copy link
Contributor

mergify bot commented Aug 22, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot dismissed rix0rrr’s stale review August 22, 2023 12:58

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Aug 22, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: d642f63
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 6087424 into aws:main Aug 22, 2023
@mergify
Copy link
Contributor

mergify bot commented Aug 22, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify bot pushed a commit that referenced this pull request Dec 15, 2023
The following PR adds validation for the case when `allowAllOutbound` and `securityGroups` are specified at the same time in `FunctionOptions`.
#26528
(#27157)

According to related issues and discussions, this PR causes existing Lambda deployments to fail.
However, since this change has already been merged and I think it is the correct change, I did not fix the validation process but added documentation to clarify the behavior.

Relates to #28170, #27669 

----

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

When we are using the Security group without VPC for @aws-cdk/aws-lambda package it was working well but when we have migrate it to the aws-cdk-lib/aws-lambda package we are getting following error message,
Cannot configure 'securityGroups' without configuring a VPC

Kindly suggest the possible solution for it.

@lpizzinidev
Copy link
Contributor Author

lpizzinidev commented Jan 4, 2024

Hi @mbhaalalhhaexchange
You should provide a vpc to your function if you want to specify the securityGroups parameter (validation).

@mbhaalalhhaexchange
Copy link

mbhaalalhhaexchange commented Jan 5, 2024

Hi @lpizzinidev , It was working fine with aws cdk v1 library (@aws-cdk/aws-lambda). So, why it is changed it in aws cdk v2 library (aws-cdk-lib/aws-lambda) ?

@lpizzinidev
Copy link
Contributor Author

@mbhaalalhhaexchange
Can you please provide info about the v1 version that you were using and a snippet of the code where you initialize your functions?

@mbhaalalhhaexchange
Copy link

Hi @lpizzinidev ,
Below are the code snippet that we are using,

` super(scope, id, {
...props,
vpc: undefined,
code: resolve(__dirname, '..', 'dist', 'src'),
lambdas: lambdas.map((lambda) => ({
...lambda,
environment: {
...(lambda.environment || {}),
DB_SECRET_ARN: props.dbSecretArn,
LOGGING_LEVEL: 'debug',
SQS_CALL_INFO_QUEUE: props.sqsCallInfoQueue.queueUrl
},
securityGroups: [props.securityGroup],
policyStatements: [
...(lambda.policyStatements || []),
new PolicyStatement({
actions: ['secretsmanager:GetSecretValue'],
resources: [props.dbSecretArn]
}),
new PolicyStatement({
actions: ['kms:Decrypt'],
resources: [props.dbSecretKmsKeyArn]
}),
new PolicyStatement({
actions: ['cloudwatch:PutMetricData'],
resources: ['']
}),
new PolicyStatement({
actions: ['sqs:SendMessage'],
resources: [props.sqsCallInfoQueue.queueArn]
}),
new PolicyStatement({
actions: ['connect:StartOutboundVoiceContact'],
resources: ['
']
})
],
memorySize: lambda.memorySize || 512,
retryAttempts: 0,
lambdaTimeout: lambda?.lambdaTimeout || 8,
runtime: Runtime.NODEJS_18_X
})),
layer: {
folderPath: resolve(__dirname, '..', 'dist', 'lambdaLayer'),
compatibleRuntimes: [Runtime.NODEJS_18_X]
}

});`

**FYI** - This **code snippet was working very well with AWS CDK V1 version for @aws-cdk/aws-lambda** package.
**Same code snippet is not working** and get this VPC related error while u**sing AWS CDK V2 version for aws-cdk-lib/aws-lambda** package.

@lpizzinidev
Copy link
Contributor Author

lpizzinidev commented Jan 5, 2024

@mbhaalalhhaexchange
securityGroups are Only used if 'vpc' is supplied.
This PR only adds validation for it, your code was working, but, since vpc: undefined, the securityGroups you specified were ignored.

You can either:

  1. Specify a vpc for your function (will update the deployment as the function will be placed in a custom VPC)
  2. Remove securityGroups: props.securityGroups (won't change the deployment as the attribute was already ignored)

@mbhaalalhhaexchange
Copy link

Hi @lpizzinidev , It is understandable this changes is introduced in AWS CDK V2 right ?
For AWS-CDK V1 it was optional to pass VPC even if security groups are specified because we never got this type of error with same code snippet with CDK C1 library.

@lpizzinidev
Copy link
Contributor Author

@mbhaalalhhaexchange

It is understandable this changes is introduced in AWS CDK V2 right ?

Yep.

For AWS-CDK V1 it was optional to pass VPC even if security groups are specified

Both parameters are still optional, we just added validation as specifying securityGroups or securityGroup (deprecated) without vpc ignores the parameters.

If you don't need to change your implementation, you can remove the securityGroups: props.securityGroups property and it will work as before (feel free to follow up if you encounter any errors after the change).

@mbhaalalhhaexchange
Copy link

Hi @lpizzinidev , We can understand that but we want to make sure like same code should be work with AWS CDK V2 as well but due to this validation , also need to modify code and i am feeling worry about for future overheads.

@mbhaalalhhaexchange
Copy link

After doing some research , some articles are claiming that AWS Team has introduced this change due to security concerns but here I do not see any security concerns.

If we do not pass to VPC and only want to pass security groups then it should allow. This condition is forcing us to do required changes in our code.

What's your thoughts on this ?

@mbhaalalhhaexchange
Copy link

Hi @lpizzinidev , What's your thoughts on this ?

@lpizzinidev
Copy link
Contributor Author

@mbhaalalhhaexchange
Passing security groups to a Lambda outside a VPC has no effect. That's why the validation was added.
Your current implementation with v1 is passing the property, but that value is not used as vpc: undefined.
If you want to manage the security groups of your function you'll need to associate it with a VPC (whether you stay on v1 or upgrade to v2).
If you want to keep the current implementation and don't manage the security groups you'll need to avoid passing the securityGroups property (which was silently ignored in v1).

@mbhaalalhhaexchange
Copy link

mbhaalalhhaexchange commented Jan 9, 2024

Hi @lpizzinidev , It is understood but it is strange that AWS has silently ignored in v1. It was not expected that they have forgot to add such type of validations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. effort/medium Medium work item – several days of effort p2 pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. star-contributor [Pilot] contributed between 25-49 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws-lambda: VpcConfig is not configured for Lambda Function without specifying Vpc prop
4 participants