Skip to content
This repository has been archived by the owner on Mar 16, 2021. It is now read-only.

Rewrite SafeRoleEnvironment to not use Assembly.LoadWithPartialName #339

Merged
merged 4 commits into from
Aug 23, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ private static string GetEnvironmentSettingValue(string key)
// Get value from Cloud Services (if it throws, just ignore)
try
{
if (SafeRoleEnvironment.IsAvailable)
var cloudKey = key.Replace(':', '-'); // no ':' supported in cloud services
if (SafeRoleEnvironment.TryGetConfigurationSettingValue(cloudKey, out var configValue))
{
var cloudKey = key.Replace(':', '-'); // no ':' supported in cloud services
return SafeRoleEnvironment.GetConfigurationSettingValue(cloudKey);
return configValue;
}
}
catch
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ public class DeploymentIdTelemetryInitializer : ITelemetryInitializer

public void Initialize(ITelemetry telemetry)
{
if (SafeRoleEnvironment.IsAvailable)
if (SafeRoleEnvironment.TryGetDeploymentId(out var id))
{
telemetry.Context.Properties[DeploymentId] = SafeRoleEnvironment.GetDeploymentId();
telemetry.Context.Properties[DeploymentId] = id;
}
}
}
Expand Down
89 changes: 31 additions & 58 deletions src/NuGet.Services.BasicSearch/SafeRoleEnvironment.cs
Original file line number Diff line number Diff line change
@@ -1,93 +1,66 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using Microsoft.WindowsAzure.ServiceRuntime;
using System;
using System.IO;
using System.Reflection;

