[upload] Add proxy server details for sos uploads by dwolstroRH · Pull Request #4158 · sosreport/sos

dwolstroRH · 2025-11-07T20:14:04Z

Related: RHEL-2356

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?
Are all passwords or private data gathered by this PR obfuscated?

jcastill · 2025-11-10T10:07:58Z

        'upload_target': None,
        'add_preset': '',
        'del_preset': '',
+<<<<<<< Updated upstream


I believe this is one of the issues causing the pylint error. You just need to resolve the conflict with the stashed changes and force push again

Thanks, fixed.

jcastill · 2025-11-10T10:14:09Z

+    """
+    Determines the correct authenticated HTTP proxy syntax for the local
+    Netcat/Ncat executable by running test commands.
+


Could you add here (or perhaps as a comment in the gh PR interface) why do we need to determine the syntax?
And what happens if we don't have this function at all?

I will add in a more verbose comment to try to explain why we use this function and why (I think) it's needed. Basically, if we don't have this function, only certain versions of ncat/nc will work correctly with the proxy command. This function tries to figure out the correct syntax of the proxy command needed based on the installed version of ncat/nc

jcastill · 2025-11-10T10:18:20Z

Make sure you add these options to any relevant man page (at least the upload and report ones).

jcastill · 2025-11-10T10:37:22Z

-                raise Exception("connection failed, did you set a user and "
-                                "password?")
+                raise Exception("connection failed, did you set a user"
+                                " and password?")


Any reason for this specific change? Unless I'm mistaken, the original line was within col limit

I honestly have no idea why I updated that. I can change it back if you want me to.

No need, I just wasn't sure if I missed something. Doesn't look bad to me, so unless anyone else objects, this change can be kept

packit-as-a-service · 2025-11-10T19:59:45Z

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-4158
And now you can install the packages.

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

pmoravec · 2025-11-14T11:13:34Z

+    # --- 3. Fallback (If no standard syntax is found) ---
+    # If the system netcat doesn't support an authenticated proxy,
+    # you might need to use a dedicated tool like 'corkscrew' or 'connect'.
+    return ("Warning: Native authenticated proxy syntax not found for 'nc'. "


Minor nit: replace nc by {nc_executable} (and have it as an f-string)?

Currently, you call the method with default nc_executable='nc' only. If you plan this argumet to have different values in future, let be prepared here.

pmoravec · 2025-11-14T11:40:36Z

+            if self.get_proxy_host():
+                self.generate_proxy_list(use_prompts=True)
            self.ui_log.info('')
+        else:
+            if self.get_proxy_host():
+                self.generate_proxy_list(use_prompts=False)


Code de-duplication: isnt it better to call (after the if not cmdline_opts.batch .. block):

if self.get_proxy_host(): self.generate_proxy_list(use_prompts=(not cmdline_opts.batch and not cmdline_opts.quiet))

?

Well, then the log_info of empty line would be entered before the prompt, not welcomed, I guess?

pmoravec · 2025-11-14T11:56:37Z

+                    self.ui_log.error(
+                        "Both proxy user and proxy password must be"
+                        " specified. Proxy server will be ignored."


What if I enter empty user and password? That should be allowed, no..?

pmoravec · 2025-11-14T12:01:51Z

+                        return {}
+
+            if self.get_proxy_host() and self.get_proxy_port():
+                proxy = (f"{proxy}{self.get_proxy_host()}:"


So proxy stores just credentials so far, which is bit confusing. Worth adding a comment or having proxy_creds variable (up to this point)?

(also a nitpick, you can ignore this comment if you think the code is self-explanatory)

pmoravec · 2025-11-14T12:03:31Z

+                proxy = (f"{proxy}{self.get_proxy_host()}:"
+                         f"{self.get_proxy_port()}")
+                proxy_list['http'] = f"http://{proxy}"
+                proxy_list['https'] = f"http://{proxy}"


Shouldnt be there https:// instead of http://? (but I am not an expert on the syntax here..)

Great catch! thanks

arif-ali

primarily around if we don't have a user/password, but just a proxy, we should be able to use the proxy on its own

arif-ali · 2025-11-21T10:55:52Z

+                session.connect(self.get_proxy_host(), self.get_proxy_port())
+
+                session.login(proxy_formatted_user, self.proxy_password,
+                              password)


same comment applies, what if it is anonymous FTP, but still need a proxy without username/password?

dwolstroRH · 2025-11-25T21:11:07Z

Make sure you add these options to any relevant man page (at least the upload and report ones).

Can you send me the link to where it is explained how to do this? I've never updated a man page before. (is it on the source)?

jcastill · 2025-11-26T08:58:47Z

I don't think there's a link that explains how to do it, neither here nor on the source, but I may have missed it.
What you need to do is add the options in two places:

In the SYNOPSIS section, i.e. below the .SH SYNOPSIS header, where you add the options only. Perhaps a good place to add them would be between these two:

          [--upload-protocol protocol]\fR
          [--experimental]\fR

And in the OPTIONS section, i.e. below .SH OPTIONS, where you add the options again, in the same order, but with a small explanation. This explanation can be the same as the one you have in the option list proxy_grp.

You should add these options to at least the sos-upload.1 man page, but I imagine having them in sos-report.1 could be necessary as well.

packit-as-a-service · 2026-01-05T17:50:04Z

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

sudo dnf install -y 'dnf*-command(copr)'
dnf copr enable packit/sosreport-sos-4158
And now you can install the packages.

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

packit-as-a-service · 2026-01-05T17:50:05Z

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

sudo dnf install -y 'dnf*-command(copr)'
dnf copr enable packit/sosreport-sos-4158
And now you can install the packages.

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

pmoravec · 2026-01-13T11:12:08Z

+        if not self.get_proxy_user():
+            msg = (f"Please provide a proxy user (if required) for proxy "
+                   f"{self.get_proxy_host()}: ")
+            self.proxy_user = input(_(msg))


If I want to provide "no user" (see Arif's point for no user/pass), we shall convert empty response to None, maybe? (I havent tested it though).

pmoravec · 2026-01-13T11:14:38Z

+                    if self.get_proxy_user() or \
+                       self.get_proxy_password():


Is it a valid use case a proxy allows user with no password? I dont know this.

If it is a valid use case, we prohibit it here.

I am not sure. Everything that I've read has discouraged that setup profusely, but technically it can be done. Maybe I can put a warning message and prompt the user to confirm they want to execute without a password?

ie, "Warning! You have entered a proxy user without an accompanying password. It is highly recommended to avoid this usage. Do you wish to proceed? (y/N)"

pmoravec · 2026-01-13T11:22:06Z

+                    if self.get_proxy_user() or \
+                       self.get_proxy_password():


Ditto the same use case (if it is valid).

pmoravec · 2026-01-13T11:27:47Z

In general, ack and thanks for such nontrivial feature. Just few minor points remain to resolve (or decide).

Related: RHEL-2356 Signed-off-by: David Wolstromer <dwolstro@redhat.com>

pmoravec

Sounds good to me, yet I tested it in a limited manner only.

jcastill · 2026-04-23T11:45:59Z

@mhradile @tiredPotato @dzalezak this PR is ready for RH QE - can we assign one of you to it?

mhradile · 2026-04-23T12:11:27Z

@mhradile @tiredPotato @dzalezak this PR is ready for RH QE - can we assign one of you to it?

Hello @jcastill , I stared but could not continue on this, would like to get back to it but currently can not. Feel free to assign it to me though.

jcastill suggested changes Nov 10, 2025

View reviewed changes

jcastill reviewed Nov 10, 2025

View reviewed changes

Comment thread sos/upload/targets/__init__.py Outdated

dwolstroRH force-pushed the add-proxy-server branch from d56d503 to c2b17c9 Compare November 10, 2025 19:57

dwolstroRH force-pushed the add-proxy-server branch from c2b17c9 to 98fd105 Compare November 10, 2025 21:38

pmoravec reviewed Nov 14, 2025

View reviewed changes

Comment thread sos/report/__init__.py

pmoravec reviewed Nov 14, 2025

View reviewed changes

arif-ali reviewed Nov 21, 2025

View reviewed changes

dwolstroRH force-pushed the add-proxy-server branch from 98fd105 to f8d63f9 Compare January 5, 2026 17:47

dwolstroRH force-pushed the add-proxy-server branch from f8d63f9 to 0194d3a Compare January 7, 2026 13:54

pmoravec reviewed Jan 13, 2026

View reviewed changes

Comment thread sos/upload/targets/__init__.py Outdated

pmoravec reviewed Jan 13, 2026

View reviewed changes

Comment thread sos/upload/targets/__init__.py Outdated

pmoravec reviewed Jan 13, 2026

View reviewed changes

dwolstroRH force-pushed the add-proxy-server branch from 0194d3a to 4841ece Compare February 5, 2026 19:55

[upload] Add proxy server details for sos uploads

0412b88

Related: RHEL-2356 Signed-off-by: David Wolstromer <dwolstro@redhat.com>

dwolstroRH force-pushed the add-proxy-server branch from 4841ece to 0412b88 Compare February 5, 2026 20:36

pmoravec approved these changes Feb 14, 2026

View reviewed changes

pmoravec added feature request Kind/Enhancement Status/RedHat QE RH QE has been requested to review Reviewed/Needs 2nd Ack Require a 2nd ack from a maintainer Kind/Upload upload component of sos labels Feb 14, 2026

dwolstroRH marked this pull request as draft February 16, 2026 16:32

Conversation

dwolstroRH commented Nov 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcastill commented Nov 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

packit-as-a-service Bot commented Nov 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pmoravec Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arif-ali left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwolstroRH commented Nov 25, 2025

Uh oh!

jcastill commented Nov 26, 2025

Uh oh!

packit-as-a-service Bot commented Jan 5, 2026

Uh oh!

packit-as-a-service Bot commented Jan 5, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmoravec commented Jan 13, 2026

Uh oh!

pmoravec left a comment

Choose a reason for hiding this comment

Uh oh!

jcastill commented Apr 23, 2026

Uh oh!

mhradile commented Apr 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pmoravec Nov 14, 2025 •

edited

Loading