MGMT-14730: Validate that manifest file size does not exceed 1MB #5281

paul-maidment · 2023-06-07T10:23:58Z

etcd resources in k8s are limited to 1.5 MiB as indicated here https://etcd.io/docs/v3.5/dev-guide/limit/#request-size-limit
however, one the the resource types that can be created from a manifest is a ConfigMap
which has a size limit of 1MiB as cited here https://kubernetes.io/docs/concepts/configuration/configmap
so this limit has been chosen based on the lowest permitted resource size (the size of the ConfigMap)

Presently we do not validate the maximum size during upload or edit.
This pull request addresses that by validating that the file does not exceed this limit.

List all the issues related to this PR

What environments does this code impact?

Automation (CI, tools, etc)
Cloud
Operator Managed Deployments
[] None

How was this code tested?

assisted-test-infra environment
dev-scripts environment
Reviewer's test appreciated
Waiting for CI to do a full test run
Manual (Elaborate on how it was tested)
[] No tests needed

Checklist

Title and description added to both, commit and PR.
Relevant issues have been associated (see CONTRIBUTING guide)
This change does not require a documentation update (docstring, docs, README, etc)
Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?
Is there a bug required (and linked) for this change?
Should this PR be backported?

openshift-ci-robot · 2023-06-07T10:24:03Z

openshift-ci · 2023-06-07T10:25:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: paul-maidment

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [paul-maidment]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

omertuc · 2023-06-07T10:32:13Z

internal/manifests/manifests.go

