CP-40754 Update host.https_only field in dbsync based on firewall state #4811
Conversation
ocaml/xapi/xapi_host.ml
Outdated
| let network_state = | ||
| Helpers.call_script !Xapi_globs.firewall_port_config_script [state; "80"] | ||
| in | ||
| Db.Host.set_https_only ~__context ~self ~value:(bool_of_string network_state) |
There was a problem hiding this comment.
This change should not be done here, but in refresh_localhost_info in dbsync_slave.ml, which is a function that runs when xapi starts up. At that point we want to query the state of the firewall and sync the DB.
scripts/plugins/firewall-port
Outdated
| @@ -52,5 +52,13 @@ case "${OP}" in | |||
| exit 1 | |||
| ;; | |||
| esac | |||
|
|
|||
| if [[ -z `iptables -S $CHAIN | grep " $PORT "` ]] | |||
There was a problem hiding this comment.
This query should go as a new operation in the case statement above, e.g. check.
There was a problem hiding this comment.
Should I put the query under each of the case statements?
There was a problem hiding this comment.
No, only under one new case. So the script will have three distinct ops: open, close, check.
There was a problem hiding this comment.
That makes more sense now, have since adjusted it
jameshensmancitrix
force-pushed
the
private/jameshen/host_https_only
branch
from
e341b42
to
54e271b
Compare
scripts/plugins/firewall-port
Outdated
| if [[ -z `iptables -S $CHAIN | grep " $PORT "` ]] | ||
| then | ||
| echo true | ||
| else |
There was a problem hiding this comment.
Indentation needs a fix. Maybe this is part of the design, but I think a more descriptive output would be better unless the output is directly used somewhere else.
There was a problem hiding this comment.
the stdout feeds into OCaml where it is fed into bool_of_string. This tells Xapi what the state of https_only should be.
There was a problem hiding this comment.
I would still use a more descriptive output and use sscanf to parse it on the OCaml side. That is safer than using bool_of_string anyway. Something like: port 80: open which can be easily parsed.
scripts/plugins/firewall-port
Outdated
| else | ||
| echo false | ||
| fi | ||
| ;; | ||
| *) | ||
| echo $"Usage: $0 {open|close} {port} {protocol}" 1>&2 |
jameshensmancitrix
force-pushed
the
private/jameshen/host_https_only
branch
3 times, most recently
from
f5f3602
to
89945a0
Compare
ocaml/xapi/dbsync_slave.ml
Outdated
| (Scanf.bscanf | ||
| (Scanf.Scanning.from_string network_state) | ||
| "Port 80 open: %B" Fun.id | ||
| ) |
There was a problem hiding this comment.
Scanf can fail with an exception. This needs to be caught and raised as internal error. Obviously we expect that the script returns the expected output. You might want to experiment with this in an OCaml shell like utop
There was a problem hiding this comment.
ksscanf takes an additional parameter that is a handler in case the string cannot be parsed:
jameshensmancitrix
force-pushed
the
private/jameshen/host_https_only
branch
from
89945a0
to
b4cbdd4
Compare
…and false if it is closed, this is captured in set_https_only to update the DB based on the tate of the network not the requested setting should there be a failure Signed-off-by: jameshensmancitrix <james.hensman@citrix.com>
jameshensmancitrix
force-pushed
the
private/jameshen/host_https_only
branch
from
b4cbdd4
to
2e1385b
Compare
|
This looks good now. But please merge only after testing. |
|
Am currently testing, I will compare with both network states switched when Xapi is not running |
|
Testing complete, xapi correctly sets https_only on the host on the basis of the port after systemctl restart xapi |
The firewall-port script returns true if port 80 is blocked … and false if it is closed, this is captured in set_https_only to update the DB based on the state of the network, not the requested setting should there be a failure
Signed-off-by: jameshensmancitrix james.hensman@citrix.com