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

Fixes #36738 - Add remediation wizard #546

Merged
merged 8 commits into from
Apr 26, 2024

Conversation

ofedoren
Copy link
Member

@ofedoren ofedoren commented Sep 27, 2023

Requires:

There are still some TODOs (marked in the code):

  • Show the wizard for failed rules only.
  • Fix pagination in review hosts step. I'm not sure why it's not rendered properly, maybe @MariaAga could help?..
  • What permissions should be checked and where? view_arf_reports, create_job_invocations.
  • Maybe we could use Log/Message model directly to load data faster instead of loading whole report.
  • What should we do with the new feature templates? It's easier to create new ones for Ansible and Script, so we have more control in case we need to update them. Logically they should live in foreman_ansible/remote_execution repos resp., because they will automatically be seeded. Even though they are expected to be used by the application itself (not by a direct user interaction), they will appear in the UI even if it doesn't make sense (no ansible plugin for example). I'd vote to keep them here, maybe with mentioning in the description to not use it directly. Also, it will be easier to track them for updates or in case we will create new ones. @adamruzicka, WDYT?
  • write JS tests :/
  • Fix OVAL tests (even though they should be removed along with all mentions of OVAL)
  • Some remediation steps/scripts require a host reboot. There is a mentioning of that in the full report, but it seems we don't parse it on proxy side, thus we don't send this as part of the report to save. I think, we might need this info to either tell the user that there is still an additional step or we could do that as part of the remediation.
  • Remediation via Script and reboot will mark the job as failed, even if it's actually OK. I guess, we're not so sophisticated as Ansible :/ @adamruzicka, WDYT? Is this something we could easily support via REX, or should we simply disallow users to use auto reboot unless they use Ansible here?
  • fix tests related to templates seeding.

To test this, you should have a host with policy associated and an arf report for that host, navigate to the /compliance/arf_reports/<id>, there is a new Remediate entry in action buttons for every rule.

Here is some recording of remediation via Shell script:

Peek.2023-09-27.16-18.webm

@ofedoren ofedoren force-pushed the feat-36738-remediate-from-ui branch 2 times, most recently from 9b32381 to 1022bf3 Compare September 27, 2023 14:24
@ofedoren ofedoren force-pushed the feat-36738-remediate-from-ui branch 2 times, most recently from 14bc8a1 to 067f71c Compare October 12, 2023 01:07
Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

What permissions should be checked and where?

I'll have to do a bit more reading to be able to answer that

What should we do with the new feature templates? ... I'd vote to keep them here, maybe with mentioning in the description to not use it directly

Agreed

@ofedoren
Copy link
Member Author

ofedoren commented Mar 8, 2024

Thanks a lot, @MariaAga, should be fixed now.

@ofedoren ofedoren force-pushed the feat-36738-remediate-from-ui branch 2 times, most recently from 164c2e3 to 6299ea1 Compare March 11, 2024 16:01
@ofedoren ofedoren force-pushed the feat-36738-remediate-from-ui branch from 6299ea1 to 502a375 Compare March 12, 2024 16:07
@ofedoren ofedoren force-pushed the feat-36738-remediate-from-ui branch from aeec4c6 to 197a874 Compare March 13, 2024 14:25
@ofedoren
Copy link
Member Author

@MariaAga, I've returned Select All option, so it should be now less weird to use the select box.

@ofedoren
Copy link
Member Author

@lhellebr, I think you might want to check the current state, rpm build for rhel8 is ready: https://dashboard.packit.dev/results/copr-builds/1425886

@MariaAga
Copy link
Member

Thanks!
all selected is lost when moving between steps (select all in review hosts, click next, click back, only 1 selected)

@ofedoren ofedoren force-pushed the feat-36738-remediate-from-ui branch from d8ecf7a to bc6c57e Compare March 28, 2024 16:55
@ofedoren
Copy link
Member Author

