EES-4535 Update Data.Processor, Notifier and Publisher function apps to run in Isolated Worker model #4673

benoutram · 2024-03-18T17:55:23Z

⚠️ This PR targets the .NET 8 update #4639. ⚠️
⚠️ This has associated faffy deploy task EES-5014. ⚠️

This PR updates the three function app's Data.Processor, Notifier and Publisher to run in an isolated worker process rather than use the in-process model where functions run in the same process as the Function host runtime and are dependent on the availability of .NET versions supported by the runtime.

This work followed the guide Migrate .NET apps from the in-process model to the isolated worker model which explains all the changes such as the differences in dependencies, logging, function signature, attributes, Program.cs, etc.

Switching to the isolated worker model:

Provides fewer version conflicts - means we aren't reliant on the host for certain built-in dependencies and we no longer need configuration to fix dependencies like EES-4463 Preserve function dependencies in build process #4227.
Full control of the process - means we can configure and manage services at start-up without needing an extensions project to run start-up behaviour first.
Standard dependency injection - With this I've changed the functions to use the Options pattern to read configuration using Options rather than reading direct from the configuration file or direct referencing Environment. This also simplifies the unit tests.
.NET version flexibility - The Functions 4.x runtime is currently limited to .NET 6.0 so this change is necessary to unblock our update to .NET 8.0. See Supported Versions.

See Benefits of the isolated worker model.

There's a plan for the in-process model to receive an update to support .NET 8.0 in future but the Azure Functions team have made it clear that .NET 8 will be the last LTS release to receive in-process model support.

Other changes

