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

Update IPTables save method #1003

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

Update IPTables save method #1003

wants to merge 2 commits into from

Conversation

frogfather
Copy link
Contributor

Service iptables save doesn't work with Centos7

Copy link
Member

@grkvlt grkvlt left a comment

Choose a reason for hiding this comment

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

Add check that command exists. Also look at IptablesCommandsTest and try to add a useful test for this?

@@ -130,8 +130,8 @@ public static String firewalldServiceIsActive() {
*
*/
public static String saveIptablesRules() {
return alternatives(sudo("service iptables save"),
chain(installPackage("iptables-persistent"), sudo("/etc/init.d/iptables-persistent save")));
return alternatives("if [ ${UID} -eq 0 ] ; then iptables–save > /etc/sysconfig/iptables ; else sudo iptables-save | sudo tee /etc/sysconfig/iptables ; fi",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe surround this with BashCommands.ifExecutableElse1("iptables-save", ...) to ensure the command exists

@grkvlt
Copy link
Member

grkvlt commented Oct 2, 2018

Service iptables save doesn't work with Centos7

That is, doesn't work with the CentOS 7 AMI we use on EC2, which has a cut-down service command only supporting the standard start, stop, restart and status command verbs.

@grkvlt
Copy link
Member

grkvlt commented Oct 2, 2018

Thanks @frogfather LGTM, will merge assuming tests pass

@grkvlt
Copy link
Member

grkvlt commented Oct 2, 2018

I think Jenkins is failing due to permissions, maybe on the ASF side?

@aledsage
Copy link
Contributor

aledsage commented Oct 9, 2018

LGTM; happy for this to be merged (once confusion with the identical-looking #1006 is cleared up).

return alternatives(sudo("service iptables save"),
chain(installPackage("iptables-persistent"), sudo("/etc/init.d/iptables-persistent save")));
return alternatives(
ifExecutableElse1("iptables–save", "if [ ${UID} -eq 0 ] ; then iptables–save > /etc/sysconfig/iptables ; else sudo iptables-save | sudo tee /etc/sysconfig/iptables ; fi"),
Copy link
Contributor

Choose a reason for hiding this comment

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

this removes service iptables save altogether, is that intended? a comment to that effect would be useful,
or if it is still useful sometimes (older OS's?) then keep it in the alternatives list

also worth checking whether sudoNew("iptables-save > /etc/sysconfig/iptables") works; the way it does echo COMMAND | sudo bash should support redirect for non-root users; if not maybe refactor to add a sudoRedirect method in BashCommands capturing the conditional tee so that you could write ifExecutableElse1("iptables-save", sudoRedirect("iptable-save", "/etc/sysconfig/iptables"))

@ahgittin
Copy link
Contributor

as @aledsage this is largely included in #1006 . @frogfather your call whether there is value in the recent comments here and if so update this PR, make sure to git merge master in to this branch, or if not just close this.

@nakomis
Copy link
Contributor

nakomis commented Nov 26, 2019

@frogfather Can you take a look at this please to see if it is still relevant, and address the comments above if appropriate

Thanks

@frogfather
Copy link
Contributor Author

frogfather commented Nov 26, 2019 via email

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