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

[upload] Add new component #3894

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

TurboTurtle
Copy link
Member

This commit marks the beginning of a series to implement the upload
functionality of collect and report into a separate component. This new
upload component is intended to be primarily used to define standard
"targets" for uploading support-relevant files to support-providing
vendors, e.g. Red Hat and Canonical.

Both `report` and `collect` will be made to leverage this component in
later commits to this series.

As a practical example, the goal for the end-user experience is a
command like the following for providing data to a support vendor:

  + Uploading in-line with report (or collect archive) creation
    `sos report --case-id 123456 --upload --upload-target=redhat`
  + Uploading a report after generation
    `sos upload sosreport-example.tar.xz --target=canonical`
  + Uploading non-sos data that is relevant to an ongoing support case
    `sos upload $some_helpful_file --target=my_awesome_vendor`

Additionally, there will still be support for "generic" targets such as
FTP or SFTP destinations that users may wish to define themselves, and
still allow for automatic transfer of generate archives to.

This first commit provides the framework for the component directly, as
well as the "upload target" abstraction which serves as an entrypoint
for standardized destinations for uploads.

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 protected]?
  • 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?

sos/upload/__init__.py Fixed Show fixed Hide fixed
sos/upload/__init__.py Fixed Show fixed Hide fixed
Copy link

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

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

@TurboTurtle
Copy link
Member Author

I finally had a few hours to sit down and work on this abstraction. Opening this draft to show what I'm envisioning for this component.

We'll leverage new SoSUploadTarget abstractions which will be self-contained ways in which to upload an archive or other file to a predetermined location such as a vendor support endpoint. In this way we remove uploading from policy, and allow end users to dictate where to send their sos reports or what have you, regardless of if it's coming from a problematic host or the user's workstation.

E.G. I could on a Fedora workstation run sos upload $my_file --target=canonical to provide a support file to the Canonical support team for a Ubuntu Server that I may, for any reason at all, not want to (or be completely unable to) upload from directly.

Most notably this allows us to onboard new vendors/partners that provide support and use sos, but don't warrant a new policy (e.g. I think the nvidia folks might appreciate this, since they support multiple distros and wrap sos via their nvsm health dump utility).

The abstraction is fairly straight forward, though I want to highlight two design choices I made in this that we may or may not want to adjust:

  • First, there is a hook for target subclasses to define prompt_for_* methods which will prompt the user for input at the beginning of an execution (goal being not just for when using sos upload as a discrete component, but also with sos report --upload). This would allow us to bail out of an upload attempt that is doomed to fail before we generate an archive. I created this when thinking of username and password prompting across different targets, however the implementation I ended up with made more sense to always do that work, and conditionalize just the prompt, rather than conditionalize the entire hook mechanism.
  • Second, and related, I decided to have the credential prompt overwrite the upload_user and upload_pass option values, rather than storing them in separate variables - namely so that we have consistency between targets and we aren't re-implementing these prompt a whole bunch.

I think the hook mechanism has merit as an idea, but beyond username and password credentials I'm not immediately thinking of anything to otherwise prompt for, but perhaps that can come later?

At the moment, I've only ported the ftp and sftp upload functions to targets. I'll be porting the other existing ones over the next few days (maybe a week) as time allows. But I also want to hear feedback on the general design if anyone has it.

@TurboTurtle TurboTurtle changed the title Upload component [upload] Add new component Jan 2, 2025
@jcastill
Copy link
Member

jcastill commented Jan 2, 2025

Is there any reason why we don't continue the work in the already opened PR #3746 ? You could have added your suggestions there.

_do_intro = True
if commons:
_do_intro = False
self.opts = commons['opts']

Check warning

Code scanning / CodeQL

Overwriting attribute in super-class or sub-class Warning

Assignment overwrites attribute opts, which was previously defined in superclass
SoSComponent
.
Assignment overwrites attribute opts, which was previously defined in superclass
SoSComponent
.
@TurboTurtle
Copy link
Member Author

TurboTurtle commented Jan 2, 2025

Is there any reason why we don't continue the work in the already opened PR #3746 ? You could have added your suggestions there.

I did leave my suggestions that the component needs to have a minimum set of features for implementation. Then when there hadn't been any code changes to the existing PR since August, and the last activity at all on that PR was from October (another review), it appeared to be a basically abandoned PR since there was no communication around ongoing work.

Having not seen any progress in that time, I began working out an sos appropriate implementation - which I could iterate over much quicker locally to get a framework in place and begin porting bits to new targets.

@TurboTurtle
Copy link
Member Author

Latest update adds an http target as well as a canonical target which serves as a port for the existing file server upload from the ubuntu policy.

