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

fix(aws-lambda-plugin)! Use AWS SigV4, require AWS region, allow custom lambda hosts #8082

Merged
merged 4 commits into from
Jun 1, 2022

Conversation

segabond
Copy link
Contributor

Summary

As per FTI-2928, customer uses Kong in front of AWS lambda in the isolated gov AWS region. Such regions use custom service endpoints, therefore customer uses host configuration to override it. Since config schema does not allow both host and region be supplied simultaneously, it is impossible to correctly sign the request using AWS SigV4 algorithm. The correct implementation of such plugin should allow both aws_region and host be supplied, as aws_region is a mandatory field for SigV4 signature algo.

Full changelog

  • Allow both host and aws_region in config
  • Require AWS region to always be configured
  • Always apply SigV4 signature
  • Fixed tests to test environment variable configuration correctly

Issues resolved

Fix FTI-2928

@segabond segabond requested a review from Tieske November 16, 2021 20:28
@CLAassistant
Copy link

CLAassistant commented Nov 16, 2021

CLA assistant check
All committers have signed the CLA.

@Tieske
Copy link
Member

Tieske commented Dec 7, 2021

If the region now is required, then this is a breaking change. And even then I don't see how we can add that info with a migration.

pinging the migrations/breaking change master; @mikefero

@RobSerafini RobSerafini added this to the 2.8.0 milestone Feb 10, 2022
@RobSerafini
Copy link
Contributor

@Tieske @aboudreault @kikito - what is the status of this PR?

@aboudreault
Copy link
Contributor

doesn't seem like we can merge this feature due to the breaking change mentioned. We cannot really guess the region if none is provided in the config. Options:

  • target this feature for 3.0 with the breaking change
  • revisit this feature implementation to preserve the current behavior until 3.0 (so keeping aws_region optional, and don't try to sign v4 if not specified?)

However, we are probably too short to include this in 2.8.

@kikito
Copy link
Member

kikito commented Feb 23, 2022

This did not make it into 2.8, I am clearing its milestone

@tyler-ball
Copy link
Contributor

Rebased off master

@mikefero mikefero added the version-compatibility Incompatibility with older data plane versions label Apr 7, 2022
@kikito
Copy link
Member

kikito commented May 6, 2022

Some context for reviewers: AWS SigV4 was released in 2012. It replaces SigV2, which was deprecated in 2020.

@tyler-ball tyler-ball added this to the 3.0 milestone May 23, 2022
@fffonion
Copy link
Contributor

@segabond As we now target this for 3.0 with the go-with-breaking-change approach, could you add this to the changelog? (and also rebase upon master to run CI again)

Copy link
Contributor

@mayocream mayocream left a comment

Choose a reason for hiding this comment

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

Please also include an entry in the changelog for this PR.

Comment on lines -157 to -158
-- if this is an AWS request, sign it
if region then
Copy link
Contributor

@mayocream mayocream May 27, 2022

Choose a reason for hiding this comment

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

The change will break the fix for Kong/kong-plugin-aws-lambda#60. Probably you should leave it there.

@segabond segabond requested a review from a team as a code owner June 1, 2022 12:09
@mayocream mayocream added pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) and removed pr/please review labels Jun 1, 2022
if not region and not host then
return error("no region or host specified")
if not region then
return error("no region specified")
Copy link
Member

Choose a reason for hiding this comment

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

For other people's reference: this is a breaking change. This conditional makes conf.aws_region mandatory, in effect.

The reason why we don't make it mandatory on the schema is that we also accept setting it via the AWS_REGION env var, and our schemas don't support doing that automatically.

@kikito kikito changed the title fix(aws-lambda-plugin) require AWS region, allow custom lambda hosts fix(aws-lambda-plugin)! Use AWS SigV4, require AWS region, allow custom lambda hosts Jun 1, 2022
@kikito kikito merged commit 22ad330 into master Jun 1, 2022
@kikito kikito deleted the fix/aws-lambda-plugin branch June 1, 2022 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog plugins/aws-lambda pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) size/XXL version-compatibility Incompatibility with older data plane versions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants