From 3f86fc8689c5b1785ce9048bc6fded629dbb68d0 Mon Sep 17 00:00:00 2001 From: Stas Date: Thu, 15 Sep 2022 15:45:39 -0700 Subject: [PATCH] fix some bugs (#2406) * - fix queries in timer retention - do not discard proxy record after proxy state is processed, since that record needs to persist * addressing comments Co-authored-by: stas --- .../ApiService/Functions/TimerProxy.cs | 17 +++++++++++------ .../ApiService/Functions/TimerRetention.cs | 13 ++++++------- src/ApiService/ApiService/UserCredentials.cs | 2 +- .../ApiService/onefuzzlib/ProxyOperations.cs | 4 ++-- .../ApiService/onefuzzlib/VmOperations.cs | 6 ++++-- .../ApiService/onefuzzlib/orm/Queries.cs | 13 +++++++++++++ .../FunctionalTests/1f-api/ApiBase.cs | 5 ++--- 7 files changed, 39 insertions(+), 21 deletions(-) diff --git a/src/ApiService/ApiService/Functions/TimerProxy.cs b/src/ApiService/ApiService/Functions/TimerProxy.cs index 82eaaecd4..c16049192 100644 --- a/src/ApiService/ApiService/Functions/TimerProxy.cs +++ b/src/ApiService/ApiService/Functions/TimerProxy.cs @@ -20,7 +20,9 @@ public class TimerProxy { var proxies = await proxyOperations.QueryAsync().ToListAsync(); - foreach (var proxy in proxies) { + foreach (var p in proxies) { + var proxy = p; + if (VmStateHelper.Available.Contains(proxy.State)) { // Note, outdated checked at the start, but set at the end of this loop. // As this function is called via a timer, this works around a user @@ -40,11 +42,14 @@ public class TimerProxy { if (VmStateHelper.NeedsWork.Contains(proxy.State)) { _logger.Info($"scaleset-proxy: update state. proxy:{proxy.Region} state:{proxy.State}"); - await proxyOperations.ProcessStateUpdate(proxy); + proxy = await proxyOperations.ProcessStateUpdate(proxy); } - if (proxy.State != VmState.Stopped && proxyOperations.IsOutdated(proxy)) { - await proxyOperations.Replace(proxy with { Outdated = true }); + if (proxy is not null && proxy.State != VmState.Stopped && proxyOperations.IsOutdated(proxy)) { + var r = await proxyOperations.Replace(proxy with { Outdated = true }); + if (!r.IsOk) { + _logger.Error($"Failed to replace proxy recordy for proxy {proxy.ProxyId} due to {r.ErrorV}"); + } } } @@ -54,8 +59,8 @@ public class TimerProxy { foreach (var region in regions) { var allOutdated = proxies.Where(x => x.Region == region).All(p => p.Outdated); if (allOutdated) { - await proxyOperations.GetOrCreate(region); - _logger.Info($"Creating new proxy in region {region}"); + var proxy = await proxyOperations.GetOrCreate(region); + _logger.Info($"Creating new proxy with id {proxy.ProxyId} in region {region}"); } // this is required in order to support upgrade from non-nsg to diff --git a/src/ApiService/ApiService/Functions/TimerRetention.cs b/src/ApiService/ApiService/Functions/TimerRetention.cs index 88be8c557..dbf3ae417 100644 --- a/src/ApiService/ApiService/Functions/TimerRetention.cs +++ b/src/ApiService/ApiService/Functions/TimerRetention.cs @@ -1,5 +1,5 @@ -using Microsoft.Azure.Functions.Worker; - +using ApiService.OneFuzzLib.Orm; +using Microsoft.Azure.Functions.Worker; namespace Microsoft.OneFuzz.Service.Functions; public class TimerRetention { @@ -33,8 +33,8 @@ public class TimerRetention { var timeRetainedOlder = now - RETENTION_POLICY; var timeRetainedNewer = now + SEARCH_EXTENT; - var timeFilter = $"Timestamp lt datetime'{timeRetainedOlder.ToString("o")}' and Timestamp gt datetime'{timeRetainedNewer.ToString("o")}'"; - var timeFilterNewer = $"Timestamp gt datetime '{timeRetainedOlder.ToString("o")}'"; + var timeFilter = Query.TimeRange(timeRetainedOlder, timeRetainedNewer); + var timeFilterNewer = Query.NewerThan(timeRetainedOlder); // Collecting 'still relevant' task containers. // NOTE: This must be done before potentially modifying tasks otherwise @@ -52,7 +52,6 @@ public class TimerRetention { } } - await foreach (var notification in _notificaitonOps.QueryAsync(timeFilter)) { _log.Verbose($"checking expired notification for removal: {notification.NotificationId}"); var container = notification.Container; @@ -66,7 +65,7 @@ public class TimerRetention { } } - await foreach (var job in _jobOps.QueryAsync($"{timeFilter} and state eq '{JobState.Enabled}'")) { + await foreach (var job in _jobOps.QueryAsync(Query.And(timeFilter, Query.EqualEnum("state", JobState.Enabled)))) { if (job.UserInfo is not null && job.UserInfo.Upn is not null) { _log.Info($"removing PII from job {job.JobId}"); var userInfo = job.UserInfo with { Upn = null }; @@ -78,7 +77,7 @@ public class TimerRetention { } } - await foreach (var task in _taskOps.QueryAsync($"{timeFilter} and state eq '{TaskState.Stopped}'")) { + await foreach (var task in _taskOps.QueryAsync(Query.And(timeFilter, Query.EqualEnum("state", TaskState.Stopped)))) { if (task.UserInfo is not null && task.UserInfo.Upn is not null) { _log.Info($"removing PII from task {task.TaskId}"); var userInfo = task.UserInfo with { Upn = null }; diff --git a/src/ApiService/ApiService/UserCredentials.cs b/src/ApiService/ApiService/UserCredentials.cs index 4083d1527..bff791b07 100644 --- a/src/ApiService/ApiService/UserCredentials.cs +++ b/src/ApiService/ApiService/UserCredentials.cs @@ -82,7 +82,7 @@ public class UserCredentials : IUserCredentials { return OneFuzzResult.Ok(new(applicationId, objectId, upn)); } else { - _log.Error($"issuer not from allowed tenant: {token.Issuer} - {allowedTenants}"); + _log.Error($"issuer not from allowed tenant. issuer: {token.Issuer} - tenants: {allowedTenants.OkV}"); return OneFuzzResult.Error(ErrorCode.INVALID_REQUEST, new[] { "unauthorized AAD issuer" }); } } else { diff --git a/src/ApiService/ApiService/onefuzzlib/ProxyOperations.cs b/src/ApiService/ApiService/onefuzzlib/ProxyOperations.cs index 5de0a97bf..c6cfe37be 100644 --- a/src/ApiService/ApiService/onefuzzlib/ProxyOperations.cs +++ b/src/ApiService/ApiService/onefuzzlib/ProxyOperations.cs @@ -15,7 +15,7 @@ public interface IProxyOperations : IStatefulOrm { bool IsAlive(Proxy proxy); Async.Task SaveProxyConfig(Proxy proxy); bool IsOutdated(Proxy proxy); - Async.Task GetOrCreate(Region region); + Async.Task GetOrCreate(Region region); Task IsUsed(Proxy proxy); // state transitions: @@ -42,7 +42,7 @@ public class ProxyOperations : StatefulOrm, IPr return await data.FirstOrDefaultAsync(); } - public async Async.Task GetOrCreate(Region region) { + public async Async.Task GetOrCreate(Region region) { var proxyList = QueryAsync(filter: TableClient.CreateQueryFilter($"region eq {region.String} and outdated eq false")); await foreach (var proxy in proxyList) { diff --git a/src/ApiService/ApiService/onefuzzlib/VmOperations.cs b/src/ApiService/ApiService/onefuzzlib/VmOperations.cs index df0b7ec79..5bc5ddd9f 100644 --- a/src/ApiService/ApiService/onefuzzlib/VmOperations.cs +++ b/src/ApiService/ApiService/onefuzzlib/VmOperations.cs @@ -218,8 +218,10 @@ public class VmOperations : IVmOperations { extensionName, extension ); - } catch (RequestFailedException ex) when (ex.Status == 409 && ex.Message.Contains("VM is marked for deletion")) { - _logTracer.Info(ex.Message); + } catch (RequestFailedException ex) when + (ex.Status == 409 && + (ex.Message.Contains("VM is marked for deletion") || ex.Message.Contains("The request failed due to conflict with a concurrent request."))) { + _logTracer.Info($"Tried to create extension but failed due to {ex.Message}"); } return; } diff --git a/src/ApiService/ApiService/onefuzzlib/orm/Queries.cs b/src/ApiService/ApiService/onefuzzlib/orm/Queries.cs index 115536d05..3dccb8ccf 100644 --- a/src/ApiService/ApiService/onefuzzlib/orm/Queries.cs +++ b/src/ApiService/ApiService/onefuzzlib/orm/Queries.cs @@ -46,12 +46,25 @@ namespace ApiService.OneFuzzLib.Orm { return EqualAny(property, convertedEnums); } + public static string EqualEnum(string property, T e) where T : Enum { + string convertedEnum = JsonSerializer.Serialize(e, EntityConverter.GetJsonSerializerOptions()).Trim('"'); + return $"{property} eq '{EscapeString(convertedEnum)}'"; + } + public static string TimeRange(DateTimeOffset min, DateTimeOffset max) { // NB: this uses the auto-populated Timestamp property, and will result in a table scan // TODO: should this be inclusive at the endpoints? return TableClient.CreateQueryFilter($"Timestamp lt {max} and Timestamp gt {min}"); } + public static string NewerThan(DateTimeOffset t) { + return TableClient.CreateQueryFilter($"Timestamp gt {t}"); + } + + public static string OlderThan(DateTimeOffset t) { + return TableClient.CreateQueryFilter($"Timestamp lt {t}"); + } + public static string StartsWith(string property, string prefix) { var upperBound = prefix[..(prefix.Length - 1)] + (char)(prefix.Last() + 1); // property name should not be escaped, but strings should be: diff --git a/src/ApiService/FunctionalTests/1f-api/ApiBase.cs b/src/ApiService/FunctionalTests/1f-api/ApiBase.cs index c03ac2251..e6ba9e560 100644 --- a/src/ApiService/FunctionalTests/1f-api/ApiBase.cs +++ b/src/ApiService/FunctionalTests/1f-api/ApiBase.cs @@ -176,10 +176,9 @@ public abstract class ApiBase { } } - public async Task Get(JsonObject root) { + public async Task Get(JsonObject root, string? subPath = null) { var body = root.ToJsonString(); - var r = await _request.Get(_endpoint, body); - var ss = await r.Content.ReadAsStringAsync(); + var r = await _request.Get(subPath is null ? _endpoint : new Uri($"{_endpoint}{subPath}"), body); return (await JsonDocument.ParseAsync(r.Content.ReadAsStream())).RootElement; }