Conditionally set AWS network configuration directly on EC2 request if associate public ip is true #447
Conversation
…f associate public ip is true
|
@jvz I tried to look into that before opening, but it doesn't look like there were ever tests removed as part of removing the conditions that I added back in this PR. See |
|
Any tests that would fail if your PR were changed incorrectly in the future. Regression tests mainly. Otherwise, features pop in and out of existence over time when their expected behavior is unknown. :) |
|
Ok, I'll have to look into adding tests on another day. I do want to point this PR out though. |
|
If you find there's no reasonable way to add tests, let me know. I don't want to block the PR for no reason. |
accordingly based on associatePublicIp value
|
@jvz I have added a couple tests in the latest commit. Please let me know if there's a better direction than the one I took! |
|
@jvz when will this get merged? |
|
I'm not the primary maintainer here, so I'm not really sure. @varyvol? |
|
I think this is ok to merge as is, I left it to see if others wanted to take a look at it |
|
Looks like this caused an issue https://issues.jenkins-ci.org/browse/JENKINS-62090 |
I upgraded the EC2 plugin to the latest available version (
ec2-1.49.1) and found that our cloud was spinning up workers withnullpublic DNS hostnames:An agent trying to connect would show these logs:
Additionally, I observed in the EC2 UI that this new instance's public DNS ip address was blank (
i-003...corresponds toec2-1.49.1;i-00f...corresponds toec2-1.45).The underlying AWS VPC, subnet, and security groups were left unchanged between the plugin versions. Our EC2 cloud template is configured (and also unchanged) with the "Public DNS" connection strategy and "Associate Public IP" set to
false.Under
ec2-1.49.1, the necessary AWS network configuration needed to provision a new EC2 instance is only set on the request's network interface specification, which seems to be inadequate for an instance to be created with a public DNS name.Reviving the
getAssociatePublicIP()conditions to set the target subnet and security group ids on theRunInstancesRequestculminates in the correct AWS EC2 request to provision a new worker with our current configuration.I smoke-tested by uploading a patched plugin with these changes. New instances are created and can connect to master once again!