Skip to content

Properly orders php layers to allow extension #4

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

Closed
FlorianRiquelme wants to merge 2 commits into from
Closed

Properly orders php layers to allow extension #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5661c14
Adds php layer last in this.addLayers() call so that the php layer is…
FlorianRiquelme Jun 10, 2023
110106f
Adds phpLayer last and consoleLayer first and then other passed in la…
FlorianRiquelme Jun 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 43 additions & 7 deletions src/function/ConsoleFunction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,48 @@
import { Stack } from 'aws-cdk-lib';
import {Duration, Stack} from 'aws-cdk-lib';
import { Construct } from 'constructs';
import { consoleLayer } from '../layers';
import { PhpFunction, PhpFunctionProps } from './PhpFunction';
import {consoleLayer, functionLayer} from '../layers';
import {Function, FunctionProps, Runtime} from "aws-cdk-lib/aws-lambda";
import {IVpc} from "aws-cdk-lib/aws-ec2";
import {VpcForServerlessApp} from "../vpc/VpcForServerlessApp";
import {functionDefaults, vpcDefaults} from "./defaults";
import {packagePhpCode} from "../package";

export class ConsoleFunction extends PhpFunction {
constructor(scope: Construct, id: string, props: PhpFunctionProps) {
super(scope, id, props);
export type PhpConsoleFunctionProps = Partial<FunctionProps> & {
phpVersion?: '8.0' | '8.1' | '8.2';
handler: string;
vpc?: IVpc | VpcForServerlessApp;
};

this.addLayers(consoleLayer(this, Stack.of(scope).region));
export class ConsoleFunction extends Function {
constructor(scope: Construct, id: string, props: PhpConsoleFunctionProps) {
const defaults = {
runtime: Runtime.PROVIDED_AL2,
code: props.code ?? packagePhpCode(),
timeout: Duration.seconds(6),
memorySize: functionDefaults.memorySize,
};

const layers = props.layers ?? [];

super(scope, id, {
...props,
...vpcDefaults(props.vpc),
layers: [],
...defaults,
});

this.addLayers(
...layers,
consoleLayer(
this,
Stack.of(scope).region
),
// Adds the function layer last so that others(in this case console depends on it) can override it
functionLayer(
this,
Stack.of(scope).region,
props.phpVersion ?? functionDefaults.phpVersion,
props.architecture ?? functionDefaults.architecture)
);
}
}
4 changes: 2 additions & 2 deletions src/function/PhpFpmFunction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ export class PhpFpmFunction extends Function {
}
const phpVersion = props.phpVersion ?? functionDefaults.phpVersion;
this.addLayers(
// Add the FPM layer first so that other layers can override it
...layers,
// Add the FPM layer last so that other layers can override it
fpmLayer(this, region, phpVersion, props.architecture ?? functionDefaults.architecture),
...layers
);
}
}
4 changes: 2 additions & 2 deletions src/function/PhpFunction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,14 @@ export class PhpFunction extends Function {
}
const phpVersion = props.phpVersion ?? functionDefaults.phpVersion;
this.addLayers(
// Add the function layer first so that other layers can override it
...layers,
// Add the function layer last so that other layers can override it
functionLayer(
this,
region,
phpVersion,
props.architecture ?? functionDefaults.architecture
),
...layers
);
}
}
22 changes: 19 additions & 3 deletions test/function/ConsoleFunction.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { describe, expect, it } from 'vitest';
import { ConsoleFunction } from '../../src';
import { compileTestStack } from '../helper';
import { Architecture } from 'aws-cdk-lib/aws-lambda';
import {Architecture, LayerVersion} from 'aws-cdk-lib/aws-lambda';
import { mapValues } from 'lodash';