As of right now, if you were to test this you would need to use sos upload --target canonical $file, as I have yet to connect this to report or collect. That's next, and should be a quick knock out tomorrow for me.

This commit marks the beginning of a series to implement the upload
functionality of collect and report into a separate component. This new
upload component is intended to be primarily used to define standard
"targets" for uploading support-relevant files to support-providing
vendors, e.g. Red Hat and Canonical.

Both `report` and `collect` will be made to leverage this component in
later commits to this series.

As a practical example, the goal for the end-user experience is a
command like the following for providing data to a support vendor:

  + Uploading in-line with report (or collect archive) creation
    `sos report --case-id 123456 --upload --upload-target=redhat`
  + Uploading a report after generation
    `sos upload sosreport-example.tar.xz --target=canonical`
  + Uploading non-sos data that is relevant to an ongoing support case
    `sos upload $some_helpful_file --target=my_awesome_vendor`

Additionally, there will still be support for "generic" targets such as
FTP or SFTP destinations that users may wish to define themselves, and
still allow for automatic transfer of generate archives to.

This first commit provides the framework for the component directly, as
well as the "upload target" abstraction which serves as an entrypoint
for standardized destinations for uploads.

Signed-off-by: Jake Hunsaker <[email protected]>
Ports the functionality of FTP and SFTP uploading into the new target
abstraction. These targets allow users to specify an upload destination
using a protocol prefix of eityher `ftp://` or `sftp://` when combined
with `--upload-url`.

Signed-off-by: Jake Hunsaker <[email protected]>
Adds two new upload targets - a generic http(s) target and a port of the
existing ubuntu policy upload to a `canonical` upload target.

Note that this also changes the `upload-method` option to
`--upload-http-method` and also removes `auto` as a valid choice,
leaving just `put` and `post`, as it is assumed that the `auto` logic
would now be redundant for independently implemented vendor targets that
subclass the generic http target.

Signed-off-by: Jake Hunsaker <[email protected]>
@jcastill
Copy link
Member

jcastill commented Jan 3, 2025

I did leave my suggestions that the component needs to have a minimum set of features for implementation. Then when there hadn't been any code changes to the existing PR since August, and the last activity at all on that PR was from October (another review), it appeared to be a basically abandoned PR since there was no communication around ongoing work.

And you didn't think of reaching out and, you know, ask? Instead you decided to do your thing alone. Again.
If you really have any interest in working together with other people in the SoS project, you'd close this partial and incomplete PR and move your comments and ideas to the already existing, and at the moment more complete, PR. If you decide to continue working on this ignoring #3746 then we all know what to expect from our contributions to SoS, and we'll have to think really hard if it's worth it to contribute at all.

@jcastill
Copy link
Member

jcastill commented Jan 3, 2025

@pmoravec @arif-ali can I have your support on this? Or do you support this kind of attitude and approach, undermining other contributor's work, time, and effort?

@pmoravec
Copy link
Contributor

pmoravec commented Jan 3, 2025

I dont understand this duplicit activity. Usually, we ask if a PR is abandoned or if we can expect some progress from its author, rather than writing own (draft) version of the same PR. Things were going slowly on the Jose's PR, that is correct, but.. I would expect a different approach in upstream collaboration than raising a competing draft proposal without prior notice.

I am off today so I cant assess this PR or compare it to Jose's one today. But I did review of Jose's current version of PR very recently and it seemed to me worth accepting.

@TurboTurtle
Copy link
Member Author

And you didn't think of reaching out and, you know, ask?

This project is not your keeper and if you want to contribute to it, then you are responsible for keeping up with your submissions. Months without movement is on no one but you and neither I nor this project am going to be held to your arbitrary schedules. You don't see us pinging other authors outside of a single effort to align labeling, what makes you special here? We have routinely posted replacement PRs when others are stale. What makes you special here?

But let's take this a step further. I have repeatedly said over the last ~2 months that I was trying to find time to work on an upload component. In public, on threads that you were apart of. "You didn't think of reaching out and, you know, ask" about that? Not a single "hey, Jake, I'm still working on this and should have something up in $days/$weeks/whatever". Pot, meet kettle.

If you really have any interest in working together with other people in the SoS project, you'd close this partial and incomplete PR and move your comments and ideas to the already existing, and at the moment more complete, PR.

Excellent strawman, really. This PR was opened as a draft to show and solicit feedback on the proposed framework of a lighter-weight, more extensible, and easier to maintain design than what is currently implemented today, and by extension of 3746 being a copy-and-move of existing code, that PR. Or, as most people would call it, this PR is stock FOSS development.

