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

automation & sequence: fix use of default policies and set Sequence as default sequence policy #5897

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kingthorin
Copy link
Member

@kingthorin kingthorin commented Nov 7, 2024

Overview

Modify automation and sequence to properly build and use a policy from policyDefinition if present in the plan. If neither policy nor policyDefinition are defined fall back to Default Policy or Sequence respectively.

Related Issues

n/a

Checklist

  • Update help
  • Update changelog
  • Run ./gradlew spotlessApply for code formatting
  • Write tests
  • Check code coverage
  • Sign-off commits
  • Squash commits
  • Use a descriptive title

@kingthorin kingthorin force-pushed the seq-policy2 branch 4 times, most recently from 5d954f9 to a4deb50 Compare November 7, 2024 19:39
@kingthorin
Copy link
Member Author

I had to rename one of the UnitTest classes so that Eclipse would properly find it and run it as a JUnit test.

@kingthorin kingthorin marked this pull request as ready for review November 7, 2024 20:03
@kingthorin
Copy link
Member Author

Still needs changelog and help updates I guess

@thc202 thc202 changed the title automation & sequence: Revise scan policy handling & try to use defaults automation & sequence: fix use of default policies and set Sequence as default sequence policy Nov 7, 2024
@kingthorin kingthorin force-pushed the seq-policy2 branch 2 times, most recently from ebd5ac4 to 02430c9 Compare November 7, 2024 22:27
@kingthorin

This comment was marked as resolved.

@kingthorin kingthorin force-pushed the seq-policy2 branch 4 times, most recently from a14df14 to d2a97d4 Compare November 8, 2024 15:28
@kingthorin kingthorin force-pushed the seq-policy2 branch 3 times, most recently from 9850833 to 9f85171 Compare November 8, 2024 15:43
@kingthorin
Copy link
Member Author

Okay I think I'm caught up to all the feedback now 😉

@thc202
Copy link
Member

thc202 commented Nov 9, 2024

There are still two cases that don't yet use the default policy, when the policyDefinition is present but it's an empty object and when not present at all.

I'm fine if this is merged without those two cases but we should fix them before releasing the add-ons.

@kingthorin kingthorin force-pushed the seq-policy2 branch 3 times, most recently from c712373 to 0cfd10f Compare November 10, 2024 22:44
@kingthorin
Copy link
Member Author

kingthorin commented Nov 10, 2024

  1. when the policyDefinition is present but it's an empty object

Handled.

  1. when not present at all.

I'm not exactly sure where to handle this. verifyParameters if both policy and policyDefinition are missing? (For the sequenceActiveScanJob anyway, I didn't check the generic one) ... Edit: or maybe in that method if policy isn't defined set it to Sequence it can then still be overridden by policyDefinition?

@psiinon
Copy link
Member

psiinon commented Nov 14, 2024

Re 2. I think the seq ascan should always default to the Sequence policy we define.
So the order is (I think):

  1. policy
  2. policyDefinition
  3. The seq policy defaul theyve specified by the options
  4. The Sequence policy

@kingthorin
Copy link
Member Author

Agreed, that's the way we're headed with this. The question for my point 2 above is where/how it should be handled within the code 😀

@thc202
Copy link
Member

thc202 commented Nov 14, 2024

#5897 (comment)

It can be done in verifyParameters when policyDefinition is not present. (The policy should not matter for this case.) It's the same handling for both jobs. It might be better to call always the parsePolicyDefinition and keep the null strength handling internally.

Comment on lines 291 to 293
" defaultStrength: \n" + " defaultThreshold: \n" + " rules: ",
"rules: \n",
"defaultStrength:"
Copy link
Member

Choose a reason for hiding this comment

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

I'd say in these cases we should use the defaults. The user is specifying something after all (even if not complete).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@kingthorin
Copy link
Member Author

kingthorin commented Nov 14, 2024

Call parse from verify?

@kingthorin
Copy link
Member Author

kingthorin commented Nov 14, 2024

#5897 (comment)

It can be done in verifyParameters when policyDefinition is not present. (The policy should not matter for this case.) It's the same handling for both jobs. It might be better to call always the parsePolicyDefinition and keep the null strength handling internally.

Then I believe it's already covered. Both activeScanJob and sequenceActiveScanJob call parse in verifyParameters.

@kingthorin kingthorin force-pushed the seq-policy2 branch 3 times, most recently from 8734388 to 454ec06 Compare November 14, 2024 17:11
@thc202
Copy link
Member

thc202 commented Nov 14, 2024

They need to call it always, currently when not present that method is not called and the null strength is not set.

@kingthorin
Copy link
Member Author

Done

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

Successfully merging this pull request may close these issues.

3 participants