Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[tests] Add backup ssl sites in case of 429 response. #7909

Merged
merged 2 commits into from
Mar 22, 2023
Merged

[tests] Add backup ssl sites in case of 429 response. #7909

merged 2 commits into from
Mar 22, 2023

Conversation

dellis1972
Copy link
Contributor

Our SSL test can fail quite often with a 429 Too Many Requests error. This makes the CI very unstable as we are constantly having to wait and retry the tests. So lets put that logic into the test itself. If we get a 429 we should try some other ssl site.

@dellis1972
[tests] Add backup ssl sites in case of 429 response.
a6b9d79 
Our SSL test can fail quite often with a 429 Too Many Requests error.
This makes the CI very unstable as we are constantly having to wait and
retry the tests. So lets put that logic into the test itself. If we get
a 429 we should try some other ssl site.
@jonpryor jonpryor merged commit 112c832 into dotnet:main Mar 22, 2023
@jonpryor jonpryor mentioned this pull request Mar 22, 2023
Closed
jonathanpeppers pushed a commit that referenced this pull request Mar 22, 2023
@dellis1972 @jonathanpeppers
[tests] Add backup ssl sites in case of 429 response. (#7909)
11e163a 
Our SSL test can fail quite often with a HTTP-429 Too Many Requests
error.  This makes the CI very unstable as we are constantly having
to wait and retry the tests.

Improve this by putting that logic into the test itself.  If we get
an HTTP-429 we should try some other SSL site.
jonathanpeppers pushed a commit that referenced this pull request Mar 22, 2023
@dellis1972 @jonathanpeppers
[tests] Add backup ssl sites in case of 429 response. (#7909)
bc401a2 
Our SSL test can fail quite often with a HTTP-429 Too Many Requests
error.  This makes the CI very unstable as we are constantly having
to wait and retry the tests.

Improve this by putting that logic into the test itself.  If we get
an HTTP-429 we should try some other SSL site.
@dellis1972 dellis1972 deleted the ssltests branch March 22, 2023 17:08
grendello added a commit to grendello/xamarin-android that referenced this pull request Mar 23, 2023
@grendello
Merge branch 'main' into xamarin-app-utils
e546144 
* main:
  [build] remove .NET 6 support (dotnet#7900)
  [profiled-aot] update `dotnet.aotprofile` for .NET 8 (dotnet#7908)
  [tests] Add backup ssl sites in case of 429 response. (dotnet#7909)
  $(AndroidPackVersionSuffix)=preview.4; net8 is 34.0.0-preview.4 (dotnet#7912)
jonpryor pushed a commit that referenced this pull request Mar 31, 2023
@dellis1972
[tests] Use msftconnecttest.com in QuoteInvalidQuoteUrlsShouldWork (#…
8d6080c 
…7919)

Context: 112c832
Context: https://bugzilla.xamarin.com/show_bug.cgi?id=14968
Context: xamarin/monodroid@5e923e3
Context: xamarin/monodroid@2e5f7bb

For historical purposes…

<https://bugzilla.xamarin.com/show_bug.cgi?id=14968> is (was?) a bug
wherein accessing an "invalid" URL such as
<http://example.com/?query&foo|bar> would result in a
`URISyntaxException` at runtime:

	Exception of type 'Java.Net.URISyntaxException' was thrown.
	  at Android.Runtime.JNIEnv.NewObject (IntPtr jclass, IntPtr jmethod, Android.Runtime.JValue[] parms)
	  at Java.Net.URI..ctor (System.String uri)
	  at Android.Runtime.AndroidEnvironment+_Proxy.IsBypassed (System.Uri host)
	  at System.Net.ServicePointManager.FindServicePoint (System.Uri address, IWebProxy proxy)
	  at System.Net.HttpWebRequest.GetServicePoint ()
	  at System.Net.HttpWebRequest.BeginGetResponse (System.AsyncCallback callback, System.Object state)
	  at System.Net.HttpWebRequest.GetResponse ()
	  at Xamarin.Android.RuntimeTests.ProxyTest.QuoteInvalidQuoteUrlsShouldWork ()
	  at (wrapper managed-to-native) System.Reflection.MonoMethod:InternalInvoke (System.Reflection.MonoMethod,object,object[],System.Exception&)
	  at System.Reflection.MonoMethod.Invoke (System.Object obj, BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture)
	  --- End of managed exception stack trace ---
	java.net.URISyntaxException: Illegal character in query at index 29: http://example.com/?query&foo|bar
		at libcore.net.UriCodec.validate(UriCodec.java:63)
		at java.net.URI.parseURI(URI.java:406)
		at java.net.URI.<init>(URI.java:204)
		at xamarin.android.nunitlite.TestSuiteInstrumentation.n_onStart(Native Method)
		at xamarin.android.nunitlite.TestSuiteInstrumentation.onStart(TestSuiteInstrumentation.java:39)
		at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701)

Index 29 is `|` (zero-based!).

The "problem" here that using `|` within the query string is accepted
under .NET & Mono, which resulted in a "breakage of expectations" for
Xamarin.Android customers ("this URL works on .NET!").

The "fix" was to use [`Uri.AbsoluteUri`][5], which replaces `|` with
`%7C`, hex-escaping the offending value and adding the
`ProxyTest.QuoteInvalidQuoteUrlsShouldWork()` unit test.

That background done, commit 112c832 updated
`ProxyTest.QuoteInvalidQuoteUrlsShouldWork()` to hit
<https://bing.com/?query&foo|bar> instead of
<http://example.com/?query&foo|bar>, on the assumption that it would
be "better" if we *didn't* use non-Microsoft URLs as part of our unit
tests.

The problem is that this didn't work (eep?! not sure how PR #7909
was green here!):

	System.Net.WebException : net_http_ssl_connection_failed
	----> System.Net.Http.HttpRequestException : net_http_ssl_connection_failed
	----> System.Security.Authentication.AuthenticationException : net_auth_SSPI
	----> Interop+AndroidCrypto+SslException : Exception_WasThrown, Interop+AndroidCrypto+SslException

Update `ProxyTest.QuoteInvalidQuoteUrlsShouldWork()` to instead hit
<http://www.msftconnecttest.com/connecttest.txt?query&foo|bar>, which
is a Microsoft-owned server -- no possible DOS'ing of 
<http://example.com> -- which *passes* for this particular test.

[0]: https://developer.android.com/reference/java/net/URI#URI(java.lang.String)
[1]: https://www.ietf.org/rfc/rfc2396.txt
[2]: https://www.ietf.org/rfc/rfc3986.html#section-3.4
[3]: https://android.googlesource.com/platform/libcore/+/jb-mr2-release/luni/src/main/java/libcore/net/UriCodec.java#46
[4]: https://android.googlesource.com/platform/libcore/+/jb-mr2-release/luni/src/main/java/java/net/URI.java#349
[5]: https://learn.microsoft.com/en-us/dotnet/api/system.uri.absoluteuri?view=net-7.0
grendello added a commit to grendello/xamarin-android that referenced this pull request Apr 6, 2023
@grendello
Merge branch 'main' into native-profiling-and-debugging
82a52d2 
* main: (94 commits)
  [ci] Remove remaining Classic test jobs. (dotnet#7924)
  Add Nightly Tests for Humanizer
  [readme] Add aka.ms links for d17.5. (dotnet#7935)
  [tests] Remove `net472` multitargeting (dotnet#7932)
  [monodroid] Fix `ld` build error on Nightly Builds. (dotnet#7925)
  Bump to xamarin/Java.Interop/main@0355acf (dotnet#7931)
  [tests] Use msftconnecttest.com in QuoteInvalidQuoteUrlsShouldWork (dotnet#7919)
  [ci] Don't set demands for megapipeline (dotnet#7929)
  [Mono.Android] Bind API-UpsideDownCake Developer Preview 1 (dotnet#7796)
  Bump to dotnet/installer@d109cba3ff 8.0.100-preview.4.23176.5 (dotnet#7921)
  [Xamarin.Android.Build.Tasks] Fix Android Version Code for Release builds (dotnet#7795)
  Bump to dotnet/installer@27d6769dfb 8.0.100-preview.4.23172.16 (dotnet#7910)
  [Xamarin.Android.Build.Tasks] Fix native code generation when marshal methods are disabled (dotnet#7899)
  [ci] Optimize 'APK's .NET' test job overhead. (dotnet#7904)
  [Mono.Android] delay JNINativeWrapper.get_runtime_types() (dotnet#7913)
  Bump external/Java.Interop from `73ebad2` to `53bfb4a` (dotnet#7914)
  [build] remove .NET 6 support (dotnet#7900)
  [profiled-aot] update `dotnet.aotprofile` for .NET 8 (dotnet#7908)
  [tests] Add backup ssl sites in case of 429 response. (dotnet#7909)
  $(AndroidPackVersionSuffix)=preview.4; net8 is 34.0.0-preview.4 (dotnet#7912)
  ...
@github-actions github-actions bot locked and limited conversation to collaborators Jan 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonathanpeppers jonathanpeppers jonathanpeppers approved these changes

@jonpryor jonpryor Awaiting requested review from jonpryor jonpryor is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dellis1972 @jonathanpeppers @jonpryor