search

microsoft / FeatureManagement-Dotnet

Microsoft.FeatureManagement provides standardized APIs for enabling feature flags within applications. Utilize this library to secure a consistent experience when developing applications that use patterns such as beta access, rollout, dark deployments, and more.
MIT License
1.03k stars 111 forks source link

Targeting evaluation is CPU dependent. #403

Closed jimmyca15 closed 4 months ago

jimmyca15 commented 5 months ago

Targeting evaluation uses BitConvert.ToInt32 as seen here. According to ToInt32's documentation, the result is dependent upon the CPU's endianness.

Targeting evaluation should be consistent across CPU architectures. Given the current documented and intended usage is server scenarios, which typically run on x86, I propose to use a replacement that would give results consistent with BitConvert.ToInt32 run on x86 systems.

rossgrambo commented 5 months ago

Not sure if there's a good replacement. We could explicitly check and reverse the bits?

if (BitConverter.IsLittleEndian)
{
    Array.Reverse(bytes);
}
zhiyuanliang-ms commented 5 months ago

@rossgrambo I think the method you proposed is fine. I don't have any better idea.

BTW, X86 and other common CPU architectures are all little-endian. So we'd better treat big-endian as special case.

jimmyca15 commented 5 months ago

Sounds reasonable, however no need to reverse the whole array. Only the bytes used for the int32 are needed.

https://learn.microsoft.com/en-us/dotnet/api/system.array.reverse?view=net-8.0#system-array-reverse(system-array-system-int32-system-int32)

zhiyuanliang-ms commented 5 months ago

I checked other FM libs.

JS FM still does not implement the targeting part.

The python FM is fine since it explicitly uses little-endian: https://github.com/microsoft/FeatureManagement-Python/blob/main/featuremanagement/_defaultfilters.py#L86

This issue also exists in Java FM: https://github.com/Azure/azure-sdk-for-java/blob/main/sdk/spring/spring-cloud-azure-feature-management/src/main/java/com/azure/spring/cloud/feature/management/filters/TargetingFilter.java#L256

The default endianness used by ByteBuffer is big-endian: https://docs.oracle.com/javase/8/docs/api/java/nio/ByteBuffer.html @mrm9084

Currently, Java FM may give result different from the result of Python and .NET FM give on the same targeting context.