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.

Sign up for GitHub

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

Jump to bottom

resolve vpcID and region dynamically #723

Merged
merged 2 commits into from
Nov 9, 2018
Merged

resolve vpcID and region dynamically #723

merged 2 commits into from
Nov 9, 2018

Conversation

M00nF1sh
Copy link
Collaborator

@M00nF1sh M00nF1sh commented Nov 7, 2018 •
edited
Loading

Changes:

  1. Refactor the option/configs handling, to make settings more coherence
  2. Resolve vpcID by ec2Metadata alone(ec2.DescribeInstances are not needed)
  3. Add vpcID and region as cloud configuration. Which will be resolved from ec2Metadata by default.(user can manually specify it as flags or environment variable to break the dependency of ec2Metadata)(fixes Fails without access to metadata server *and instance id* #687 )
  4. Break dependency of vpc ID in AWS api calls.
  5. Remove the availability zone populate step in IP targets registration, since it's not needed.

Tests done:

  • power ingress successfully.

Pending work:

  • fix lint & tests

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 7, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: M00nF1sh

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:

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 7, 2018
@M00nF1sh M00nF1sh changed the title [WIP]resolve vpcID and region dynamically resolve vpcID and region dynamically Nov 7, 2018
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 7, 2018
bigkraig
bigkraig reviewed Nov 9, 2018
View reviewed changes
cmd/options.go Outdated
// check port collisions
if !ing_net.IsPortAvailable(options.HealthzPort) {
func (options *Options) Validate() error {
if !net.IsPortAvailable(options.HealthzPort) {
return fmt.Errorf("port %v is alreadt in use. Please check the flag --healthz-port", options.HealthzPort)
Copy link

Choose a reason for hiding this comment

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

s/alreadt/already/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nice find! 😄 I think it's time to enable typoCheck in golangci

@bigkraig
Copy link

bigkraig commented Nov 9, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2018
@k8s-ci-robot k8s-ci-robot merged commit c304d75 into kubernetes-sigs:master Nov 9, 2018
@M00nF1sh M00nF1sh deleted the aws_resolve branch December 7, 2018 01:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bigkraig bigkraig bigkraig left review comments

Assignees

@bigkraig bigkraig

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. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fails without access to metadata server *and instance id*
3 participants
@M00nF1sh @k8s-ci-robot @bigkraig