✨ Add addresses to machine status #1314

rsmitty · 2019-10-31T20:34:33Z

What this PR does / why we need it:
This PR is pretty similar to one merged in the GCP provider a few hours ago.

It will add the ability to fish out the internal and external IPs
associated with an awsmachine resource and populate those values in
.status.addresses.

Happy to backport to release-0.4 after merge as well.

Signed-off-by: Spencer Smith robertspencersmith@gmail.com

k8s-ci-robot · 2019-10-31T20:34:41Z

Welcome @rsmitty!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-aws 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-aws has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2019-10-31T20:34:41Z

Hi @rsmitty. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

controllers/awsmachine_controller.go

pkg/cloud/scope/machine.go

controllers/awsmachine_controller.go

rsmitty · 2019-11-01T13:12:07Z

Went ahead and amended the v1 -> corev1 import issues. Thanks for the heads up there. I need a little more time to explore the ENI stuff. Will tackle that this morning.

rsmitty · 2019-11-01T19:56:39Z

pkg/cloud/services/ec2/instances.go

@@ -485,7 +485,8 @@ func (s *Service) UpdateResourceTags(resourceID *string, create map[string]strin
 	return nil
 }

-func (s *Service) getInstanceENIs(instanceID string) ([]*ec2.NetworkInterface, error) {
+// GetInstanceENIs returns a list of ec2 network interfaces for a given instance
+func (s *Service) GetInstanceENIs(instanceID string) ([]*ec2.NetworkInterface, error) {


Made this a public func so that I could call it from machine controller.

rsmitty · 2019-11-01T19:57:13Z

pkg/cloud/services/ec2/instances.go

@@ -310,16 +310,16 @@ func (s *Service) runInstance(role string, i *infrav1.Instance) (*infrav1.Instan
 	}

 	if len(i.NetworkInterfaces) > 0 {
-		instances := make([]*ec2.InstanceNetworkInterfaceSpecification, 0, len(i.NetworkInterfaces))
+		netInterfaces := make([]*ec2.InstanceNetworkInterfaceSpecification, 0, len(i.NetworkInterfaces))


these were mistakenly called instances I think. updated the name to be more clear that they're network interfaces.

rsmitty · 2019-11-01T19:58:59Z

controllers/awsmachine_controller.go

+func (r *AWSMachineReconciler) getAddresses(instance *infrav1.Instance, ec2svc *ec2.Service) ([]corev1.NodeAddress, error) {
+	addresses := []corev1.NodeAddress{}
+
+	enis, err := ec2svc.GetInstanceENIs(instance.ID)


every instance has one or more ENIs attached regardless of if it's specified in the spec, as you can have a pre-created ENI or an ephemeral one for the instance. changed the logic to just range through the ENIs and fish out IPs and DNS names.

rsmitty · 2019-11-01T20:00:29Z

@detiber I reworked the logic for this to just look for the ENIs directly. PTAL and see what you think. It results in a pretty nice status output like:

...
  status:
    addresses:
    - address: ip-172-31-11-240.ec2.internal
      type: InternalDNS
    - address: 172.31.11.240
      type: InternalIP
    - address: ec2-x-x-x-x.compute-1.amazonaws.com
      type: ExternalDNS
    - address: x.x.x.x
      type: ExternalIP
...

rsmitty · 2019-11-05T22:02:50Z

@detiber gentle ping on this when you get a second.

vincepri

Few comments, logic LGTM

/ok-to-test

controllers/awsmachine_controller.go

rsmitty · 2019-11-06T19:47:20Z

Okay, @vincepri, I think I've got a pretty decent implementation of what we talked about earlier today. Updated the logic to add an addresses field to the instance structs, as well as moving the logic to populate that field into the SDKToInstance function. It then bubbles up from there to get added into the awsmachine resource's status inside the controller.

rsmitty · 2019-11-06T20:35:48Z

/retest

vincepri

/approve
/milestone v0.5.0
/assign @detiber @randomvariable

k8s-ci-robot · 2019-11-07T15:02:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rsmitty, vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

This PR will add the ability to fish out the internal and external IPs associated with an awsmachine resource and populate those values in .status.addresses. Signed-off-by: Spencer Smith <robertspencersmith@gmail.com>

detiber · 2019-11-07T15:30:04Z

/lgtm

k8s-ci-robot · 2019-11-07T15:39:12Z

@rsmitty: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-cluster-api-provider-aws-make-conformance	`9cf4eee`	link	`/test pull-cluster-api-provider-aws-make-conformance`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 31, 2019

k8s-ci-robot requested review from ashish-amarnath and chuckha October 31, 2019 20:34

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 31, 2019

detiber reviewed Nov 1, 2019

View reviewed changes

rsmitty force-pushed the master branch from 27fe99a to 0932224 Compare November 1, 2019 13:10

rsmitty force-pushed the master branch from 0932224 to 64ee4cc Compare November 1, 2019 19:55

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 1, 2019

rsmitty commented Nov 1, 2019

View reviewed changes

rsmitty force-pushed the master branch from 64ee4cc to 9b6616d Compare November 5, 2019 22:09

vincepri approved these changes Nov 6, 2019

View reviewed changes

controllers/awsmachine_controller.go Outdated Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 6, 2019

rsmitty force-pushed the master branch 3 times, most recently from d8fccd5 to 7c8b1ea Compare November 6, 2019 19:16

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 6, 2019

rsmitty force-pushed the master branch 2 times, most recently from 4a23102 to f722910 Compare November 6, 2019 19:22

k8s-ci-robot assigned detiber Nov 7, 2019

vincepri approved these changes Nov 7, 2019

View reviewed changes

k8s-ci-robot assigned randomvariable Nov 7, 2019

k8s-ci-robot added this to the v0.5.0 milestone Nov 7, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2019

✨ Add addresses to machine status

9cf4eee

This PR will add the ability to fish out the internal and external IPs associated with an awsmachine resource and populate those values in .status.addresses. Signed-off-by: Spencer Smith <robertspencersmith@gmail.com>

rsmitty force-pushed the master branch from f722910 to 9cf4eee Compare November 7, 2019 15:27

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2019

k8s-ci-robot merged commit 2f2c8da into kubernetes-sigs:master Nov 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add addresses to machine status #1314

✨ Add addresses to machine status #1314

rsmitty commented Oct 31, 2019 •

edited

k8s-ci-robot commented Oct 31, 2019

k8s-ci-robot commented Oct 31, 2019

rsmitty commented Nov 1, 2019

rsmitty Nov 1, 2019

rsmitty Nov 1, 2019

rsmitty Nov 1, 2019 •

edited

rsmitty commented Nov 1, 2019

rsmitty commented Nov 5, 2019

vincepri left a comment

rsmitty commented Nov 6, 2019

rsmitty commented Nov 6, 2019

vincepri left a comment

k8s-ci-robot commented Nov 7, 2019

detiber commented Nov 7, 2019

k8s-ci-robot commented Nov 7, 2019

✨ Add addresses to machine status #1314

✨ Add addresses to machine status #1314

Conversation

rsmitty commented Oct 31, 2019 • edited

k8s-ci-robot commented Oct 31, 2019

k8s-ci-robot commented Oct 31, 2019

rsmitty commented Nov 1, 2019

rsmitty Nov 1, 2019

Choose a reason for hiding this comment

rsmitty Nov 1, 2019

Choose a reason for hiding this comment

rsmitty Nov 1, 2019 • edited

Choose a reason for hiding this comment

rsmitty commented Nov 1, 2019

rsmitty commented Nov 5, 2019

vincepri left a comment

Choose a reason for hiding this comment

rsmitty commented Nov 6, 2019

rsmitty commented Nov 6, 2019

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 7, 2019

detiber commented Nov 7, 2019

k8s-ci-robot commented Nov 7, 2019

rsmitty commented Oct 31, 2019 •

edited

rsmitty Nov 1, 2019 •

edited