Add support for EIPs associated with secondary ENIs #1513
Conversation
| @@ -76,7 +76,7 @@ public void testOnDemandUpdateRateLimiting() throws Throwable { | |||
| assertTrue(replicator.onDemandUpdate()); | |||
| Thread.sleep(10); // give some time for execution | |||
| assertFalse(replicator.onDemandUpdate()); | |||
| Thread.sleep(10); | |||
| Thread.sleep(1000); | |||
There was a problem hiding this comment.
Yeah. I'm guessing both those onDemandUpdate() calls didn't have time to finish so the rate limiter counts never triggered. Increasing the sleep give them time to finish and the counter to increment.
| @@ -89,7 +89,6 @@ public void testForceGzip() throws Exception { | |||
| @Test | |||
| public void testForceGzipOtherHeader() throws Exception { | |||
| noneGzipRequest(); | |||
| when(request.getHeader("Test")).thenReturn("ok"); | |||
There was a problem hiding this comment.
Updating the Mockito version to 3.4.0 changes the strictness of mock checks. That call request.GetHeader("Test") was never called during the test so it fails with unnecessarystubbingexception. It's not necessary for the test so I removed it.
The update to Mockito 3.4.0 is necessary so I can mock the static function AmazonInfoUtils.readEc2MetadataUrl() in AmazonInfoTest. Without it this PR becomes much larger as I'd have to split out the AmazonInfoUtils into an interface which can then be mocked and add implementations where it's currently referenced.
There was a problem hiding this comment.
Why did the ClusterSampleData change?
There was a problem hiding this comment.
In the update to Mockito 3.4.0 the ArgumentMatcher for anyString() changed from accepting "any string or null" to "any string". In the testStatusUpdate() and testDeleteStatusOverride() tests in PeerReplicationResourceTest, the provided parameter for status in the downstream instanceResource.statusUpdate() call is null, which results in the Mock not matching and our response returning null.
There are two solutions to this:
- Refactor the fields in the returned
ReplicationInstance(current change in this PR) to correctly capture the intent of thestatusUpdate()anddeleteStatusUpdate()calls.
The current master branch callsstatusUpdate(String newStatus = null, String isReplication = "true", String lastDirtyTimestamp = "12345678")anddeleteStatusUpdate(String isReplication = "true", String newStatus = null, String lastDirtyTimestamp = "12345678"), which to me seems odd that we'd setnewStatus = nullin each rather than an actual status. - Change the mock expression for
statusUpdate()anddeleteStatusUpdate()toisNull()for the parameters that returnnull.
I prefer the first option as I believe it's the more correct change, but I'm happy to go with 2) if we want to minimize the changes in this PR.
Add support for EIPs associated with secondary ENIs
Currently, Eureka client only supports registering EIPs that are directly associated to an instance and available via the root
public-ipv4IMDS key. This PR adds support to register an EIP that is attached via a secondary ENI. Due to the complexities of discovering and registering multiple public IPs for a single instance, we only register the first found public IP address. If an instance has both an EIP directly associated and and EIP associated via an ENI, the directly associated EIP will be registered. If the instance only has a EIP(s) associated via a ENI(s), the first found EIP will be registered. Additional use cases (i.e., registering multiple public IPv4 addresses) will not be supported.