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.

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

authorize: add databroker server and record version to result, force sync via polling #2024

Merged
merged 5 commits into from Mar 31, 2021

Conversation

calebdoxsey
Copy link
Contributor

Summary

Add databroker_server_version and databroker_record_version to the rego policy and evaluator result. This is useful for auditing.

  • Change UpdateRecord so that it takes in the server version as well, and updates all 3 values within a transaction so that the version is consistent
  • Change how forceSync works so that it polls for the record to appear instead of overwriting it. This is necessary so that we don't apply changes out of order

Checklist

  • reference any related issues
  • updated docs
  • updated unit tests
  • updated UPGRADING.md
  • add appropriate tag (improvement / bug / etc)
  • ready for review

@calebdoxsey calebdoxsey requested a review from a team as a code owner March 26, 2021 20:54
@calebdoxsey calebdoxsey requested a review from wasaga March 26, 2021 20:54
@codeclimate
Copy link

codeclimate bot commented Mar 26, 2021

Code Climate has analyzed commit c918f14 and detected 0 issues on this pull request.

View more on Code Climate.

@codecov
Copy link

codecov bot commented Mar 26, 2021

Codecov Report

Merging #2024 (c918f14) into master (4218f49) will decrease coverage by 0.0%.
The diff coverage is 45.3%.

@@           Coverage Diff            @@
##           master   #2024     +/-   ##
========================================
- Coverage    59.6%   59.6%   -0.1%     
========================================
  Files         158     159      +1     
  Lines       10847   10908     +61     
========================================
+ Hits         6469    6504     +35     
- Misses       3640    3663     +23     
- Partials      738     741      +3     
Impacted Files Coverage Δ
internal/databroker/config_source.go 56.0% <ø> (+1.7%) ⬆️
internal/identity/manager/sync.go 0.0% <ø> (ø)
authorize/sync.go 32.0% <36.2%> (+32.0%) ⬆️
authorize/evaluator/result.go 41.1% <41.1%> (ø)
authorize/evaluator/store.go 75.5% <56.2%> (+0.7%) ⬆️
authorize/evaluator/evaluator.go 69.0% <100.0%> (+10.7%) ⬆️
authorize/grpc.go 78.9% <100.0%> (-0.2%) ⬇️
pkg/grpc/databroker/syncer.go 95.8% <100.0%> (ø)
config/validate.go 15.7% <0.0%> (-3.0%) ⬇️
internal/databroker/server.go 50.0% <0.0%> (-1.0%) ⬇️
... and 5 more

Copy link
Contributor

@wasaga wasaga left a comment

Choose a reason for hiding this comment

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

I'm unsure about waitForRecord

authorize/sync.go Outdated Show resolved Hide resolved
authorize/sync.go Outdated Show resolved Hide resolved
authorize/sync.go Outdated Show resolved Hide resolved
authorize/sync.go Outdated Show resolved Hide resolved
@calebdoxsey
Copy link
Contributor Author

This conflicts with #2041. Will wait on that review before making changes.

authorize/sync.go Show resolved Hide resolved
@calebdoxsey calebdoxsey force-pushed the cdoxsey/350-authorize-databroker-versions branch from a22c27e to c918f14 Compare March 31, 2021 15:47
@calebdoxsey calebdoxsey merged commit d7ab817 into master Mar 31, 2021
@calebdoxsey calebdoxsey deleted the cdoxsey/350-authorize-databroker-versions branch March 31, 2021 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants