Add check for iptables rule to keepalived-monitor #70
Conversation
openshift-ci-robot
added
the
approved
openshift-ci-robot
added
the
do-not-merge/hold
pkg/monitor/dynkeepalived.go
Outdated
| if err != nil { | ||
| log.Error("Failed to check for haproxy firewall rule") | ||
| } else { | ||
| filePath := "/var/run/keepalived/iptables-rule-exists" |
There was a problem hiding this comment.
I guess it will be better to set filename only once and not every loop iteration
There was a problem hiding this comment.
IIRC, Keepalived monitor runs also in worker nodes, I guess we shouldn't run this check for worker nodes.
Maybe I'm missing something here, but it seems to me that haproxy-monitor is the right place to add this logic to.
There was a problem hiding this comment.
haproxy-monitor doesn't share its /var/run mount with keepalived. I would also argue that haproxy doesn't care about this file. It's consumed by keepalived, so it should be done in the keepalived monitor.
There was a problem hiding this comment.
Oh, and I moved the filename to a const at the top of the file. Good suggestion.
pkg/monitor/dynkeepalived.go
Outdated
| } | ||
| } | ||
| } else { | ||
| _, err := os.Stat(filePath) |
There was a problem hiding this comment.
Can't we just call to os.remove and consider err=nil and err = file not exist as OK?
There was a problem hiding this comment.
Probably, but once I DRY the file existence logic it's one more if statement versus a second clause in another. Plus this way the logic for both creation and removal is the same.
Addressing some review comments on openshift#70. No functional changes, just some grammar/naming/efficiency improvements.
cybertron
force-pushed
the
keepalived-ipt-check
branch
from
15f8f27
to
793b819
Compare
openshift-ci-robot
added
the
needs-rebase
Addressing some review comments on openshift#70. No functional changes, just some grammar/naming/efficiency improvements.
cybertron
force-pushed
the
keepalived-ipt-check
branch
from
793b819
to
3979ad7
Compare
openshift-ci-robot
removed
the
needs-rebase
|
/retest |
Addressing some review comments on openshift#70. No functional changes, just some grammar/naming/efficiency improvements.
cybertron
force-pushed
the
keepalived-ipt-check
branch
from
3979ad7
to
455b0b8
Compare
|
/retest |
|
/lgtm |
The keepalived container is missing the shim script that allows us to call the host iptables. However, we can do the check in the monitor container and just use a file to indicate whether the rule is present.
A PREROUTING rule does not apply to traffic originating from the same host, and as a result our redirect doesn't apply when the node holding the API VIP attempts to contact it. This adds an OUTPUT rule to handle that case. The only difference is that it goes to the OUTPUT chain instead of PREROUTING, and a "-o lo" param needs to be added to the rule spec.
Addressing some review comments on openshift#70. No functional changes, just some grammar/naming/efficiency improvements.
We need these fields to avoid hard-coding ports in the keepalived healthchecks.
We are now templating scripts, which means we need to keep their executable bit set when rendering them. This will ensure the permissions set on the template are maintained for the rendered version.
cybertron
force-pushed
the
keepalived-ipt-check
branch
from
455b0b8
to
510b82e
Compare
|
/retest packet failure |
|
/retest |
pkg/monitor/iptables.go
Outdated
| return false, err | ||
| } | ||
|
|
||
| ruleSpec, err := getHAProxyRuleSpec(apiVip, apiPort, lbPort, false) |
There was a problem hiding this comment.
The magic param at the end doesn't really convey what it is for. One would have to look at the function definition to know, and even then it's not totally clear. Can we change it to maybe add a type for it?
cybertron
force-pushed
the
keepalived-ipt-check
branch
from
dad4d9e
to
c422795
Compare
|
/retest I'm told metal-ipi is working again... |
|
/hold cancel The reasons for the original hold have long since been addressed. |
openshift-ci-robot
removed
the
do-not-merge/hold
Fix spaces vs tabs on constant section
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bcrochet, celebdor, cybertron The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
In order to have keepalived use the loadbalanced api endpoint, we need to know whether the iptables rule to redirect traffic to haproxy is present. Since the keepalived container doesn't have the necessary bits to work with iptables itself, we can instead do it in the monitor container and just use a file to indicate whether the rule is present. This also allows us to reuse the haproxy-monitor code for inspecting iptables, which means it should be less likely to get out of sync. This backports openshift#70 to release-4.5.
In order to have keepalived use the loadbalanced api endpoint, we need to know whether the iptables rule to redirect traffic to haproxy is present. Since the keepalived container doesn't have the necessary bits to work with iptables itself, we can instead do it in the monitor container and just use a file to indicate whether the rule is present. This also allows us to reuse the haproxy-monitor code for inspecting iptables, which means it should be less likely to get out of sync.