-
Notifications
You must be signed in to change notification settings - Fork 3
feat: fetch hostname suffix from API #103
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,144 @@ | ||
using System; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Coder.Desktop.App.Models; | ||
using Coder.Desktop.CoderSdk.Coder; | ||
using Coder.Desktop.Vpn.Utilities; | ||
using Microsoft.Extensions.Logging; | ||
|
||
namespace Coder.Desktop.App.Services; | ||
|
||
public interface IHostnameSuffixGetter | ||
{ | ||
public event EventHandler<string> SuffixChanged; | ||
|
||
public string GetCachedSuffix(); | ||
} | ||
|
||
public class HostnameSuffixGetter : IHostnameSuffixGetter | ||
{ | ||
private const string DefaultSuffix = ".coder"; | ||
|
||
private readonly ICredentialManager _credentialManager; | ||
private readonly ICoderApiClientFactory _clientFactory; | ||
private readonly ILogger<HostnameSuffixGetter> _logger; | ||
|
||
// _lock protects all private (non-readonly) values | ||
private readonly RaiiSemaphoreSlim _lock = new(1, 1); | ||
private string _domainSuffix = DefaultSuffix; | ||
private bool _dirty = false; | ||
private bool _getInProgress = false; | ||
private CredentialModel _credentialModel = new() { State = CredentialState.Invalid }; | ||
|
||
public event EventHandler<string>? SuffixChanged; | ||
|
||
public HostnameSuffixGetter(ICredentialManager credentialManager, ICoderApiClientFactory apiClientFactory, | ||
ILogger<HostnameSuffixGetter> logger) | ||
{ | ||
_credentialManager = credentialManager; | ||
_clientFactory = apiClientFactory; | ||
_logger = logger; | ||
credentialManager.CredentialsChanged += HandleCredentialsChanged; | ||
HandleCredentialsChanged(this, _credentialManager.GetCachedCredentials()); | ||
} | ||
|
||
~HostnameSuffixGetter() | ||
{ | ||
_credentialManager.CredentialsChanged -= HandleCredentialsChanged; | ||
} | ||
|
||
private void HandleCredentialsChanged(object? sender, CredentialModel credentials) | ||
{ | ||
using var _ = _lock.Lock(); | ||
_logger.LogDebug("credentials updated with state {state}", credentials.State); | ||
_credentialModel = credentials; | ||
if (credentials.State != CredentialState.Valid) return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should you set the stored domain back to the default in this case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it would be better to just always create the task if the credentials changed (i.e. remove this check here) and do the check once in the task. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should set the stored domain back to the default. The most common scenario for getting invalid creds is probably a logout or expired token --- so it's not particularly likely that the Default is more correct. In any case, once we sign back in we'll get the correct value. In terms of avoiding the check and always creating the task, seems like a lot of churn (locking, creating tasks, dropping logs) to avoid one conditional. |
||
|
||
_dirty = true; | ||
if (!_getInProgress) | ||
{ | ||
_getInProgress = true; | ||
Task.Run(Refresh).ContinueWith(MaybeRefreshAgain); | ||
} | ||
} | ||
|
||
private async Task Refresh() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably just take the credentials model as an argument. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We want to clear the |
||
{ | ||
_logger.LogDebug("refreshing domain suffix"); | ||
CredentialModel credentials; | ||
spikecurtis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
using (_ = await _lock.LockAsync()) | ||
{ | ||
credentials = _credentialModel; | ||
if (credentials.State != CredentialState.Valid) | ||
{ | ||
_logger.LogDebug("abandoning refresh because credentials are now invalid"); | ||
return; | ||
} | ||
|
||
_dirty = false; | ||
} | ||
|
||
var client = _clientFactory.Create(credentials); | ||
using var timeoutSrc = new CancellationTokenSource(TimeSpan.FromSeconds(10)); | ||
var connInfo = await client.GetAgentConnectionInfoGeneric(timeoutSrc.Token); | ||
|
||
// older versions of Coder might not set this | ||
var suffix = string.IsNullOrEmpty(connInfo.HostnameSuffix) | ||
? DefaultSuffix | ||
// and, it doesn't include the leading dot. | ||
: "." + connInfo.HostnameSuffix; | ||
|
||
var changed = false; | ||
using (_ = await _lock.LockAsync(CancellationToken.None)) | ||
{ | ||
if (_domainSuffix != suffix) changed = true; | ||
_domainSuffix = suffix; | ||
} | ||
|
||
if (changed) | ||
{ | ||
_logger.LogInformation("got new domain suffix '{suffix}'", suffix); | ||
// grab a local copy of the EventHandler to avoid TOCTOU race on the `?.` null-check | ||
var del = SuffixChanged; | ||
del?.Invoke(this, suffix); | ||
} | ||
else | ||
{ | ||
_logger.LogDebug("domain suffix unchanged '{suffix}'", suffix); | ||
} | ||
} | ||
|
||
private async Task MaybeRefreshAgain(Task prev) | ||
{ | ||
if (prev.IsFaulted) | ||
{ | ||
_logger.LogError(prev.Exception, "failed to query domain suffix"); | ||
// back off here before retrying. We're just going to use a fixed, long | ||
// delay since this just affects UI stuff; we're not in a huge rush as | ||
// long as we eventually get the right value. | ||
await Task.Delay(TimeSpan.FromSeconds(10)); | ||
} | ||
|
||
using var l = await _lock.LockAsync(CancellationToken.None); | ||
if ((_dirty || prev.IsFaulted) && _credentialModel.State == CredentialState.Valid) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You could probably also accomplish the same thing without needing a bool by throwing in the credential state guard at the top of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea of |
||
{ | ||
// we still have valid credentials and we're either dirty or the last Get failed. | ||
_logger.LogDebug("retrying domain suffix query"); | ||
_ = Task.Run(Refresh).ContinueWith(MaybeRefreshAgain); | ||
return; | ||
} | ||
|
||
// Getting here means either the credentials are not valid or we don't need to | ||
// refresh anyway. | ||
// The next time we get new, valid credentials, HandleCredentialsChanged will kick off | ||
// a new Refresh | ||
_getInProgress = false; | ||
return; | ||
} | ||
|
||
public string GetCachedSuffix() | ||
{ | ||
using var _ = _lock.Lock(); | ||
return _domainSuffix; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should just fetch this on-demand from the CredentialManager (since it's cheap) rather than storing it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this, but it complicates things considerably in terms of race conditions because you can't synchronize changes to
GetCachedCredentials
with the other items we track here like_dirty
, so its hard to guarantee the correct behavior. Writing updates to this while holding the lock ends up being much simpler.