github/azure-sdk-for-cpp
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Updated attestation SDK to remove ServiceVersion extensible enumeration (#3799)

Browse Source 
Jeff has pointed out that the current practice in the SDK of having a ServiceVersion which contains the current API versions of the service (for instance:

```c++
  class ServiceVersion final {
  public:
    explicit ServiceVersion(std::string version) : m_version(std::move(version)) {}
    AZ_STORAGE_QUEUES_DLLEXPORT const static ServiceVersion V2018_03_28;
    AZ_STORAGE_QUEUES_DLLEXPORT const static ServiceVersion V2020_10_01;
  };
)

```
Has a problem because the `ServiceVersion` construct has an implication that each of the `ServiceVersion` values listed is fully supported by the SDK.

The reality is that the SDK client team only tests the most recent API version listed in the SDK (the value which is the default version listed in the `ServiceClient` constructor).

How do we resolve this issue?

There are a few possible solutions that we’ve explored:
1)	Test all the API versions listed in the `ServiceVersion` enumeration.
2)	Remove the unsupported values from the `ServiceVersion` enumeration.
3)	Remove the `ServiceVersion` enumeration
4)	Remove the ability to set the API version at all.

Each of these solutions has some fairly significant drawbacks.

1)	Test all the API versions listed.
The core problem with this is that the SDK team is small and adding tests to support every possible API version is going to be prohibitively expensive.
2)	Remove the unsupported values from the `ServiceVersion` enumeration.
This is a breaking change and it means that moving to a new API version requires a breaking change to the SDK, even if the changes between API versions is strictly additive.
3)	Remove the ServiceVersion enumeration.
This is also a breaking change for shipping SDKs (specifically KeyVault and Storage Queues). However, it is a one-time breaking change and we don’t have evidence of customers actually using the feature.
4)	Remove the ability to set the API version at all.
Having *some* mechanism to set the API version is an important “escape hatch” which will allow customers to specify a specific API version even if that API version is not fully supported.

After discussing this a LOT, [@Ahson Khan](mailto:ahkha@microsoft.com), [@Rick Winter](mailto:Rick.Winter@microsoft.com), [@Jeffrey Richter](mailto:jeffreyr@microsoft.com), [@George Arama](mailto:George.Arama@microsoft.com), and [@Larry Osterman](mailto:Larry.Osterman@microsoft.com) came to the conclusion that we should probably take option #3, but leave the ClientOptions.Version value as a std::string.
This commit is contained in:
Larry Osterman 2022-07-06 09:29:55 -07:00 committed by GitHub
parent cb1c35eb77
commit 2bf0d38236
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 7 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

30
sdk/attestation/azure-security-attestation/inc/azure/attestation/attestation_client_options.hpp
View File

@ -16,28 +16,6 @@
namespace Azure { namespace Security { namespace Attestation {
/** @brief Version to be used when communicating with the Attestation service.
*/
class ServiceVersion final
: public Azure::Core::_internal::ExtendableEnumeration<ServiceVersion> {
public:
/**
* @brief Construct a new Service Version object
*
* @param version The string version for the Attestation service.
*/
explicit ServiceVersion(std::string version)
: Azure::Core::_internal::ExtendableEnumeration<ServiceVersion>(std::move(version))
{
}
/**
* @brief Use to send request to the 2020-10-01 version of Attestation service.
*
*/
AZ_ATTESTATION_DLLEXPORT static const ServiceVersion V2020_10_01;
};
/**
* @brief The TokenValidationCallbackFn represents a callback which is called to allow the caller
* to perform additional token validation options beyond the validations performed by the
@ -116,7 +94,7 @@ namespace Azure { namespace Security { namespace Attestation {
{
/** @brief Version to use when communicating with the attestation service.
*/
ServiceVersion Version;
std::string Version;
/** @brief Options sent when validating tokens received by the attestation service.
*/
@ -130,7 +108,7 @@ namespace Azure { namespace Security { namespace Attestation {
* the service.
*/
AttestationClientOptions(
ServiceVersion version = ServiceVersion::V2020_10_01,
std::string version = "2020-10-01",
AttestationTokenValidationOptions const& tokenValidationOptions = {})
: Azure::Core::_internal::ClientOptions(), Version(version),
TokenValidationOptions(tokenValidationOptions)
@ -145,7 +123,7 @@ namespace Azure { namespace Security { namespace Attestation {
{
/** @brief Version to use when communicating with the attestation service.
*/
ServiceVersion Version;
std::string Version;
/** @brief Options sent when validating tokens received by the attestation service.
*/
AttestationTokenValidationOptions TokenValidationOptions;
@ -157,7 +135,7 @@ namespace Azure { namespace Security { namespace Attestation {
* the service.
*/
AttestationAdministrationClientOptions(
ServiceVersion version = ServiceVersion::V2020_10_01,
std::string version = "2020-10-01",
AttestationTokenValidationOptions const& tokenValidationOptions = {})
: Azure::Core::_internal::ClientOptions(), Version(version),
TokenValidationOptions(tokenValidationOptions)

3
sdk/attestation/azure-security-attestation/src/attestation_administration_client.cpp
View File

@ -42,7 +42,7 @@ AttestationAdministrationClient::AttestationAdministrationClient(
std::string const& endpoint,
std::shared_ptr<Core::Credentials::TokenCredential const> credential,
AttestationAdministrationClientOptions const& options)
: m_endpoint(endpoint), m_apiVersion(options.Version.ToString()),
: m_endpoint(endpoint), m_apiVersion(options.Version),
m_tokenValidationOptions(options.TokenValidationOptions),
m_tracingFactory(options, "security.attestation", PackageVersion::ToString())
{
@ -56,7 +56,6 @@ AttestationAdministrationClient::AttestationAdministrationClient(
perRetrypolicies.emplace_back(
std::make_unique<BearerTokenAuthenticationPolicy>(credential, tokenContext));
}
m_apiVersion = options.Version.ToString();
std::vector<std::unique_ptr<HttpPolicy>> perCallpolicies;
m_pipeline = std::make_shared<Azure::Core::Http::_internal::HttpPipeline>(

2
sdk/attestation/azure-security-attestation/src/attestation_client.cpp
View File

@ -45,7 +45,7 @@ AttestationClient::AttestationClient(
perRetrypolicies.emplace_back(
std::make_unique<BearerTokenAuthenticationPolicy>(credential, tokenContext));
}
m_apiVersion = options.Version.ToString();
m_apiVersion = options.Version;
std::vector<std::unique_ptr<HttpPolicy>> perCallpolicies;
m_pipeline = std::make_shared<Azure::Core::Http::_internal::HttpPipeline>(

3
sdk/attestation/azure-security-attestation/src/attestation_client_options.cpp
View File

@ -4,9 +4,8 @@
#include "azure/attestation/attestation_client_options.hpp"
namespace Azure { namespace Security { namespace Attestation {
const ServiceVersion ServiceVersion::V2020_10_01("2020-10-01");
const AttestationDataType AttestationDataType ::Binary("Binary");
const AttestationDataType AttestationDataType::Binary("Binary");
const AttestationDataType AttestationDataType::Json("JSON");
}}} // namespace Azure::Security::Attestation