SoS is an open source project and contributions are welcome and desired from all interested parties, but that doesn't mean that we coalesce on the first proposal. FOSS, by nature, moves forward with improved ideas and designs, not just anything and everything that gets thrown together and happens to function. No one - not myself, nor anyone else who has, does, or will contribute to this project - owes you, me, or anyone else deference to open PRs when they believe they can provide a better solution. If you think you have a better design for a PR I have open, by all means please post it.

If you decide to continue working on this ignoring #3746 then we all know what to expect from our contributions to SoS, and we'll have to think really hard if it's worth it to contribute at all.

Once again an astounding strawman, and quite bluntly a great example of why I not only started working towards this component, but also why #3887 is where it's at. Nobody here owes you anything. You do not get to claim work at the exclusion of others. There have been over 300 contributors to this project, and they all have brought code that not only goes through review and requests for changes, but have also been rejected due to one reason or another and from those rejections they have made timely efforts to rectify and listen to reviewer feedback. The goal of this project is to provide a high quality, easy to use end-user experience when performing troubleshooting and we don't get there without adhereing to certain standards. 300 other people have made changes to other's code and had their own code changed in turn. This is not some vanity exercise it is pursuit of better code. 300 other people have understood this, accepted this, and worked with us to improve the utility. 300 other people have understood that this project at its core is no different than any other FOSS project.

I invite you to be one of those people going forward.

@jcastill
Copy link
Member

jcastill commented Jan 3, 2025

This project is not your keeper and if you want to contribute to it, then you are responsible for keeping up with your submissions. Months without movement is on no one but you and neither I nor this project am going to be held to your arbitrary schedules. You don't see us pinging other authors outside of a single effort to align labeling, what makes you special here? We have routinely posted replacement PRs when others are stale. What makes you special here?

Nothing makes me special, and I never claimed so. But yes, both @arif-ali and I pinged contributors to ask what was happening with old issues and PRs.
My "arbitrary schedules" are anything but arbitrary. I had to jump to work on the AI plugins as soon as that became a priority, and then I had to wait because some changes were coming to RH portal authentication and uploads, and then these changes were delayed again. I don't need to inform you (and it seems it's you only, because no other maintainers complained) about what's causing a delay. But if you asked before starting the work, I'd have told you.

But let's take this a step further. I have repeatedly said over the last ~2 months that I was trying to find time to work on an upload component. In public, on threads that you were apart of. "You didn't think of reaching out and, you know, ask" about that? Not a single "hey, Jake, I'm still working on this and should have something up in $days/$weeks/whatever". Pot, meet kettle.

Where have you said this? I said in IRC a couple of times that I was working on this and aiming to push some changes, and then internal things delayed.

Pot, meet kettle.

Yeah, but hey, the difference is that you knew I was working on the PR (why would you assume I abandoned it?), and I didn't know you decided to do so. Even though you claim you did, I don't know where.

SoS is an open source project and contributions are welcome and desired from all interested parties, but that doesn't mean that we coalesce on the first proposal. FOSS, by nature, moves forward with improved ideas and designs, not just anything and everything that gets thrown together and happens to function. No one - not myself, nor anyone else who has, does, or will contribute to this project - owes you, me, or anyone else deference to open PRs when they believe they can provide a better solution. If you think you have a better design for a PR I have open, by all means please post it.

Again, I never said anybody owes me anything. I would have loved to work on this together. I respected your suggestions, and would have followed all of them completely, like I've done every time. By nature, this is a collaborative process, and for that we need to work improving each other's work, for sure, but not ignoring or competing, the efficiency of the project suffers then.

Once again an astounding strawman, and quite bluntly a great example of why I not only started working towards this component, but also why #3887 is where it's at. Nobody here owes you anything. You do not get to claim work at the exclusion of others.

I'm not claiming work at the exclusion of others, I'm claiming that we should coordinate to avoid repeating work. Other contributors provide feedback, and PR owners take it or leave it. And we work together. If there is a major issue with a PR, then this should be brought to the attention of those working on it, but to simply start working in parallel defeats the purpose of actually being a team.

There have been over 300 contributors to this project, and they all have brought code that not only goes through review and requests for changes, but have also been rejected due to one reason or another and from those rejections they have made timely efforts to rectify and listen to reviewer feedback. The goal of this project is to provide a high quality, easy to use end-user experience when performing troubleshooting and we don't get there without adhereing to certain standards. 300 other people have made changes to other's code and had their own code changed in turn.

Yes, and I'm the #4 on the list, so I know quite well how to work within this project.

This is not some vanity exercise it is pursuit of better code.

I'm sorry but I'm not entirely sure your actions reflect this idea. This is supposed to be a team effort, but when this is brought up you start assigning intentions and negative connotations to requests for better communication.

