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

Support specifying VPC #225

Merged

Conversation

timoreimann
Copy link
Contributor

What this PR does / why we need it:
This change adds a field to DONetwork specifying the VPC UUID and updates the controllers to pass the ID into droplet and load balancer create requests. That way, users can create custom VPCs and have their CAPI-managed clusters join them.

Which issue(s) this PR fixes:
Fixes #177

Special notes for your reviewer:
I was looking into writing a dedicated e2e test for this feature. The part I struggled with is how to customize the subnet definition in the Cluster resource and, more importantly, the subnet defined in the Calico config file. I think I can solve the former by introducing another DO-specific environment variable, but not sure if the same pattern applies for the Calico config.

Happy to follow up on this before calling the PR done if anyone is willing to provide some guidance. Thanks!

Release note:

Add support for specifying the VPC UUID

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. 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 Dec 14, 2020
@k8s-ci-robot
Copy link

Hi @timoreimann. 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.

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 14, 2020
@cpanato
Copy link
Member

cpanato commented Dec 14, 2020

/ok-to-test

@k8s-ci-robot 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 Dec 14, 2020
@timoreimann timoreimann force-pushed the support-specifying-vpcuuid branch 2 times, most recently from 8cf86d3 to ae9ca2c Compare December 14, 2020 12:45
@k8s-ci-robot 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 Dec 14, 2020
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 14, 2020
@prksu
Copy link
Contributor

prksu commented Dec 14, 2020

/milestone v0.5

@k8s-ci-robot k8s-ci-robot added this to the v0.5 milestone Dec 14, 2020
@prksu
Copy link
Contributor

prksu commented Dec 14, 2020

@timoreimann so the users should to create the vpc before using that right?
how about automatically creating vpc through docluster-controller. so users just configure the vpc spec under DONetwork that contains name and ip range entity.

@timoreimann
Copy link
Contributor Author

@prksu I like the idea to let the controller manage the VPC. I'm also thinking though that some users may want to use a pre-existing VPC, in which case the controller should probably verify that the VPC exists. Any thoughts on covering both cases? (If so, then I could first tackle the simpler case of the two followed by the other in a future PR.)

Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 16, 2020
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2021
This change adds a field to DONetwork specifying the VPC UUID and
updates the controllers to pass the ID into droplet and load balancer
create requests.
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 12, 2021
@timoreimann
Copy link
Contributor Author

@prksu I moved the VPC UUID below a newly created struct named VPC. This should provide space for adding more VPC-specific fields, like the subnet.

WDYT about getting this PR in first so that users who are fine with specifying the VPC ID explicitly and configuring the subnet etc. accordingly can benefit from it, while we leave the more complex auto-discovery approach to a future PR? The latter still has a few open questions for me, especially how a dynamically retrieved subnet can be fed into the CNI configuration. We might need more time to sort that out.

Copy link
Contributor

@prksu prksu left a comment

Choose a reason for hiding this comment

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

@prksu I moved the VPC UUID below a newly created struct named VPC. This should provide space for adding more VPC-specific fields, like the subnet.

That's what I thought too.
And yes, we can leave the rest as the follow up. Thanks @timoreimann

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 13, 2021
@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, prksu, timoreimann

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 merged commit 3ab6d66 into kubernetes-sigs:master Jan 13, 2021
@k8s-ci-robot k8s-ci-robot modified the milestones: v0.5, v0.4 Jan 13, 2021
@timoreimann timoreimann deleted the support-specifying-vpcuuid branch January 13, 2021 12:06
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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support custom VPC
4 participants