Final merge to main for Key Vault 7.3

[gearama]

Bringing the code from the feature branch into main for KV 7.3

Pull Request Checklist

Please leverage this checklist as a reminder to address commonly occurring feedback when submitting a pull request to make sure your PR can be reviewed quickly:

See the detailed list in the contributing guide.

• C++ Guidelines
• Doxygen docs
• Unit tests
• No unwanted commits/changes
• Descriptive title/description
  • PR is single purpose
  • Related issue listed
• Comments in source
• No typos
• Update changelog
• Not work-in-progress
• External references or docs updated
• Self review of PR done
• Any breaking changes?

[gearama]

@ahsonkhan your comments have been addressed , please review and if it 's ok approve so then i can merge and run the live tasts over the weekend

[ahsonkhan]

Thanks @gearama. Are we targeting this change for the July release?

I would like to discuss the ServiceVersion change some more, to understand the goal better. Let's find some time today for it.

cc @JeffreyRichter

[vhvb1989]

Why are we removing ServiceVersion?

[JeffreyRichter]

I requested the removal of ServiceVersion type and the hard-coding of the version string - I think we should do this in all our client libraries. Here's why: When we build a client library, we only test it with the latest service version and therefore, we can only support customers using this specific version. Different versions (newer or older) may be breaking and it is highly unlikely that our client library will work with a different service version. In which case, customers using this feature will cause our support burden to be very high.

By formalizing the service version concept with a specific data type and with some constant service version values, we are implying that this is a fully supported feature, and that the client library is guaranteed to work with older service versions. But we never test this, and we can't make this guarantee. By removing the ServiceVersion type and just having a simple string, we are not formalizing this concept, we are just implying to a customer that they could try a different string version and the client library may or may not work for them - our documentation for the service version string should even indicate that changing this value is not guaranteed to work. This way, customers understand that we have not tested this and do not support it. But, if it works for them and the specific operations and specific service versions they are trying, then great! We're happy to have made this possible for them.

[vhvb1989]

ok, if removing ServiceVersion is fine, then this LGTM.

@JeffreyRichter , I am sort of confused by this change, because on one hand we say we would never introduce breaking changes and we have the extensible enums protecting the SDK client from Server's responses.
But on the other hand, we are saying we don't test and/or promise that the SDK client will work using an older Service Version?

Isn't one statement sort or contradicting the other? I am probably super wrong, but somehow the first statement makes me thing that old Service Versions previously shipped by the SDK are guaranteed to work, and in fact, a self-documented way for the SDK to show what versions have been supported by that SDK in the past.

Without this, there's not any hint for users to know if the client was ever used with a previous Service Version

[ahsonkhan]

Beyond the service version change discussion, this PR looks good to me overall.

Given we are shipping a beta release, I recommend we keep the ServiceVersion class for now, to remain consistent with previously shipped client SDKs (and previous version of KeyVault), and revisit this more broadly. Because it is reversible, either way, if we feel strongly to keep this PR as is, in the interest of time, that works too :)

Though I understand your concern that there might be an implied support to the customer or possible mismatch of expectations, @JeffreyRichter, that can be addressed with similar documentation.

I'd like to discuss the trade-offs a bit, and if we really want to do so, come up with a strategy to update all SDKs going forward. And take our time with such a change, along with update the guidelines as necessary - https://azure.github.io/azure-sdk/cpp_introduction.html#service-versions.

[JeffreyRichter]

First, swaggers frequently have a mistake in them or lack of clarity that improving the swagger introduces a break which we have to allow because the change causes the swagger to more accurately reflect the true service behavior.

Second, Azure's policy DOES allow services to make breaking changes (we need to do this to be agile and competitive). The policy requires that the service team announce to customers that the latest version is breaking, and the old versions must work as-is for at least 3 more years. So, under this policy, we cannot guarantee that any client lib is backward compatible.

Third, services occasionally break due to legal, security, or compliance issues. In these cases, the customer notice is just 1 year and sometimes less depending on the severity of the issue. These are rare but they do happen.

In short: Service will break customers from time to time and we need to be cognizant of this.

[JeffreyRichter]

I'm not sure where the C++ guidance on ServiceVersion came from but we can (and should) change that (in my opinion).

[ahsonkhan]

First, swaggers frequently have a mistake in them or lack of clarity that improving the swagger introduces a break which we have to allow because the change causes the swagger to more accurately reflect the true service behavior.

Second, Azure's policy DOES allow services to make breaking changes (we need to do this to be agile and competitive). The policy requires that the service team announce to customers that the latest version is breaking, and the old versions must work as-is for at least 3 more years. So, under this policy, we cannot guarantee that any client lib is backward compatible.

Third, services occasionally break due to legal, security, or compliance issues. In these cases, the customer notice is just 1 year and sometimes less depending on the severity of the issue. These are rare but they do happen.

In short: Service will break customers from time to time and we need to be cognizant of this.