300 other people have understood this, accepted this, and worked with us to improve the utility. 300 other people have understood that this project at its core is no different than any other FOSS project.

I've worked with them as well. But don't let that be in the way of claiming that I'm asking for special treatment, or that I don't understand FOSS projects, or any other thing that you want to claim to attack my personality and approach to the project, instead of making an effort to work as part of a team.

@pmoravec
Copy link
Contributor

pmoravec commented Jan 5, 2025

But let's take this a step further. I have repeatedly said over the last ~2 months that I was trying to find time to work on an upload component. In public, on threads that you were apart of. "You didn't think of reaching out and, you know, ask" about that? Not a single "hey, Jake, I'm still working on this and should have something up in $days/$weeks/whatever". Pot, meet kettle.

Where have you said your interest in writing an upload component, please? I dont recall it (but I could forget it / misunderstood / skipped that conversation) and I cant find it either in mail notifications or in in search here (https://github.com/search?q=repo%3Asosreport%2Fsos+upload+component&type=pullrequests).

Excellent strawman, really. This PR was opened as a draft to show and solicit feedback on the proposed framework of a lighter-weight, more extensible, and easier to maintain design than what is currently implemented today, and by extension of 3746 being a copy-and-move of existing code, that PR. Or, as most people would call it, this PR is stock FOSS development.

So Jake's and Jose's work can be merged, at the end? Jake working on class hierarchy, Jose on the code changes below (very roughly saying)? (I have to review this PR proposal, just trying to get the idea / target / circumstances).

I am asking this way since I see Jose's PR in quite good shape, lot of work done there, so no big reason to throw it away. I.e. we can merge Jose's work (once suggested changes resolved) to have the upload subcommand working, and meantime agree on the class hiearchy changes within this PR..?

@arif-ali
Copy link
Member

arif-ali commented Jan 8, 2025

I will reserve my comments on the issues in a private matter and don't plan to add anything public. But based on the conversations, it makes sense to collaborate on the efforts so that work done is not wasted on either side and the end goal is accomplished and that is getting a good feature out there.

Based on what I am seeing on the PRs, and I haven't reviewed heavily (I need some time), the 2 PRs have one idea of creating a new component for upload. This PR is abstracting a few more things away, especially with the target. One suggestion I have on this is that we want the same experience for our customers so that they don't have to specify the --target canonical by default, and this is implied by default if you're running on Ubuntu platforms. If, however, this is the direction we're going people will have muscle memory and they will forget, and need to ensure this is documented in our Company newsletters for this to be the new option.

@pmoravec
Copy link
Contributor

pmoravec commented Jan 9, 2025

I havent reviewed this draft PR yet (I should do tomorrow), but I guess default value of --target should be automatically determined from calculated (or user-specified) Policy (in a complete PR).

@TurboTurtle
Copy link
Member Author

but I guess default value of --target should be automatically determined from calculated (or user-specified) Policy (in a complete PR).

One suggestion I have on this is that we want the same experience for our customers so that they don't have to specify the --target canonical by default

I'll reserve other comments pending our maintainer call, but for this specifically - yes, --target would default to the loaded policy and would not be a required option. The existing default behavior of Ubuntu hosts upload to Canonical and RHEL hosts upload to Red Hat would persist and users would not need to manually set the target.

Comment on lines +263 to +266
Users should only upload data using this utility to locations that are both \
well-known and trusted by them and/or their organizations. This utility does \
not accept any responsibility for the safety, security, or handling of any \
data transmitted with it to any location.\n\
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useful banner.

Comment on lines +30 to +35
1. The setting of `--upload-user` and `--upload-pass` in either the
commandline execution of sos or a given config file.
2. The setting of the SOSUPLOADUSER and SOSUPLOADPASS env vars
3. A prompt for user input, iff the target has set the
`prompt_for_credentials` class var to `True`. This prompt will be
skipped if `--batch` is set.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current ordering is 2., then 1., then 3., due to:

    def get_upload_user(self):
        """Helper function to determine if we should use the policy default
        upload user or one provided by the user

        :returns: The username to use for upload
        :rtype: ``str``
        """
        return (os.getenv('SOSUPLOADUSER', None) or
                self.upload_user or
                self._upload_user)

method, am I right? Why the change?

(on one side, cmdline option should override more-generic env.variable that is more "persistent", but config file is yet "more persistent" than env.variable; also when user sets both env.var and cmdline option, we should prefer the value that is obfuscated (in process list))

@pmoravec
Copy link
Contributor

Is target more likely protocol, or policy, or both? How the class hierarchy can handle Red Hat policy "ambiguity" in providing two different protocols (ftp and http), with automatic failover from http to ftp?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants