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

Refactor URI overrides #10051

Merged
merged 26 commits into from
Oct 10, 2023
Merged

Refactor URI overrides #10051

merged 26 commits into from
Oct 10, 2023

Conversation

Shadowghost
Copy link
Contributor

@Shadowghost Shadowghost commented Jul 28, 2023

Refactor how URI overrides are loaded and created. It should now respect JELLYFIN_PublishedServerUrl again.
The override set via JELLYFIN_PublishedServerUrl supercedes any other override set via the UI or config file (and overrides them).

Changes

  • Fix GetAllBindInterfaces -> fixes Kestrel binding on startup
  • Do not exclude interfaces from unicast binding if the interface is not multicast capable
  • Properly handle multicast binding for Linux, Windows and macOS
  • Exlcude localhost manually from multicast on macOS due to MulticastInterface set to loopback index does not work on OSX dotnet/runtime#24340
  • Refactor URI override handling
  • Respect JELLYFIN_PublishedServerUrl
  • Introduce new class PublishedServerUriOverride
  • Subnet parser now properly creates the catchall subnets if 0.0.0.0 and ::0 are input without subnet mask
  • Cleanup NetworkManager logging and only show network information on startup (on network change printing is now debug-only)

Issues

@Shadowghost Shadowghost force-pushed the networking-fixes branch 2 times, most recently from 7ca6845 to 01dcf34 Compare July 28, 2023 11:42
@satmandu
Copy link

Is there a good way to generate a docker image from this PR for testing?

I don't have a build env setup to build Jellyfin, and we need to test it with Docker anyways...

@Shadowghost
Copy link
Contributor Author

I don't think so. But you can test it without docker too (I'll do it myself if I find the time...)

@PrplHaz4
Copy link
Contributor

Is there a good way to generate a docker image from this PR for testing?

I don't have a build env setup to build Jellyfin, and we need to test it with Docker anyways...

These instructions served me well in the past to test without standing up the whole jf toolchain:
https://jellyfin.org/docs/general/contributing/development#building-and-testing-inside-a-docker-container

@satmandu
Copy link

satmandu commented Aug 8, 2023

This PR fixes #10005 for me.

@Bond-009
Copy link
Member

Failing test on windows

[xUnit.net 00:00:00.57]     Jellyfin.Networking.Tests.NetworkExtensionsTests.TryParse_ValidHostStrings_True(address: "www.google.co.uk") [FAIL]
  Failed Jellyfin.Networking.Tests.NetworkExtensionsTests.TryParse_ValidHostStrings_True(address: "www.google.co.uk") [149 ms]
  Error Message:
   Assert.True() Failure
Expected: True
Actual:   False
  Stack Trace:
     at Jellyfin.Networking.Tests.NetworkExtensionsTests.TryParse_ValidHostStrings_True(String address) in D:\a\1\s\tests\Jellyfin.Networking.Tests\NetworkExtensionsTests.cs:line 30
   at InvokeStub_NetworkExtensionsTests.TryParse_ValidHostStrings_True(Object, Object, IntPtr*)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
Results File: D:\a\_temp\VssAdministrator_fv-az644-602_2023-08-15_01_15_02.trx

Failed!  - Failed:     1, Passed:   122, Skipped:     0, Total:   123, Duration: 1 s - Jellyfin.Networking.Tests.dll (net7.0)

@Bond-009 Bond-009 added this to In progress in Release 10.9.z via automation Aug 15, 2023
@Shadowghost
Copy link
Contributor Author

Failing tests are caused by a DNS resolution error when resolving www.google.co.uk. Not sure how to handle this, they are running fine locally.

@cvium
Copy link
Member

cvium commented Aug 15, 2023

Failing tests are caused by a DNS resolution error when resolving www.google.co.uk. Not sure how to handle this, they are running fine locally.

We shouldn't have non-integration test making external calls

@Shadowghost
Copy link
Contributor Author

Shadowghost commented Aug 15, 2023

The difference between this PR and earlier is that we're now explicitly calling Dns.GetHostAddresses() with IPv4 or IPv6 parameter instead of using the generic overload. According to the .NET docs this seems to be unnecessary though, so I can revert it and the test will pass again.
I'd revert the change anyway but should I remove the test case too?

@satmandu
Copy link

The refactor is working fine for me.

@satmandu
Copy link

@Shadowghost Any luck on getting this merged?

@Shadowghost
Copy link
Contributor Author

I still want to fix a bug concerning multicast/broadcast binding on Windows. But it is a bit annoying to debug and I don't have that much time right now...

@satmandu
Copy link

@Shadowghost Any chance of leaving that Windows issue to a later PR, since this already fixes two issues? (I'm currently building your branch rebased against master, but I would love to not have to do that.)

@satmandu
Copy link

satmandu commented Oct 6, 2023

Thanks for rebasing! I don't know what happened on my end, but the rebased version of course works well.

@Shadowghost
Copy link
Contributor Author

@satmandu @cvium bugs should be fixed. I tried a lot of combinations but apparently Windows and Linux need different handling of broadcast and multicast if you don't want to bind on all interfaces...

Emby.Server.Implementations/Udp/UdpServer.cs Outdated Show resolved Hide resolved
Copy link
Member

@crobibero crobibero left a comment

Choose a reason for hiding this comment

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

works for me on macOS

@Bond-009 Bond-009 merged commit dc27d8f into jellyfin:master Oct 10, 2023
16 of 18 checks passed
Release 10.9.z automation moved this from In progress to Done Oct 10, 2023
@Shadowghost Shadowghost deleted the networking-fixes branch May 17, 2024 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
7 participants