Skip to content

Add plan limits for max size per artifact type

Erick Bajao requested to merge eb-report-file-size-mechanism into master

What does this MR do?

Solves https://gitlab.com/gitlab-org/gitlab/-/issues/211378

This adds a mechanism to limit maximum file size per file type of artifact.

Some dev notes for the reviewer

Initially, I had to modify PlanLimits#exceeded? to support the fallback_limit parameter. This allows us to supply a numerical value or a proc that returns the value as fallback basis for comparison if plan limit is disabled for a given limit name.

But as I went deeper into the implementation, I hit a roadblock wherein the workhorse_authorize requires the max size if length is not present. So I had to support fetching the max_artifact_size from the plan limits. I ended up adding PlanLimits#limit_for for this. But I left the fallback_limit support in the #exceeded? even though I was not able to use it. It’s still useful and is also to be consistent with the #limit_for API.

This resulted into two helper methods in Ci::JobArtifact.max_artifact_size and Ci::JobArtifact.too_large? but internally too_large? is relying on max_artifact_size instead of calling PlanLimits#exceeded? to keep it simple.

I also took out the LSIF artifact type max size defined in a constant inside the Ci::AuthorizeJobArtifactService. Currently, the file size validation does not happen anyway in /authorize. because the file size parameter is not being passed at this point yet. So technically, the LSIF artifact size still defaults to the project wide setting.

I also considered updating the Limitable concern to use the fallback_limit and then update the Ci::JobArtifact to validate on save, but I chose to do the least amount of change needed to deliver this feature for now. I was also not sure if there would be breaking side-effects if ever I start validating this on the Ci::JobArtifact, like if we currently update existing artifacts and end up failing to because they were too large. If we really have to do this in the model validation though, I’m open to do it on a follow-up issue.

Also, there was a suggestion to do https://gitlab.com/gitlab-org/gitlab/-/issues/211378#note_306094594 but when I was implementing this from top-to-bottom, the tests drove me to a much simpler design. I can see the value of implementing a dedicated FileType class though. But I didn’t see it crucial to implementing this feature.

Screenshots

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Kamil Trzciński

Merge request reports