[WIP] ClusterActuator: support ELB for api server #115
Conversation
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ashish-amarnath If they are not already assigned, you can assign the PR to them by writing 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 |
k8s-ci-robot
added
size/M
| @@ -15,13 +15,15 @@ package ec2 | |||
|
|
|||
| import ( | |||
| "github.com/aws/aws-sdk-go/service/ec2/ec2iface" | |||
| "github.com/aws/aws-sdk-go/service/elb/elbiface" | |||
There was a problem hiding this comment.
Should this maybe directly start with ELBv2 aka NLBs or ALBs?
There was a problem hiding this comment.
We discussed this as we were scoping out the MVP and decided to go with an ELB Classic configured for tcp-passthrough. The main reason for this is to avoid hitting hairpining-related issues when an API server attempts to talk to itself through the NLB.
If we can sufficiently work through those issues, then we can move towards an NLB-based approach. ALB is more difficult because we need to allow client-cert based authentication.
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
|
@ashish-amarnath: The following tests failed, say
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. |
| ID string `json:"id"` | ||
|
|
||
| // TODO: figure out other fields for the loadbalancer | ||
| Name string `json:"name` |
There was a problem hiding this comment.
I believe the json tag is missing a closing " here.
There was a problem hiding this comment.
FIxed locally. Thanks!
| "sigs.k8s.io/cluster-api-provider-aws/cloud/aws/providerconfig/v1alpha1" | ||
| ) | ||
|
|
||
| func (s *Service) reconcileLoadbalancer(lb v1alpha1.LoadBalancer) error { |
There was a problem hiding this comment.
Should this support multiple load balancers? Or is this supposed to be called in a loop?
There was a problem hiding this comment.
So the idea here was to support the loadbalancer for the API server only. I don't think the cluster network status should track other loadbalancers.
| }, | ||
| } | ||
|
|
||
| out, err := s.ELB.DescribeLoadBalancers(req) |
There was a problem hiding this comment.
The error here should be wrapped using errors.Wrapf(...) from github.com/pkg/errors.
There was a problem hiding this comment.
Thanks for reminding! will keep that in mind :)
|
@ashish-amarnath Any updates on this PR? I'd prefer to move the elb logic in another package possibly, this is addressed in #147. |
|
@ashish-amarnath given the urgency of the ELB work, I'll cherry-pick your commit and work on another branch tomorrow, let me know if there is any pushback. |
|
@vincepri Sorry for the delayed response. Currently I am with limited
internet access and haven’t had a chance to make good progress on this yet.
…On Wed, Oct 3, 2018 at 5:12 PM Vince Prignano ***@***.***> wrote:
@ashish-amarnath <https://github.com/ashish-amarnath> given the urgency
of the ELB work, I'll cherry-pick your commit and work on another branch
tomorrow, let me know if there is any pushback.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#115 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AV0ASteT0FfF8bQtQ7WhND_MjwduVhxMks5uhXyzgaJpZM4W6BMy>
.
|
|
closing, since #158 was merged |
Update actuator Interface based on upstream changes
What this PR does / why we need it:
The cluster actuator should create and manage a load balancer for the apiserver if a user does not provide an existing load balancer.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #52
Special notes for your reviewer:
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
Release note: