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.

Sign up for GitHub

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

Jump to bottom

Don't operate firewall when system role is Common Criteria #13625

Merged
merged 1 commit into from Nov 11, 2021
Merged

Don't operate firewall when system role is Common Criteria #13625

merged 1 commit into from Nov 11, 2021

Conversation

Amrysliu
Copy link
Contributor

@Amrysliu Amrysliu commented Nov 8, 2021 •
edited

When system role is Common Criteria, the firewall is not installed in
the system by default. Then it will report Unit file firewalld.service does not exist when disabling it in setup_multimachine.pm.

@Amrysliu Amrysliu marked this pull request as draft November 8, 2021 05:28
@Amrysliu Amrysliu force-pushed the fix_netfilter_in_s390x branch 3 times, most recently from 205a315 to 2884e44 Compare November 9, 2021 11:58
@Amrysliu Amrysliu marked this pull request as ready for review November 9, 2021 12:01
mdoucha
mdoucha requested changes Nov 9, 2021
View reviewed changes
Copy link
Contributor

@mdoucha mdoucha left a comment

Choose a reason for hiding this comment

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

It might be better to check for firewall presence using systemctl list-unit-files --all $firewall* instead of hardcoding SYSTEM_ROLE condition. Note that the asterisk at the end is required, otherwise it won't find anything.

@Amrysliu
Copy link
Contributor Author

Amrysliu commented Nov 9, 2021

It might be better to check for firewall presence using systemctl list-unit-files --all $firewall* instead of hardcoding SYSTEM_ROLE condition. Note that the asterisk at the end is required, otherwise it won't find anything.

Thanks. Modified.

@mdoucha
Copy link
Contributor

mdoucha commented Nov 9, 2021

  1. It'd be better to create a reusable library function for this
  2. You need to pass the contents of $self->firewall, not the plain string "firewall".

@Amrysliu
Copy link
Contributor Author

  1. It'd be better to create a reusable library function for this
  2. You need to pass the contents of $self->firewall, not the plain string "firewall".

Good suggestions. Thanks.

@mdoucha
Copy link
Contributor

mdoucha commented Nov 10, 2021

The library function should go into lib/Utils/Systemd.pm

@Amrysliu
Don't operate firewall when system role is Common Criteria
c6c9a96 
When system role is Common Criteria, the firewall is not installed in
the system by default. Then it will report `Unit file firewalld.service
does not exist` when disabling it in setup_multimachine.pm.

Related: https://progress.opensuse.org/issues/102116
@Amrysliu Amrysliu force-pushed the fix_netfilter_in_s390x branch 2 times, most recently from fcacba7 to c6c9a96 Compare November 10, 2021 09:46
mdoucha
mdoucha approved these changes Nov 10, 2021
View reviewed changes
Copy link
Contributor

@mdoucha mdoucha left a comment

Choose a reason for hiding this comment

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

LGTM

@Amrysliu
Copy link
Contributor Author

LGTM

Thanks for your help and reviewing.

rfan1
rfan1 approved these changes Nov 11, 2021
View reviewed changes
Copy link
Contributor

@rfan1 rfan1 left a comment

Choose a reason for hiding this comment

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

LGTM

tinawang123
tinawang123 approved these changes Nov 11, 2021
View reviewed changes
Copy link
Contributor

@tinawang123 tinawang123 left a comment

Choose a reason for hiding this comment

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

LGTM

@Amrysliu Amrysliu merged commit 976c0d2 into os-autoinst:master Nov 11, 2021
@Amrysliu Amrysliu deleted the fix_netfilter_in_s390x branch November 11, 2021 05:59
lilyeyes
lilyeyes reviewed Nov 11, 2021
View reviewed changes
Copy link
Contributor

@lilyeyes lilyeyes left a comment

Choose a reason for hiding this comment

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

LGTM

@jouyingbin
Copy link
Contributor

LGTM

jouyingbin
jouyingbin reviewed Nov 11, 2021
View reviewed changes
Copy link
Contributor

@jouyingbin jouyingbin left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jouyingbin jouyingbin jouyingbin left review comments

@lilyeyes lilyeyes lilyeyes left review comments

@tinawang123 tinawang123 tinawang123 approved these changes

@mdoucha mdoucha mdoucha approved these changes

@rfan1 rfan1 rfan1 approved these changes

@grisu48 grisu48 Awaiting requested review from grisu48

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@Amrysliu @mdoucha @jouyingbin @lilyeyes @tinawang123 @rfan1