namespace NuGet.Services.BasicSearch
{
/// <summary>
/// Safe role environment which allows this application to be run on both Azure Cloud Services and Azure Web Sites.
/// A safe wrapper for <see cref="RoleEnvironment"/>.
/// How this service is deployed (e.g. Cloud Service, Web App) determines whether or not <see cref="RoleEnvironment"/> is accessible.
/// <see cref="Microsoft.WindowsAzure.ServiceRuntime"/> is loaded from the machine and not from the bin directory, so it may or may not be present on the machine.
/// </summary>
internal static class SafeRoleEnvironment
{
private const string _serviceRuntimeAssembly = "Microsoft.WindowsAzure.ServiceRuntime";
private const string _roleEnvironmentTypeName = "Microsoft.WindowsAzure.ServiceRuntime.RoleEnvironment";
private const string _isAvailablePropertyName = "IsAvailable";

public static bool IsAvailable { get; private set; }

static SafeRoleEnvironment()
{
// Find out if the code is running in the cloud service context.
Assembly assembly = GetServiceRuntimeAssembly();
if (assembly != null)
{
Type roleEnvironmentType = assembly.GetType(_roleEnvironmentTypeName, false);
if (roleEnvironmentType != null)
{
PropertyInfo isAvailableProperty = roleEnvironmentType.GetProperty(_isAvailablePropertyName);

try
{
IsAvailable = isAvailableProperty != null && (bool)isAvailableProperty.GetValue(null, new object[] { });
}
catch (TargetInvocationException)
{
IsAvailable = false;
}
}
}
}

/// <summary>
/// Delegate the call because we don't want RoleEnvironment appearing in the function scope of the caller because that
/// would trigger the assembly load: the very thing we are attempting to avoid
/// </summary>
/// <param name="configurationSettingName"></param>
/// <returns></returns>
public static string GetConfigurationSettingValue(string configurationSettingName)
public static bool TryGetConfigurationSettingValue(string configurationSettingName, out string value)
{
return RoleEnvironment.GetConfigurationSettingValue(configurationSettingName);
return TryGetField(() => RoleEnvironment.GetConfigurationSettingValue(configurationSettingName), out value);
}

public static string GetDeploymentId()
public static bool TryGetDeploymentId(out string id)
{
if (IsAvailable)
{
return RoleEnvironment.DeploymentId;
}

return string.Empty;
return TryGetField(() => RoleEnvironment.DeploymentId, out id);
}

public static string GetLocalResourceRootPath(string name)
public static bool TryGetLocalResourceRootPath(string name, out string path)
{
return RoleEnvironment.GetLocalResource(name).RootPath;
return TryGetField(() => RoleEnvironment.GetLocalResource(name).RootPath, out path);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can GetLocalResource return null? What is the expected behaviour in that case?

Copy link
Author

Choose a reason for hiding this comment

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

If GetLocalResource is null, it will throw a NullReferenceException, which will be bubbled up to the caller of this method. This is the same behavior as before.

}

/// <summary>
/// Loads and returns the latest available version of the service runtime assembly.
/// </summary>
/// <returns>Loaded assembly, if any.</returns>
private static Assembly GetServiceRuntimeAssembly()
private static bool _assemblyIsAvailable = true;
private static bool TryGetField<T>(Func<T> getValue, out T value)
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for generics here. The return value is always string.

Copy link
Author

Choose a reason for hiding this comment

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

Good catch.

The idea, however, is that this will be a proxy for every use of RoleEnvironment, so keeping it like this will make it easier for us to add additional methods if we need to access fields of RoleEnvironment that do not return a string.

If I had noticed that all of our uses of RoleEnvironment were strings initially I would have made it a Func<string>, but at this point there's no reason for the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

This introduces an unnecessary complexity to the code. Agree that it's not a bug deal.

{
Assembly assembly = null;
value = default(T);

try
{
assembly = Assembly.LoadWithPartialName(_serviceRuntimeAssembly);
if (!_assemblyIsAvailable || !RoleEnvironment.IsAvailable)
{
// If RoleEnvironment isn't available, we can't access it.
return false;
}

value = getValue();
return true;
}
catch (Exception e)
{
if (!(e is FileNotFoundException || e is FileLoadException || e is BadImageFormatException))
if (e is FileNotFoundException || e is FileLoadException || e is BadImageFormatException)
{
// If an exception related to loading files is thrown, the assembly is not available.
// Cache the fact that it is not available so we don't repeatedly throw and catch exceptions.
_assemblyIsAvailable = false;
return false;
}
else
{
// If an exception unrelated to loading files is thrown, the assembly must have thrown the exception itself.
// Rethrow the exception so it can be handled by the caller.
throw;
}
}

return assembly;
}
}
}
16 changes: 8 additions & 8 deletions src/NuGet.Services.BasicSearch/Startup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -94,21 +94,21 @@ public class Startup
// Overwrite the index's Azure Directory cache path if configured ot use an Azure Local Storage resource.
if (!string.IsNullOrEmpty(config.AzureDirectoryCacheLocalResourceName))
{
if (!SafeRoleEnvironment.IsAvailable)
if (SafeRoleEnvironment.TryGetLocalResourceRootPath(config.AzureDirectoryCacheLocalResourceName, out var path))
{
_logger.LogWarning(
"Cannot use Azure Local Resource {LocalResourceName} for caching when the RoleEnvironment is not available",
config.AzureDirectoryCacheLocalResourceName);
}
else
{
config.AzureDirectoryCachePath = SafeRoleEnvironment.GetLocalResourceRootPath(config.AzureDirectoryCacheLocalResourceName);
config.AzureDirectoryCachePath = path;

_logger.LogInformation(
"Set Azure Directory cache path to Azure Local Resource = {LocalResourceName}, Path = {LocalResourcePath}",
config.AzureDirectoryCacheLocalResourceName,
config.AzureDirectoryCachePath);
}
else
{
_logger.LogWarning(
"Cannot use Azure Local Resource {LocalResourceName} for caching when the RoleEnvironment is not available",
config.AzureDirectoryCacheLocalResourceName);
}
}

// redirect all HTTP requests to HTTPS
Expand Down