[sos] Add 'upload' component to upload existing reports and files #3746
Conversation
|
Huge thanks to @pmoravec for all the help reviewing this, suggesting improvements, and finding bugs. |
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
|
@arif-ali about these ones: sos/upload/init.py:103:0: C0325: Unnecessary parens after '=' keyword (superfluous-parens) ************* Module sos.upload sos/upload/init.py:42:46: W0613: Unused argument 'in_place' (unused-argument) sos/upload/init.py:43:17: W0613: Unused argument 'hook_commons' (unused-argument) sos/upload/init.py:155:24: R1722: Consider using 'sys.exit' instead (consider-using-sys-exit) |
jcastill
force-pushed
the
jcastillo-add-upload-subsystem
branch
from
d5b6c64 to
0e6bc72
Compare
|
With R1725, I made the changes a few months back, and hence enabled the check, so let's do this here too. With the unused variable. If your 100% sure you're going to be using them then potentially you could add the following before the line |
jcastill
force-pushed
the
jcastillo-add-upload-subsystem
branch
from
0e6bc72 to
9c60d66
Compare
Done, should be in the version I just pushed.
Nice one! But I ended up removing it. I'll re-add them in the future when I have ready the code for hooking report etc. |
man/en/sos-upload.1
Outdated
|
|
||
| .PP | ||
| .SH DESCRIPTION | ||
| upload is an sos subcommand to upload sos reports, logs, vmcores, or other files to a policy defined remote location, or an user defined one. |
There was a problem hiding this comment.
Nitpick: s/an user/a user/.
There was a problem hiding this comment.
I think is 'an' because 'user' starts with a vowel, no?
There was a problem hiding this comment.
The rule is "first sound of word", not first letter :) E.g. https://english.stackexchange.com/questions/105116/is-it-a-user-or-an-user (though I am not sure how authoritative that source is).
There was a problem hiding this comment.
Nice, TIL. Fixed in the next push.
sos/policies/distros/redhat.py
Outdated
| @@ -298,7 +298,7 @@ def get_upload_url(self): | |||
| self.ui_log.info("No case id provided, uploading to SFTP") | |||
| return RH_SFTP_HOST | |||
| rh_case_api = "/support/v1/cases/%s/attachments" | |||
| return RH_API_HOST + rh_case_api % self.case_id | |||
| return RH_API_HOST + rh_case_api % self.commons['cmdlineopts'].case_id | |||
There was a problem hiding this comment.
Why this change? AFAIK self.case_id might be blank (and common's case_id set) only in scenario "case id not in cmdline, batch not in cmdline" - should not upload query for case_id, then? (or am I wrong here with my assumption)?
(that concern is valid for sure (while I can be wrong on its impact to this code change):
# python3 bin/sos upload /var/tmp/sosreport-pmoravec-rhel8-012345678-2024-08-13-gbiatgg.tar.xz
sos upload (version 4.7.2)
This utility is used to upload files to a policy-default location.
The archive to be uploaded may contain data considered sensitive and its content
should be reviewed by the originating organization before being passed to any
third party.
No configuration changes will be made to the system running this utility.
Press ENTER to continue, or CTRL-C to quit
Attempting to upload file /var/tmp/sosreport-pmoravec-rhel8-012345678-2024-08-13-gbiatgg.tar.xz to case
No case id provided, uploading to SFTP
No case id provided, uploading to SFTP
Attempting upload to Red Hat Secure FTP
..
There was a problem hiding this comment.
This is one of the things we talked about internally when I first started playing with 'upload'. If you remember, the issue was that without this change, we were getting 'None' on the case_id and it was failing to build the url, and so failed to upload. I have the feeling that I've done something wrong on the upload side and I'm not passing the case_id correctly.
My hope is that more experienced eyes, or at least fresher, can tell me where I'm failing.
|
When I run: and "Press ENTER to continue", and then nothing, then I get a final error: I think the upload did not succeed at the end.. |
| defined location. These files can be either sos reports, | ||
| sos collections, or other kind of files like: vmcores, | ||
| application cores, logs, etc. | ||
|
|
| def _fmt_msg(self, msg): | ||
| width = 80 | ||
| _fmt = '' | ||
| for line in msg.splitlines(): | ||
| _fmt = _fmt + fill(line, width, replace_whitespace=False) + '\n' | ||
| return _fmt |
There was a problem hiding this comment.
We declare this method on many places / in most of components. Isn't it worth declaring in SosComponent directly?
There was a problem hiding this comment.
This should be done in another PR, right?
There was a problem hiding this comment.
I would prefer having extra commit within this PR, but I am ok with a new issue/PR.
There was a problem hiding this comment.
I'll do it here then :)
|
When pressing Ctrl+C on |
|
@pmoravec thank you for finding this, I thought we solved these issues:
I'll check the double messaging here, looks horrible.
I'll check this one as well, I remember we had a similar issue with a previous implementation.
No, it should not succeed in that case. |
I'll check this, should be easy to fix. |
Fixed. I used exit() instead of _exit(), which is the one implemented in Soscomponent. |
There was a problem hiding this comment.
At a bare minimum, a new component should be implementing all the abstractions that it needs to operate solo, not acting as a wrapper to existing functionality.
This means the upload logic needs to be separated from its current location in Policy, and implemented as a discrete unit. Policy should then control the default setting, and users should be able to direct sos to chose an upload target/profile/whatever we want to call it as an override. E.G. if I have an sos report locally on my Fedora workstation that was taken from a RHEL box, and I am unable due to some network policy to directly upload from the RHEL box, then on my Fedora system I should be able to send that sos report to Red Hat.
Further, any current or future usage of the component's functionality should go through the actual component code. Much like we do with sos clean, when --clean is used for a report being generated. We hook into the component from within report, to ensure we use the exact code flow for cleaning the archive as we would by running a clean after-the-fact.
jcastill
force-pushed
the
jcastillo-add-upload-subsystem
branch
from
9c60d66 to
72dd27c
Compare
I agree with everything above, but the idea behind this PR is to be a first implementation to get the upload component started, and then move things carefully from policy to upload. Could this approach be acceptable? |
|
I support this initial implementation of the feature to let enhance I was thinking to raise the same concern, but I realized I would see beneficial for the discussion about refactorization if we already has some implementation in hand. With the current code, it is hard for me to specify "cut this away from here and put it (there)", if we have no "(there)". With the If somebody sees as a potential threat "we accept this initial implementation, but will never refactor the code as needed, and we dont want that technical debt here", then I can make a commitment: once there will be an agreement about the refactorisation and if nobody(*) will have time to implement it, I will work on such PR. (*) nobody including Jose as the primary person to implement. I assume he will be the primary person to make his own feature to make it complete. On the other side, there can be various reasons he won't be able to do the refactorisation (time, other work on sos, willingness, whatever). And then anybody else (with me as the volunteer with above commitment) can contribute that way. |
On this note, I already started moving things around from policies/distros just after I sent this PR - this is not something I want to leave abandoned, or done in six months time or more, but as soon as possible. But also I want to make sure I cover all the possible cases, and the upload code in policies has been there for a long time, working perfectly, so want to be extra careful while refactoring. |
| [--case-id id]\fR | ||
| [--upload-url url]\fR | ||
| [--upload-user user]\fR | ||
| [--upload-pass pass]\fR | ||
| [--upload-directory dir]\fR | ||
| [--upload-method]\fR | ||
| [--upload-no-ssl-verify]\fR | ||
| [--upload-protocol protocol]\fR |
There was a problem hiding this comment.
@jcastill Could the --upload-protocol s3 flags be included in this work? Unfortunately, it contains unique flags that made S3 easier to implement at the time.
[--upload-s3-endpoint endpoint]
[--upload-s3-region region]
[--upload-s3-bucket bucket]
[--upload-s3-access-key access_key]
[--upload-s3-secret-key secret_key]
[--upload-s3-object-prefix object_prefix]
The existing flags and how the provided values were used were not well aligned for S3, even though valid for FTP, HTTP, etc. protocols. I didn't want to cause any breakage for existing upload protocols while trying to make them work for all protocols, so S3 ended up with unique flags.
I planned to attempt a refactor at some point (sos v5?) where the original protocols and s3 overlap. For example, allowing synonymous flags:
--upload-user~--upload-s3-access-key--upload-pass~--upload-s3-secret-key--upload-directory~--upload-s3-object-prefix--upload-url~--upload-s3-endpoint
However, I haven't been able to dedicate the time yet.
There was a problem hiding this comment.
Could the --upload-protocol s3 flags be included in this work? Unfortunately, it contains unique flags that made S3 easier to implement at the time.
Yes, I'll make sure I include them in the next iteration of this PR.
I planned to attempt a refactor at some point (sos v5?) where the original protocols and s3 overlap. For example, allowing synonymous flags:
--upload-user ~ --upload-s3-access-key --upload-pass ~ --upload-s3-secret-key --upload-directory ~ --upload-s3-object-prefix --upload-url ~ --upload-s3-endpointHowever, I haven't been able to dedicate the time yet.
Let me know if I can help. My original idea was to have this PR as a starting point and then move stuff out of the generic policy and the OS-specific ones in a second PR, but that was rejected, so I'm working on the full change now. As soon as I finish with that, we can start working on the refactor of S3 it that's OK with you. In fact we need to do some work with S3 uploads for the RH customer portal, so we could do both things in parallel.
There was a problem hiding this comment.
Let me know if I can help. My original idea was to have this PR as a starting point and then move stuff out of the generic policy and the OS-specific ones in a second PR, but that was rejected, so I'm working on the full change now. As soon as I finish with that, we can start working on the refactor of S3 it that's OK with you. In fact we need to do some work with S3 uploads for the RH customer portal, so we could do both things in parallel.
When you have a branch published for public view and somewhat functional let me know. I'll branch off of it and start migrating the s3 portions in then submit a PR targeting your branch for you to review.
As for the s3 refactoring, we can look into it and I'd be more than happy to try and make some time. I believe a few lend themselves easily, or at least I don't immediately recall any issues with using them, like URL, user, and password. One I do recall bringing up some questions is the --upload-directory. For example, should this be only the prefixes inside the bucket? Or should it split the directory like {bucket}/{prefix} on only the first slash? There may have been others, but I would have to review the LinuxPolicy.get_upload_xxxx() functions and internal self._vars again.
Without some "group think" I decided not to implement something I (or others) may have been unhappy with later but stuck with unless making breaking changes.
There was a problem hiding this comment.
There may be less to refactor than I first thought as I haven't reviewed the code in almost a year. I guess ended up implementing some of it already. Hope I'm still happy with my choices after a year 😄
sos/sos/policies/distros/__init__.py
Lines 615 to 629 in 2aa4fcf
arif-ali
added
the
Status/Need More Info
jcastill
force-pushed
the
jcastillo-add-upload-subsystem
branch
from
72dd27c to
68d9933
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
jcastill
force-pushed
the
jcastillo-add-upload-subsystem
branch
from
68d9933 to
637a418
Compare
|
@TrevorBenson Could you check if I missed anything on the S3 side? The issue with the deb built is about a circular import. I'm checking it now. |
jcastill
force-pushed
the
jcastillo-add-upload-subsystem
branch
from
637a418 to
37d7394
Compare
jcastill
added
enhancement
Kind/Ubuntu
jcastill
force-pushed
the
jcastillo-add-upload-subsystem
branch
from
37d7394 to
43493f9
Compare
There was a problem hiding this comment.
While this is certainly an improved PR from earlier, this ultimately is mainly moving existing code around, which means we aren't solving a lot of the issues that uploading via sos has today; rigid structure that is designed around a policy-driven execution, which is the main thing that's been said we need to change with upload, and a monolithic design that is hard(er) to maintain as time goes on. Further, this still retains the "gotchas" of the current design - if a user passes incomplete or incorrect details that would cause us to always fail an upload, and it is something we can catch before the upload attempt, we don't catch it and we end up with an understandably frustrated user with a failed upload. Again, that's an issue we have today and it's not unique to this PR's designs, but if we're going to go through the trouble of expanding functionality into a new component, we should be addressing as much as we can with that effort.
On a more technical level, there are still issues, some of these may have comments below as well.
- We shouldn't be overloading the term
profilewhich already has a specific technical meaning within the project. - The extensibility of this to new destinations is limited due to the monolithic nature of the base profile class. New profiles/destinations should not carry the bits that are not relevant to them.
- profile/destination determination should be more intuitive and not require additional lines in a large if-block of statically assigned conditionals
- reduction in options we rely on the user to know and use, Options Are Evil and we should do as much as we can without requiring user input, for example we shouldn't be needing an upload-method option if we're trying to make uploads more deterministic and easier to manipulate.
- we create a manifest section and then add nothing to it?
| if self.opts.batch and self.opts.password: | ||
| self.exit( | ||
| "\nsos-collector was called with incompatible options --batch " | ||
| "and --password.\nIf you need to use --password," | ||
| " please omit batch mode.\n", | ||
| 1 | ||
| ) | ||
|
|
| # add manifest section for upload | ||
| self.manifest.components.add_section('upload') |
There was a problem hiding this comment.
We don't do anything with this later?
| self.profiles_data = { | ||
| 'redhat': { | ||
| 'mod': "sos.upload.profiles.redhat", | ||
| 'class': "RHELUpload", | ||
| 'description': "Red Hat" | ||
| }, | ||
| 'ubuntu': { | ||
| 'mod': "sos.upload.profiles.ubuntu", | ||
| 'class': "UbuntuUpload", | ||
| 'description': "Ubuntu" | ||
| }, | ||
| 'generic': { | ||
| 'mod': "sos.upload.profiles", | ||
| 'class': "Upload", | ||
| 'description': "Generic" | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
We shouldn't be maintaining a statically defined dict like this. Any kind of enumeration should be dynamic such that new destinations are automatically accounted for.
| def set_upload_profile(self): | ||
| if self.opts.upload_profile: | ||
| if self.opts.upload_profile == "redhat": | ||
| return self.load_upload_profile(self.profiles_data['redhat']) | ||
| if self.opts.upload_profile == "ubuntu": | ||
| return self.load_upload_profile(self.profiles_data['ubuntu']) | ||
| return self.determine_local_profile() |
There was a problem hiding this comment.
Again, this should not be done via statically defined elements.
| def determine_local_profile(self): | ||
| self.ui_log.info( | ||
| "Trying to determine local upload profile..." | ||
| ) | ||
|
|
||
| from sos.policies.distros.redhat import RHELPolicy | ||
| from sos.policies.distros.ubuntu import UbuntuPolicy | ||
|
|
||
| if isinstance(self.policy, RHELPolicy): | ||
| return self.load_upload_profile(self.profiles_data['redhat']) | ||
| if isinstance(self.policy, UbuntuPolicy): | ||
| return self.load_upload_profile(self.profiles_data['ubuntu']) | ||
| self.ui_log.info( | ||
| "Couldn't determine local profile. Using generic profile." | ||
| ) | ||
| return self.load_upload_profile(self.profiles_data['default']) |
There was a problem hiding this comment.
Ditto here. If we need to reference a policy, which is a valid code flow even if we're wanting to extend uploading from different workstations, we should be leveraging the actual loaded policy. This comparison is very inefficient.
| if self.commons['cmdlineopts'].upload_protocol == 'sftp': | ||
| return self.RH_SFTP_HOST | ||
| if not self.commons['cmdlineopts'].case_id: | ||
| return self.RH_SFTP_HOST |
There was a problem hiding this comment.
I still haven't determined the cleanest implementation for #3891, but I am considering something like:
| if self.commons['cmdlineopts'].upload_protocol == 'sftp': | |
| return self.RH_SFTP_HOST | |
| if not self.commons['cmdlineopts'].case_id: | |
| return self.RH_SFTP_HOST | |
| if self.commons['cmdlineopts'].upload_protocol == 'sftp': | |
| return self.RH_SFTP_HOST | |
| if self.commons['cmdlineopts'].upload_protocol == 's3': | |
| bucket = self.get_upload_s3_bucket() | |
| prefix = self.get_upload_s3_object_prefix() | |
| return f"s3://{bucket}/{prefix}" | |
| if not self.commons['cmdlineopts'].case_id: | |
| return self.RH_SFTP_HOST |
This leaves off the case id as a prefix (aka directory) within the bucket and only leaves it as part of the archive name.
I had not added it to the PR to get feedback yet. I think it is likely an acceptable compromise, at least until Red Hat decides to use an s3 bucket as an additional upload target for support cases.
There was a problem hiding this comment.
I'll test this but it looks like it will work. Having it here could help receive feedback from others as well.
And I'll make sure I add you as co-author with all these changes.
| if self.get_upload_url().startswith(self.RH_SFTP_HOST): | ||
| return "Red Hat Secure FTP" | ||
| return self._get_obfuscated_upload_url(self.upload_url) |
There was a problem hiding this comment.
Related to the above comment for get_upload_url() and #3891:
| if self.get_upload_url().startswith(self.RH_SFTP_HOST): | |
| return "Red Hat Secure FTP" | |
| return self._get_obfuscated_upload_url(self.upload_url) | |
| if self.get_upload_url().startswith(self.RH_SFTP_HOST): | |
| return "Red Hat Secure FTP" | |
| if self.get_upload_url().startswith('s3://'): | |
| return f"{self.get_upload_url()} on endpoint {self.get_upload_s3_endpoint()}" | |
| return self._get_obfuscated_upload_url(self.upload_url) |
There was a problem hiding this comment.
Looks good to me but lets wait for other's feedback.
| @classmethod | ||
| def add_parser_options(cls, parser): | ||
| parser.usage = 'sos upload FILE [options]' | ||
| upload_grp = parser.add_argument_group( | ||
| 'Upload Options', | ||
| 'These options control how upload manages files' | ||
| ) | ||
| upload_grp.add_argument("upload_file", metavar="FILE", | ||
| help="The file or archive to upload") |
There was a problem hiding this comment.
Would this PR intend to have the upload arguments inherited in other argument parsers, reducing code duplication?
Or does this require maintaining all three locations for consistency?
report/__init__.pycollector/__init__.pyupload/__init__.py
There was a problem hiding this comment.
I hit this "code duplication problem" with arguments else-where, in collector/report/cleaner combination. We have similar duplication there that I am not happy with. Maybe worth to have some "component(s)<->parameters" abstraction / declaration, where say yaml or json file describes the allowed parameters and inheritance (of arguments and also components relations/inheritancy)?
But that is bit independent task to this PR.
|
Thank you for the feedback, I'll start working on it next week.
The idea behind this from Pavel and I was to start moving things around to have the functionality to upload files via the command line ready in sos, and once that's working, start improving the code. That's the reason why I started originally only hooking to the policy, as ugly as it was, to start making changes incrementally in different PRs and have some good testing sessions before changing things further.
Would "upload_profile" work better? Or perhaps 'role'?
This is one of the things we could improve later on once the functionality is up and running. Is that acceptable?
That's where I need some guidance. I was looking at the collect code iIrc to try to mimic it and not reinvent the wheel, and I indeed ended up reinventing the wheel.
That I can work on this specific PR.
Yes I wanted to ask about this. Would we need upload to be added in the manifest? I imagine so, but I didn't explore it further just in case we didn't want to. Would it be OK if we proceed with the changes here, moving code around, yes, and adding other stuff, and then we all work on the improvements to upload that you envision in the next PR? |
This commit marks the beginning of the addition of a new 'upload' component for sos, which can be used to upload already created sos reports, collects, or other files like logs or vmcores to a policy defined location. The user needs to specify a file location, and can make use of any of the options that exist nowadays for the --upload option. This first commit includes: - The initial framework for the 'upload' component. - The new man page for 'sos upload'. - The code in the component 'help' to show information about the component. - The code in sos/__init__.py to deal with the component. - The code for uploads to Red Hat and Ubuntu systems. - The code to allow uploads specifying remote destination, called profiles in this implementation. For example, you could generate a sos report in a CentOS system and specify a profile 'redhat' to upload to the Red Hat Customer Portal. - And modifications to setup.py to build the man pages. Related: RHEL-23032, SUPDEV-138, CLIOT-481 Co-authored-by: Jose Castillo <[email protected]> Co-authored-by: Pavel Moravec <[email protected]> Co-authored-by: Trevor Benson <[email protected]> Signed-off-by: Jose Castillo <[email protected]>
jcastill
force-pushed
the
jcastillo-add-upload-subsystem
branch
from
17009f1 to
439785b
Compare
What we can add to the manifest section? Just arguments and nothing else..? Since any outcome of upload itself is impossible to have it (we first complete manifest, then attempt to upload the stuff, then we have its outcome, right..?) Or what information can be present there..? |
This commit marks the beginning of the addition of a new 'upload' component for sos, which can be used to upload already created sos reports, collects, or other files like logs or vmcores to a policy defined location.
The user needs to specify a file location, and can make use of any of the options that exist nowadays for the --upload option.
This first commit includes:
Related: RHEL-23032, SUPDEV-138, CLIOT-481
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines