Fix Blake2b hash #5089
Fix Blake2b hash #5089
Conversation
Signed-off-by: Terry Quigley <[email protected]>
Signed-off-by: Terry Quigley <[email protected]>
Signed-off-by: Terry Quigley <[email protected]>
terryquigleysas
requested review from
cwperks,
DarshitChanpura,
derek-ho,
nibix,
peternied,
RyanL1997,
reta and
willyborankin
as code owners
Signed-off-by: Terry Quigley <[email protected]>
|
Note: In a mixed cluster state, this will yield inconsistent results: Some nodes will hash value A to hash X, while other nodes will hash value A to hash Y. This will especially affect aggregations. Are we okay with this (genuine question)? In any case, this should be documented. |
@nibix Thank you for your comment. That is not something that I would have been aware of. I still strongly lean towards putting this change in.
|
|
@terryquigleysas One way to work around that issue would be to gate the new behavior by a config option that can be changed at runtime (In config.yml for example). That way, the behavior can be changed from old to new nearly instantaneously, thus reducing the chance of inconsistent aggregations massively. |
@nibix What would you suggest naming the property? |
|
Just to avoid any misunderstanding: I am in no position to give authoritative rulings on such changes. I can only give my opinion and my recommendations. Any unclear issues need to be clarified in a community driven process. To reiterate the issue:
A dynamically changeable config flag would solve this the following way:
If the config flag would be initially set to the new behavior, the issue would be actually not avoidable. Having said this, this approach has indeed the downside that it needs manual intervention of an admin after the completion of the rolling upgrade. There would be also not an easy way to automate that. Another alternative solution might be to use the cluster state to check whether the cluster is in a mixed state or not. The cluster state API provides methods for that. However, TBH, I am not sure how easy these APIs are accessible from the very low level code we are talking about. |
|
FYI There is a class called ClusterInfoHolder that listens to changes in cluster state and can be interrogated to find the min node version in a cluster. |
|
How should we proceed here? As far I see, there are 2 choices:
|
This has the downside that it makes certain uses cases impossible in mixed cluster states. If there are, for example, alerting solutions on such data, these might produce false positives in this phase. What are the exact high availability and compatibility promises OpenSearch makes? I guess we need to know these in order to decide whether this is viable.
One downside is here also that the change happens to an kind of uncontrolled point in time. If there are use cases which depend on specific hashes, this also might present a challenge to react to the changed hashes in the right point in time. I think there are a couple of further options:
|
|
@nibix @cwperks Thank you for the comments and suggestions. I have been on vacation for a few days and just catching up on this. I have looked at checking for the min cluster version. Unfortunately I don't think it is feasible for several reasons. As mentioned above, making these settings available to the relevant code doesn't look trivial. There would likely be an unwanted performance hit for the checks, and even then it would still result in erratic results in various scenarios. I think we could however use an existing option to support setting the default masking algorithm to revert to the legacy behavior- see https://opensearch.org/docs/latest/security/access-control/field-masking/#advanced-use-an-alternative-hash-algorithm. For example:
This means that:
What do you think? |
Signed-off-by: Terry Quigley <[email protected]>
Signed-off-by: Terry Quigley <[email protected]>
terryquigleysas
force-pushed
the
fix_blake2b_hash
branch
2 times, most recently
from
0be2540 to
ae98611
Compare
This is a genuine question, because I really do not know: With the standard K8S deployment methods, is it possible to achieve that only nodes with a certain version do get a deviating opensearch.yml config? If this is possible, I think it's a good way to go. |
We can certainly do it in our K8S deployments. We did similar upgrading from 1.x to 2.x. I've updated the PR to include the new functionality. |
|
For me, it looks good now. I have triggered again the failing CI jobs. However, I guess, we still need documentation regarding that breaking change. However, I cannot give pointers where that would be. @cwperks can you help out? |
|
There seem to be still a couple of test failures. Can you please have a look? |
@terryquigleysas has previously made changes to the documentation website. I think the right location would be https://github.com/opensearch-project/documentation-website/blob/main/_security/access-control/field-masking.md?plain=1 |
|
I guess, for the chosen solution we also need to have a documentation that is being read by users that are applying the update. Users upgrading won't pro-actively navigate into the field masking docs. Is there a thing like release notes? |
For 3.0 there will be a migration guide and list of breaking changes similar to: https://opensearch.org/docs/latest/breaking-changes/ |
Strangely these hadn't been failing earlier. On the case now. |
Signed-off-by: Terry Quigley <[email protected]>
…/security into fix_blake2b_hash
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5089 +/- ##
==========================================
+ Coverage 71.64% 71.65% +0.01%
==========================================
Files 335 335
Lines 22748 22755 +7
Branches 3599 3601 +2
==========================================
+ Hits 16297 16305 +8
- Misses 4651 4652 +1
+ Partials 1800 1798 -2
|
|
+1 on using a config - it's okay to change the default and document the behavior here- https://opensearch.org/docs/latest/security/access-control/field-masking/ Breaking change here- #5031 & I am not sure if we can add a log line in-case of incorrect aggregations in mixed cluster state. @terryquigleysas |
@shikharj05 Thank you for confirming the locations where the documentation needs to change. I will pick up those once this PR goes in. The addition of the BLAKE2B_LEGACY_DEFAULT option means that the mixed-cluster aggregation scenario can be avoided. |
|
Since the default implementation will change, users might still land into mixed-cluster issue (if they don't update the config). Hence, I was checking if it's possible to add a log line. |
@shikharj05 It's a fair point regarding the edge case and one I addressed above. I think the best approach is to fix the bug as it is here and document the change in the two locations you mention. I could add a log line saying the existing hash has been implemented incorrectly but that would have to be done separately to be backported to the 1.x and 2,x versions. |
Description
This may be considered a "Breaking Change" for 3.0.0 as the hashes will now be different - correct, but different from before.
Issues Resolved
Resolves #4274
Testing
Updated existing tests.
Ran Bulk Integration Test action against the branch.
Local testing.
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.