feat: disallow unique entry file name #1328
Conversation
⏱ Benchmark resultsComparing with 84189b1 largeDepsEsbuild: 2.1s⬇️ 31.44% decrease vs. 84189b1 LegendlargeDepsNft: 7.4s⬇️ 51.51% decrease vs. 84189b1 LegendlargeDepsZisi: 14.8s⬇️ 39.87% decrease vs. 84189b1 |
Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com>
| let hasConflict = false | ||
|
|
||
| srcFiles.forEach((srcFile) => { | ||
| if (featureFlags.zisi_disallow_new_entry_name && srcFile.includes(ENTRY_FILE_NAME)) { |
There was a problem hiding this comment.
Why are you using includes here?
There was a problem hiding this comment.
To catch dirs as well as files that contain the name. It would also catch names that only include ___netlify-lambda-entry as part of it, but it is simpler, the alternative would be:
basename(srcFile, extname(srcFile)) === ENTRY_FILE_NAME || srcFile.includes(`/${ENTRY_FILE_NAME}/`)There was a problem hiding this comment.
I see. I find it a bit disconcerting that we're checking so loosely for that string anywhere in the file or directory names, even if that's a very unique string.
But more generally, I wonder if we need to do this type of check at all. We're the ones adding the entry file, so we know exactly what is its name and extension. Can we check, at the point of inserting it, whether there's already a file in the zip with that name? At that point, it becomes a simple equality check, which should also be more performant?
There was a problem hiding this comment.
The problem is that it is not just one file, but multiple that we are forbidding. When AWS searches for the entrypoint this is what it looks for (tested just now):
/___netlify-lambda-entry.js/___netlify-lambda-entry/index.js/___netlify-lambda-entry.mjs/___netlify-lambda-entry.cjs
So any user file named like this should not be allowed. Checking when adding the entry file is a little too late for now, because this PR is meant to check if ___netlify-lambda-entry is already used. And as we currently do not always generate an entry file, we would not always check for these files.
Alternatively we could have a Set with all those relative paths and check if the user files is included?
There was a problem hiding this comment.
Ah, I see!
Alternatively we could have a Set with all those relative paths and check if the user files is included?
That makes sense to me. If you're okay with that, I would suggest this route.
There was a problem hiding this comment.
Worse, some combinations make Lambda crash I just noticed:
Crashes:
Rename the folder:
I will look into the Set.
There was a problem hiding this comment.
Disallowing /___netlify-lambda-entry/ and ___netlify-lambda-entry.(c|m)?js would look like this:
I am not sure I like this, and also does a lot more per iteration.
@@ -1,4 +1,4 @@
-import { basename, extname, resolve } from 'path'
+import { basename, extname, relative, resolve } from 'path'
import type { FeatureFlags } from '../../../feature_flags.js'
import { FunctionBundlingUserError } from '../../../utils/error.js'
@@ -24,6 +24,12 @@
return `export { handler } from '${importPath}'`
}
+const LAMBDA_ENTRY_FILES = new Set([
+ ENTRY_FILE_NAME + ModuleFileExtension.JS,
+ ENTRY_FILE_NAME + ModuleFileExtension.MJS,
+ ENTRY_FILE_NAME + ModuleFileExtension.CJS,
+])
+
// They are in the order that AWS Lambda will try to find the entry point
const POSSIBLE_LAMBDA_ENTRY_EXTENSIONS = [ModuleFileExtension.JS, ModuleFileExtension.MJS, ModuleFileExtension.CJS]
@@ -65,7 +71,12 @@
let hasConflict = false
srcFiles.forEach((srcFile) => {
- if (featureFlags.zisi_disallow_new_entry_name && srcFile.includes(ENTRY_FILE_NAME)) {
+ const relativeSrcFile = relative(basePath, srcFile)
+
+ if (
+ featureFlags.zisi_disallow_new_entry_name &&
+ (relativeSrcFile.startsWith(`${ENTRY_FILE_NAME}/`) || LAMBDA_ENTRY_FILES.has(relativeSrcFile))
+ ) {
throw new FunctionBundlingUserError(
`'${ENTRY_FILE_NAME}' is a reserved word and cannot be used as a file or directory name.`,
{
| let hasConflict = false | ||
|
|
||
| srcFiles.forEach((srcFile) => { | ||
| if (featureFlags.zisi_disallow_new_entry_name && srcFile.includes(ENTRY_FILE_NAME)) { |
* feat: disallow unique entry file name * Apply suggestions from code review Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com> * chore: review fixes * chore: fixes * chore: fix name --------- Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Summary
Ref https://github.com/netlify/pod-compute/issues/349
This disallows functions to be named
___netlify-lambda-entryand also disallows any additional files/directories in the bundled function to contain___netlify-lambda-entry.This is behind a FF. I also thought about silently ignoring function files named like this, but I think that would be more surprising.
The release plan is to roll this feature out and monitor how often this error happens and if it happens, disable FF and investigate and maybe pick another name. But I suspect it not being used at all. The expected case is that no error is thrown at all when activating the FF.
For us to review and ship your PR efficiently, please perform the following steps:
This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing
a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)