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

Add spec and parser for luksmeta command #3525

Merged
merged 16 commits into from
Oct 13, 2022

Conversation

danzatt
Copy link
Contributor

@danzatt danzatt commented Sep 15, 2022

Signed-off-by: daniel.zatovic daniel.zatovic@gmail.com

All Pull Requests:

Check all that apply:

  • Have you followed the guidelines in our Contributing document, including the instructions about commit messages?
  • Is this PR to correct an issue?
  • Is this PR an enhancement?

Complete Description of Additions/Changes:

  • added luks1_block_devices which depends on LuksDump parser and filters just LUKS1 devices and returns their paths (based on the UUID)
  • added luksmeta spec which runs luksmeta show command on every LUKS1 device to show information about custom metadata embedded in the keyslots
  • added LuksMeta parser to parse the luksmeta show command

Sorry, something went wrong.

Copy link
Contributor

@xiangce xiangce left a comment

Choose a reason for hiding this comment

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

I also find this KCS-3617741, please check if this error "Device is not initialized (/dev/mapper/vg_luks-lv_luks)" should be handled or not in this parser. Thanks.

insights/parsers/luksmeta.py Outdated Show resolved Hide resolved
insights/parsers/luksmeta.py Outdated Show resolved Hide resolved
insights/parsers/luksmeta.py Outdated Show resolved Hide resolved
insights/specs/datasources/luks_devices.py Outdated Show resolved Hide resolved
insights/parsers/luksmeta.py Show resolved Hide resolved
@danzatt
Copy link
Contributor Author

danzatt commented Sep 19, 2022

I have gone through the remarks and fixed them now.

Copy link
Contributor

@xiangce xiangce left a comment

Choose a reason for hiding this comment

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

Thanks for the update, it looks good to me now.

insights/parsers/luksmeta.py Show resolved Hide resolved

# LUKS1 contains exactly 8 keyslots
if len(content) != 8:
raise SkipComponent
Copy link
Contributor

Choose a reason for hiding this comment

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

@danzatt your comment implies that there must be 8 keyslots. Is it an error if there are not exactly 8?

@danzatt danzatt force-pushed the luksmeta branch 2 times, most recently from 19cca14 to 77d8cec Compare September 27, 2022 14:28
insights/specs/default.py Outdated Show resolved Hide resolved
insights/collect.py Outdated Show resolved Hide resolved
@danzatt
Copy link
Contributor Author

danzatt commented Sep 29, 2022

Yes, indeed it should be simpler to run the luksmeta command against all devices. I have changed the PR now and removed the luks1_block_devices dependency. (I fixed the cryptsetup/luksmeta paths too.)

from insights.parsers.luksmeta import LuksMeta


@combiner(LuksDump)
Copy link
Contributor

Choose a reason for hiding this comment

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

@danzatt - from my perspective, It seems this combiner function is not so required. Firstly, it's based on the single parser, if really want such an interface to return the "luks1 block devices", you can totally implement it in the LuksDump parser itself. Or, you can move this check into the LuksDevices combiner as a double safety-check here L77: if luks_metas and uuid in luksmeta_by_uuid:

return luks1_devices


@combiner(LuksDump, optional=[LuksMeta])
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that the LuksMeta should not be an optional for this combiner, since it's required for the check of L77.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My reasoning behind this is that every LUKS1/2 device has to have one LuksDump object, whereas LuksMeta objects are created only for devices that are LUKS1 and are initialized using luksmeta. Therefore, if the device is not formatted using luksmeta, we just pass-through LuksDump dictionary, and if it is initialized using luksmeta we add a "luksmeta" entry into the LuksDump output. So the LuksDump must always be present, and optionally for luksmeta devices, we also add the information from LuksMeta parser.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just checked your latest update to the test of this combiner, it now looks good to me. Sorry for the late reply, I was just back from a long leave.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, thank you for the review.

insights/combiners/cryptsetup.py Show resolved Hide resolved
@danzatt danzatt force-pushed the luksmeta branch 2 times, most recently from eae440a to e3e72a1 Compare October 5, 2022 19:51
@danzatt
Copy link
Contributor Author

danzatt commented Oct 5, 2022

Is there anything I can do to get this merged?

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>
Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>
Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>
Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>
Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>
Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>
Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>
Otherwise TypeError: LocalSpecs() takes no arguments
is emitted.

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>
Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>
To make a combiner which combines LuksMeta and cryptsetup luksDump
outputs, we need to include UUID information in LuksMeta parser.

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>
Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>
We match LuksDump and the associated LuksMeta based on the UUID of the
device they were obtained from.

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>
Instead we run luksmeta against all devices in /dev/disk/by-uuid

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>
Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>
Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>
Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>
@xiangce xiangce merged commit d5b0862 into RedHatInsights:master Oct 13, 2022
xiangce pushed a commit that referenced this pull request Oct 13, 2022
* Add spec and parser for luksmeta command

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Add LuksDump parser to collect.py

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Change luksmeta command documentation

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Improve error handling in the luksmeta parser

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Change LuksMeta parser to inherit from dict.

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Extend test coverage

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Fix luksmeta parser documentation

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Specs class cannot be decorated using datasource

Otherwise TypeError: LocalSpecs() takes no arguments
is emitted.

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Change luks1_block_devices to a combinder

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Add device UUID to LuksMeta parser output.

To make a combiner which combines LuksMeta and cryptsetup luksDump
outputs, we need to include UUID information in LuksMeta parser.

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Convert LuksMeta to CommandParser

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Add LuksDevices combiner

We match LuksDump and the associated LuksMeta based on the UUID of the
device they were obtained from.

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Don't run luks1_block_devices during collection

Instead we run luksmeta against all devices in /dev/disk/by-uuid

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Fix cryptsetup LocalSpecs to use full path

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Remove luks1_block_devices combiner

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Skip LuksDevices combiner if there are no results

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>
(cherry picked from commit d5b0862)
xiangce pushed a commit that referenced this pull request Sep 6, 2024
* Add spec and parser for luksmeta command

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Add LuksDump parser to collect.py

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Change luksmeta command documentation

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Improve error handling in the luksmeta parser

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Change LuksMeta parser to inherit from dict.

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Extend test coverage

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Fix luksmeta parser documentation

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Specs class cannot be decorated using datasource

Otherwise TypeError: LocalSpecs() takes no arguments
is emitted.

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Change luks1_block_devices to a combinder

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Add device UUID to LuksMeta parser output.

To make a combiner which combines LuksMeta and cryptsetup luksDump
outputs, we need to include UUID information in LuksMeta parser.

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Convert LuksMeta to CommandParser

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Add LuksDevices combiner

We match LuksDump and the associated LuksMeta based on the UUID of the
device they were obtained from.

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Don't run luks1_block_devices during collection

Instead we run luksmeta against all devices in /dev/disk/by-uuid

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Fix cryptsetup LocalSpecs to use full path

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Remove luks1_block_devices combiner

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>

* Skip LuksDevices combiner if there are no results

Signed-off-by: daniel.zatovic <daniel.zatovic@gmail.com>
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.

None yet

3 participants