@@ -353,6 +353,10 @@ func (m *Manifests) validateAllowedToModifyManifests(ctx context.Context, cluste
 }

 func (m *Manifests) validateUserSuppliedManifest(ctx context.Context, clusterID strfmt.UUID, manifestContent []byte, fileName string) error {
+	maxFileSizeBytes := 1048576


Suggested change

maxFileSizeBytes := 1048576

maxFileSizeBytes := 1024 * 1024 * 1024

I think this should be

maxFileSizeBytes := 1024 * 1024

omertuc · 2023-06-07T10:33:06Z

internal/manifests/manifests.go

@@ -353,6 +353,10 @@ func (m *Manifests) validateAllowedToModifyManifests(ctx context.Context, cluste
 }

 func (m *Manifests) validateUserSuppliedManifest(ctx context.Context, clusterID strfmt.UUID, manifestContent []byte, fileName string) error {
+	maxFileSizeBytes := 1048576
+	if len(manifestContent) > maxFileSizeBytes {
+		return m.prepareAndLogError(ctx, http.StatusBadRequest, errors.Errorf("Manifest content of file %s for cluster ID %s exceeds the maximum file size of 1MB", fileName, string(clusterID)))


Suggested change

return m.prepareAndLogError(ctx, http.StatusBadRequest, errors.Errorf("Manifest content of file %s for cluster ID %s exceeds the maximum file size of 1MB", fileName, string(clusterID)))

return m.prepareAndLogError(ctx, http.StatusBadRequest, errors.Errorf("Manifest content of file %s for cluster ID %s exceeds the maximum file size of 1MiB", fileName, string(clusterID)))

omertuc · 2023-06-07T10:34:17Z

Can you please link to the installer code that enforces this limit? Is this 1MiB or 1MB?

openshift-ci-robot · 2023-06-07T11:04:59Z

paul-maidment · 2023-06-07T11:05:24Z

Can you please link to the installer code that enforces this limit? Is this 1MiB or 1MB?

Explanation added to comments about ConfigMap size limit in Kubernetes.
And this is in MiB so updated to reflect this throughout the PR

openshift-ci-robot · 2023-06-07T11:18:19Z

codecov · 2023-06-07T11:39:50Z

Codecov Report

Merging #5281 (0e754ef) into master (4a22712) will increase coverage by 1.25%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5281      +/-   ##
==========================================
+ Coverage   67.47%   68.72%   +1.25%     
==========================================
  Files         221      221              
  Lines       33045    34296    +1251     
==========================================
+ Hits        22298    23571    +1273     
+ Misses       8731     8677      -54     
- Partials     2016     2048      +32

Impacted Files	Coverage Δ
internal/manifests/manifests.go	`72.42% <100.00%> (+0.34%)`	⬆️

... and 5 files with indirect coverage changes

omertuc · 2023-06-07T11:41:15Z

internal/manifests/manifests.go

@@ -353,6 +353,12 @@ func (m *Manifests) validateAllowedToModifyManifests(ctx context.Context, cluste
 }

 func (m *Manifests) validateUserSuppliedManifest(ctx context.Context, clusterID strfmt.UUID, manifestContent []byte, fileName string) error {
+	// Ultimately manifests are consumed into a ConfigMap. The max permitted size of a ConfigMap is 1MiB


Ultimately manifests are consumed into a ConfigMap

Not sure I understand, can you explain this? consumed by who?

config map is not exactly a correct term. MachineConfig for example is a manifest as well but it's not a configmap, i would describe it as a etcd resource in k8s is limited to 1MB

Sorry, I meant to say "stored in a configmap", have updated the code

OK, changed this now to say
// etcd resources in k8s are limited to 1MiB

What is your source that etcd resources are limited to 1MiB? All I see is docs talking about configmaps being limited

My source is this internal conversation https://redhat-internal.slack.com/archives/CUPJTHQ5P/p1686135268686879
Where it is acknowledged that the etcd max size is 1.5MiB
https://etcd.io/docs/v3.5/dev-guide/limit/#request-size-limit
However, also mentions that the effective limit is 1MiB for ConfigMaps
https://kubernetes.io/docs/concepts/configuration/configmap

Is there a massive concern to setting the manifest limit to 1MiB ? Do you want to make the distinction between different types of content and change the limit accordingly?

Should we try to find out more about what the limit should be, who should we ask?

If we set the limit to 1MiB then it would be possible to upload a manifest content of this size and that might lead to an attempt to create a config map of this size.

That's why I think we should lower it to 1MiB as this is the lowest value in common between ConfigMaps and etcd resources.

If we want to analyse manifest content to determine what size should be uploaded, I think this would get complicated quickly.

So I think it's best to set to 1 MiB

Is there a massive concern to setting the manifest limit to 1MiB

No, I just want to understand why we are doing what we're doing

Do you want to make the distinction between different types of content and change the limit accordingly?

Not necessarily, but if we don't do that, we need to at-least acknowledge in a comment explicitly that our blanket limit of 1MiB is too conservative but is done to easily have the correct limit also for configmaps without worrying about the exact type of resource

I have updated the comments and commit message to reflect this concern

openshift-ci-robot · 2023-06-07T12:08:56Z

openshift-ci-robot · 2023-06-08T09:41:55Z

omertuc · 2023-06-08T09:43:33Z

/lgtm

openshift-ci-robot · 2023-06-08T10:33:45Z

/retest-required

Remaining retests: 0 against base HEAD bba0db7 and 2 for PR HEAD 49136dc in total

omertuc · 2023-06-08T12:35:32Z

/test edge-unit-test

openshift-ci-robot · 2023-06-08T13:49:51Z

/retest-required

Remaining retests: 0 against base HEAD bba0db7 and 2 for PR HEAD 49136dc in total

omertuc · 2023-06-08T14:23:43Z

/hold test failures

etcd resources in k8s are limited to 1.5 MiB as indicated here https://etcd.io/docs/v3.5/dev-guide/limit/#request-size-limit however, one the the resource types that can be created from a manifest is a ConfigMap which has a size limit of 1MiB as cited here https://kubernetes.io/docs/concepts/configuration/configmap so this limit has been chosen based on the lowest permitted resource size (the size of the ConfigMap) Presently we do not validate the maximum size during upload or edit. This pull request addresses that by validating that the file does not exceed this limit.

paul-maidment · 2023-06-11T08:42:52Z

This should fix the failures. Will unhold when this is the case and seek another review.

One change sets the filesize the correct limit of 1024 * 1024 bytes.
The second change makes thee generation of a dummy file more efficient by using a StringBuilder.

omertuc · 2023-06-12T08:58:27Z

/lgtm

paul-maidment · 2023-06-12T10:52:06Z

/unhold

droslean · 2023-06-12T12:11:35Z

/hold

droslean · 2023-06-12T12:11:40Z

/hold cancel

droslean · 2023-06-12T12:11:50Z

/refresh

paul-maidment · 2023-06-12T15:14:13Z

/hold

openshift-ci · 2023-06-12T15:20:07Z

@paul-maidment: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

paul-maidment · 2023-06-13T12:47:57Z

/unhold

…nshift#5281) etcd resources in k8s are limited to 1.5 MiB as indicated here https://etcd.io/docs/v3.5/dev-guide/limit/#request-size-limit however, one the the resource types that can be created from a manifest is a ConfigMap which has a size limit of 1MiB as cited here https://kubernetes.io/docs/concepts/configuration/configmap so this limit has been chosen based on the lowest permitted resource size (the size of the ConfigMap) Presently we do not validate the maximum size during upload or edit. This pull request addresses that by validating that the file does not exceed this limit.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 7, 2023

openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 7, 2023

openshift-ci bot requested review from CrystalChun and vrutkovs June 7, 2023 10:25

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 7, 2023

omertuc reviewed Jun 7, 2023

View reviewed changes

paul-maidment force-pushed the MGMT-14730 branch 2 times, most recently from 813ef8e to 580d656 Compare June 7, 2023 11:04

omertuc reviewed Jun 7, 2023

View reviewed changes

paul-maidment force-pushed the MGMT-14730 branch 2 times, most recently from 53fe42a to 45519be Compare June 7, 2023 12:07

paul-maidment force-pushed the MGMT-14730 branch 3 times, most recently from 0a3c68a to 49136dc Compare June 8, 2023 09:41

openshift-ci bot assigned omertuc Jun 8, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 8, 2023

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2023

paul-maidment force-pushed the MGMT-14730 branch from 49136dc to 0e754ef Compare June 11, 2023 08:39

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 11, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 12, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 12, 2023

filanov added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jun 12, 2023

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 12, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 12, 2023

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 12, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 13, 2023

openshift-merge-robot merged commit cfb9b37 into openshift:master Jun 13, 2023
14 checks passed

	maxFileSizeBytes := 1048576
	maxFileSizeBytes := 1024 * 1024 * 1024

	return m.prepareAndLogError(ctx, http.StatusBadRequest, errors.Errorf("Manifest content of file %s for cluster ID %s exceeds the maximum file size of 1MB", fileName, string(clusterID)))
	return m.prepareAndLogError(ctx, http.StatusBadRequest, errors.Errorf("Manifest content of file %s for cluster ID %s exceeds the maximum file size of 1MiB", fileName, string(clusterID)))

MGMT-14730: Validate that manifest file size does not exceed 1MB #5281

MGMT-14730: Validate that manifest file size does not exceed 1MB #5281

Conversation

paul-maidment commented Jun 7, 2023 • edited

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Jun 7, 2023 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci bot commented Jun 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omertuc commented Jun 7, 2023

openshift-ci-robot commented Jun 7, 2023 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

paul-maidment commented Jun 7, 2023 • edited

openshift-ci-robot commented Jun 7, 2023 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

codecov bot commented Jun 7, 2023 • edited

Codecov Report

omertuc Jun 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omertuc Jun 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jun 7, 2023 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Jun 8, 2023 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

omertuc commented Jun 8, 2023

openshift-ci-robot commented Jun 8, 2023

omertuc commented Jun 8, 2023

openshift-ci-robot commented Jun 8, 2023

omertuc commented Jun 8, 2023

paul-maidment commented Jun 11, 2023

omertuc commented Jun 12, 2023

paul-maidment commented Jun 12, 2023

droslean commented Jun 12, 2023

droslean commented Jun 12, 2023

droslean commented Jun 12, 2023

paul-maidment commented Jun 12, 2023

openshift-ci bot commented Jun 12, 2023

paul-maidment commented Jun 13, 2023

paul-maidment commented Jun 7, 2023 •

edited

openshift-ci-robot commented Jun 7, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Jun 7, 2023 •

edited by openshift-ci bot

paul-maidment commented Jun 7, 2023 •

edited

openshift-ci-robot commented Jun 7, 2023 •

edited by openshift-ci bot

codecov bot commented Jun 7, 2023 •

edited

omertuc Jun 7, 2023 •

edited

omertuc Jun 8, 2023 •

edited

openshift-ci-robot commented Jun 7, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Jun 8, 2023 •

edited by openshift-ci bot