ofedoren commented Apr 8, 2024

Depends on theforeman/foreman#10113.

@adamruzicka
Copy link
Contributor

/packit build

@lhellebr
Copy link

I have tested this PR together with theforeman/foreman#10113 and without any deeper testing, I can confirm the workflow works.

@adamruzicka
Copy link
Contributor

All the prerequisite PRs seem to be merged, kicked off the tests again

@ofedoren ofedoren force-pushed the feat-36738-remediate-from-ui branch from bc6c57e to d8df9c3 Compare April 25, 2024 10:42
Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

The only test failures seem to be coming from oval, which is not a thing that should be solved here

Copy link
Member

@asteflova asteflova left a comment

Choose a reason for hiding this comment

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

I found a couple of places where it would be nice to adjust the wording so that we don't have to explain in more detail in documentation. And I'm adding a few other very minor suggestions as well.

Feel free to reject any or all of my suggestions (but if possible, please explain why you're rejecting them so that I can learn :))

Overall, I like how you're doing this because in the docs, I really will be able to just explain where to find the wizard and the web UI will guide users through the whole process without requiring additional documentation. Great!

<WizardHeader
title={__('Review hosts')}
description={__(
'The remediation will be applied to the current host by default. Here you can select additional hosts which fail the same rule.'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'The remediation will be applied to the current host by default. Here you can select additional hosts which fail the same rule.'
'By default, remediation is applied to the current host. Optionally, remediate any additional hosts that fail the rule.'

Copy link
Member

Choose a reason for hiding this comment

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

  • Starting the sentences with "By default" and "Optionally" immediately sets the context for the user.
  • s/will be applied/is applied/ -- for simplification (easier to read)
  • s/Here// -- redundant
  • s/select hosts/remediate hosts/ -- for clarity (users can see that they are expected to select something; explaining what they achieve by selecting is more helpful)

<WizardHeader
title={__('Select remediation method')}
description={__(
'Choose whether to run a remote job or show the snippet for manual remediation.'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'Choose whether to run a remote job or show the snippet for manual remediation.'
'You can remediate by running a remote job or you can display a snippet for manual remediation.'
  • s/Choose whether/You can/ -- users can see that they are choosing from options so I believe this is an opportunity to simplify the wording.

const description =
method === 'manual'
? __(
'Please review the remediation snippet and apply to the host manually.'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'Please review the remediation snippet and apply to the host manually.'
'Review the remediation snippet and apply it to the host manually.'

'Please review the remediation snippet and apply to the host manually.'
)
: __(
'Please review the remediation snippet that will be applied to selected host(s).'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'Please review the remediation snippet that will be applied to selected host(s).'
'Review the remediation snippet that will be applied to selected host(s).'

{method === 'manual' ? null : (
<p>
{__(
'You can tick the checkbox below to reboot the system(s) automatically after the remediation is applied.'
Copy link
Member

Choose a reason for hiding this comment

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

You probably don't need this. The checkbox that says "Reboot the system(s)" is self-explanatory.

Removing this line from the info box might actually help because this part of the wizard is a bit too complex already; simplifying it by dropping the line would help.

);

const rebootAlertTitle = isRebootRequired()
? __('A reboot is required.')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
? __('A reboot is required.')
? __('A reboot is required after applying remediation .')


const rebootAlertTitle = isRebootRequired()
? __('A reboot is required.')
: __('A reboot might be needed.');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
: __('A reboot might be needed.');
: __('A reboot might be required after applying remediation.');

@ofedoren
Copy link
Member Author

Thanks, @asteflova, all the suggested changes makes the whole wizard more mature :) Applied all the suggestions.

@adamruzicka adamruzicka merged commit 85d4141 into theforeman:master Apr 26, 2024
24 of 26 checks passed
@adamruzicka
Copy link
Contributor

Thank you @ofedoren , @MariaAga, @lhellebr & @asteflova !

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.

5 participants