Fix cloudfront ecdsa integration tests #6637
Conversation
alextwoods
added
the
no-api-surface-area-change
| // there is no waiter for keyGroup updates, but it may take up to 1 minute for the updates to propagate | ||
| try { | ||
| System.out.println("Waiting 1 minute for keygroup updates to propagate in distribution..."); | ||
| Thread.sleep(Duration.ofMinutes(1).toMillis()); | ||
| } catch (InterruptedException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
Can we use the generic Waiter interface? It's designed to handle this case :)
There was a problem hiding this comment.
I haven't found anything reliable to wait on for that case - you can retrieve the KeyGroup and check that it includes both keys, but the distribution will not use them. But we're not doing a distribution update if it already exists, so there isn't something I can wait on there. I originally tried with:
Waiter.run(() -> cloudFrontClient.getKeyGroup(r -> r.id(keyGroupResp.keyGroup().id())))
.until( (newGroupResp) -> {
return newGroupResp.keyGroup().keyGroupConfig().items().containsAll(expectedKeys);
}).orFailAfter(Duration.ofMinutes(1));
However - this returns successfully after the first response, but the tests all still fail.
Maybe I can wait on making a successful signed request with both keys... but thats basically just the meat of the actual test cases. I'll keep poking at it and see if I can find something else from the service I can query to wait on.
There was a problem hiding this comment.
Ah I see, there's another waiter test util which allows us to pass any predicate, like how long time has passed. https://github.com/aws/aws-sdk-java-v2/blob/master/test/test-utils/src/main/java/software/amazon/awssdk/testutils/Waiter.java#L64
There was a problem hiding this comment.
Anyway, it's such a small thing and probably not worth the time 🤷🏼♀️
There was a problem hiding this comment.
I've updated to use a Waiter.until to wait for the expected propagation delay.
I had a few last ideas to try out - I was hoping that waiting on updates to show in the KeyGroup summaries (from listGroups) would better represent the state, but it also is updated instantly and tests fail. I also looked into using an invalidation - this does work but has two issues:
- At this point during the setup, in theory its possible for the KeyGroup to exist, but the distribution to not yet be created, so we can't rely on the distribution to create the invalidation....
- We are limited to a certain number of invalidations and they do incur a cost.
I think for handling this edge case (should only ever happen when running on an integration test account that was previously used with only the RSA key - it will not happen on a fresh account) - waiting for a set time is our best option :-/
|
|
This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one. |
2 participants
Re-release #6627 with fixes for integration test.
Motivation and Context
#6627 was reverted because of failing integration tests before being released.
Modifications
This PR includes all of the changes from #6627 but updates the integration test to fix the gap - it ensures that if the distribution's key group exists that it has all required public keys. The key group on the release account had only the RSA key and not the ECDSA key - we now check that the keys are all int he group and if not, make an update to it.
Testing
Ran on an integ account with the key group existing BUT missing the ECDSA key.
Screenshots (if appropriate)
Types of changes
License