[redhat|policy] Check for archive size before upload #3534

jcastill · 2024-02-21T12:36:15Z

The Red Hat Customer Portal has a max limit for
single http requests of 1Gb. From sos side, we never checked this and so customers had to wait for the whole upload to be attempted before receiving an error from the portal.
This patch attempts to stop the upload before it starts, informing customers of such limit, and switching to secure FTP where there's no limit.

Related: RHEL-22732

Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

Is the commit message split over multiple lines and hard-wrapped at 72 characters?
Is the subject and message clear and concise?
Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
Does the commit contain a Signed-off-by: First Lastname email@example.com?
Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?

packit-as-a-service · 2024-02-21T12:41:14Z

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

sudo yum install -y dnf-plugins-core on RHEL 8
sudo dnf install -y dnf-plugins-core on Fedora
dnf copr enable packit/sosreport-sos-3534
And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

jcastill · 2024-02-21T12:41:56Z

I tried to add the check as early as possible, before uploading, but making sure it was a RH only check, so I chose to use the upload_archive() in redhat.py. Another option was to add it to the 'if' just below where it is, returning a boolean, but I thought that could add more complexity to that if.

pmoravec · 2024-02-24T20:33:15Z

sos/policies/distros/redhat.py

@@ -232,6 +232,9 @@ class RHELPolicy(RedHatPolicy):
    _upload_url = RH_SFTP_HOST
    _upload_method = 'post'
    _device_token = None
+    # Max size for an http single request is 1Gb
+    _max_size_request = 1073741824


This is the byte-precise granted size Red Hat Portal allows, right? (rather, one byte less due to size >= self._max_size_request)

I just want to prevent sporadic cases when a border-line-sized sosreport passes the check but fails to be uploaded - rather go the other way, failover to SFTP even for sizes slightly below the limit.

Yes, that's right. I checked with a bit more than 1Gb but can check the exact size as well in case we need to change the value.

sos/policies/distros/redhat.py

TurboTurtle · 2024-02-24T21:37:13Z

sos/policies/distros/redhat.py

+        # There's really no need to transform the size to Gb,
+        # so we don't need to call any size converter implemented
+        # in tools.py
+        if len(str(size)) >= 10 and (size >= self._max_size_request):


What?

The raw size comparison is the only thing needed.

TurboTurtle · 2024-02-24T21:38:10Z

sos/policies/distros/redhat.py

+                _("Size of archive is bigger than Red Hat Customer Portal "
+                  f"limit for uploads of {self._max_size_human_readable} via "
+                  "sos http upload. \n")
+                  )


It seems strange to stick 1Gb in a variable for it only to be used once in an f-string, rather than just making the warning message contain that static value.

Sure. I'll change that.

sos/policies/distros/redhat.py

TurboTurtle · 2024-02-24T21:39:50Z

sos/policies/distros/redhat.py

    def upload_archive(self, archive):
        """Override the base upload_archive to provide for automatic failover
        from RHCP failures to the public RH dropbox
        """
        try:
+            self.check_file_too_big(archive)


This should be gated by checking that we are in fact trying to upload to the RH customer portal - the default is for the policy to set the target, but it's entirely possible for it to not be RH_API_HOST.

Ack, but it will be good to know about the scenarios where the target is not RH_API_HOST when going through sos/policies/distros/redhat.py

jcastill · 2024-04-24T10:48:50Z

I think I've addressed all the notes, but apologies if I forgot anything! This is how the output looks right now:

Creating compressed archive...

Your sosreport has been generated and saved in:
	/var/tmp/sosreport-localhost-03093474-2024-04-24-wbdtdaf.tar.xz

 Size	1.41GiB
 Owner	root
 sha256	0d7bebe2a81a92f3b8849fa807a233e0eadaf8c99717a25e1f92480d6a492ed0

Please send this file to your support representative.

Size of archive is bigger than Red Hat Customer Portal limit for uploads of 1.0G  via sos http upload. 

Attempting upload to Red Hat Secure FTP

We present GiB in "Size" and I print G in the warning. It may be worth changing the value I print so it's consistent.

TurboTurtle

