aws-vpc-move-ip: Enable eni lookup for AWS shared networks via RAM #1549
Conversation
|
Can one of the admins verify this patch? |
|
I've added a couple of suggestions to improve patch. Can you also prefix the commits with "aws-vpc-move-ip:" so it's easy to see which agent they change from git log? |
sbotman
changed the title
|
Hello, While I find the changes interesting, I would like to know if all test cases are being considered and we're still fully backward compatible with all features this agent provides. Some examples:
Can you also provide some usage guide on the new parameters in this PR for documentation purposes? |
|
Hello, I understand your concerns but this logic didn't change, so single or multiple tables are working fine. Only when you have 1 AWS account (call it the network account) containing all VPC's and sharing these networks via RAM (resource access manager) to another accounts containing the workloads (EC2 instances) then you don't get the instance-id information. This is because you need to check the route-table by assuming the role in the network account and check the table there, but because the workload is not part of this account, you lose this information. You do get the network-interface-id information, so by setting the lookup_type=NetworkInterfaceId on the resource creation, you can do the matching on network-interface-id. By default the option is InstanceId. Hope this covers the documentation part. |
|
Thank you Sander, I think that cover all the questions I had! :-) |
|
The final product all looks good to me. Since this PR isn't merged yet, we don't need separate commits to fix mistakes that were made while developing this PR. I think a total of two commits makes the most sense:
The other five current commits should be part of an edited "Implemented optional eni lookup" commit. Can you squash all the commits (except for the region fix commit, which should be separate) into the original commit? I think you can do that in an interactive git-rebase by moving the region commit to the end of the history and then changing "pick" to "fixup" for all the fix commits, and then doing a force-push. |
…lt instance id. In a shared network pattern where the cluster resides in shared subnets the instance ids of the nodes are not retrievable but the eni ids are and this optional feature gives transparent support in that situation.
|
Tnx @nrwahl2 for all the reviews! |
|
@oalbrigt If you have any concerns about the overall approach (or you run it by anyone else who does), let us know. As I mentioned in one of the review comments, I don't have the cycles to research exactly what this feature is doing and test it out right now. Also need to get f4c8daa into RHBZ#1872999. I misread and thought the |
|
I have tested the final script and ran into an interesting use case. Once you specify that you want to assume a role, the So I think the --profile option should become optional and only added if set. Otherwise the script is working fine for me. |
|
Right. The parameter is optional (see metadata), so it sounds like that's a mistake in the initial code. I guess you could update the profile parameter to |
|
Since it's not really related to this change, shall we move that into another request? |
|
Sure :) |
|
ok to test |
|
LGTM. Thanks. |
In a shared network pattern where the cluster resides in shared subnets the instance ids of the nodes are not retrievable but the eni ids are and this optional feature gives transparent support in that situation.