Added support for Key Rotation. #24452

vcolin7 · 2021-09-29T18:26:34Z

Added tests and code snippets. Also updated documentation for Key Vault Keys clients and code snippets.

Will add quickstart samples in a future PR.

…d existing code snippets.

check-enforcer · 2021-09-29T18:37:02Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run java - [service] - ci

azure-sdk · 2021-09-29T18:41:23Z

API changes have been detected in this PR. You can review API changes here

maorleger

Thanks for getting this in for this milestone! A few questions, mostly for me to better learn our Java pattern

.../src/main/java/com/azure/security/keyvault/keys/implementation/models/KeyRotationPolicy.java

maorleger · 2021-09-29T20:04:38Z

.../src/main/java/com/azure/security/keyvault/keys/implementation/models/KeyRotationPolicy.java

+
+    /**
+     * Get the actions that will be performed by Key Vault over the lifetime of a key. At the moment,
+     * {@link LifetimeAction} can only have two items at maximum: one for rotate, one for notify. The notification time


Unless this is internal, I would leave out the At the moment... as well as The notification time... since that might change without an accompanying code change

This class is internal, but I will make sure no public classes contain this wording.

Would we not have any mention if this in our documentation even after the service goes GA? I can understand they might change the values but I'm not sure how many people actually would bother to go into Microsoft Docs and check :P

Totally valid point... my concern is that this will change at some point at the service side and this doc-comment will become stale. We'll probably never go back and change until a customer notices it. From my testing key rotation returns really good error messages for these cases (but let me know if you find that not to be the case because we'll want to improve that).

All that said, this is just a suggestion - feel free to ignore it because it can definitely go either way.

.../src/main/java/com/azure/security/keyvault/keys/implementation/models/KeyRotationPolicy.java

maorleger · 2021-09-29T20:05:56Z

...java/com/azure/security/keyvault/keys/implementation/models/KeyRotationPolicyAttributes.java

+    private String expiryTime;
+
+    @JsonProperty(value = "created", access = JsonProperty.Access.WRITE_ONLY)
+    private Long createdOn;


Does Java have a better type for dates that we can use instead of a Long?

See my other comment on this.

maorleger · 2021-09-29T20:07:30Z

...java/com/azure/security/keyvault/keys/implementation/models/KeyRotationPolicyAttributes.java

+@Fluent
+public final class KeyRotationPolicyAttributes {
+    @JsonProperty(value = "expiryTime")
+    private String expiryTime;


What do other Java libraries use to model durations? we use string in JS because it's idiomatic for us, but Java might have a different way to model these types... consider checking with the service bus feature crew on how they model ISO8601 durations

Eh feel free to ignore me if this is all internal / serialization / generated code... I really don't know Java 😄

In the Java SDK we do have a preferred datetime type: OffsetDateTime. For serialization/deserialization internal classes can keep using String for expiryTime and Long for createdOn and updatedOn, but I think it would be better to use OffsetDateTime in public classes like you suggest.

...ult-keys/src/samples/java/com/azure/security/keyvault/keys/KeyClientJavaDocCodeSnippets.java

azure-sdk · 2021-09-30T06:24:43Z

API changes have been detected in this PR. You can review API changes here

azure-sdk · 2021-09-30T17:57:20Z

API changes have been detected in this PR. You can review API changes here

# Conflicts: # sdk/keyvault/azure-security-keyvault-keys/src/test/java/com/azure/security/keyvault/keys/KeyClientTestBase.java # sdk/keyvault/azure-security-keyvault-keys/src/test/resources/session-records/KeyAsyncClientManagedHsmTest.releaseKey[1].json # sdk/keyvault/azure-security-keyvault-keys/src/test/resources/session-records/KeyClientManagedHsmTest.releaseKey[1].json # sdk/keyvault/azure-security-keyvault-keys/src/test/resources/session-records/KeyClientTest.releaseKey[1].json

azure-sdk · 2021-09-30T23:48:16Z

API changes have been detected in this PR. You can review API changes here

maorleger

🚀 reviewed the public API and a just a few comments - I would suggest having someone from the Java team review the implementation

maorleger · 2021-10-01T21:35:14Z

.../main/java/com/azure/security/keyvault/keys/implementation/models/LifetimeActionTrigger.java

+    private String timeAfterCreate;
+
+    @JsonProperty(value = "timeBeforeExpiry")
+    private String timeBeforeExpiry;


As we discussed there might be a better type for this - Duration maybe or whatever servicebus queue is using. Whatever is idiomatic for Java to represent ISO8601 Durations.

I also know that this isn't the publicly exposed class but just calling it out here 👍

Same for timeAfterCreate and timeBeforeExpiry

I am refraining of using Duration for the time being since it doesn't play nice with durations longer than days. The alternative Period class is not meant for anything other than days/months/years. I'd rather keep using a String for now and hear what our architects have to say about this when we do the API review.

maorleger · 2021-10-01T21:37:16Z

...ault-keys/src/main/java/com/azure/security/keyvault/keys/models/KeyRotationPolicyAction.java

+ * Defines the types of key rotation policy actions that can be executed.
+ */
+public enum KeyRotationPolicyAction {
+    ROTATE("rotate"),


FYI I know the swagger says "rotate" and "notify" but please be aware that while the service accepts both it will return "Rotate" and "Notify" - if that matters for types / deserialization?

The serialization/deserialization library uses the fromString() method in this enum, which ignores casing, to create an appropriate instance.

* ADT ownership transitioning (#24404) * Remove SchemaRegistryClient caching (#24380) * Remove builder caching references. * Remove caching from SchemaRegistryAsyncClient. Make methods public for Response. * Remove cached tests. * Adding service annotation. * [Amqp-core, EH]: Prepending namespace|entitypath consistenty in log, first untrack processor subscriber then notify and adding retry to EventHubConsumer[Receiver]Client (#24417) * Added encryption scope blob sas * Hide HttpHeaders.toMultiMap API (#24428) * . (#24440) * mgmt, bug fix, container group without ports (#24418) * Update TRC in Azure Core (#24436) * Sync eng/common directory with azure-sdk-tools for PR 2046 (#24431) * Pass package name from calling pipeline to uniquely identify pull request review * Update log summary * Update eng/common/scripts/Detect-Api-Changes.ps1 Co-authored-by: Wes Haggard <weshaggard@users.noreply.github.com> Co-authored-by: praveenkuttappan <prmarott@microsoft.com> Co-authored-by: praveenkuttappan <55455725+praveenkuttappan@users.noreply.github.com> Co-authored-by: Wes Haggard <weshaggard@users.noreply.github.com> * Enable API change detection in PR pipeline (#24234) * Enable API change detection in PR pipeline * Communication: Add TokenCredentialAddHostHeaderPolicy for TokenCredential Requests (#24442) * Communication: Add TokenCredentialAdditionalHeaderPolicy for CallingServerClientBuilder * Add TokenCredentialAddHostHeaderPolicyTests * Fixing comment * Use URL class to get hostname * Fix style errors Co-authored-by: Melissa Neubert <mneubert@microsoft.com> * Add Compliance stage with policheck (#24276) * Add Compliance stage with policheck * Add vmImage pool * Fix issues flagged by PoliCheck * Move credscan into the compliance stage * update readme for storage libraries to include BOM information. (#22858) update readme for storage libraries * Enable dependency validation of a single library (#24241) * Enable validation of a library via it's POM file. * Incorporate feedback and remove unused code. * PR feedback * Move Form Recognizer beta to public (#24453) * Increment version for appconfiguration releases (#24450) * Rename certificates-refresh-interval to certificates-refresh-interval--in-ms in keyvault jca (#24339) * Prepare to release azure-spring-bom and azure-spring-cloud-dependencies. (#24425) * mgmt, support validateMoveResources (#24465) * Updated `KeyVaultCredentialPolicy` to extend `BearerTokenAuthenticationPolicy` in Key Vault clients. (#24199) * Replaced all uses of KeyVaultCredentialPolicy with BearerTokenAuthenticationPolicy in Key Vault clients. Removed the KeyVaultCredentialPolicy and ScopeTokeCache classes from all Track 2 Key Vault libraries. * We now pass the appropriate scope to BearerTokenAuthenticationPolicy creating a new instance in client builders, tests and samples. * Added tests and recordings for KEK tests on MHSM. Fixed and cleaned up tests. * Removed unused imports. * Renamed MHSM_SCOPE to MANAGED_HSM_SCOPE in all client builders. * Reintroduced KeyVaultCredentialPolicy and modified it to extend from BearerTokenAuthenticationPolicy while extracting the scope provided in bearer challenges returned by the Key Vault service. * Fixed CvheckStyle errors. * Made changes to KeyVaultCredentialPolicy so we don't set the body of a request as null, but an empty String instead. * Removed scope constants from Key vault client builders. * Attempted to fix flaky live tests. * Removed verify test for HSM as the FromSource test already verifies the build's code coverage and running in parallel against the same HSM can cause problems for some tests. * Reverted KeyVaultCredentialPolicy in all libraries to set the request body to null instead of an empty string when sending the first unauthenticated request to get a bearer challenge. Also stored the value of the "Content-Length" header in the pipeline context for use in a subsequent request. * Fixed KV Administration client live tests that failed due to the authentication policy changes. Also fixed some flaky live tests. * Fixed CheckStyle issues. * Fixed another CheckStyle issue. * Fixed issue that caused an NPE in KeyVaultCredentialPolicy if the content of the request being originally sent were null from the beginning. * Updated KeyVaultCredentialPolicy in all other libraries. * Made an attempt at fixing the backup async live tests. * Added sleep timer when running against service for restore operations. * Applied PR feedback. * [Storage] Try GMavenPlus to unblock Java 17 adoption. (#24471) * lets try. * fix java8 * fix java8 again:/ * track 1 * Update Form recognizer readme (#24476) * [Storage] Bump Groovy version to 3 that works with Java 17. (#24477) * bump groovy version * Revert "bump groovy version" This reverts commit a80c805. * use different spocks depending on java version. * rename. * Use New Javadoc Codesnippet Tooling to Support Java 17 (#24475) Use New Javadoc Snippet Tooling to Support Java 17 * Delete unused tests pipeline for track 1 blob package (#24488) * [Storage] Fix track 1 tests. (#24490) * Fix track 1 tests. * revert that. * Use Different Dummy Javadoc Option (#24491) * Update Jackson, Netty, and Reactor Versions (#24312) * Adding additional logging to ReactorDispatcher and ReactorExecutor. Adding closing logic (#24457) * Closing ReactorExecutor if it has never been run. * Adding documentation to ReactorDispatcher. * Updating ReactorExecutor to schedule close work when reactor has not started or scheduler is closed. * Adding tests. * In method invocations, adding catch for RejectedExecutionException in the case that the scheduler is disposed. * Adding assertion for ReactorExecutorTest that an onError is also called. * Adding documentatioln to reactor connection and timeout to closing execturo. * Splitting try/catch conditions. * Add documentation to RequestResponseChannel. * Using testPublisher for AmqpChannelProcessorTest. Using Flux.never(). * Adding Andy to the IoT CODEOWNERS (#24438) Adding Andy to the IoT CODEOWNERS * [Form Recognizer] Update to latest swagger (#24494) * Update docker-start-proxy.ps1 (#24495) Update to the latest version of the container Co-authored-by: Sean Kane <68240067+seankane-msft@users.noreply.github.com> * Added support for Key Rotation. (#24452) * Added support for Key Rotation. * Added tests and updated recordings where necessary. * Added code snippets for Key Rotation. Updated client documentation and existing code snippets. * Applied PR feedback. * Removed unused import. * Updated releaseKey test for MHSM. * Fixed tests after merge from main. * Renamed Mixed Audio models ( Addressed comments in apiview ) (#24481) * Suggestions after apiview review * Changes for the comments of API Review * tests added back * Added StartRecordingOptions class * Annotation added for new class * Setters return type changed * Renaming enum names ( feedback on APIView review ) Co-authored-by: Ninika Sharma <ninsharm@microsoft.com> * Prepare Azure Core Libraries for October 2021 Release (#24498) Prepare Azure Core Libraries for October 2021 Release * [EventGrid] Regenerate code using the latest rest commit sha (#24482) * Add Storage Live Test Run to Core Live Test Run (#24499) Add Storage Live Test Run to Core Live Test Run * [Storage] Run CI and live tests on Java 17 (#24492) * does this work? * try this. * Revert "try this." This reverts commit f157e60. * does this help ? * hmm? * hungry? * use java 17 in ci. * fix at least nio. * move it. * fixes. * disable these tests on java 17. CGLib doesn't work * fix that. * add support for setting throughput on database creation (#24456) * add support for setting throughput on database creation * added section to readme * removed locale from links * fix checkstyle issues * do not overwrite cosmosTemplate * Increment version for core releases (#24504) Increment package version after release of Core libraries * fix(*): use library RedirectPolicy now that it is available (#24502) * Fix azure-core-http-jdk-httpclient Tests (#24511) Fix azure-core-http-jdk-httpclient Tests * Add Form recognizer migration guide (#24472) * Fixed some test build failures Co-authored-by: David R. Williamson <drwill@microsoft.com> Co-authored-by: Connie Yau <conniey@microsoft.com> Co-authored-by: Anu Thomas Chandy <anuamd@hotmail.com> Co-authored-by: Alan Zimmer <48699787+alzimmermsft@users.noreply.github.com> Co-authored-by: Soyoung Eom <soeom@microsoft.com> Co-authored-by: Weidong Xu <weidxu@microsoft.com> Co-authored-by: Vinay Gera <vigera@microsoft.com> Co-authored-by: Azure SDK Bot <53356347+azure-sdk@users.noreply.github.com> Co-authored-by: praveenkuttappan <prmarott@microsoft.com> Co-authored-by: praveenkuttappan <55455725+praveenkuttappan@users.noreply.github.com> Co-authored-by: Wes Haggard <weshaggard@users.noreply.github.com> Co-authored-by: Melissa Neubert <melissa.neubert1@gmail.com> Co-authored-by: Melissa Neubert <mneubert@microsoft.com> Co-authored-by: Chidozie Ononiwu (His Righteousness) <31145988+chidozieononiwu@users.noreply.github.com> Co-authored-by: Pallavi Taneja <pallavit@users.noreply.github.com> Co-authored-by: Sameeksha Vaity <savaity@microsoft.com> Co-authored-by: liuzhicheng <70368631+zhichengliu12581@users.noreply.github.com> Co-authored-by: Rujun Chen <Rujun.Chen@microsoft.com> Co-authored-by: vcolin7 <vicolina@microsoft.com> Co-authored-by: Kamil Sobol <61715331+kasobol-msft@users.noreply.github.com> Co-authored-by: jamdavi <73593426+jamdavi@users.noreply.github.com> Co-authored-by: Sean Kane <68240067+seankane-msft@users.noreply.github.com> Co-authored-by: ninikasharma <67986119+ninikasharma@users.noreply.github.com> Co-authored-by: Ninika Sharma <ninsharm@microsoft.com> Co-authored-by: Shawn Fang <45607042+mssfang@users.noreply.github.com> Co-authored-by: Blackbaud-MikeLueders <Blackbaud-MikeLueders@users.noreply.github.com> Co-authored-by: Christian Whitehead (MSFT) <35080559+chrwhit@users.noreply.github.com>

vcolin7 added 3 commits September 29, 2021 11:22

Added support for Key Rotation.

6e22d9b

Added tests and updated recordings where necessary.

29822a1

Added code snippets for Key Rotation. Updated client documentation an…

4e396ab

…d existing code snippets.

vcolin7 requested review from maorleger, heaths, g2vinay and mccoyp September 29, 2021 18:26

vcolin7 requested a review from samvaity as a code owner September 29, 2021 18:26

ghost added the KeyVault label Sep 29, 2021

vcolin7 self-assigned this Sep 29, 2021

vcolin7 added this to the [2021] October milestone Sep 29, 2021

maorleger reviewed Sep 29, 2021

View reviewed changes

Applied PR feedback.

32f8e12

Removed unused import.

e844f93

vcolin7 added 3 commits September 30, 2021 11:57

Updated releaseKey test for MHSM.

67502a3

Fixed tests after merge from main.

825c3cd

maorleger approved these changes Oct 1, 2021

View reviewed changes

vcolin7 merged commit e89f243 into Azure:main Oct 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for Key Rotation. #24452

Added support for Key Rotation. #24452

vcolin7 commented Sep 29, 2021

check-enforcer bot commented Sep 29, 2021

azure-sdk commented Sep 29, 2021

maorleger left a comment

maorleger Sep 29, 2021

vcolin7 Sep 30, 2021

vcolin7 Sep 30, 2021

maorleger Sep 30, 2021

maorleger Sep 29, 2021

vcolin7 Sep 30, 2021

maorleger Sep 29, 2021

maorleger Sep 29, 2021

vcolin7 Sep 30, 2021

azure-sdk commented Sep 30, 2021

azure-sdk commented Sep 30, 2021

azure-sdk commented Sep 30, 2021

maorleger left a comment

maorleger Oct 1, 2021

vcolin7 Oct 1, 2021

maorleger Oct 1, 2021

vcolin7 Oct 1, 2021

Added support for Key Rotation. #24452

Added support for Key Rotation. #24452

Conversation

vcolin7 commented Sep 29, 2021

check-enforcer bot commented Sep 29, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

azure-sdk commented Sep 29, 2021

maorleger left a 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

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

azure-sdk commented Sep 30, 2021

azure-sdk commented Sep 30, 2021

azure-sdk commented Sep 30, 2021

maorleger left a 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