feat: Check BIOS for determining GCE residency. #2577

amanda-tarafa · 2023-10-27T07:20:32Z

@jskeet a few comments on this one:

The first commit is the implementation for Linux which is more or less straightforward.
The second commit is the implementation for Windows, checking against Windows Management Instrumentation and not the Registry. Checking against WMI is what's recommend internally.

I'm happy to rewrite the second commit using reflection so we don't need the extra System.Management dependency, but then we'd have to ask users to include the dll themselves as we cannot add it conditionally on OS, or at least I haven't found how to do that.

jskeet

Lots of small comments; happy with the general thrust though. Have scheduled a meeting to chat about some of these bits.

jskeet · 2023-10-27T07:53:42Z

Src/Support/Google.Apis.Auth/OAuth2/ComputeCredential.cs

            return false;
        }
+
+        private async static Task<bool> IsGoogleBiosAsync()


Nit: private static async

jskeet · 2023-10-27T07:54:11Z

Src/Support/Google.Apis.Auth/OAuth2/ComputeCredential.cs

+                }
+                else
+                {
+                    Logger.Info("GCE residency detection through BIOS checking is only supported in Windows and Linux platforms.");


Change "in" to "on" maybe?

Yep, better with "on".

jskeet · 2023-10-27T07:54:54Z

Src/Support/Google.Apis.Auth/OAuth2/ComputeCredential.cs

+            bool IsWindows()
+            {
+#if NET45 || NET461
+                // RuntimeInformation.IsOsPlatform is not available for these targets.


Unless we're using Mono, I guess? We should probably work out what that will do - so long as it's not disastrous, that's fine.

I was mistaken with this, RuntimeInformation.IsOsPlatform is supported in NET461. So that only leaves NET45, but that's going away in January because of #2561, so I'm changing this to always return false in NET45.

jskeet · 2023-10-27T07:56:05Z

Src/Support/Google.Apis.Auth/OAuth2/ComputeCredential.cs

+                    productName = await streamReader.ReadLineAsync().ConfigureAwait(false);
+                }
+                productName = productName?.Trim();
+                return productName == "Google" || productName == "Google Compute Engine";


Do we yet know whether this will be accurate for custom universe?

I've asked.

jskeet · 2023-10-27T07:57:39Z

Src/Support/Google.Apis.Auth/OAuth2/ComputeCredential.cs

@@ -295,7 +296,11 @@ public static Task<bool> IsRunningOnComputeEngine()
            return isRunningOnComputeEngineCached.Value;
        }

-        private static async Task<bool> IsRunningOnComputeEngineNoCache()
+        private static async Task<bool> IsRunningOnComputeEngineNoCacheAsync() =>
+            await IsMetadataServerAvailableAsync().ConfigureAwait(false)


Should this be the other way round? Possibly something to chat about on a VC - especially if we're going to increase the timeout. (I can see multiple angles to this...)

We talked about this. Keeping as is for the time being to cause the least disruption to users that were relying on these retries to wait for the metadata server to be up.

jskeet · 2023-10-27T08:00:17Z

Src/Support/Google.Apis.Auth/OAuth2/ComputeCredential.cs

+                System.Management.ManagementClass biosClass = new ("Win32_BIOS");
+                using var instances = biosClass.GetInstances();
+
+                foreach(var instance in instances) using (instance)


