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

Fix for Livetv when bind interfaces specified. #5905

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

@BaronGreenback
Copy link
Contributor

@BaronGreenback BaronGreenback commented Apr 24, 2021

Solves: #5897

A valid interface is now used, taking the bind interfaces into account.

Test units added.

@cvium
Copy link
Member

@cvium cvium commented Apr 25, 2021

Fix this instead

public string GetLoopbackHttpApiUrl()

@BaronGreenback BaronGreenback force-pushed the BaronGreenback:TVFix branch from c48ea0c to 5741fa7 Apr 26, 2021
}

return GetLocalApiUrl("127.0.0.1", Uri.UriSchemeHttp, HttpPort);
// Passing an external address cause GetBindInterface to return an externally accessible interface (if possible).

This comment has been minimized.

@cvium

cvium Apr 26, 2021
Member

Why external? It should be whatever local IP it is binding to.

This comment has been minimized.

@BaronGreenback

BaronGreenback Apr 26, 2021
Author Contributor

It's for multi-interface systems. It doesn't have any effect on a single adapter system. (Hence the testing unit)

You mentioned it might be accessed via the client so the default is to use the external facing interface unless the user has specified otherwise.

This comment has been minimized.

@BaronGreenback

BaronGreenback Apr 26, 2021
Author Contributor

The "8.8.8.8" causes GetBindInterface to 'see' the client coming from an external address, so uses the interface that is best to route to that location.

@joshuaboniface joshuaboniface requested review from cvium and jellyfin/server Apr 29, 2021
Co-authored-by: Cody Robibero <cody@robibe.ro>
@Ullmie02 Ullmie02 added this to Active PRs in Current Release via automation May 1, 2021
BaronGreenback and others added 3 commits May 1, 2021
Co-authored-by: David Ullmer <davidullmer@outlook.de>
Co-authored-by: David Ullmer <davidullmer@outlook.de>
Co-authored-by: David Ullmer <davidullmer@outlook.de>
@cvium
Copy link
Member

@cvium cvium commented May 1, 2021

I'm still not entirely convinced this is the correct solution. The name of the function indicates it's solely for loopback. Clients wouldn't (or shouldn't) be using it then

@BaronGreenback
Copy link
Contributor Author

@BaronGreenback BaronGreenback commented May 1, 2021

I'm still not entirely convinced this is the correct solution. The name of the function indicates it's solely for loopback. Clients wouldn't (or shouldn't) be using it then

Have changed the method name to GetExternalFacingHttpApiUrl as it better describes its function.

@cvium
Copy link
Member

@cvium cvium commented May 1, 2021

That's not what I had in mind. Are we certain it's the correct behaviour?

@BaronGreenback
Copy link
Contributor Author

@BaronGreenback BaronGreenback commented May 1, 2021

On a one interface system this will give the interface address.

On a two+interface system, the assumption is the client will connect via the interface that faces the router.

@cvium
Copy link
Member

@cvium cvium commented May 1, 2021

And what if the client is just ffmpeg?

@BaronGreenback
Copy link
Contributor Author

@BaronGreenback BaronGreenback commented May 1, 2021

All interfaces should be accessible locally - unless the user has put restrictions in place.

@BaronGreenback BaronGreenback changed the title Add loopback for livetv to bind interfaces. Fix for Livetv when bind interfaces specified. May 3, 2021
@BaronGreenback
Copy link
Contributor Author

@BaronGreenback BaronGreenback commented May 3, 2021

@cvium: After some thought - have changed how the interface is selected.

  1. Any user specified bind address (preference on internal first)
  2. Any internal interface.

This better matches how the original liveTV connected via an internal address (127.0.0.1).

@BaronGreenback BaronGreenback force-pushed the BaronGreenback:TVFix branch from 69993b7 to 7936ea5 May 3, 2021
@@ -1201,14 +1201,25 @@ public string GetSmartApiUrl(string hostname, int? port = null)
}

/// <inheritdoc/>
public string GetLoopbackHttpApiUrl()
public string GetInterfaceHttpApiUrl()

This comment has been minimized.

@cvium

cvium May 3, 2021
Member

Interface is a bit ambiguous.

This comment has been minimized.

@BaronGreenback

BaronGreenback May 3, 2021
Author Contributor

Changed to GetUrlForUseByHttpApi

This comment has been minimized.

@cvium

cvium May 3, 2021
Member

GetLoopbackHttpApiUrl isn't that far from incorrect now. Maybe GetLocalHttpApiUrl or GetLanHttpApiUrl?

Emby.Server.Implementations/ApplicationHost.cs Outdated Show resolved Hide resolved

return GetLocalApiUrl("127.0.0.1", Uri.UriSchemeHttp, HttpPort);
return GetLocalApiUrl(bind.Address.ToString(), Uri.UriSchemeHttp);

This comment has been minimized.

@cvium

cvium May 3, 2021
Member

You're using FirstOrDefault but you're not handling the null here (I assume that's the default)

This comment has been minimized.

@BaronGreenback

BaronGreenback May 4, 2021
Author Contributor

Changed to First() - as an address will always be returned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Current Release
  
Active PRs
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants