Add PAT/NAT config #455
Add PAT/NAT config #455
Conversation
|
Sorry, i forget to squash my commit... |
|
@nabbar Thanks a lot of your PR! I will be able to deeply review it in the next week.
Don't worry, you can squash it later after the review or when you prefer. |
|
git squash done |
There was a problem hiding this comment.
@nabbar Initial review, overall it looks good. I have some doubts on the changed behaviour (see comments).
We'll need some changes to the integration tests to also test that options. My idea will be:
-
add some new
TestFailover(https://github.com/sorintlab/stolon/blob/master/tests/integration/ha_test.go#L449) flavors to test with advertise address and port different than listen address and port. -
add some new tests to https://github.com/sorintlab/stolon/blob/master/tests/integration/init_test.go to check that when providing wrong options it'll error.
cmd/keeper/cmd/keeper.go
Outdated
|
|
||
| if p.pgListenAddress != "*" { | ||
| lst = []string{"127.0.0.1"} | ||
| } |
There was a problem hiding this comment.
just noticed that listening on localhost is probably not needed anymore by the stolon keeper since we use unix sockets. Perhaps it's useful for people doing local connections (local backups scripts etc...) so we could keep it to avoid possible regression.
There was a problem hiding this comment.
yes i think so, but I wanted to keep as much functionality as possible.
If your are agree, I will remove it :)
cmd/keeper/cmd/keeper.go
Outdated
| if cfg.pgAdvertiseAddress == "" && cfg.pgListenAddress == "" { | ||
| die("--pg-listen-address and/or --pg-advertise-address is required") | ||
| } else if cfg.pgListenAddress == "" { | ||
| cfg.pgListenAddress = "*" |
There was a problem hiding this comment.
I'll prefer the user to always specifiy the listen address and not default to '*' when not specified.
There was a problem hiding this comment.
oki, no problem, I'm quizzing myself for a while about the default values to apply.
cmd/keeper/cmd/keeper.go
Outdated
| cfg.pgPort = cfg.pgAdvertisePort | ||
| } else if cfg.pgAdvertisePort == "" { | ||
| cfg.pgAdvertisePort = cfg.pgPort | ||
| } |
There was a problem hiding this comment.
This looks strange since if pgPort is not specified it'll be set to advertise port and if pgAdvertisePort is not specified it'll be set to pgPort. But pgPort has a default to 5432 so it'll probably never happen.
I'll prefer to keep the original behavior and just set pgAdvertisePort to pgPort when pgAdvertisePort is not specified.
cmd/keeper/cmd/keeper.go
Outdated
| if ip == nil { | ||
| log.Warnf("provided --pgListenAddress %q: is not an ip address but a hostname. This will be advertized to the other components and may have undefined behaviors if resolved differently by other hosts", cfg.pgListenAddress) | ||
| log.Warnf("provided --pg-advertise-address %q: is not an ip address but a hostname. This will be advertized to the other components and may have undefined behaviors if resolved differently by other hosts", cfg.pgAdvertiseAddress) |
There was a problem hiding this comment.
The log can look strange to an user if it hasn't provided pgAdvertiseAddress. Perhaps to could be change to
PGAdvertiseAddress: %q: ... and when setting pgAdvertiseAddress to pgListenAddress logging that we are setting PGAdvertiseAddress to PGListenAddress.
There was a problem hiding this comment.
Yes as following previous comment, I think this must be change to apply correct message.
|
I will do this fix, this WE or next week. |
The advertise address:port will be publish in the keeper information to be used by client, sentinel, proxy, ctl or other keeper. The listening address/port will be used only for postgres instance configuration. Fix : - localhost not need for listenAddress postgres instance - check init flag : listenaddress must be explicit, advertise listen address will use listenaddres if not specified - check init flag : use specified flag for warn/error message - check init flag : don't touch init pgport and just set advertise pg port if not specified
|
@nabbar Is it ready? If you want I can help you to write the tests. |
|
Hello, Sorry, I am little busy this days... Thanks in advance |
|
Hi, Is there any reason this was never finished, it seems to be a valuable addition to stolon? Is there anything i could do to push this? Regards David |
Hello,
First, thanks a lot for your work.
I needed to use a NAT / PAT configuration for postgres instances managed by the keeper. Also, I allowed myself to modify the code a bit to allow these features.
I named the flag as "advertise" which are optional flags.
This advertise flag are added for listening pg address and listening pg port.
The flags are managed as :
I hope this code will be consistent with your idea of the project.
I tried to make as few changes as possible but with the greatest flexibility possible.
Regards,
Nicolas.