KM upgrade E2E test #2920
KM upgrade E2E test #2920
Conversation
ptrus
force-pushed
the
ptrus/feature/e2e-km-upgrade
branch
11 times, most recently
from
2f826af to
e72e3fb
Compare
Codecov Report
@@ Coverage Diff @@
## master #2920 +/- ##
==========================================
+ Coverage 68.13% 68.48% +0.35%
==========================================
Files 360 360
Lines 34934 34947 +13
==========================================
+ Hits 23802 23935 +133
+ Misses 8041 7933 -108
+ Partials 3091 3079 -12
Continue to review full report at Codecov.
|
ptrus
requested review from
kostko,
peterjgilbert,
pro-wh and
Yawning
as code owners
| // TODO: with the change that keymanager initially registers without | ||
| // status, before it is initialized, we should wait here that keymanager | ||
| // is initialized. Add a method to wait for KM initialization? | ||
| time.Sleep(10 * time.Second) |
There was a problem hiding this comment.
If the "register as a keymanager on the first status update" fix is ok (see changelog fragment), i'll fix this properly by waiting for KM initialization, but didn't want to implement that before the fix is acked, as it might not be needed if we decide for a different fix (e.g. maybe just retrying the initialization)
There was a problem hiding this comment.
Wouldn't something be needed in either case with the "key manager always registers"? E.g., even if you retry, the retry could happen later than the key manager actually registering with correct ExtraInfo? So maybe we should do #2130 and then you could call WaitReady on the key manager node(s) to check if they are really ready?
There was a problem hiding this comment.
Yeah would be, but if i revert the "km always registers" and only replace it it with retrying the initialization no changes are needed here - so i think i'll do just that.
There was a problem hiding this comment.
Once we have #2130 we should change the code here to wait for ready, and we can also bring back the "km always registers" fix, since i think is generally a bit nicer than the current "hack" to only register after initial failure :-)
There was a problem hiding this comment.
Maybe mention this in #2130 (and add a comment in the code) so that we don't forget about it?
There was a problem hiding this comment.
done, i'll mention on the issue once this is merged so i can permalink to code
| // TODO: with the change that keymanager initially registers without | ||
| // status, before it is initialized, we should wait here that keymanager | ||
| // is initialized. Add a method to wait for KM initialization? | ||
| time.Sleep(10 * time.Second) |
There was a problem hiding this comment.
Wouldn't something be needed in either case with the "key manager always registers"? E.g., even if you retry, the retry could happen later than the key manager actually registering with correct ExtraInfo? So maybe we should do #2130 and then you could call WaitReady on the key manager node(s) to check if they are really ready?
ptrus
force-pushed
the
ptrus/feature/e2e-km-upgrade
branch
3 times, most recently
from
98ecaeb to
b8d456f
Compare
|
|
||
| set | ||
| }, | ||
| threshold: 9002, |
There was a problem hiding this comment.
At some point, we probably should get rid of the hack that sets the threshold to 2 when using test keys. Also either leave a comment here, or pick a different way to force the MRENCLAVE to change.
ptrus
force-pushed
the
ptrus/feature/e2e-km-upgrade
branch
from
b8d456f to
40ca1f3
Compare
ptrus
force-pushed
the
ptrus/feature/e2e-km-upgrade
branch
from
40ca1f3 to
1165a40
Compare
ptrus
force-pushed
the
ptrus/feature/e2e-km-upgrade
branch
from
1165a40 to
de2f992
Compare
ptrus
force-pushed
the
ptrus/feature/e2e-km-upgrade
branch
2 times, most recently
from
4adae78 to
c822ade
Compare
ptrus
force-pushed
the
ptrus/feature/e2e-km-upgrade
branch
from
c822ade to
92b62f2
Compare
|
thx for the reviews |
Closes: #2517
TODO:
OASIS_UNSAFE_KM_POLICY_THRESHOLDbuild-time-flag to ensure different binaries, just duplicate the test-keymanager crate with a change, it will make the code simpler