Skip to content

Add resource-based permissions support for Lambda aliases #161

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 25, 2025
Merged

Add resource-based permissions support for Lambda aliases #161

merged 1 commit into from
Mar 25, 2025

Conversation

a-hilaly
Copy link
Member

@a-hilaly a-hilaly commented Mar 25, 2025 •
edited
Loading

this commit adds support for managing Lambda resource based permissions directly
on aliases. Now users can declaratively define, update, and remove permissions
for Lambda functions accessed through aliases.

The implementation leverages the AWS::Lambda AddPermission and RemovePermission
APIs to synchronize the desired permissions state defined in the CRD with the
actual alias permisions in AWS.

Example alias:

apiVersion: lambda.services.k8s.aws/v1alpha1
kind: Alias
metadata:
  name: alias1
spec:
  name: alias1
  functionName: test-function-w-ack
  functionVersion: $LATEST
  description: some alias
  permissions:
    - statementID: "1"
      action: lambda:InvokeFunction
      principal: s3.amazonaws.com
      sourceARN: arn:aws:s3:::mybucket
    - statementID: "2"
      action: lambda:InvokeFunction
      principal: s3.amazonaws.com
      sourceARN: arn:aws:s3:::mybucket2
    - statementID: "3"
      action: lambda:InvokeFunction
      principal: s3.amazonaws.com
      sourceARN: arn:aws:s3:::mybucket3
    - statementID: "4"
      action: lambda:InvokeFunction
      principal: s3.amazonaws.com
      sourceARN: arn:aws:s3:::mybucket4

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

@ack-prow ack-prow bot requested review from jlbutler and rushmash91 March 25, 2025 00:40
@ack-prow ack-prow bot added the approved label Mar 25, 2025
@a-hilaly a-hilaly force-pushed the alias/add-permissions branch 2 times, most recently from b2a38c6 to 1ecb947 Compare March 25, 2025 01:30
michaelhtm
michaelhtm approved these changes Mar 25, 2025
View reviewed changes
Copy link
Member

@michaelhtm michaelhtm left a comment

Choose a reason for hiding this comment

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

Thank you @a-hilaly..left a few nits.
mainly about the yaml indent changes
Other than that lgtm 💯

apis/v1alpha1/generator.yaml Outdated
Comment on lines 10 to 31
- CreateCodeSigningConfigInput.Tags
- CreateEventSourceMappingInput.DocumentDBEventSourceConfig
- CreateEventSourceMappingInput.KMSKeyArn
- CreateEventSourceMappingInput.MetricsConfig
- CreateEventSourceMappingInput.ProvisionedPollerConfig
- CreateEventSourceMappingInput.Tags
- CreateEventSourceMappingOutput.FilterCriteriaError
- CreateEventSourceMappingOutput.DocumentDBEventSourceConfig
- CreateEventSourceMappingOutput.KMSKeyArn
- CreateEventSourceMappingOutput.MetricsConfig
- CreateEventSourceMappingOutput.ProvisionedPollerConfig
- FunctionCode.SourceKMSKeyArn
- CreateFunctionInput.LoggingConfig
- CreateFunctionOutput.RuntimeVersionConfig
- CreateFunctionOutput.LoggingConfig
- CreateFunctionUrlConfigInput.InvokeMode
- CreateFunctionUrlConfigOutput.InvokeMode
- PublishVersionOutput.LoggingConfig
- PublishVersionOutput.RuntimeVersionConfig
- VpcConfig.Ipv6AllowedForDualStack
- AddPermissionInput.FunctionName # We grab this from the Alias resource
- AddPermissionInput.Qualifier # We grab this from the Alias resource
Copy link
Member

Choose a reason for hiding this comment

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

nit: why indent?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just got a new yaml linter..

pkg/resource/alias/hooks.go Outdated
continue
}
rlog.Debug("removing permission", "statement_id", *p.StatementID)
if err = rm.removePermission(ctx, desired, *p.StatementID); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

nit: why dereference StatementID here and change it back to pointer in removePermission?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch, changed.

@a-hilaly a-hilaly force-pushed the alias/add-permissions branch 3 times, most recently from 9749a48 to 6577d49 Compare March 25, 2025 02:19
rushmash91
rushmash91 approved these changes Mar 25, 2025
View reviewed changes
Copy link
Member

@rushmash91 rushmash91 left a comment

Choose a reason for hiding this comment

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

Awesome @a-hilaly ! left one minor comment

pkg/resource/alias/hooks.go
// get the policy for the function using the alias name as the qualifier. now we don't
// have to worry about function versions..
input := &svcsdk.GetPolicyInput{
FunctionName: aws.String(functionName),
Copy link
Member

Choose a reason for hiding this comment

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

nice!

pkg/resource/alias/hooks.go Outdated
// Yes the API doesn't support updating permissions directly... yikes.
func comparePermissions(
desired []*svcapitypes.AddPermissionInput,
existing []*svcapitypes.AddPermissionInput,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
existing []*svcapitypes.AddPermissionInput,
latest []*svcapitypes.AddPermissionInput,

nit: can we call this latest for consistency

pkg/resource/alias/hooks.go
rlog := ackrtlog.FromContext(ctx)
exit := rlog.Trace("rm.removePermission")
var err error
defer func() { exit(err) }()
Copy link
Member

Choose a reason for hiding this comment

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

nice!

@a-hilaly
Add resource-based permissions support for Lambda aliases
52a04eb 
this commit adds support for managing Lambda resource based permissions directly
on aliases. Now users can declaratively define, update, and remove permissions
for Lambda functions accessed through aliases.

The implementation leverages the AWS::Lambda `AddPermission` and `RemovePermission`
APIs to synchronize the desired permissions state defined in the CRD with the
actual alias permisions in AWS.
@a-hilaly a-hilaly force-pushed the alias/add-permissions branch from 6577d49 to 52a04eb Compare March 25, 2025 02:44
@ack-prow ACK Prow
Copy link

ack-prow bot commented Mar 25, 2025

@a-hilaly: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
lambda-verify-attribution 52a04eb link false /test lambda-verify-attribution

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@rushmash91
Copy link
Member

thanks @a-hilaly 🚀
/lgtm

@ack-prow ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Mar 25, 2025
@ack-prow ACK Prow
Copy link

ack-prow bot commented Mar 25, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-hilaly, michaelhtm, rushmash91

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [a-hilaly,michaelhtm,rushmash91]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ack-prow ack-prow bot merged commit 35cd334 into aws-controllers-k8s:main Mar 25, 2025
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rushmash91 rushmash91 rushmash91 approved these changes

@michaelhtm michaelhtm michaelhtm approved these changes

@jlbutler jlbutler Awaiting requested review from jlbutler

Assignees

@rushmash91 rushmash91

Labels
approved lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@a-hilaly @rushmash91 @michaelhtm