If we need to dispose of every instance, should we do that regardless of whether we have an early "out"? Or are they only allocated lazily by the enumerator?
(Either way, I'd prefer to use more braces and indentation instead of this formatting, personally.)

Changed braces and indetantion, and also, we are now iterating over the whole collection to dispose of all possible instances (unlikely there's more than one though).

jskeet · 2023-10-27T08:00:45Z

Src/Support/Google.Apis.Auth/OAuth2/ComputeCredential.cs

+                foreach(var instance in instances) using (instance)
+                {
+                    // We should only find one instance for Win32_BIOS class.
+                    // This will throw a ManagementException "NotFound" if the "Manufacturer" property


But if there are multiple instances, it could be present in one other than the first one, no?

Yes, in principle I guess it could happen. We are now iterating over all instances.

jskeet · 2023-10-27T08:01:11Z

Src/Support/Google.Apis.Auth/OAuth2/ComputeCredential.cs

+                    // We should only find one instance for Win32_BIOS class.
+                    // This will throw a ManagementException "NotFound" if the "Manufacturer" property
+                    // is not present. That's fine, we are catching the exception elsewhere and logging it.
+                    return instance["Manufacturer"]?.ToString() == "Google";


This looks like a somewhat different check to the Linux one - is that okay?

Yes, it's different checks.

jskeet

Talked about all of this - Amanda will make a few tweaks, but most of my comments were irrelevant :)

amanda-tarafa

@jskeet I've added a third commit with the changes in case you want and can to take a look. Else since we had talked about these, I'll squash in a single commit and merge later today.

amanda-tarafa · 2023-10-27T16:07:19Z

Src/Support/Google.Apis.Auth/OAuth2/ComputeCredential.cs

@@ -295,7 +296,11 @@ public static Task<bool> IsRunningOnComputeEngine()
            return isRunningOnComputeEngineCached.Value;
        }

-        private static async Task<bool> IsRunningOnComputeEngineNoCache()
+        private static async Task<bool> IsRunningOnComputeEngineNoCacheAsync() =>
+            await IsMetadataServerAvailableAsync().ConfigureAwait(false)


We talked about this. Keeping as is for the time being to cause the least disruption to users that were relying on these retries to wait for the metadata server to be up.

amanda-tarafa · 2023-10-27T16:08:15Z

Src/Support/Google.Apis.Auth/OAuth2/ComputeCredential.cs

            return false;
        }
+
+        private async static Task<bool> IsGoogleBiosAsync()


amanda-tarafa · 2023-10-27T16:08:54Z

Src/Support/Google.Apis.Auth/OAuth2/ComputeCredential.cs

