keepalived vip (vrrp) requires 224.0.0.18/32 #11327

pecameron · 2016-10-11T18:21:39Z

Modified the ipf pod startup to check for iptables rule to allow 224.0.0.18 and if missing add it. By default the rule is added to the head of the INPUT chain. This can be overridden by OPENSHIFT_HA_IPTABLES_CHAIN environment variable in the ipf dc.

Existing deployments do not have OPENSHIFT_HA_IPTABLES_CHAIN and no changes will
be made to the iptables. It is assumed that they are configured
properly. New deployments will have OPENSHIFT_HA_IPTABLES_CHAIN and as long as it
is not empty ("") the rule for 224.0.0.18 will be inserted if not
present.

Added --iptables-chain="" option to oadm ipfailover command to pass
the desired iptables chain. Default is INPUT. When string is ""
no changes are made to iptables.

When keepalived traffic over 224.0.0.18 is blocked multiple nodes
will report as master. This change checks for an iptables rule and
adds it if missing.

Resolves: 1381632
See: openshift/openshift-docs PR 3051

Signed-off-by: Phil Cameron pcameron@redhat.com

pecameron · 2016-10-11T18:25:34Z

@knobunc @rajatchopra @ramr - this change makes the ipf pod create a rule for 224.0.0.18 if none exists.
PTAL

stevekuznetsov · 2016-10-11T18:49:38Z

images/ipfailover/keepalived/lib/failover-functions.sh