Regarding 1, a swagger update to truly reflect the service behavior wouldn't really be a breaking change, so we wouldn't have to worry about that. As far as I understood things, we don't want to design our client SDK and versioning to try to paper over swagger or service-side breaking changes, anyway.

Regarding 2 & 3, since our SDK follows semver, if the service was to actually make breaking changes, we'd follow suite and rev the major version, and that newer SDK version will not support older ServiceVersion enum values, as expected.

[gearama]

/azp run cpp - keyvault

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[LarryOsterman]

I requested the removal of ServiceVersion type and the hard-coding of the version string - I think we should do this in all our client libraries. Here's why: When we build a client library, we only test it with the latest service version and therefore, we can only support customers using this specific version. Different versions (newer or older) may be breaking and it is highly unlikely that our client library will work with a different service version. In which case, customers using this feature will cause our support burden to be very high.

By formalizing the service version concept with a specific data type and with some constant service version values, we are implying that this is a fully supported feature, and that the client library is guaranteed to work with older service versions. But we never test this, and we can't make this guarantee. By removing the ServiceVersion type and just having a simple string, we are not formalizing this concept, we are just implying to a customer that they could try a different string version and the client library may or may not work for them - our documentation for the service version string should even indicate that changing this value is not guaranteed to work. This way, customers understand that we have not tested this and do not support it. But, if it works for them and the specific operations and specific service versions they are trying, then great! We're happy to have made this possible for them.

That makes sense. However given those restrictions, why do we have a service version construct in the public API surface at all? And should the service throw if it encounters a service version which isn't supported? That way if a client specifies version 4 of the API but the SDK doesn't support version 4, it throws an exception to let the client know that it cannot be supported.

[JeffreyRichter]

The reason to even offer the ServiceVersion field is that it provides an escape hatch for customers. C++ and many other (but not all) languages allow only one version of a client library to be loaded in a process and it's possible that the customer may want the old service behavior for some of their code and the new service behavior for other parts of their code. This "feature" allows the customer to TRY doing this. Without this feature, the customer MUST modify all their code to expect/use the new service behavior. If this fails, then the customer is forced to update all their code.

Another example where the ServiceVersion field can be useful is when not all clouds support the latest version of a service. This is particularly true for Azure Stack where the customers "own" upgrading the Azure services on the customer's timeframe. Unlike public clouds where MS owns updating the services. For these scenarios, the customer HAS to choose the api-version that is installed/available to them.

Another (rare) example (related to the example above) is something that the AzCopy utility makes BIG usage of: AzCopy allows customers to copy blogs from Azure Stack to a public cloud and vice versa. If this tool were written in C++, it can only load 1 version of the C++ Blob client library, but it must send download requests to Azure Stack using one version and upload requests to the public cloud using another version - the ServiceVersion field makes this possible. And, since Storage remains largely backward compatible and doesn't require newer esoteric features, this actually works very well in practice.

[LarryOsterman]

The reason to even offer the ServiceVersion field is that it provides an escape hatch for customers. C++ and many other (but not all) languages allow only one version of a client library to be loaded in a process and it's possible that the customer may want the old service behavior for some of their code and the new service behavior for other parts of their code. This "feature" allows the customer to TRY doing this. Without this feature, the customer MUST modify all their code to expect/use the new service behavior. If this fails, then the customer is forced to update all their code.

Another example where the ServiceVersion field can be useful is when not all clouds support the latest version of a service. This is particularly true for Azure Stack where the customers "own" upgrading the Azure services on the customer's timeframe. Unlike public clouds where MS owns updating the services. For these scenarios, the customer HAS to choose the api-version that is installed/available to them.

Another (rare) example (related to the example above) is something that the AzCopy utility makes BIG usage of: AzCopy allows customers to copy blogs from Azure Stack to a public cloud and vice versa. If this tool were written in C++, it can only load 1 version of the C++ Blob client library, but it must send download requests to Azure Stack using one version and upload requests to the public cloud using another version - the ServiceVersion field makes this possible. And, since Storage remains largely backward compatible and doesn't require newer esoteric features, this actually works very well in practice.

We should probably talk about that tomorrow AM (@ahsonkhan has made a similar request). IMHO, the only time ServiceVersion makes sense is if the client supports multiple versions. And then the client needs to reject requests to create the client with API versions that aren't supported by the client (since there is a presumption that there are breaking changes to the service API associated with each service API version). If the client doesn't understand the ServiceVersion provided by the customer, then the client should reject the ServiceVersion. And yes, that means that it's possible that if we update the client to remove support for a particular API version, it means we may force customers to change their code.

If the client is expected to support multiple versions, then IMHO it is incumbent on the client to ensure that it is tested against each of the versions it supports. There should be no situations where the client supports multiple versions but is tested against only one of them.

For the AzCopy scenario you described above, it only works if the AZ SDK client supports multiple versions. And if the AZ SDK client supports multiple versions, then the existing ServiceVersion construct expresses that support explicitly. And as I said: If a SDK client claims to support multiple API versions, then the SDK client MUST have test collateral for each API version.