Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ipv6 and ipv4 default usage #882

Conversation

alaaeddineelamri
Copy link
Contributor

@alaaeddineelamri alaaeddineelamri commented Jul 5, 2021

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

  • Short description and the purpose of this PR is present above this paragraph
  • Your name is present in the AUTHORS file (optional)

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
  • PR name is meaningful
  • Purpose of the PR is understood
  • Separate commit for this PR in the CHANGELOG.md, PR number referenced is same
  • Commit descriptions are understandable and well formatted
Source code quality
  • Source code changes are understandable
  • Variable and function names are meaningful
  • Code comments are correct (logically and spelling)
  • Required documentation changes are present and part of the PR
  • bareos-check-sources --since-merge does not report any problems
  • git status should not report modifications in the source tree after building and testing
Tests
  • Decision taken that a system- or unittest is required (if not, then remove this paragraph)
  • The decision towards a systemtest is reasonable compared to a unittest
  • Testname matches exactly what is being tested
  • Output of the test leads quickly to the origin of the fault

@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/ipv6-ipv4-dual-stack-automatic-usage branch from 9de36da to 495b268 Compare July 5, 2021 10:59
@alaaeddineelamri alaaeddineelamri marked this pull request as draft July 14, 2021 09:04
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/ipv6-ipv4-dual-stack-automatic-usage branch 3 times, most recently from b7740a0 to 78d4685 Compare July 19, 2021 08:46
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/ipv6-ipv4-dual-stack-automatic-usage branch from 78d4685 to 46ea9c0 Compare July 21, 2021 21:54
@arogge arogge self-assigned this Jul 22, 2021
Copy link
Member

@arogge arogge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/lib/address_conf.h Outdated Show resolved Hide resolved
core/src/lib/bnet_server_tcp.cc Outdated Show resolved Hide resolved
core/src/lib/res.cc Show resolved Hide resolved
@@ -186,19 +186,134 @@ static bool do_connection_test(std::string path_to_config, TlsPolicy tls_policy)
return true;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't reviewed this file yet.

core/src/lib/address_conf.cc Outdated Show resolved Hide resolved
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/ipv6-ipv4-dual-stack-automatic-usage branch 4 times, most recently from 45f70f4 to 42fa276 Compare July 30, 2021 15:06
Copy link
Member

@pstorz pstorz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found some trailing spaces in the configuration files

@pstorz pstorz force-pushed the dev/alaaeddineelamri/master/ipv6-ipv4-dual-stack-automatic-usage branch from 618db81 to 6fc13d9 Compare August 12, 2021 09:52
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/ipv6-ipv4-dual-stack-automatic-usage branch 4 times, most recently from bb51901 to c1e4bcb Compare August 19, 2021 17:17
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/ipv6-ipv4-dual-stack-automatic-usage branch 2 times, most recently from 62f3b61 to 8a460db Compare September 1, 2021 14:38
@alaaeddineelamri alaaeddineelamri marked this pull request as ready for review September 2, 2021 11:56
@bruno-at-bareos
Copy link
Contributor

bruno-at-bareos commented Sep 2, 2021

Bonus Points :-)

  • Make true also for SD Addresses, FD Addresses
  • keep a way to make Bareos listening on only one protocol (for example ipv6 only)
  • ensure communication are respecting normal behaviour, mean connect first ipv6, failback to ipv4.

For example during 16,17,18 this configuration allow Dir to only listen on ipv6

  DIRAddresses  = { 
     ipv6 = { addr = :: ; port = 9101 } 
 #    ipv4 = { addr = 0.0.0.0; port = 9101 } 
  }

telnet 127.0.0.1 9100 didn't allow connection with 20 this make the director to listen again to both stack.

@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/ipv6-ipv4-dual-stack-automatic-usage branch 2 times, most recently from 41b5abf to a1152a4 Compare September 3, 2021 08:30
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/ipv6-ipv4-dual-stack-automatic-usage branch 3 times, most recently from 67cecf2 to 7d4eeec Compare September 13, 2021 12:20
Copy link
Member

@arogge arogge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@@ -5,6 +5,10 @@ and since Bareos version 20 this project adheres to [Semantic Versioning](https:

## [Unreleased]

### 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]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Copy link
Contributor

@bruno-at-bareos bruno-at-bareos Sep 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/ipv6-ipv4-dual-stack-automatic-usage branch from 81187d5 to b3d039b Compare September 22, 2021 13:59
@arogge arogge requested a review from pstorz September 22, 2021 14:41
@alaaeddineelamri alaaeddineelamri changed the title ipv6 ipv4 dual stack default usage ipv6 and ipv4 default usage Sep 23, 2021
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/ipv6-ipv4-dual-stack-automatic-usage branch from b3d039b to 881bd2b Compare September 23, 2021 08:16
Copy link
Member

@pstorz pstorz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix typo

CHANGELOG.md Outdated Show resolved Hide resolved
@arogge
Copy link
Member

arogge commented Sep 23, 2021

It is probably a good idea to improve the documentation on Dir Address, FD Address and SD Address to explain the new default and how to get IPv4/IPv6 only.

@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/ipv6-ipv4-dual-stack-automatic-usage branch from 881bd2b to c33be63 Compare September 23, 2021 09:03
alaaeddineelamri and others added 21 commits September 23, 2021 14:33
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`
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.
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/ipv6-ipv4-dual-stack-automatic-usage branch from c33be63 to 97622e3 Compare September 23, 2021 12:44
@arogge arogge merged commit 855348e into bareos:master Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants