Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AWS for cloud-controller-manager #47215

Merged
merged 4 commits into from
Jun 12, 2017
Merged

AWS for cloud-controller-manager #47215

merged 4 commits into from
Jun 12, 2017

Conversation

ublubu
Copy link
Contributor

@ublubu ublubu commented Jun 9, 2017

fixes #47214

This implements the NodeAddressesByProviderID and InstanceTypeByProviderID methods used by the cloud-controller-manager for the AWS provider.

NodeAddressesByProvider uses DescribeInstances (for normal addresses) and DescribeAddresses (for Elastic IP addresses).

InstanceTypeByProviderID uses DescribeInstances.

NONE

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 9, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Jun 9, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @ublubu. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot 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.

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 9, 2017
return "", fmt.Errorf("ProviderID \"%s\" didn't match expected format \"aws://InstanceID\"", providerID)
}

return matches[1], nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will cause OutOfIndex error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh whoops. Fixed it and added a test for this.

@wlan0
Copy link
Member

wlan0 commented Jun 9, 2017

@ublubu how was the kubelet obtaining node addresses before? Was it using the metadata service?

I'm asking because I'm wondering why there is a new function to describe addresses. Shouldn't there be existing methods for this very purpose?

@ublubu
Copy link
Contributor Author

ublubu commented Jun 9, 2017

@wlan0 The kubelet uses the local metadata service.

In the kubelet:

nodeAddresses, err := instances.NodeAddresses(kl.nodeName)

AWS implementation of NodeAddresses using metadata service:

func (c *Cloud) NodeAddresses(name types.NodeName) ([]v1.NodeAddress, error) {

IIUC, the new methods are run in the CCM on a master, where a node's metadata service is not available.

@wlan0
Copy link
Member

wlan0 commented Jun 9, 2017

@ublubu The code looks good. Thanks for the PR!

LGTM

@thockin @luxas PTAL

@thockin
Copy link
Member

thockin commented Jun 9, 2017

@k8s-bot ok to test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 9, 2017
@thockin
Copy link
Member

thockin commented Jun 9, 2017

/lgtm
/approve

@dchen1107 same question - 1.7.0 or 1.7.1?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thockin, ublubu

Associated issue: 47214

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 9, 2017
@wlan0
Copy link
Member

wlan0 commented Jun 9, 2017

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@wlan0
Copy link
Member

wlan0 commented Jun 9, 2017

Addresses #47257

@thockin
Copy link
Member

thockin commented Jun 10, 2017

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@luxas
Copy link
Member

luxas commented Jun 11, 2017

@thockin May I set the v1.7 label on this?
The risk (if any) seems to be really low...

@thockin thockin added this to the v1.7 milestone Jun 11, 2017
@thockin
Copy link
Member

thockin commented Jun 11, 2017

@dchen1107 this is thoroughly flag gated already

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit ea3a896 into kubernetes:master Jun 12, 2017
@justinsb
Copy link
Member

It seems this PR uses an incompatible provider id format.

@wlan0
Copy link
Member

wlan0 commented Jun 13, 2017

What should the format be?

@justinsb
Copy link
Member

Addressed in #47395 I believe

@wlan0
Copy link
Member

wlan0 commented Jun 13, 2017

@justinsb Thanks!

@ublubu ublubu deleted the aws-addresses branch June 24, 2017 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cloudprovider AWS needs implementations for *ByProviderID methods
9 participants