returning error if the Storage operation for secret management fails (#8361)

Author: soninaren

release_notes.md

@@ -2,6 +2,7 @@
 <!-- Please add your release notes in the following format:
 - My change description (#PR)
 -->
+- Return 500 when when writing secrets to storage fails (#8361)
 
 **Release sprint:** Sprint 119
 [ [bugs](https://github.com/Azure/azure-functions-host/issues?q=is%3Aissue+milestone%3A%22Functions+Sprint+119%22+label%3Abug+is%3Aclosed) | [features](https://github.com/Azure/azure-functions-host/issues?q=is%3Aissue+milestone%3A%22Functions+Sprint+119%22+label%3Afeature+is%3Aclosed) ]

src/WebJobs.Script.WebHost/Controllers/KeysController.cs

@@ -216,8 +216,6 @@ private async Task<IActionResult> AddOrUpdateSecretAsync(string keyName, string
                     return NotFound();
                 case OperationResult.Conflict:
                     return StatusCode(StatusCodes.Status409Conflict);
-                case OperationResult.Forbidden:
-                    return StatusCode(StatusCodes.Status403Forbidden);
                 default:
                     return StatusCode(StatusCodes.Status500InternalServerError);
             }

src/WebJobs.Script.WebHost/OperationResult.cs

@@ -10,7 +10,6 @@ public enum OperationResult
         Created,
         Updated,
         NotFound,
-        Conflict,
-        Forbidden
+        Conflict
     }
 }

src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs

@@ -328,13 +328,7 @@ await ModifyFunctionSecretsAsync(secretsType, keyScope, secrets =>
                 }
 
                 return secrets;
-            }, secretsFactory).ContinueWith(t =>
-            {
-                if (t.IsFaulted && t.Exception.InnerException is KeyVaultErrorException)
-                {
-                    result = OperationResult.Forbidden;
-                }
-            });
+            }, secretsFactory);
 
             return new KeyOperationResult(secret, result);
         }

test/WebJobs.Script.Tests/Security/SecretManagerTests.cs

@@ -6,9 +6,11 @@
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
+using System.Net;
 using System.Security.Cryptography;
 using System.Threading;
 using System.Threading.Tasks;
+using Azure;
 using Microsoft.Azure.WebJobs.Extensions.Http;
 using Microsoft.Azure.WebJobs.Logging;
 using Microsoft.Azure.WebJobs.Script.Diagnostics;
@@ -500,6 +502,27 @@ public async Task AddOrUpdateFunctionSecrets_WithFunctionNameAndNoSecret_Generat
         }
 
         [Fact]
+        public async Task AddOrUpdateFunctionSecret_WhenStorageWriteError_ThrowsException()
+        {
+            using (var directory = new TempDirectory())
+            {
+                CreateTestSecrets(directory.Path);
+
+                KeyOperationResult result;
+
+                ISecretsRepository repository = new TestSecretsRepository(false, true);
+                using var secretManager = CreateSecretManager(directory.Path, simulateWriteConversion: false, secretsRepository: repository);
+                try
+                {
+                    result = await secretManager.AddOrUpdateFunctionSecretAsync("function-key-3", "9876", "TestFunction", ScriptSecretsType.Function);
+                }
+                catch (RequestFailedException ex)
+                {
+                    Assert.Equal(ex.Status, (int)HttpStatusCode.InternalServerError);
+                }
+            }
+        }
+
         public async Task AddOrUpdateFunctionSecret_ClearsCache_WhenFunctionSecretAdded()
         {
             using (var directory = new TempDirectory())
@@ -1136,7 +1159,7 @@ private Mock<IKeyValueConverterFactory> GetConverterFactoryMock(bool simulateWri
             return mockValueConverterFactory;
         }
 
-        private SecretManager CreateSecretManager(string secretsPath, ILogger logger = null, IMetricsLogger metricsLogger = null, IKeyValueConverterFactory keyConverterFactory = null, bool createHostSecretsIfMissing = false, bool simulateWriteConversion = true, bool setStaleValue = true)
+        private SecretManager CreateSecretManager(string secretsPath, ILogger logger = null, IMetricsLogger metricsLogger = null, IKeyValueConverterFactory keyConverterFactory = null, bool createHostSecretsIfMissing = false, bool simulateWriteConversion = true, bool setStaleValue = true, ISecretsRepository secretsRepository = null)
         {
             logger = logger ?? _logger;
             metricsLogger = metricsLogger ?? new TestMetricsLogger();
@@ -1147,7 +1170,7 @@ private SecretManager CreateSecretManager(string secretsPath, ILogger logger = n
                 keyConverterFactory = mockValueConverterFactory.Object;
             }
 
-            ISecretsRepository repository = new FileSystemSecretsRepository(secretsPath, logger, _testEnvironment);
+            ISecretsRepository repository = secretsRepository ?? new FileSystemSecretsRepository(secretsPath, logger, _testEnvironment);
             var secretManager = new SecretManager(repository, keyConverterFactory, logger, metricsLogger, _hostNameProvider, _startupContextProvider);
 
             if (createHostSecretsIfMissing)
@@ -1216,12 +1239,19 @@ private class TestSecretsRepository : ISecretsRepository
             private int _writeCount = 0;
             private Random _rand = new Random();
             private bool _enforceSerialWrites = false;
+            private bool _forceWriteErrors = false;
 
             public TestSecretsRepository(bool enforceSerialWrites)
             {
                 _enforceSerialWrites = enforceSerialWrites;
             }
 
+            public TestSecretsRepository(bool enforceSerialWrites, bool forceWriteErrors)
+                : this(enforceSerialWrites)
+            {
+                _forceWriteErrors = forceWriteErrors;
+            }
+
             public event EventHandler<SecretsChangedEventArgs> SecretsChanged;
 
             public ConcurrentDictionary<string, ScriptSecrets> FunctionSecrets { get; } = new ConcurrentDictionary<string, ScriptSecrets>(StringComparer.OrdinalIgnoreCase);
@@ -1260,6 +1290,11 @@ public Task<ScriptSecrets> ReadAsync(ScriptSecretsType type, string functionName
 
             public async Task WriteAsync(ScriptSecretsType type, string functionName, ScriptSecrets secrets)
             {
+                if (_forceWriteErrors)
+                {
+                    throw new RequestFailedException((int)HttpStatusCode.InternalServerError, "Error");
+                }
+
                 if (_enforceSerialWrites && _writeCount > 1)
                 {
                     throw new Exception("Concurrent writes detected!");