describe('ConsoleFunction', () => {
Expand All @@ -15,8 +15,24 @@ describe('ConsoleFunction', () => {
const consoleFunction = template.findResources('AWS::Lambda::Function');
const layers = consoleFunction.Console63CA37A7.Properties.Layers;
expect(layers).length(2);
expect(layers[0]).to.match(/arn:aws:lambda:us-east-1:534081306603:layer:php-81:\d+/);
expect(layers[1]).to.match(/arn:aws:lambda:us-east-1:534081306603:layer:php-81:\d+/);
expect(layers[0]).to.match(/arn:aws:lambda:us-east-1:534081306603:layer:console:\d+/);
Copy link
Member

@mnapoli mnapoli Jun 21, 2023

Choose a reason for hiding this comment

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

I'm looking into this but I really don't understand how this could be what we want 🤔

The test ensures that the console layer is first, but it should be last, right?

Copy link
Member

@mnapoli mnapoli Jun 21, 2023

Choose a reason for hiding this comment

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

I have been testing with master and it is working out fine, here is the CloudFormation template output:

image

Copy link
Member

@mnapoli mnapoli Jun 21, 2023

Choose a reason for hiding this comment

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

I just tested the deployed function and everything was working correctly. TBH I don't think there is a problem to solve?

Maybe you are on a bugged CDK version?

Copy link
Member

@mnapoli mnapoli Jun 28, 2023

Choose a reason for hiding this comment

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

Update: another report here: brefphp/bref#1580

Not sure why I can't reproduce this, this is so weird!

Copy link
Author

@FlorianRiquelme FlorianRiquelme Jun 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry I am (and still am) on vacation, I'll try to keep a closer eye on this conversation :)

Using the ConsoleFunction from the constructs yields the same result that you are seeing, deployed the layers are still reversed for me.

Now the interesting part: If i create a lambda function myself and setting the layers property to use php first and then console, the CF template looks the same(console first, php last), but deployed it is now in the correct order. 😕

I used cdk version 2.8.0 initially and just checked with the latest 2.8.5 and the behaviour is still the same

});

it('adds additional layers before console and php layer', () => {
const template = compileTestStack((stack) => {
new ConsoleFunction(stack, 'Console', {
handler: 'index.php',
layers: [LayerVersion.fromLayerVersionArn(stack, 'Layer', 'arn:aws:lambda:us-east-1:123456789012:layer:layer-name:1')],
});
});

const consoleFunction = template.findResources('AWS::Lambda::Function');
const layers = consoleFunction.Console63CA37A7.Properties.Layers;
expect(layers).length(3);
expect(layers[2]).to.match(/arn:aws:lambda:us-east-1:534081306603:layer:php-81:\d+/);
expect(layers[1]).to.match(/arn:aws:lambda:us-east-1:534081306603:layer:console:\d+/);
expect(layers[0]).to.match(/arn:aws:lambda:us-east-1:123456789012:layer:layer-name:1/);
});

it('supports ARM', () => {
Expand All @@ -29,7 +45,7 @@ describe('ConsoleFunction', () => {

mapValues(template.findResources('AWS::Lambda::Function'), (resource) => {
expect(resource.Properties.Architectures).toEqual(['arm64']);
expect(resource.Properties.Layers[0]).matches(
expect(resource.Properties.Layers[1]).matches(
/arn:aws:lambda:us-east-1:534081306603:layer:arm-php-81:\d+/
);
});
Expand Down
17 changes: 16 additions & 1 deletion test/function/PhpFpmFunction.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { describe, expect, it } from 'vitest';
import { PhpFpmFunction } from '../../src';
import { cleanupTemplate, compileTestStack } from '../helper';
import { Architecture } from 'aws-cdk-lib/aws-lambda';
import {Architecture, LayerVersion} from 'aws-cdk-lib/aws-lambda';
import { mapValues } from 'lodash';

describe('PhpFpmFunction', () => {
Expand Down Expand Up @@ -37,4 +37,19 @@ describe('PhpFpmFunction', () => {

template.resourceCountIs('AWS::Lambda::Function', 2);
});

it('adds additional layer before php layer', () => {
const template = compileTestStack((stack) => {
new PhpFpmFunction(stack, 'Console', {
handler: 'index.php',
layers: [LayerVersion.fromLayerVersionArn(stack, 'Layer', 'arn:aws:lambda:us-east-1:123456789012:layer:layer-name:1')],
});
});

const phpFunction = template.findResources('AWS::Lambda::Function');
const layers = phpFunction.Console63CA37A7.Properties.Layers;
expect(layers).length(2);
expect(layers[1]).to.match(/arn:aws:lambda:us-east-1:534081306603:layer:php-81-fpm:\d+/);
expect(layers[0]).to.match(/arn:aws:lambda:us-east-1:123456789012:layer:layer-name:1/);
});
});
17 changes: 16 additions & 1 deletion test/function/PhpFunction.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { describe, expect, it } from 'vitest';
import { PhpFunction } from '../../src';
import { cleanupTemplate, compileTestStack } from '../helper';
import { Architecture } from 'aws-cdk-lib/aws-lambda';
import {Architecture, LayerVersion} from 'aws-cdk-lib/aws-lambda';
import { mapValues } from 'lodash';

describe('PhpFunction', () => {
Expand Down Expand Up @@ -44,4 +44,19 @@ describe('PhpFunction', () => {

template.resourceCountIs('AWS::Lambda::Function', 2);
});

it('adds additional layer before php layer', () => {
const template = compileTestStack((stack) => {
new PhpFunction(stack, 'Console', {
handler: 'index.php',
layers: [LayerVersion.fromLayerVersionArn(stack, 'Layer', 'arn:aws:lambda:us-east-1:123456789012:layer:layer-name:1')],
});
});

const phpFunction = template.findResources('AWS::Lambda::Function');
const layers = phpFunction.Console63CA37A7.Properties.Layers;
expect(layers).length(2);
expect(layers[1]).to.match(/arn:aws:lambda:us-east-1:534081306603:layer:php-81:\d+/);
expect(layers[0]).to.match(/arn:aws:lambda:us-east-1:123456789012:layer:layer-name:1/);
});
});