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

Limit feature file size #1330

Closed
AhmedGrati opened this issue Sep 5, 2023 · 4 comments · Fixed by #1335
Closed

Limit feature file size #1330

AhmedGrati opened this issue Sep 5, 2023 · 4 comments · Fixed by #1335
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@AhmedGrati
Copy link

AhmedGrati commented Sep 5, 2023

What would you like to be added:
Currently, we are processing feature files without any restriction on the file size, since we are expecting that the file is coming from a trusted source. However, processing large files can present some bottlenecks. Therefore, we should limit the size of features' files.
For more information: #1285 (comment)

Why is this needed:
Improve confidence in the system, by covering edge cases concerning feature file size.

@AhmedGrati AhmedGrati added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 5, 2023
@AhmedGrati
Copy link
Author

FYI @marquiz.
BTW I have two questions concerning this:
1- Should we make the file size limit configurable, with a default value of 64kB, so users can adjust the limit based on their use cases?
2- If the size of a file is bigger than the limit, should we process it in chunks, or raise an error to the user?

@marquiz
Copy link
Contributor

marquiz commented Sep 5, 2023

Thanks @AhmedGrati for creating this issue

1- Should we make the file size limit configurable, with a default value of 64kB, so users can adjust the limit based on their use cases?

I don't think we need to make it configurable. I believe 64kB (or 32kB) should be plenty, enough. Let's revisit this if somebody really comes up with a use case for bigger feature files.

2- If the size of a file is bigger than the limit, should we process it in chunks, or raise an error to the user?

I would consider it as a "transaction", all or nothing, not advertise partial results. I.e. skip the file if it's too big

ping @eero-t

@eero-t
Copy link

eero-t commented Sep 5, 2023

It's possible that some feature files could in addition to labels, include extensive documentation for those labels as comments, which I think to be a valid use-case.

Main things that should IMHO be caught is accidentally giving NFD the old binary hook as feature file, or things intentionally trying to break NFD.

=> I think the limit can be fixed size, fairly big, and documented [1]. 64kB sounds fine, but it could be even larger.

I would consider it as a "transaction", all or nothing, not advertise partial results. I.e. skip the file if it's too big

Agree, log an error [2] instead of parsing file, and move to next file.

[1] E.g: "NFD logs an error and skips feature files larger than XX KiB. Larger files are assumed to be configuration errors."
[2] "Skipping feature file '%s' with invalid size (%d KiB > max %d KiB)"

@AhmedGrati
Copy link
Author

/assign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants