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

[sunbeam] Update plugin to collet more details #3646

Merged

Conversation

arif-ali
Copy link
Member

  • Add juju related info for sunbeam
  • Add logs from the sunbeam snap home user

Resolves: SET-682


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?

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

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

@arif-ali arif-ali marked this pull request as draft May 19, 2024 21:35
@arif-ali arif-ali force-pushed the sos-arif-sunmbeam-juju-additions branch from 24c32b1 to eb14b81 Compare May 20, 2024 07:00
@pmoravec
Copy link
Contributor

Sounds good to me (from a pure sos side, not knowing either command or file to be collected).

@arif-ali
Copy link
Member Author

arif-ali commented May 20, 2024

I think this does a beg a question, should we improve some of the commands to be able collect commands as a user, so we're not doing blanket su - <user> for every command we want to collect.

The problem also I have this which I discovered last night, is that 2 of these commands will also collect certs within the command itself. If the command could be different, do I put the same login the the postproc(), it just seems like duplicating code. I'll have a think on how best to do it, but others might have an idea already. i.e. would I be ok to use do_cmd_private_sub in setup() to avoid all the complications here?

This is here for collaboration with my team at the moment, and do some improvements.

@pmoravec
Copy link
Contributor

I think some support for "running under user" within Plugin class makes a lot of sense. Three other plugins (aap_eda, saphana and sapnw) will utilize from it as well.

@arif-ali arif-ali force-pushed the sos-arif-sunmbeam-juju-additions branch from eb14b81 to d24d664 Compare May 21, 2024 20:56
@arif-ali arif-ali force-pushed the sos-arif-sunmbeam-juju-additions branch from d24d664 to c9cff03 Compare June 7, 2024 08:53

if self.get_option("juju-allow-login"):
self.exec_cmd(
f'su - {sunbeam_user} -c "sunbeam utils juju-login"')
Copy link
Member Author

Choose a reason for hiding this comment

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

This did not work with runas argument as this command has some interactive related items, and the only way to get this working was via su

@arif-ali arif-ali force-pushed the sos-arif-sunmbeam-juju-additions branch from c9cff03 to 46f1221 Compare June 10, 2024 14:15
@arif-ali arif-ali marked this pull request as ready for review June 10, 2024 14:15
@arif-ali arif-ali requested a review from pmoravec June 13, 2024 13:52
@@ -37,10 +48,90 @@ def setup(self):
'sunbeam cluster list --format yaml',
])

sunbeam_user = self.get_option("sunbeam-user")
user_pwd = pwd.getpwnam(sunbeam_user)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the user does not exist, getpwnam raises an exception that will end up in sos_logs/sunbeam-plugin-errors.txt:

>>> pwd.getpwnam('no-user')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
KeyError: "getpwnam(): name not found: 'no-user'"
>>> 

Technically it doesnt matter much since even catching the exception will cause the plugin wont collect anything (due to if user_pwd), just that scenario looks bit non-professional.

Leaving on @arif-ali to decide if it is worth fixing it - I dont want to block this "form of error reporting" point from merging.

Copy link
Member Author

@arif-ali arif-ali Jun 19, 2024

Choose a reason for hiding this comment

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

good catch, I agree it's looks non-professional if that error file is created; I will fix and re-upload. Thanks for catching this; this is exactly why we have others reviewing things

* Add juju related info for sunbeam
* Add logs from the sunbeam snap home user
* Only collect juju details if juju is logged in
* Obfuscate the certs from the controller config

Resolves: SET-682

Signed-off-by: Arif Ali <[email protected]>
@arif-ali arif-ali force-pushed the sos-arif-sunmbeam-juju-additions branch from 46f1221 to 45d1139 Compare June 19, 2024 10:26
@TurboTurtle TurboTurtle merged commit d61bf37 into sosreport:main Jun 19, 2024
32 checks passed
@arif-ali arif-ali deleted the sos-arif-sunmbeam-juju-additions branch October 2, 2024 13:46
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.

3 participants