Moves all Notifier email template id's to key vault. This simplifies the infrastructure deployment, removing all the ARM template parameters for Notifier email template id's that are currently populated by Azure DevOps.
Moves all sensitive GovUKNotify settings (API key and email template id's) from local.settings.json into appsettings.Local.json which is already gitignored and provides new appsettings.Local.json.example file.
Removes unused SampleGuids class.
Removes unused PublicStatisticsDb connection string from Publisher's local.settings.json file.
Adds LocalHttpPort setting to all function projects local.settings.json files so that they don't rely on manual port configuration when running them in an IDE (or by any means other than pnpm start / start.ts which already configures the port).
Moves Notifier service interfaces to Notifier.Services.Interfaces namespace.

UI test result

benoutram · 2024-03-21T09:02:00Z

Updated UI test result after recent changes and with latest changes from #4639.

mmyoungman · 2024-03-19T15:42:36Z

infrastructure/templates/template.json

-    "ees-notify-apikey": "[resourceId('Microsoft.KeyVault/vaults/secrets', variables('keyVaultName'), 'ees-notify-apikey')]",
-    "ees-notify-apikey-admin": "[resourceId('Microsoft.KeyVault/vaults/secrets', variables('keyVaultName'), 'ees-notify-apikey-admin')]",
-    "ees-notify-token": "[resourceId('Microsoft.KeyVault/vaults/secrets', variables('keyVaultName'), 'ees-notify-token')]",
+    "ees-admin-govuknotify-api-key": "[resourceId('Microsoft.KeyVault/vaults/secrets', variables('keyVaultName'), 'ees-admin-govuknotify-api-key')]",


I assume the faffy deploy ticket you mentioned previously is for all these changes?

I assume the plan is to add these before the deploy and then remove the old one's afterwards?

Yes it is, I've put the details in the description of that ticket for which variables to add and remove and at what stages each thing needs doing 🙂 .

mmyoungman · 2024-03-19T15:49:58Z

infrastructure/templates/template.json

-        "ReleaseAmendmentSupersededSubscribersEmailTemplateId": "[parameters('releaseAmendmentSupersededSubscribersEmailTemplateId')]",
-        "SubscriptionConfirmationEmailTemplateId": "[parameters('subscriptionConfirmationEmailTemplateId')]",
-        "TableStorageConnString": "[concat('@Microsoft.KeyVault(SecretUri=', reference(variables('ees-storage-notifications')).secretUriWithVersion, ')')]"
+        "AppSettings:BaseUrl": "[concat(concat('https://', variables('notificationsAppName')), '.azurewebsites.net/api')]",


I like this new naming scheme - the whole Blah:Blah. What inspired this?

Ok I see now - it's to do with how the configuration is accessed in C# i.e. .Configure<AppSettingOptions>(hostContext.Configuration.GetSection(AppSettingOptions.AppSettings)) in Notifier's Program.cs

Yes it's to do with the Configuration API and the Options pattern. Variables in a hierarchy can be set in the environment with a double underscore or colon as the separator. We already do this for other variables in the infrastructure using the Options pattern.

mmyoungman · 2024-03-19T15:58:43Z

infrastructure/templates/template.json

+        "AppSettings:TableStorageConnectionString": "[concat('@Microsoft.KeyVault(SecretUri=', reference(variables('ees-storage-notifications')).secretUriWithVersion, ')')]",
+        "AppSettings:TokenSecretKey": "[concat('@Microsoft.KeyVault(SecretUri=', reference(variables('ees-notifier-token-secret-key'), '2018-02-14').secretUriWithVersion, ')')]",
+        "GovUkNotify:ApiKey": "[concat('@Microsoft.KeyVault(SecretUri=', reference(variables('ees-notifier-govuknotify-api-key'), '2018-02-14').secretUriWithVersion, ')')]",
+        "GovUkNotify:EmailTemplates:ReleaseAmendmentPublishedId": "[concat('@Microsoft.KeyVault(SecretUri=', reference(variables('ees-notifier-templateid-release-amendment-published'), '2018-02-14').secretUriWithVersion, ')')]",


If we add variables in the future for other projects using this naming scheme, what would we name them? Just wondering if it's worth changing these to something like GovUkNotify:EmailTemplates:Notifier:TemplateNameId?

Happy to defer to you as to what you think is best. Probably not worth the bother as you could easily see what a particular project is accessing via it's GovUkNotifyOptions class.

These environment variables are defined within the Notifier function app.

If we wanted other projects to use similar names they can do without conflicting, so I don't see the need to add :Notifier: into the hierarchy.

Adding that here would mean the Options class will need a sub-class called Notifier adding into its hierarchy which would be a bit weird unless we intended to use one Options class for the entire project.

If the same Options class was applicable to multiple projects the same variable like GovUkNotify:ApiKey can be defined under multiple app's.

mmyoungman · 2024-03-19T16:15:20Z

src/GovUk.Education.ExploreEducationStatistics.Notifier/Functions/ReleaseNotifier.cs

            {
                "release_link",
-                $"{webApplicationBaseUrl}find-statistics/{msg.PublicationSlug}/{msg.ReleaseSlug}"
+                $"{_appSettingOptions.PublicAppUrl}/find-statistics/{msg.PublicationSlug}/{msg.ReleaseSlug}"


I imagine you're already aware, but AppendingTrailingSlash only appends a slash if it doesn't end with a slash already. My thought when looking at this was that maybe that's not just because the variable being set inconsistently across environments - there is some deeper reason that prevented this from being fixed in the past. I got a feeling it may be something like the frontend and backend both using the variable.

If you've already puzzled this out, feel free to ignore. Regardless, probably worth highlighting for testing.

Before I made this change I looked to see how this was configured and in all environments it was this environment variable config within the Notifier function config of the ARM template:

"WebApplicationBaseUrl": "[concat(variables('publicAppUrl'), '/')]",

The equivalent with my changes in the ARM template is now this:

"AppSettings:PublicAppUrl": "[variables('publicAppUrl')]",

With the renaming it's now clear that it's the pubic app URL and it doesn't have a trailing forward slash, so everywhere it's used has to add one but I think I've got that covered. Concatenating it before without one like this {webApplicationBaseUrl}find-statistics looked weird to me.

mmyoungman · 2024-03-21T09:45:03Z

src/GovUk.Education.ExploreEducationStatistics.Notifier/appsettings.Local.json.example

+    "ApiKey": "",
+    "EmailTemplates" : {
+      "ReleaseAmendmentPublishedId": "",
+      "ReleaseAmendmentPublishedSupersededSubscribersId": "",
+      "ReleasePublishedId": "",
+      "ReleasePublishedSupersededSubscribersId": "",
+      "SubscriptionConfirmationId": "",
+      "SubscriptionVerificationId": ""


Not super fussed, but I wouldn't mind these being "change-me" (or something else) by default - that made it easy to find where you needed to change the variable, as you can grep for it.

Cool, I've changed these values back to change-me like they were before they were moved and renamed.

src/GovUk.Education.ExploreEducationStatistics.Notifier/host.json

src/GovUk.Education.ExploreEducationStatistics.Notifier/appsettings.Local.json.example

…isher local.settings.json

…cal.settings.json

benoutram force-pushed the EES-4535-migrate-functions-to-isolated-worker-model branch from 3e71bbd to 465592d Compare March 19, 2024 10:09

benoutram mentioned this pull request Mar 19, 2024

EES-4975 Update projects to .NET 8, ASP.NET Core 8, Entity Framework 8 and C# 12 #4639

Merged

benoutram force-pushed the EES-4975-dotnet8 branch from 506fbaa to 2968bf3 Compare March 20, 2024 09:54

benoutram force-pushed the EES-4535-migrate-functions-to-isolated-worker-model branch from 465592d to e13674b Compare March 20, 2024 10:12

benoutram force-pushed the EES-4975-dotnet8 branch from 2968bf3 to 00383c5 Compare March 20, 2024 10:32

benoutram force-pushed the EES-4535-migrate-functions-to-isolated-worker-model branch from e13674b to 285ea24 Compare March 20, 2024 10:33

benoutram mentioned this pull request Mar 20, 2024

Add Notifier README file #4555

Merged

benoutram force-pushed the EES-4975-dotnet8 branch from 00383c5 to becd192 Compare March 20, 2024 15:20

benoutram force-pushed the EES-4535-migrate-functions-to-isolated-worker-model branch from 8734922 to 6d8438f Compare March 20, 2024 15:22

benoutram force-pushed the EES-4975-dotnet8 branch from becd192 to 744690f Compare March 20, 2024 16:01

benoutram force-pushed the EES-4535-migrate-functions-to-isolated-worker-model branch 2 times, most recently from 0d37a4d to 4c68d9a Compare March 20, 2024 18:55

mmyoungman approved these changes Mar 21, 2024

View reviewed changes

benoutram force-pushed the EES-4535-migrate-functions-to-isolated-worker-model branch from 4c68d9a to 3a54ab4 Compare March 21, 2024 11:13

mmyoungman approved these changes Mar 21, 2024

View reviewed changes

benoutram force-pushed the EES-4975-dotnet8 branch from 744690f to d2d5907 Compare March 21, 2024 13:05

benoutram force-pushed the EES-4535-migrate-functions-to-isolated-worker-model branch from 3a54ab4 to b15cd36 Compare March 21, 2024 13:07

benoutram force-pushed the EES-4975-dotnet8 branch from d2d5907 to c218bce Compare March 21, 2024 14:25

benoutram added 4 commits March 21, 2024 14:28

EES-4535 Remove unused SampleGuids.cs

4fb35da

EES-4535 Remove unused PublicStatisticsDb connection string from Publ…

e091a65

…isher local.settings.json

EES-4535 Add LocalHttpPort setting to function project settings in lo…

3037154

…cal.settings.json

EES-4535 Add TestFunctionContext to help with testing functions

0486ecc

benoutram force-pushed the EES-4535-migrate-functions-to-isolated-worker-model branch from b15cd36 to e4cb7c8 Compare March 21, 2024 14:28

benoutram added 2 commits March 21, 2024 14:55

EES-4535 Update Notifier function app to use the isolated worker model.

9627bbd

EES-4535 Update Processor function app to use the isolated worker model.

9c129bc

benoutram force-pushed the EES-4535-migrate-functions-to-isolated-worker-model branch from e4cb7c8 to 315b610 Compare March 21, 2024 16:21

mmyoungman approved these changes Mar 21, 2024

View reviewed changes

benoutram added 2 commits March 21, 2024 16:37

EES-4535 Update Publisher function app to use the isolated worker model.

0534b38

EES-4535 Move ReleaseNotifierTests to Notifier.Tests.Functions namespace

2f9198b

benoutram force-pushed the EES-4535-migrate-functions-to-isolated-worker-model branch from 315b610 to 2f9198b Compare March 21, 2024 16:37

benoutram merged commit 6b77ae7 into EES-4975-dotnet8 Mar 21, 2024
2 checks passed

benoutram deleted the EES-4535-migrate-functions-to-isolated-worker-model branch March 21, 2024 16:43

This was referenced Mar 25, 2024

EES-5012 Test disabling the ability to trigger scheduled publishing of release versions immediately #4693

Merged

Revert config change made for EES-5012 in #4693 #4698

Merged

This was referenced Jul 2, 2024

EES-5013 Suppress SQL command console logging in Function apps #5024

Merged

EES-5011 Change Publisher queue message types to remove use of System.ValueTuple #5025

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EES-4535 Update Data.Processor, Notifier and Publisher function apps to run in Isolated Worker model #4673

EES-4535 Update Data.Processor, Notifier and Publisher function apps to run in Isolated Worker model #4673

benoutram commented Mar 18, 2024 •

edited

Loading

benoutram commented Mar 21, 2024

mmyoungman Mar 19, 2024

benoutram Mar 21, 2024

mmyoungman Mar 19, 2024

mmyoungman Mar 19, 2024

benoutram Mar 21, 2024

mmyoungman Mar 19, 2024

benoutram Mar 21, 2024 •

edited

Loading

mmyoungman Mar 19, 2024

benoutram Mar 21, 2024

mmyoungman Mar 21, 2024

benoutram Mar 21, 2024 •

edited

Loading

EES-4535 Update Data.Processor, Notifier and Publisher function apps to run in Isolated Worker model #4673

EES-4535 Update Data.Processor, Notifier and Publisher function apps to run in Isolated Worker model #4673

Conversation

benoutram commented Mar 18, 2024 • edited Loading

Other changes

UI test result

benoutram commented Mar 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoutram Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoutram Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

benoutram commented Mar 18, 2024 •

edited

Loading

benoutram Mar 21, 2024 •

edited

Loading

benoutram Mar 21, 2024 •

edited

Loading