Merged
Conversation
9de36da to
495b268
Compare
b7740a0 to
78d4685
Compare
78d4685 to
46ea9c0
Compare
arogge
reviewed
Jul 23, 2021
Member
arogge
left a comment
There was a problem hiding this comment.
I see progress, but I also think it is still some way to go.
It looks like passing family=0 to AddAddress() should already do the trick, but it doesn't.
The change you did to the configuration parser code seems to target the wrong directive (i.e. Dir Port instead of Dir Address).
I'm really unhappy with the overall state of the address setup code. It feels way too complicated and it seems to be more or less impossible to improve at this point.
core/src/tests/configs/dual_stacking/default_dir_values/bareos-dir.d/director/bareos-dir.conf
Outdated
Show resolved
Hide resolved
core/src/tests/configs/dual_stacking/default_dir_values/bareos-dir.d/catalog/MyCatalog.conf
Outdated
Show resolved
Hide resolved
| @@ -186,19 +186,134 @@ static bool do_connection_test(std::string path_to_config, TlsPolicy tls_policy) | |||
| return true; | |||
| } | |||
|
|
|||
Member
There was a problem hiding this comment.
I haven't reviewed this file yet.
45f70f4 to
42fa276
Compare
pstorz
requested changes
Aug 10, 2021
Member
pstorz
left a comment
There was a problem hiding this comment.
Found some trailing spaces in the configuration files
core/src/tests/configs/dual_stacking/dir_port_set/bareos-dir.d/director/bareos-dir.conf
Outdated
Show resolved
Hide resolved
core/src/tests/configs/dual_stacking/dir_v4address_set/bareos-dir.d/director/bareos-dir.conf
Outdated
Show resolved
Hide resolved
core/src/tests/configs/dual_stacking/dir_v4address_set/bareos-dir.d/director/bareos-dir.conf
Outdated
Show resolved
Hide resolved
618db81 to
6fc13d9
Compare
bb51901 to
c1e4bcb
Compare
62f3b61 to
8a460db
Compare
Contributor
|
Bonus Points :-)
For example during 16,17,18 this configuration allow Dir to only listen on ipv6 telnet 127.0.0.1 9100 didn't allow connection with 20 this make the director to listen again to both stack. |
41b5abf to
a1152a4
Compare
67cecf2 to
7d4eeec
Compare
arogge
reviewed
Sep 22, 2021
Member
arogge
left a comment
There was a problem hiding this comment.
I tried a few things with the configuration, and realized that if you set Dir Address to a valid IPv6 listen address (i.e. Dir Address = :: or Dir Address = ::1), the Director will listen on IPv4 0.0.0.0 but not on IPv6.
Any idea what is going on in that case?
CHANGELOG.md
Outdated
|
|
||
| ### Breaking Changes | ||
|
|
||
| - when setting an IPv6 address using the [DIR|SD|FD] Addresses directive, now bareos only listens on IPv6 only instead of both IPv4 and IPv6 on dual-stack. If you were using the [DIR|SD|FD] Addresses directive to create a dual-stack socket that would listen on both IPv6 AND IPv4, it won't work that way anymore. You should now also explicitely specify the IPv4 address in the directive [PR #882] |
Member
There was a problem hiding this comment.
Suggested change
| - when setting an IPv6 address using the [DIR|SD|FD] Addresses directive, now bareos only listens on IPv6 only instead of both IPv4 and IPv6 on dual-stack. If you were using the [DIR|SD|FD] Addresses directive to create a dual-stack socket that would listen on both IPv6 AND IPv4, it won't work that way anymore. You should now also explicitely specify the IPv4 address in the directive [PR #882] | |
| - when setting an IPv6 address using the [DIR|SD|FD] Addresses directive, now bareos only listens on IPv6 only instead of both IPv4 and IPv6 on dual-stack. If you were using the [DIR|SD|FD] Addresses directive to create a dual-stack socket that would listen on both IPv6 AND IPv4, it won't work that way anymore. You should now also explicitely specify the IPv4 address in the directive or remove the directive as listening on IPv6 and IPv4 is now the default. [PR #882] |
Contributor
There was a problem hiding this comment.
I hope we are clear, with good sample of configuration in documentation ... I mean we have Dir Address and Dir Addresses
Only ipv6
DIR Address = ::
DIRPort = 9101
could be also declared as
DIRAddresses = {
ipv6 = { addr = :: ; port = 9101 }
}
compared to ipv6 + ipv4 (dual socket)
DIRAddresses = {
ipv6 = { addr = :: ; port = 9101 }
ipv4 = { addr = 0.0.0.0; port = 9101 }
}
new default ?
Member
There was a problem hiding this comment.
the last one is the new default, and the first one didn't work previously (we just fixed it a few minutes ago).
You can also do Dir Address = 0.0.0.0 to get IPv4 only. I hope it is quite obvious the way it is implemented right now.
81187d5 to
b3d039b
Compare
arogge
approved these changes
Sep 22, 2021
b3d039b to
881bd2b
Compare
Member
|
It is probably a good idea to improve the documentation on |
881bd2b to
c33be63
Compare
made sure that IPV6 sockets only listen for V6 when created.
* the tests launche the bareos director * checks for the expected addresses and ports to be correctly set in the director, * and then creates a new socket and tries to bind on the same expected default/set ports, which should fail because the director is supposed to connect on them first.
Added a check for an address before adding Single Port addresses. This was added because in case the `DirAddress` is set BEFORE `DirPort` in the config file, the process would go on and assign an extra IPV6 address, which should not be the case. The `DirAddress` is supposed to be the only address reachable. In this case only the V4 address is setup which overall just changes the listening port.
The function inet_ntop() is available on windows.
Now the full address and port is printed. On windows, the WSAGetLastError() is also printed. Also call InitMsg() in bsock_test_connection_setup so that log messages are printed.
keeping changes in the parsing process, without touching `bnet_server_tcp`
This reverts commit ff6d987.
previously, if you tried to add an IPv6 address using one of the directives Dir Address, FD Address or SD Address, it would listen on 0.0.0.0. This patch sees if the string looks like an IPv6 address and adds it correctly leading to a less surprising behaviour.
c33be63 to
97622e3
Compare
arogge
approved these changes
Sep 24, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When no address or port directive is specified, Bareos listens by default only on IPV4. This pull request tries to make sure Bareos listens on both IPv4 and IPv6 by default. Also when the user only specifies a port, Bareos now listens on that port with both IPV4 and IPV6
Please check
If you have any questions or problems, please give a comment in the PR.
Helpful documentation and best practices
Checklist for the reviewer of the PR (will be processed by the Bareos team)
General
Source code quality
bareos-check-sources --since-mergedoes not report any problemsgit statusshould not report modifications in the source tree after building and testingTests