@@ -46,6 +46,14 @@ function setup_failover() {
    echo "ERROR: Module ip_vs is NOT available."
  fi

+  echo "  - Check for iptables rule for keepalived multicast (224.0.0.18) ..."
+  iptables -S | grep 224.0.0.18 > /dev/null
+  if [ $? -eq 1 ]; then


Prefer explicit result check:

if ! iptables -S | grep -q "224.0.0.18"; then

Might be better to explicitly check if there is an input rule matching that multicast address - iptables has a
check option with -C and change as @stevekuznetsov mentioned. Something like:

if ! iptables -C INPUT -d 224.0.0.18/32 -j ACCEPT > /dev/null ; then ... iptables -I INPUT 1 -d 224.0.0.18/32 -j ACCEPT > /dev/null fi

I made Steve's change. Ram, I have found the reference in OS_FIREWALL_ALLOW as well.That is why I just look for the address. Also, an admin can put the rule anywhere it will work.

pecameron · 2016-10-12T12:42:15Z

@stevekuznetsov @ramr @rajatchopra @knobunc I made Steve's change. PTAL

knobunc

I also want to see this documented so we don't surprise people.

knobunc · 2016-10-12T14:51:04Z

images/ipfailover/keepalived/lib/failover-functions.sh

@@ -46,6 +46,13 @@ function setup_failover() {
    echo "ERROR: Module ip_vs is NOT available."
  fi

+  echo "  - Check for iptables rule for keepalived multicast (224.0.0.18) ..."


Can we please make this behavior configurable by an ENV? Default it to enabled.

knobunc · 2016-10-12T14:52:02Z

images/ipfailover/keepalived/lib/failover-functions.sh

+  if ! iptables -S | grep 224.0.0.18 > /dev/null ; then
+    # Add the rule to the beginning of the INPUT chain.
+    echo "  - adding iptables rule to access 224.0.0.18."
+    iptables -I INPUT 1 -d 224.0.0.18/32 -j ACCEPT > /dev/null


Make the chain ENV configurable. Default to INPUT.

pecameron · 2016-10-12T19:34:45Z

@openshift/team-atomicopenshift PTAL

This version makes sure an iptables rule to pass 224.0.0.18 exists. If not a rule is added to the head of chain $OPENSHIFT_HA_CHAIN, if present otherwise chain INPUT.

pecameron · 2016-10-12T19:36:17Z

@stevekuznetsov @knobunc I have made your recommended changes PTAL
How are the change requests resolved?

knobunc

That change looks good. But the other change hasn't been fulfilled... I want a way not to mess with people's chains.

pecameron · 2016-10-12T19:53:51Z

@knobunc I am confused. The new OPENSHIFT_HA_CHAIN variable allows the desired chain to be set in the ipf's dc. What would you like to change?

pecameron · 2016-10-12T19:55:58Z

@knobunc click the "View changes" button at the end of the red x line

knobunc · 2016-10-12T19:58:45Z

@pecameron I want a second knob that I can set to tell the ipfailover not to touch iptables at all.

pecameron · 2016-10-12T21:15:05Z

@knobunc I made the change to not alter existing deployments on upgrade. PTAL

stevekuznetsov · 2016-10-13T12:29:39Z

Looks like this was closed accidentally.

knobunc

LGTM. @ramr PTAL

pecameron · 2016-10-13T13:41:50Z

@stevekuznetsov It was closed by accident. You still have a requested change. PTAL

@knobunc there needs to be a doc change for this. I will write it up and get a doc PR going. Do we need a different bug number?

stevekuznetsov · 2016-10-13T14:06:43Z

I like @ramr idea of a more explicit iptables probe rather than a grep for some address - why do you prefer the grep?

knobunc · 2016-10-13T14:23:02Z

@stevekuznetsov I'd argue for the grep because we want to do this by default (for new ipfailover setups), and the user may have already created a rule in iptables to handle it. Since we don't know how the chains are laid out, it could be in a different chain that they jump to (e.g. OS_FIREWALL_ALLOW). However, I see your argument for a more robust check now that we allow specification of a chain, and allow the feature to be disabled. So I could be swayed. Thoughts?

stevekuznetsov · 2016-10-13T17:00:42Z

I'm always in favor of more restrictive checks, but if we can't control the iptables rules and want to handle the cases where users have custom setups, we can't make the more restrictive check. grep sounds fine to me in that case.

pecameron · 2016-10-13T21:08:02Z

@knobunc @stevekuznetsov @ramr After speaking with Ben we decided to add a new option, --iptables-chain to the oadm ipfailover command. I updated the (origin based) docs and tests as well. PTAL
I am updating the openshift-docs files as well and will add the PR here when it is available.

stevekuznetsov · 2016-10-13T21:17:49Z

images/ipfailover/keepalived/lib/failover-functions.sh

+  # (OPENSHIFT_HA_IPTABLES_CHAIN) make sure the rule to pass keepalived
+  # multicast (224.0.0.18) is in the table.
+  chain="${OPENSHIFT_HA_IPTABLES_CHAIN:-""}"
+  if [ ! -z ${chain} ]; then


Unless this needs POSIX compliance, prefer [[ over [. In ether case, use -n instead of ! -z.

I made the changes

ramr · 2016-10-13T23:04:38Z

pkg/cmd/experimental/ipfailover/ipfailover.go

@@ -95,6 +95,7 @@ func NewCmdIPFailoverConfig(f *clientcmd.Factory, parentName, name string, out,
 	cmd.Flags().BoolVar(&options.Create, "create", options.Create, "Create the configuration if it does not exist.")

 	cmd.Flags().StringVar(&options.VirtualIPs, "virtual-ips", "", "A set of virtual IP ranges and/or addresses that the routers bind and serve on and provide IP failover capability for.")
+	cmd.Flags().StringVar(&options.IptablesChain, "iptables-chain", "INPUT", "Add a rule to this iptables chain to accept 224.0.0.28 multicast packets if no rule exists. When iptables-chain is empty donot change iptables.")


Maybe something like:
"If specified and no matching iptables rules exist, adds a rule to this iptables chain that accepts packets sent to an multicast address 224.0.0.28. Defaults to using the iptables INPUT chain.

Nice wording.

ramr · 2016-10-13T23:06:19Z

Minor corrections but otherwise looks good to me

knobunc · 2016-10-14T14:18:34Z

[test]

knobunc · 2016-10-14T17:23:53Z

"FAILURE: Generated docs up to date, but generated man pages out of date. Please run hack/update-generated-docs.sh"

pecameron · 2016-10-14T18:15:24Z

[test]

marun · 2016-10-14T19:18:06Z

flake #11374 re-[test]

knobunc · 2016-10-14T19:57:30Z

We need to re-merge this after @marun's fix for #11374 gets in.

dcbw · 2016-10-14T22:00:50Z

test/cmd/router.sh

@@ -71,6 +71,7 @@ os::cmd::expect_success_and_text 'oadm ipfailover --virtual-ips="1.2.3.4" --dry-
 os::cmd::expect_success_and_text 'oadm ipfailover --virtual-ips="1.2.3.4" --dry-run -o yaml' 'name: ipfailover'
 os::cmd::expect_success_and_text 'oadm ipfailover --virtual-ips="1.2.3.4" --dry-run -o name' 'deploymentconfig/ipfailover'
 os::cmd::expect_success_and_text 'oadm ipfailover --virtual-ips="1.2.3.4" --dry-run -o yaml' '1.2.3.4'
+os::cmd::expect_success_and_text 'oadm ipfailover --virtual-ips="1.2.3.4" --iptables-chain="MY_CHAIN"--dry-run -o yaml' 'value: MY_CHAIN'


Do we need a space here between MY_CHAIN" and --dry-run ?

Yes, and if this current syntax doesn't fail this test we need to look into why.

@dcbw - Yes it does. Good spot, thanks for looking at this.
I fixed it.

stevekuznetsov · 2016-10-17T12:59:07Z

test/cmd/router.sh

@@ -71,6 +71,7 @@ os::cmd::expect_success_and_text 'oadm ipfailover --virtual-ips="1.2.3.4" --dry-
 os::cmd::expect_success_and_text 'oadm ipfailover --virtual-ips="1.2.3.4" --dry-run -o yaml' 'name: ipfailover'
 os::cmd::expect_success_and_text 'oadm ipfailover --virtual-ips="1.2.3.4" --dry-run -o name' 'deploymentconfig/ipfailover'
 os::cmd::expect_success_and_text 'oadm ipfailover --virtual-ips="1.2.3.4" --dry-run -o yaml' '1.2.3.4'
+os::cmd::expect_success_and_text 'oadm ipfailover --virtual-ips="1.2.3.4" --iptables-chain="MY_CHAIN"--dry-run -o yaml' 'value: MY_CHAIN'


Yes, and if this current syntax doesn't fail this test we need to look into why.

pecameron · 2016-10-17T17:03:57Z

[test]

knobunc · 2016-10-18T18:42:23Z

@stevekuznetsov PTAL.

stevekuznetsov · 2016-10-18T18:54:27Z

@knobunc which part should I be looking at?

stevekuznetsov

Bash-isms LGTM, small questions about output suppression.

stevekuznetsov · 2016-10-18T18:55:50Z

images/ipfailover/keepalived/lib/failover-functions.sh

+  chain="${OPENSHIFT_HA_IPTABLES_CHAIN:-""}"
+  if [[ -n ${chain} ]]; then
+    echo "  - check for iptables rule for keepalived multicast (224.0.0.18) ..."
+    if ! iptables -S | grep 224.0.0.18 > /dev/null ; then


If something were to go wrong with this, you'll get stuff from stderr popping out -- maybe this is intended? If not, > /dev/null 2>&1.

I can go either way with this. I'll make the change.

stevekuznetsov · 2016-10-18T18:56:04Z

images/ipfailover/keepalived/lib/failover-functions.sh

+    if ! iptables -S | grep 224.0.0.18 > /dev/null ; then
+      # Add the rule to the beginning of the chain.
+      echo "  - adding iptables rule to $chain to access 224.0.0.18."
+      iptables -I ${chain} 1 -d 224.0.0.18/32 -j ACCEPT > /dev/null


Why do we want to suppress the output of this command?

The echo above notes that we are doing it. I'll display the output (if any)

knobunc · 2016-10-18T18:58:49Z

@stevekuznetsov just asking you to re-review and flip your NAK to an ACK if appropriate. Thanks!

Modified the ipf pod startup to check for iptables rule to allow 224.0.0.18 and if missing add it. By default the rule is added to the head of the INPUT chain. This can be overridden by OPENSHIFT_HA_CHAIN environment variable in the ipf dc. Existing deployments do not have OPENSHIFT_HA_CHAIN and no changes will be made to the iptables. It is assumed that they are configured properly. New deployments will have OPENSHIFT_HA_CHAIN and as long as it is not empty ("") the rule for 224.0.0.18 will be inserted if not present. Added --iptables-chain="" option to oadm ipfailover command to pass the desired iptables chain. Default is INPUT. When string is "" no changes are made to iptables. When keepalived traffic over 224.0.0.18 is blocked multiple nodes will report as master. This change checks for an iptables rule and adds it if missing. Documentation changes are in openshift-docs (PR ....) Resolves: 1381632 Signed-off-by: Phil Cameron <pcameron@redhat.com>

pecameron · 2016-10-18T19:58:55Z

@stevekuznetsov I made your suggested changes.

openshift-bot · 2016-10-18T20:02:39Z

Evaluated for origin test up to b5d81e1

stevekuznetsov · 2016-10-18T20:09:06Z

LGTM

knobunc · 2016-10-18T20:18:17Z

LGTM [merge]

openshift-bot · 2016-10-18T22:32:37Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10206/) (Base Commit: a941850)

knobunc · 2016-10-19T18:19:51Z

Flake #11442 [merge]

pecameron · 2016-10-20T17:21:29Z

@knobunc you merged this yesterday and it seems to be stuck.

stevekuznetsov · 2016-10-20T17:51:54Z

@pecameron as is written in the comment above, you are in the merge queue and will just have to wait your turn

pecameron · 2016-10-20T17:58:55Z

@stevekuznetsov Thanks for the reply. I didn't know how long this should take and after 23 hours I thought it might be hung.

openshift-bot · 2016-10-21T03:21:43Z

Evaluated for origin merge up to b5d81e1

openshift-bot · 2016-10-21T04:53:42Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10373/) (Base Commit: 4a1aec0) (Image: devenv-rhel7_5212)

stevekuznetsov suggested changes Oct 11, 2016

View reviewed changes

pecameron force-pushed the bz1381632 branch from 35bf516 to 8f7346d Compare October 12, 2016 12:41

knobunc requested changes Oct 12, 2016

View reviewed changes

pecameron force-pushed the bz1381632 branch 2 times, most recently from 773f68a to 3ee8514 Compare October 12, 2016 19:32

knobunc requested changes Oct 12, 2016

View reviewed changes

pecameron force-pushed the bz1381632 branch from 3ee8514 to 730c455 Compare October 12, 2016 21:11

pecameron closed this Oct 12, 2016

stevekuznetsov reopened this Oct 13, 2016

knobunc approved these changes Oct 13, 2016

View reviewed changes

stevekuznetsov approved these changes Oct 13, 2016

View reviewed changes

pecameron force-pushed the bz1381632 branch from 730c455 to 67300b8 Compare October 13, 2016 21:00

stevekuznetsov suggested changes Oct 13, 2016

View reviewed changes

ramr reviewed Oct 13, 2016

View reviewed changes

pecameron force-pushed the bz1381632 branch from 67300b8 to dcd35ec Compare October 14, 2016 13:36

pecameron force-pushed the bz1381632 branch from bfa5a15 to e0c4c18 Compare October 14, 2016 18:14

dcbw added the component/networking label Oct 14, 2016

dcbw reviewed Oct 14, 2016

View reviewed changes

stevekuznetsov suggested changes Oct 17, 2016

View reviewed changes

pecameron force-pushed the bz1381632 branch from e0c4c18 to eb48a1c Compare October 17, 2016 13:22

stevekuznetsov approved these changes Oct 18, 2016

View reviewed changes

pecameron force-pushed the bz1381632 branch from eb48a1c to b5d81e1 Compare October 18, 2016 19:57

openshift-bot merged commit f60c1df into openshift:master Oct 21, 2016

pecameron deleted the bz1381632 branch October 21, 2016 13:58

keepalived vip (vrrp) requires 224.0.0.18/32 #11327

keepalived vip (vrrp) requires 224.0.0.18/32 #11327

Conversation

pecameron commented Oct 11, 2016 • edited Loading

pecameron commented Oct 11, 2016

Choose a reason for hiding this comment

ramr Oct 11, 2016 • edited by stevekuznetsov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pecameron commented Oct 12, 2016

knobunc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pecameron commented Oct 12, 2016

pecameron commented Oct 12, 2016

knobunc left a comment

Choose a reason for hiding this comment

pecameron commented Oct 12, 2016

pecameron commented Oct 12, 2016

knobunc commented Oct 12, 2016

pecameron commented Oct 12, 2016

stevekuznetsov commented Oct 13, 2016

knobunc left a comment

Choose a reason for hiding this comment

pecameron commented Oct 13, 2016

stevekuznetsov commented Oct 13, 2016

knobunc commented Oct 13, 2016

stevekuznetsov commented Oct 13, 2016

pecameron commented Oct 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramr commented Oct 13, 2016

knobunc commented Oct 14, 2016

knobunc commented Oct 14, 2016

pecameron commented Oct 14, 2016

marun commented Oct 14, 2016

knobunc commented Oct 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pecameron commented Oct 17, 2016

knobunc commented Oct 18, 2016

stevekuznetsov commented Oct 18, 2016

stevekuznetsov left a comment

Choose a reason for hiding this comment

stevekuznetsov Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knobunc commented Oct 18, 2016

pecameron commented Oct 18, 2016

openshift-bot commented Oct 18, 2016

stevekuznetsov commented Oct 18, 2016

knobunc commented Oct 18, 2016

openshift-bot commented Oct 18, 2016

knobunc commented Oct 19, 2016

pecameron commented Oct 20, 2016

stevekuznetsov commented Oct 20, 2016

pecameron commented Oct 20, 2016

openshift-bot commented Oct 21, 2016

openshift-bot commented Oct 21, 2016 • edited Loading

pecameron commented Oct 11, 2016 •

edited

Loading

ramr Oct 11, 2016 •

edited by stevekuznetsov

Loading

stevekuznetsov Oct 18, 2016 •

edited

Loading

openshift-bot commented Oct 21, 2016 •

edited

Loading