Skip to content
This repository has been archived by the owner on Jul 12, 2023. It is now read-only.

Add externalIPv4/externalIPv6 configs and API methods for them #27

Merged
merged 4 commits into from
Sep 11, 2020

Conversation

prlanzarin
Copy link
Contributor

@prlanzarin prlanzarin commented Sep 10, 2020

What is the current behavior you want to change?

Currently, externalAddress allows only for a single IP that will be either IPv4 or IPv6. That's not friendly with dual stack IPv4+IPv6 environments.

Fixes Kurento/bugtracker#522

What is the new behavior provided by this change?

This pull request splits the externalAddress config into two new ones

  • externalIPv4
  • externalIPv6

They can be used concurrently and will mangle compatible IPv_X candidates.

externalAddress will still have precedence of these two new options: if it's set, externalIPv4/externalIPv6 will be ignored

From the WebRtcEndpoint.conf.ini explanation:

;; External IPv4 address of the media server.
;;
;; This setting follows the same rationale of the externalAddress setting, but
;; the idea here is to couple it with the externalIPv6 setting to achieve dual
;; stack IPv4+IPv6 candidate mangling.
;;
;; Right now, externalAddress only allows a single IP (which has to be either
;; IPv4 or IPv6), so it's not dual stack friendly. This option will mangle
;; IPv4 candidates with the specified one.
;;
;; For the sake of backwards compatibility, externalAddress has preference over
;; this: when externalAddress is set, this option will be ignored.
;;
;; <externalIPv4> is an IPv4 address.
;;
; externalIPv4=10.20.30.40

;; External IPv6 address of the media server.
;;
;; Refer to the detailing of externalIPv4 which should explain this as well.
;;
;; For the sake of backwards compatibility, externalAddress has preference over
;; this: when externalAddress is set, this option will be ignored.
;;
;; <externalIPv6> is an IPv6 address.
;;
; externalIPv6=2001:0db8:85a3:0000:0000:8a2e:0370:7334

How has this been tested?

  • Compiled on 16.04
  • Manually tested:
    • WebRtcEndpoint.conf.ini#externalIPv4/IPv6: IPv4|IPv6, by observing about:webrtc and chrome:webrtc-internals dumps to verify the generated server-side candidates and see if they match the configured values. Also bumped KMS's logs to see if they right, compatible candidates were being mangled.
    • WebRtcEndpoint#setExternalIPv4/IPv6(IPv4|IPv6), via the JS client, to verify the generated server-side candidates and see if they match the configured values. Also bumped KMS's logs to see if they right, compatible candidates were being mangled.
    • Pending: unit tests

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature / enhancement (non-breaking change which improves the project)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • My change requires a change to the documentation
  • My change requires a change in other repository

Checklist

  • I have read the Contribution Guidelines
  • I have added an explanation of what the changes do and why they should be included
  • I have written new tests for the changes, as applicable, and have successfully run them locally

This setting follows the same rationale of the externalAddress setting, but the idea here is to make it dual stack friendly by splitting it into IPv4 and IPv6 specific configs and mangling compatible candidates with them

For the sake of backwards compatibility, externalAddress has preference over these two. So when it`s set, they will be ignored
@jenkinskurento
Copy link

Hi there, thanks for your Pull Request!

A Kurento member needs to verify that this patch is reasonable to test. In case it is, they should write a comment with the phrase test this please. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by Kurento members will still work. Regular contributors can be whitelisted to skip this step.

@j1elo
Copy link
Member

j1elo commented Sep 11, 2020

Amazing addition! thank you very much. I'm right now adding a couple of easy unit tests, and this will be ready for merging.

@j1elo
Copy link
Member

j1elo commented Sep 11, 2020

I think this should deprecate the externalAddress setting, as users could now configure both externalIPv4 and externalIPv6 to their desired values. What do you think?

…nalIPv6

Add Javadoc/Jsdoc texts for new parameters externalIPv4, externalIPv6.
Edit externalAddress to make all of them homogeneous.
This is now superseded by the combination of externalIPv4 and
externalIPv6.
@prlanzarin
Copy link
Contributor Author

@j1elo Yeah, agreed.

@j1elo j1elo self-assigned this Sep 11, 2020
@j1elo j1elo merged commit ea5813a into Kurento:master Sep 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation about externalAddress
3 participants