+                }
+                else
+                {
+                    Logger.Info("GCE residency detection through BIOS checking is only supported in Windows and Linux platforms.");


Yep, better with "on".

amanda-tarafa · 2023-10-27T17:20:54Z

Src/Support/Google.Apis.Auth/OAuth2/ComputeCredential.cs

+            bool IsWindows()
+            {
+#if NET45 || NET461
+                // RuntimeInformation.IsOsPlatform is not available for these targets.


I was mistaken with this, RuntimeInformation.IsOsPlatform is supported in NET461. So that only leaves NET45, but that's going away in January because of #2561, so I'm changing this to always return false in NET45.

amanda-tarafa · 2023-10-27T17:28:02Z

Src/Support/Google.Apis.Auth/OAuth2/ComputeCredential.cs

+                    productName = await streamReader.ReadLineAsync().ConfigureAwait(false);
+                }
+                productName = productName?.Trim();
+                return productName == "Google" || productName == "Google Compute Engine";


I've asked.

amanda-tarafa · 2023-10-27T17:36:05Z

Src/Support/Google.Apis.Auth/OAuth2/ComputeCredential.cs

+                System.Management.ManagementClass biosClass = new ("Win32_BIOS");
+                using var instances = biosClass.GetInstances();
+
+                foreach(var instance in instances) using (instance)


Changed braces and indetantion, and also, we are now iterating over the whole collection to dispose of all possible instances (unlikely there's more than one though).

amanda-tarafa · 2023-10-27T17:36:44Z

Src/Support/Google.Apis.Auth/OAuth2/ComputeCredential.cs

+                foreach(var instance in instances) using (instance)
+                {
+                    // We should only find one instance for Win32_BIOS class.
+                    // This will throw a ManagementException "NotFound" if the "Manufacturer" property


Yes, in principle I guess it could happen. We are now iterating over all instances.

amanda-tarafa · 2023-10-27T17:36:58Z

Src/Support/Google.Apis.Auth/OAuth2/ComputeCredential.cs

+                    // We should only find one instance for Win32_BIOS class.
+                    // This will throw a ManagementException "NotFound" if the "Manufacturer" property
+                    // is not present. That's fine, we are catching the exception elsewhere and logging it.
+                    return instance["Manufacturer"]?.ToString() == "Google";


Yes, it's different checks.

jskeet · 2023-10-27T18:39:24Z

Yup, looks good.

amanda-tarafa · 2023-10-27T18:40:56Z

Yup, looks good.

Thanks, merging and rebasing. I'll prepara a PR for a release on Monday as well.

Features * #2577 Checks BIOS for detecting GCE residency.

amanda-tarafa requested a review from jskeet October 27, 2023 07:20

amanda-tarafa requested a review from a team as a code owner October 27, 2023 07:20

jskeet reviewed Oct 27, 2023

View reviewed changes

jskeet approved these changes Oct 27, 2023

View reviewed changes

amanda-tarafa commented Oct 27, 2023

View reviewed changes

feat: Check BIOS for GCE residency.

5374a51

amanda-tarafa force-pushed the compute-detection branch from aa44b7f to 5374a51 Compare October 27, 2023 18:44

amanda-tarafa merged commit 8cc1a76 into main Oct 27, 2023

amanda-tarafa deleted the compute-detection branch October 27, 2023 18:50

amanda-tarafa added a commit that referenced this pull request Oct 27, 2023

Update version: 1.62.1 -> 1.63.0

ba71688

Features * #2577 Checks BIOS for detecting GCE residency.

amanda-tarafa mentioned this pull request Oct 27, 2023

Update version: 1.62.1 -> 1.63.0 #2579

Merged

amanda-tarafa added a commit that referenced this pull request Oct 30, 2023

Update version: 1.62.1 -> 1.63.0

c672824

Features * #2577 Checks BIOS for detecting GCE residency.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Check BIOS for determining GCE residency. #2577

feat: Check BIOS for determining GCE residency. #2577

amanda-tarafa commented Oct 27, 2023

jskeet left a comment

jskeet Oct 27, 2023

amanda-tarafa Oct 27, 2023

jskeet Oct 27, 2023

amanda-tarafa Oct 27, 2023

jskeet Oct 27, 2023

amanda-tarafa Oct 27, 2023

jskeet Oct 27, 2023 •

edited by amanda-tarafa

Loading

amanda-tarafa Oct 27, 2023

jskeet Oct 27, 2023

amanda-tarafa Oct 27, 2023

jskeet Oct 27, 2023

amanda-tarafa Oct 27, 2023

jskeet Oct 27, 2023

amanda-tarafa Oct 27, 2023

jskeet Oct 27, 2023

amanda-tarafa Oct 27, 2023

jskeet left a comment

amanda-tarafa left a comment

amanda-tarafa Oct 27, 2023

amanda-tarafa Oct 27, 2023

amanda-tarafa Oct 27, 2023

amanda-tarafa Oct 27, 2023

amanda-tarafa Oct 27, 2023

amanda-tarafa Oct 27, 2023

amanda-tarafa Oct 27, 2023

amanda-tarafa Oct 27, 2023

jskeet commented Oct 27, 2023

amanda-tarafa commented Oct 27, 2023

feat: Check BIOS for determining GCE residency. #2577

feat: Check BIOS for determining GCE residency. #2577

Conversation

amanda-tarafa commented Oct 27, 2023

jskeet 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

jskeet Oct 27, 2023 • edited by amanda-tarafa Loading

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

jskeet left a comment

Choose a reason for hiding this comment

amanda-tarafa 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

jskeet commented Oct 27, 2023

amanda-tarafa commented Oct 27, 2023

jskeet Oct 27, 2023 •

edited by amanda-tarafa

Loading