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

Ensure the consistency of targeting evaluation across CPU architectures. #405

Merged
merged 5 commits into from
Apr 7, 2024
Merged

Ensure the consistency of targeting evaluation across CPU architectures. #405

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
30ec65c
check system endianness
zhiyuanliang-ms Mar 20, 2024
4eeed58
remove period
zhiyuanliang-ms Mar 20, 2024
a8bbe48
update reverse method
zhiyuanliang-ms Mar 21, 2024
ec9ae15
update comment
zhiyuanliang-ms Mar 28, 2024
17e13d8
adjust comment
zhiyuanliang-ms Apr 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/Microsoft.FeatureManagement/Targeting/TargetingEvaluator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,14 +343,23 @@ private static bool IsTargeted(string contextId, double from, double to)
{
byte[] hash;

//
// Cryptographic hashing algorithms ensure adequate entropy across hash
using (HashAlgorithm hashAlgorithm = SHA256.Create())
{
hash = hashAlgorithm.ComputeHash(Encoding.UTF8.GetBytes(contextId));
}


//
// Use first 4 bytes for percentage calculation
// Cryptographic hashing algorithms ensure adequate entropy across hash
// Endianness check ensures the consistency of targeting evaluation result across different architectures
if (!BitConverter.IsLittleEndian)
{
Array.Reverse(hash, 0, 4);
}

//
// The first 4 bytes of the hash will be used for percentage calculation
uint contextMarker = BitConverter.ToUInt32(hash, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of want to explicitly subset the hash to the first 4 bytes just to make things clear. But not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean:

byte[] relevantBytes = new byte[4];

Array.Copy(hash, 0, relevantBytes, 0, 4);


if (!BitConverter.IsLittleEndian)
{
    Array.Reverse(relevantBytes);
}

uint contextMarker = BitConverter.ToUInt32(relevantBytes, 0);

I used to implement it like this in 30ec65c43b35ebe46ef7c7327b327b150e7a0f18

Copy link
Contributor

@rossgrambo rossgrambo Mar 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like that, but I was hoping for something more like:

uint contextMarker = BitConverter.ToUInt32(hash.Take(4), 0);

// or more explicitly
byte[] firstFourBytes = hash.Take(4);

uint contextMarker = BitConverter.ToUInt32(firstFourBytes, 0);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's allocation for no reason.

Copy link
Contributor

@rossgrambo rossgrambo Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reason is for the next developer who's reading it to understand it- the way it is now makes it look like we reverse the first 4 bytes but somehow use all of the bytes to determine a resulting uint.

If we'd rather not add the extra array, maybe we could adjust the comments to make it more clear to me. What if we reordered/adjusted them like this:

//
// Cryptographic hashing algorithms ensure adequate entropy across hash
using (HashAlgorithm hashAlgorithm = SHA256.Create())
{
    hash = hashAlgorithm.ComputeHash(Encoding.UTF8.GetBytes(contextId));
}

//
// Endianness check ensures the consistency of targeting evaluation result across different architectures
if (!BitConverter.IsLittleEndian)
{
    Array.Reverse(hash, 0, 4);
}

//
// Only the first 4 bytes of the hash will converted to a uint
uint contextMarker = BitConverter.ToUInt32(hash, 0);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure comments, but no need to add allocation for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

//
// Cryptographic hashing algorithms ensure adequate entropy across hash
using (HashAlgorithm hashAlgorithm = SHA256.Create())
{
    hash = hashAlgorithm.ComputeHash(Encoding.UTF8.GetBytes(contextId));
}

//
// Endianness check ensures the consistency of targeting evaluation result across different architectures
if (!BitConverter.IsLittleEndian)
{
    Array.Reverse(hash, 0, 4);
}

//
// Only the first 4 bytes of the hash will converted to a uint
uint contextMarker = BitConverter.ToUInt32(hash, 0);

I like this suggestion. Comment adjusted.


double contextPercentage = (contextMarker / (double)uint.MaxValue) * 100;
Expand Down