This looks mostly good to me now, just would like to have the upload URL check moved to be the gating logic on the size check as noted (so that we don't perform the size check if a user has specified a different --upload-url.

TurboTurtle · 2024-05-01T23:40:21Z

sos/policies/distros/redhat.py

+        # so we don't need to call any size converter implemented
+        # in tools.py
+        if (size >= self._max_size_request) and\
+                self.get_upload_url().startswith(RH_API_HOST):


The check against the upload URL should be done prior to the call to check_file_too_big(), otherwise we are doing the size comparison needlessly.

The Red Hat Customer Portal has a max limit for single http requests of 1Gb. From sos side, we never checked this and so customers had to wait for the whole upload to be attempted before receiving an error from the portal. This patch attempts to stop the upload before it starts, informing customers of such limit, and switching to secure FTP where there's no limit. Related: RHEL-22732 Signed-off-by: Jose Castillo <jcastillo@redhat.com>

jcastill · 2024-05-15T07:48:34Z

I changed this a bit from the previous iteration. I considered two options. First one - have the check for the RH URL and the file size in the same if:

		try:
			if self.get_upload_url().startswith(RH_API_HOST) and\
			   self.check_file_too_big(archive):
				self.upload_url = RH_SFTP_HOST

Second option - modify the function to return upload URL, and assign it to self.upload_url after gatekeeping, like this:

		try:
			if self.get_upload_url().startswith(RH_API_HOST):
				self.upload_url = self.check_file_too_big(archive)

I've chosen the second one to do the size check after the URL check, and not at the same time. And in both cases I considered, I removed the check for user and password because after the change to token auth these we always None.
Let me know if I missed anything or if you think there's a better way to do this.

TurboTurtle

LGTM

pmoravec · 2024-05-16T08:01:53Z

sos/policies/distros/redhat.py

+                _("Size of archive is bigger than Red Hat Customer Portal "
+                  "limit for uploads of "
+                  f"{convert_bytes(self._max_size_request)} "
+                  " via sos http upload. \n")
+                  )


Is it worth mentioning here "File will be uploaded to SFTP server instead"?

pmoravec

ACK with current change, optionally worth to extend the warning message.

jcastill · 2024-05-16T08:32:14Z

ACK with current change, optionally worth to extend the warning message.

It happens "naturally" in the code flow. This is the result:

Please send this file to your support representative.

Size of archive is bigger than Red Hat Customer Portal limit for uploads of 1.0G  via sos http upload. 

Attempting upload to Red Hat Secure FTP

That's why I chose not to mention it again.
Another thing - I wondered about using 'bold' in the message but I'm not sure if that would be too much - do we have a preferred way to present certain important messages to the user?

pmoravec · 2024-05-16T09:27:31Z

Ah right, I forgot the context / the subsequent message. Then the current log is sufficient, I would say.

I think no bold text is needed here, it is just a warning message, not even an error :)

jcastill requested a review from pmoravec February 21, 2024 12:36

pmoravec reviewed Feb 24, 2024

View reviewed changes

sos/policies/distros/redhat.py Outdated Show resolved Hide resolved

pmoravec reviewed Feb 24, 2024

View reviewed changes

sos/policies/distros/redhat.py Outdated Show resolved Hide resolved

TurboTurtle requested changes Feb 24, 2024

View reviewed changes

jcastill force-pushed the jcastillo-check-upload-file-size branch from a912e55 to 75e7dba Compare April 24, 2024 08:33

TurboTurtle reviewed May 1, 2024

View reviewed changes

jcastill force-pushed the jcastillo-check-upload-file-size branch from 75e7dba to d324f10 Compare May 15, 2024 07:45

TurboTurtle approved these changes May 15, 2024

View reviewed changes

pmoravec reviewed May 16, 2024

View reviewed changes

pmoravec approved these changes May 16, 2024

View reviewed changes

TurboTurtle merged commit 11879fb into sosreport:main May 16, 2024
39 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[redhat|policy] Check for archive size before upload #3534

[redhat|policy] Check for archive size before upload #3534

jcastill commented Feb 21, 2024

packit-as-a-service bot commented Feb 21, 2024

jcastill commented Feb 21, 2024

pmoravec Feb 24, 2024

jcastill Feb 25, 2024

TurboTurtle Feb 24, 2024

TurboTurtle Feb 24, 2024

jcastill Feb 25, 2024

TurboTurtle Feb 24, 2024

jcastill Feb 25, 2024

jcastill commented Apr 24, 2024

TurboTurtle left a comment

TurboTurtle May 1, 2024

jcastill commented May 15, 2024

TurboTurtle left a comment

pmoravec May 16, 2024

pmoravec left a comment

jcastill commented May 16, 2024

pmoravec commented May 16, 2024

[redhat|policy] Check for archive size before upload #3534

[redhat|policy] Check for archive size before upload #3534

Conversation

jcastill commented Feb 21, 2024

packit-as-a-service bot commented Feb 21, 2024

jcastill commented Feb 21, 2024

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

jcastill commented Apr 24, 2024

TurboTurtle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcastill commented May 15, 2024

TurboTurtle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmoravec left a comment

Choose a reason for hiding this comment

jcastill commented May 16, 2024

pmoravec commented May 16, 2024