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

Patch lambda to allow imports #3420

Merged
merged 9 commits into from
Feb 15, 2024
Merged

Conversation

VenelinMartinov
Copy link
Contributor

@VenelinMartinov VenelinMartinov commented Feb 12, 2024

Should address #2392

We regressed in our handling of imported lambdas in 5.28, when we picked up upstream https://github.com/hashicorp/terraform-provider-aws/releases/tag/v4.51.0 with https://github.com/hashicorp/terraform-provider-aws/pull/28963/files, which introduces the ExactlyOneOf constraints in the lambda resource.

This PR reverts the upstream ExactlyOneOf constraints and replaces them with the previously applied ConflictsWith. This in turn allows imports of lambdas via pulumi import and the import resource option to work properly.

I've added a GRPC test for a lambda imported via pulumi import as well as an integration test for a lambda imported via the import resource option and a test which checks that we still fail to create lambdas without any code-related properties (which should be the only case that now passes the validation step which previously didn't).

Note that this slightly worsens the behaviour in the case when none of the code-related properties are specified. Previously that'd tirgger a failure during Check and print a sensible error message but now we will fail during Create with the following much less legible error:

  aws:lambda:Function (testLambda):
    error: 1 error occurred:
        * creating Lambda Function (testLambda-e9e5e22): operation error Lambda: CreateFunction, https response error StatusCode: 400, RequestID: 88b5f9d7-42d6-4a3b-becc-96cdf954f596, api error ValidationException: 2 validation errors detected: Value '' at 'code.s3Key' failed to satisfy constraint: Member must have length greater than or equal to 1; Value '' at 'code.s3Bucket' failed to satisfy constraint: Member must have length greater than or equal to 3

I don't see a sensible way around it though and I think the change is still worthwhile.

Summary of effects:

<5.28 5.28-6.22 This PR
pulumi import works? doesn't work works
import option works doesn't work works
no code lambda errors during update? errors during validation errors during update
other lambdas works works works

Copy link

Does the PR have any schema changes?

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Maintainer note: consult the runbook for dealing with any breaking changes.

@VenelinMartinov VenelinMartinov changed the title Repro for 2392 Patch lambda to allow imports Feb 13, 2024
@VenelinMartinov VenelinMartinov marked this pull request as ready for review February 13, 2024 11:32
@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Feb 13, 2024

Actually thinking a bit more on the bad error message, Value '' at 'code.s3Key' failed to satisfy constraint: must mean that none of the code related properties are specified, so we can probably replace the error message with something more sensible. I'll try that next.

I'm not aware of a way to both support the imported lambdas AND have the error about new lambdas with no code properties happen during validation though.

Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

LGTM

Can you create a bridge issue to support ExactlyOneOf in import?


// Check that we can change a property on the lambda
secondStack.SetConfig("runtime", "nodejs16.x")
secondStack.Up()
Copy link
Member

Choose a reason for hiding this comment

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

There's no asserts here are we sure this is a strong test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The framework has the asserts by default: https://github.com/pulumi/providertest/blob/dc14d64784583753791733c6ac090d972d1ec746/pulumitest/up.go#L18

I've also verified that the test fails without the patch.

@t0yv0
Copy link
Member

t0yv0 commented Feb 13, 2024

Tests are crucial here as the patches are a brittle mechanism an we are trying to move fast - so we need good reminders where it stops working. Would appreciate solutions without patches long-term e.g. lean into supported primitives if possible..

@t0yv0 t0yv0 self-requested a review February 13, 2024 22:33
@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Feb 14, 2024

Can you create a bridge issue to support ExactlyOneOf in import?

The issue is not the bridge support for ExactlyOneOf - when we call Read on the lambda we get none of the ExactlyOneOf properties (filename, s3_bucket, image_uri) - instead we get a sourceCodeHash. That seems to be the way the code is represented after getting uploaded initially. I've pasted the GRPC logs from an imported lambda here: #3371 (comment)

We (and TF) previously supported creating a lambda without any of the code-related propeties (filename, s3_bucket, image_uri) but since the addition of the ExactlyOneOf this no longer works. Creating lambdas without code-related properties is necessary for imports. And for imports via the resource option we don't even have sourceCodeHash in the inputs: #2392 (comment)

Would appreciate solutions without patches long-term e.g. lean into supported primitives if possible

I am not aware of any non-patch way of handling this, LMK if you disagree.

@t0yv0
Copy link
Member

t0yv0 commented Feb 14, 2024

How does pure TF handle this import.

@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Feb 14, 2024

Badly:

# __generated__ by Terraform
# Please review these resources and move them into your main configuration files.

# __generated__ by Terraform
resource "aws_lambda_function" "test_lambda" {
  architectures                  = ["x86_64"]
  code_signing_config_arn        = null
  description                    = "Lambda function that sets security headers on a Cloudfront origin response."
  filename                       = null
  function_name                  = "lambdaEdge-3d0ab09"
  handler                        = "__index.handler"
  image_uri                      = null
  kms_key_arn                    = null
  layers                         = []
  memory_size                    = 128
  package_type                   = "Zip"
  publish                        = null
  reserved_concurrent_executions = -1
  role                           = "arn:aws:iam::616138583583:role/lambdaEdgeRole-8823a3f"
  runtime                        = "nodejs12.x"
  s3_bucket                      = null
  s3_key                         = null
  s3_object_version              = null
  skip_destroy                   = false
  source_code_hash               = "he1IIDJ8LmknDnaymtZVuMvwRjS8b2tCIxkpgwQlSe4="
  tags                           = {}
  tags_all                       = {}
  timeout                        = 5
  ephemeral_storage {
    size = 512
  }
  tracing_config {
    mode = "PassThrough"
  }
}

terraform plan -generate-config-out=generated.tf:

╷
│ Error: Invalid combination of arguments
│
│   with aws_lambda_function.test_lambda,
│   on generated.tf line 4:
│   (source code not available)
│
│ "filename": one of `filename,image_uri,s3_bucket` must be specified
╵
╷
│ Error: Invalid combination of arguments
│
│   with aws_lambda_function.test_lambda,
│   on generated.tf line 7:
│   (source code not available)
│
│ "image_uri": one of `filename,image_uri,s3_bucket` must be specified
╵
╷
│ Error: Invalid combination of arguments
│
│   with aws_lambda_function.test_lambda,
│   on generated.tf line 16:
│   (source code not available)
│
│ "s3_bucket": one of `filename,image_uri,s3_bucket` must be specified

However they don't have an import resource option, so this isn't that much of a regression for them - it only affects the import CLI command.

@VenelinMartinov VenelinMartinov enabled auto-merge (squash) February 15, 2024 12:06
@VenelinMartinov VenelinMartinov merged commit d80e951 into master Feb 15, 2024
18 checks passed
@VenelinMartinov VenelinMartinov deleted the vvm/repro_lambda_import_issue branch February 15, 2024 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants