Skip to content

Avoid crashing update-redirects when CLI output is empty #1944

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Sep 29, 2025
Merged

Avoid crashing update-redirects when CLI output is empty #1944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7164ded
Avoid crashing update-redirects when CLI output is empty
cotti Sep 25, 2025
57e0a4e
Merge branch 'main' into fix/kvs_update_resiliency
cotti Sep 25, 2025
a65bdc0
Merge branch 'main' into fix/kvs_update_resiliency
cotti Sep 26, 2025
0aab60d
Merge branch 'main' into fix/kvs_update_resiliency
cotti Sep 26, 2025
a428236
Output message if an exception occurs during the execution attempt lo…
cotti Sep 26, 2025
a06d5e0
Merge branch 'main' into fix/kvs_update_resiliency
cotti Sep 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Elastic.Documentation/ExternalCommands/ExternalCommandExecutor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ protected string[] CaptureMultiple(bool muteExceptions, int attempts, string bin
}
catch (Exception ex)
{
collector.EmitWarning("", $"An exception occurred on attempt {i} to capture output of {binary}: {ex?.Message}");
if (ex is not null)
e = ex;
}
Expand Down
50 changes: 40 additions & 10 deletions ...es/Elastic.Documentation.Assembler/Deploying/Redirects/AwsCloudFrontKeyValueStoreProxy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@ public void UpdateRedirects(string kvsName, IReadOnlyDictionary<string, string>
if (string.IsNullOrEmpty(eTag))
return;

var existingRedirects = ListAllKeys(kvsArn);
var listingSuccessful = TryListAllKeys(kvsArn, out var existingRedirects);

if (!listingSuccessful)
return;

var toPut = sourcedRedirects
.Select(kvp => new PutKeyRequestListItem { Key = kvp.Key, Value = kvp.Value })
Expand All @@ -51,7 +54,14 @@ private string DescribeKeyValueStore(string kvsName)
try
{
var json = CaptureMultiple("aws", "cloudfront", "describe-key-value-store", "--name", kvsName);
Copy link
Member

Choose a reason for hiding this comment

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

Can we have a retry mechanism here? It seems like this is a temporary error that only happens occasionally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... We have a retry mechanism on ExternalCommandExecutor, defaulting to 10 tries. Since we don't rethrow from it, It seems the mechanism is working. Is it worth it to have another? Each cycle can take up to 30s, accounting for the execution timeout.
I'll add individual error logging as well, that can be interesting too.

Copy link
Member

Choose a reason for hiding this comment

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

I see.

Kinda unrelated.. but I'm starting to wonder how do we handle paging for describe-key-valiue-store?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Paging is sort of handled partially by the AWS CLI itself. max-items determines the amount of items the CLI will output in the end, and page-size determines the internal paging in the CLI when communicating with AWS.

This means that if we had --max-items 50 --page-size 10, the CLI would call AWS up to 5 times before giving us the answer. The eTag it gives us as part of the output determines where in the query we are, and if it is null we have reached the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are now logging as warnings when exceptions occur in an execution attempt loop. They shouldn't be errors since later attempts might be successful.

var describeResponse = JsonSerializer.Deserialize<DescribeKeyValueStoreResponse>(string.Concat(json), AwsCloudFrontKeyValueStoreJsonContext.Default.DescribeKeyValueStoreResponse);
var concatJson = string.Concat(json);
if (string.IsNullOrWhiteSpace(concatJson))
{
Collector.EmitError("", "The output from cloudfront:describe-key-value-store was empty");
return string.Empty;
}

var describeResponse = JsonSerializer.Deserialize<DescribeKeyValueStoreResponse>(concatJson, AwsCloudFrontKeyValueStoreJsonContext.Default.DescribeKeyValueStoreResponse);
if (describeResponse?.KeyValueStore is { ARN.Length: > 0 })
return describeResponse.KeyValueStore.ARN;

Expand All @@ -71,7 +81,13 @@ private string AcquireETag(string kvsArn)
try
{
var json = CaptureMultiple("aws", "cloudfront-keyvaluestore", "describe-key-value-store", "--kvs-arn", kvsArn);
var describeResponse = JsonSerializer.Deserialize<DescribeKeyValueStoreResponse>(string.Concat(json), AwsCloudFrontKeyValueStoreJsonContext.Default.DescribeKeyValueStoreResponse);
var concatJson = string.Concat(json);
if (string.IsNullOrWhiteSpace(concatJson))
{
Collector.EmitError("", "The output from cloudfront-keyvaluestore:describe-key-value-store was empty");
return string.Empty;
}
var describeResponse = JsonSerializer.Deserialize<DescribeKeyValueStoreResponse>(concatJson, AwsCloudFrontKeyValueStoreJsonContext.Default.DescribeKeyValueStoreResponse);
if (describeResponse?.ETag is not null)
return describeResponse.ETag;

Expand All @@ -85,23 +101,29 @@ private string AcquireETag(string kvsArn)
}
}

private HashSet<string> ListAllKeys(string kvsArn)
private bool TryListAllKeys(string kvsArn, out HashSet<string> keys)
{
_logger.LogInformation("Acquiring existing redirects");
var allKeys = new HashSet<string>();
keys = [];
string[] baseArgs = ["cloudfront-keyvaluestore", "list-keys", "--kvs-arn", kvsArn, "--page-size", "50", "--max-items", "50"];
string? nextToken = null;
try
{
do
{
var json = CaptureMultiple("aws", [.. baseArgs, .. nextToken is not null ? (string[])["--starting-token", nextToken] : []]);
var response = JsonSerializer.Deserialize<ListKeysResponse>(string.Concat(json), AwsCloudFrontKeyValueStoreJsonContext.Default.ListKeysResponse);
var concatJson = string.Concat(json);
if (string.IsNullOrWhiteSpace(concatJson))
{
Collector.EmitError("", "The output from cloudfront-keyvaluestore:list-keys was empty");
throw new JsonException("Empty output from cloudfront-keyvaluestore:list-keys");
}
var response = JsonSerializer.Deserialize<ListKeysResponse>(concatJson, AwsCloudFrontKeyValueStoreJsonContext.Default.ListKeysResponse);

if (response?.Items != null)
{
foreach (var item in response.Items)
_ = allKeys.Add(item.Key);
_ = keys.Add(item.Key);
}

nextToken = response?.NextToken;
Expand All @@ -110,9 +132,9 @@ private HashSet<string> ListAllKeys(string kvsArn)
catch (Exception e)
{
Collector.EmitError("", "An error occurred while acquiring existing redirects in the KeyValueStore", e);
return [];
return false;
}
return allKeys;
return true;
}


Expand All @@ -138,7 +160,15 @@ private string ProcessBatchUpdates(
};
var responseJson = CaptureMultiple(false, 1, "aws", "cloudfront-keyvaluestore", "update-keys", "--kvs-arn", kvsArn, "--if-match", eTag,
$"--{operation.ToString().ToLowerInvariant()}", payload);
var updateResponse = JsonSerializer.Deserialize<UpdateKeysResponse>(string.Concat(responseJson), AwsCloudFrontKeyValueStoreJsonContext.Default.UpdateKeysResponse);

var concatJson = string.Concat(responseJson);
if (string.IsNullOrWhiteSpace(concatJson))
{
Collector.EmitError("", "The output from cloudfront-keyvaluestore:update-keys was empty");
throw new JsonException("Empty output from cloudfront-keyvaluestore:update-keys");
}

var updateResponse = JsonSerializer.Deserialize<UpdateKeysResponse>(concatJson, AwsCloudFrontKeyValueStoreJsonContext.Default.UpdateKeysResponse);

if (string.IsNullOrEmpty(updateResponse?.ETag))
throw new Exception("Failed to get new ETag after update operation.");
Expand Down
Loading