mgmt, testproxy migration, azure-resourcemanager-resources #35476
Conversation
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
|
Deferring to @scbedd for issues related to installing the tooling (Question 1). |
...sourcemanager-test/src/main/java/com/azure/resourcemanager/test/ResourceManagerTestBase.java
Outdated
Show resolved
Hide resolved
...emanager-test/src/main/java/com/azure/resourcemanager/test/policy/TextReplacementPolicy.java
Outdated
Show resolved
Hide resolved
|
Hi @samvaity , how about question 0? this one is resolved offline. |
|
Hi @samvaity, the tests fail in CI in playback mode, but from my local they are successful. Do you have any suggestion or insight for this failure? Not sure if there is additional steps to be done in CI. Thanks. |
haolingdong-msft
changed the title
|
To directly answer question 2. The proxy is using the default Reference for what you need to do to make this error go away. |
Hey @haolingdong-msft , can you try a fresh clone of your forked branch? I have a feeling that you have some files locally cached that are causing this not to repro locally. If you look at the build error it's actually telling you exactly what the problem is in CI. This makes me think that you have changes locally that either A) pull down different recordings or B) something else that I'm unaware of. |
|
Hi @scbedd, this uri mismatch is because in playback mode, we will set subscription id to ZERO_UUID (00000000-0000-0000-0000-000000000000), but in RECORD mode, we will use a real sub id passed from env variable, it causes match error. To fix this, I have add a sanitizer to redact real sub id to ZERO_UUID. I want to use matcher to skip comparing sub id in url previously, but I found our matcher does not support url parth. I have already fixed it in this commit 906806f. Please let me know if you have better solution to solve this. |
|
Hi @samvaity @scbedd , I found when I updated session records locally, and want to re-push to assets repo, the recordings in assets repo cannot get updated. Command I use is: I also tried to delete assets.json and .assets dir, and tried to initialize assets.json using below command, but still meet error. Could you please help to suggest？ |
Hey @haolingdong-msft , the reason that the push is failing and the tag isn't being updated is because there is no content being pushed! This specific part of your log
Indicates to me that no files are being moved into the assets repo. If no files are moved, the tag won't be created, because there will be no new changes to commit! You definitely have an updated assets.json in this PR, so I'm not certain why you are invoking this script to complete your Once you've completed initial migration, you only need run your tests in record mode, then You do not want to re-use the migration script to push recordings. Once the assets.json exists, your recording framework should be dropping the updated recordings already in the |
|
/azp run java - resourcemanager - ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hey @scbedd, if you look at my previous comment. I used initial push because I tried testproxy -push first, it succeeds, but seems assets repo not get updated and CI still fails. I tried again just now, seems this time
|
haolingdong-msft
requested review from
XiaofeiCao and
ArthurMa1978
as code owners
| @@ -86,7 +86,9 @@ public Mono<HttpResponse> process(HttpPipelineCallContext context, HttpPipelineN | |||
| return next.process() | |||
| .doOnError(throwable -> { | |||
| networkCallRecord.setException(new NetworkCallError(throwable)); | |||
| recordedData.addNetworkCall(networkCallRecord); | |||
| if (recordedData != null) { // when using testproxy, recordedData will be set to null | |||
There was a problem hiding this comment.
If recordedData is null, what TextReplacementPolicy.java does in test proxy setup?
If it does nothing (as it not able to do the addNetworkCall), can we not change this file, and not use it in TestProxyTestBase?
There was a problem hiding this comment.
I had the same thoughts that I had shared with @haolingdong-msft too. We should not be depending on recordedData/networkCall as with TestProxyTestBase we never initialize this data.
| /** | ||
| * @return random password | ||
| */ | ||
| public static String password() { | ||
| // do not record | ||
| String password = new ResourceNamer("").randomName("Pa5$", 12); | ||
| LOGGER.info("Password: {}", password); | ||
| return password; | ||
| } | ||
|
|
||
| private static String sshPublicKey; | ||
|
|
||
| /** | ||
| * @return an SSH public key | ||
| */ | ||
| public static String sshPublicKey() { | ||
| if (sshPublicKey == null) { | ||
| try { | ||
| KeyPairGenerator keyGen = KeyPairGenerator.getInstance("RSA"); | ||
| keyGen.initialize(1024); | ||
| KeyPair pair = keyGen.generateKeyPair(); | ||
| PublicKey publicKey = pair.getPublic(); | ||
|
|
||
| RSAPublicKey rsaPublicKey = (RSAPublicKey) publicKey; | ||
| ByteArrayOutputStream byteOs = new ByteArrayOutputStream(); | ||
| DataOutputStream dos = new DataOutputStream(byteOs); | ||
| dos.writeInt("ssh-rsa".getBytes(StandardCharsets.US_ASCII).length); | ||
| dos.write("ssh-rsa".getBytes(StandardCharsets.US_ASCII)); | ||
| dos.writeInt(rsaPublicKey.getPublicExponent().toByteArray().length); | ||
| dos.write(rsaPublicKey.getPublicExponent().toByteArray()); | ||
| dos.writeInt(rsaPublicKey.getModulus().toByteArray().length); | ||
| dos.write(rsaPublicKey.getModulus().toByteArray()); | ||
| String publicKeyEncoded = new String(Base64.getEncoder().encode(byteOs.toByteArray()), StandardCharsets.US_ASCII); | ||
| sshPublicKey = "ssh-rsa " + publicKeyEncoded; | ||
| } catch (NoSuchAlgorithmException | IOException e) { | ||
| throw LOGGER.logExceptionAsError(new IllegalStateException("failed to generate ssh key", e)); | ||
| } | ||
| } | ||
| return sshPublicKey; | ||
| } |
There was a problem hiding this comment.
I assume these are duplicate code? Either use a common base, or move them to util class.
There was a problem hiding this comment.
I am fine with some duplicate code, but not lots of them.
There was a problem hiding this comment.
Currently ResourceManagerTestProxyTestBase is just a copy of ResourceManagerTestBase, with updates in 'code change' section in description to fit in TestProxy.
Let me try to refine the code to make it easier to reuse the code in ResourceManagerTestBase and easier to see the difference.
...ager-test/src/main/java/com/azure/resourcemanager/test/ResourceManagerTestProxyTestBase.java
Outdated
Show resolved
Hide resolved
| protected HttpClient generateHttpClientWithProxy(NettyAsyncHttpClientBuilder clientBuilder, ProxyOptions proxyOptions) { | ||
| if (clientBuilder == null) { | ||
| clientBuilder = new NettyAsyncHttpClientBuilder(); | ||
| } | ||
| if (proxyOptions != null) { | ||
| clientBuilder.proxy(proxyOptions); | ||
| } else { | ||
| try { | ||
| System.setProperty(USE_SYSTEM_PROXY, VALUE_TRUE); | ||
| List<Proxy> proxies = ProxySelector.getDefault().select(new URI(AzureEnvironment.AZURE.getResourceManagerEndpoint())); | ||
| if (!proxies.isEmpty()) { | ||
| for (Proxy proxy : proxies) { | ||
| if (proxy.address() instanceof InetSocketAddress) { | ||
| String host = ((InetSocketAddress) proxy.address()).getHostName(); | ||
| int port = ((InetSocketAddress) proxy.address()).getPort(); | ||
| switch (proxy.type()) { | ||
| case HTTP: | ||
| return clientBuilder.proxy(new ProxyOptions(ProxyOptions.Type.HTTP, new InetSocketAddress(host, port))).build(); | ||
| case SOCKS: | ||
| return clientBuilder.proxy(new ProxyOptions(ProxyOptions.Type.SOCKS5, new InetSocketAddress(host, port))).build(); | ||
| default: | ||
| } | ||
| } | ||
| } | ||
| } | ||
| String host = null; | ||
| int port = 0; | ||
| if (System.getProperty(HTTPS_PROXY_HOST) != null && System.getProperty(HTTPS_PROXY_PORT) != null) { | ||
| host = System.getProperty(HTTPS_PROXY_HOST); | ||
| port = Integer.parseInt(System.getProperty(HTTPS_PROXY_PORT)); | ||
| } else if (System.getProperty(HTTP_PROXY_HOST) != null && System.getProperty(HTTP_PROXY_PORT) != null) { | ||
| host = System.getProperty(HTTP_PROXY_HOST); | ||
| port = Integer.parseInt(System.getProperty(HTTP_PROXY_PORT)); | ||
| } | ||
| if (host != null) { | ||
| clientBuilder.proxy(new ProxyOptions(ProxyOptions.Type.HTTP, new InetSocketAddress(host, port))); | ||
| } | ||
| } catch (URISyntaxException e) { } | ||
| } | ||
| return clientBuilder.build(); | ||
| } |
There was a problem hiding this comment.
I assume this also duplicate code?
haolingdong-msft
changed the title
| @@ -241,7 +241,7 @@ protected void beforeTest() { | |||
| if (isPlaybackMode()) { | |||
| if (interceptorManager.getRecordedData() == null) { | |||
| skipInPlayback(); | |||
| return; | |||
| // return; | |||
There was a problem hiding this comment.
Remove return here because when using testproxy, interceptorManager.getRecordedData() will be null. If return directly, when running in playback mode, below logics to initialize clients will be skiped.
There will be nullpointer error since the client is not initialized when running tests in playback.
There was a problem hiding this comment.
Since this is TextProxyTestBase, I take interceptorManager.getRecordedData() is always null, and skipInPlayback() is always called?
And why do we want skipInPlayback() always called? (regardless whether the test want to skip in playback or not)
There was a problem hiding this comment.
isSkipInPlayback = true seems meaning it does not clean up the resource after test complete?
There was a problem hiding this comment.
isSkipInPlayback = trueseems meaning it does not clean up the resource after test complete?
Yes, if we set to true, it will not do test resource clean up calls. Previously I think it is playback mode, there is no need to do resource clean up. But to be consistent with previous logic, we can do resouce clean up as well, that is to delete below whole part. What do you think?
if (interceptorManager.getRecordedData() == null) {
skipInPlayback();
return;
}There was a problem hiding this comment.
Yeah, you can do this.
Later we may get to test case with @DoNotRecord(skipInPlayback = true), which may have problem under TestProxy setting (I kind of assume code here was to handle that, but not sure). You can handle that later.
There was a problem hiding this comment.
@DoNotRecord(skipInPlayback = true) -> there is this case in resources. I will delete this part logic and try on playback and record mode.
| "AssetsRepo": "Azure/azure-sdk-assets", | ||
| "AssetsRepoPrefixPath": "java", | ||
| "TagPrefix": "java/resourcemanager/azure-resourcemanager-resources", | ||
| "Tag": "java/resourcemanager/azure-resourcemanager-resources_da922b0432" |
There was a problem hiding this comment.
There was a problem hiding this comment.
And retry-after seems not handled.
https://github.com/Azure/azure-sdk-assets/blob/java/resourcemanager/azure-resourcemanager-resources_da922b0432/java/sdk/resourcemanager/azure-resourcemanager-resources/src/test/resources/session-records/DeploymentsTests.canDeployVirtualNetwork.json#L741
(but I am not sure whether there is logic to set it to 0, after load the data. but previously the recorded data is 0
Wondering whether playback on long LRO (e.g. sql server) can finish in 30 seconds (current cut-off time for each case in playback).
There was a problem hiding this comment.
api-version still there is expected, but in test base (this mather only works in playback mode), we will ignore match api-version, the logic is here:
joshfree
added
the
test-proxy
|
I am also noticing the sub-id in teh response body
This can be sanitized with the sanitizer |
|
Thanks Sameeksha. We may also want to sanitize the LRO headers in response, e.g. Though in playback, seems the match function can handle the difference of real subscriptionId and the all 0 ones (or LRO won't work). |
...emanager-test/src/main/java/com/azure/resourcemanager/test/policy/TextReplacementPolicy.java
Outdated
Show resolved
Hide resolved
| interceptorManager.addSanitizers(Arrays.asList(new TestProxySanitizer("(?<=/subscriptions/)([^/?]+)", ZERO_UUID, TestProxySanitizerType.URL))); | ||
| interceptorManager.addSanitizers(Arrays.asList(new TestProxySanitizer("(?<=%2Fsubscriptions%2F)([^/?]+)", ZERO_UUID, TestProxySanitizerType.URL))); | ||
| // Retry-After | ||
| interceptorManager.addSanitizers(Arrays.asList(new TestProxySanitizer("Retry-After", null, "0", TestProxySanitizerType.HEADER))); |
There was a problem hiding this comment.
One corner case would be service does not set retry-after header.
Though I thought SDK will set the default LRO poll interval to something like 0 during playback, so we are likely fine.
|
LGTM. Have one comment. You can merge after revert changes in "TextReplacementPolicy.java" and CI pass. |
Sanitizers seem to do the work in playback mode(thought match function would do that too). |
|
I've encountered timeout reported by Netty http client when batch running playback client using log here: timeout error stack
This only occurs when |
|
Strange... |
|
Issue logged: #35992 |
Description
Before running test, you will need to set below environment variables as usual:
AZURE_SUBSCRIPTION_ID: Use the id value from az account show in the Azure CLI 2.0.
AZURE_CLIENT_ID: Use the appId value from the output taken from a service principal output.
AZURE_CLIENT_SECRET: Use the password value from the service principal output.
AZURE_TENANT_ID: Use the tenant value from the service principal output.
AZURE_TEST_MODE=RECORD
Code change:
subscription id not match failure like below:
Question 0:
I met .net environment error (see below) when trying to install testproxy into PATH. Before solving the env issue and initiate assets.json, I want to first check the recorded session records file content, so I run in IDE and run the tests in RECORD mode, the test can pass, but I found the recordings seems not correct, please refer to the file changes in this pr.
@samvaity Is this expected? Must we first initiate assets.json and then do the recording?
Please note I have updated this
TextReplacementPolicy(sdk/resourcemanager/azure-resourcemanager-test/src/main/java/com/azure/resourcemanager/test/policy/TextReplacementPolicy.java) since currently testproxy will setrecordedDatatonullwhich will causeNullPointorExceptionin theTextReplacementPolicy.Question 1:
The testproxy installation issue above can be resolved by clean up the
%AppData%\Roaming\NuGet\NuGet.Configfile. After deleting theNuGet.Configfile, installation succeeds.But after I added back the needed sources "CosmosProd" to
NuGet.Config, installation fails again..This will cause updating testproxy in the future.@samvaity , seems the testproxy source is conflict with my existing source called
CosmosProd. I have pasted my existing sources below. I am a newbee in .net world, wondering can the command in doc be updated to avoid this kind of failure?nuget.config
Question 2:
Encounter below error when initialize assets.json. @samvaity , could you please check the failure and help to advice?
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines