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

minikube tunnel #3015

Merged
merged 43 commits into from
Oct 18, 2018
Merged

minikube tunnel #3015

merged 43 commits into from
Oct 18, 2018

Conversation

balopat
Copy link
Contributor

@balopat balopat commented Jul 25, 2018

An implementation of minikube tunnel based on #2413.

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
  • If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

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 cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 25, 2018
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@balopat balopat force-pushed the tunnel branch 2 times, most recently from c0314fd to ed3b4ba Compare July 25, 2018 01:31
@balopat
Copy link
Contributor Author

balopat commented Jul 25, 2018

cla signed.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 25, 2018
@balopat
Copy link
Contributor Author

balopat commented Jul 25, 2018

cc @dlorenc

docs/contributors/mocking.md Outdated Show resolved Hide resolved
mocks/github.com/docker/machine/libmachine_mocks/API.go Outdated Show resolved Hide resolved
pkg/minikube/tunnel/loadbalancer_patcher.go Outdated Show resolved Hide resolved
pkg/minikube/tunnel/loadbalancer_patcher.go Outdated Show resolved Hide resolved
pkg/minikube/tunnel/loadbalancer_patcher.go Outdated Show resolved Hide resolved
pkg/minikube/tunnel/loadbalancer_patcher.go Show resolved Hide resolved
pkg/minikube/tunnel/reporter.go Outdated Show resolved Hide resolved
@balopat
Copy link
Contributor Author

balopat commented Jul 30, 2018

Couple of things while I was working on the comments:
1.) I found a bug in the implementation: the daemon currently survives while I stop, delete and recreate a machine however doesn't pick up the new ip address. I either have to change the design to pick up the new ip address or to share fate with the machine (stop the daemon when minikube stops).
2.) as a newbie golanger I overdesigned the abstractions for testability and created unnecessarily large amount of interface code for DI. As I'm reading more about this, there seems to be a lot of opportunity to simplify by passing func types around instead.
A larger rework is coming soon.

@balopat balopat changed the title Tunnel [wip] tunnel Aug 20, 2018
@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 Aug 20, 2018
@balopat
Copy link
Contributor Author

balopat commented Aug 21, 2018

A lot changed in the last month:

  • I reemoved the mocking framework and rolled my own stubs and fakes.
  • I introduced the tunnel registry in order to be able to cleanup for orphaned tunnels (see design doc updates too)
  • the tunnel process now shares fate with the minikube cluster - if the cluster stops, the tunnel exits

I still have some outstanding tasks:

  • refactor and validate windows version
  • refactor and validate linux version
  • setup script for the whitebox integration tests in the tunnel package
  • write integration test for tunnel
  • found a bug - number of /usr/local/bin/docker-machine-driver-hyperkit processes keep increasing
  • found another issue - with hyperkit created machines the network interface (bridge100) sometimes does not forward packets. ifconfig deletem en8 && ifconfig addm en8 seems to help - maybe just mention it in the docs?
  • update godocs
  • restructure commits

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: balopat

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

@balopat balopat mentioned this pull request Oct 1, 2018
return e
}
}
defer f.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

If the content of this JSON file has any importance, check the error from f.Close().

if err != nil {
return fmt.Errorf("error removing tunnel %s", err)
}
defer f.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

Check the error here too.

},
{
name: "Register + Remove + List",
test: func(t *testing.T, reg *persistentRegistry) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Still this. table driven tests should never contain test-specific logic: http://go/gotestcomments#when-to-use-table-driven-tests

},
{
name: "Register + Remove + List",
test: func(t *testing.T, reg *persistentRegistry) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Name your errors 'err' when possible.

func (t *minikubeTunnel) updateTunnelStatus() *Status {
glog.V(3).Info("updating tunnel status...")
t.status.MinikubeState, _, t.status.MinikubeError = t.clusterInspector.getStateAndHost()
//TODO(balintp): clean this up to be more self contained
Copy link
Contributor

Choose a reason for hiding this comment

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

Clean this up, or remove the TODO. The grossness is self-evident.

machineAPI libmachine.API,
configLoader config.Loader,
v1Core v1.CoreV1Interface, registry *persistentRegistry, router router) (*minikubeTunnel, error) {
clusterInspector := &clusterInspector{
Copy link
Contributor

Choose a reason for hiding this comment

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

name this "ci" so we don't have to guess if you mean the struct name or the local variable.

return fmt.Errorf("there is already a running tunnel for this machine: %s", id)
}

func newTunnel(machineName string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep function signatures as a single line when possible. When it isn't possible, there is your hint that you should take a config struct.


}

type minikubeTunnel struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's difficult to tell the difference between the minikubeTunnel struct and tunnel struct. Can you adjust one of the names to be more distinct?

minikube.tunnel.minikubeTunnel is a rather perverse naming stutter, but since the struct name is private I won't worry too much about this.


exists, conflict, _, err := t.router.Inspect(t.status.TunnelID.Route)
if err != nil {
t.status.RouteError = fmt.Errorf("error checking for route state: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this call return now? That would help to de-indent the non-error flow.

Same elsewhere in this function. https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow


func NewManager() *Manager {
return &Manager{
delay: 5 * time.Second,
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this magic number to a const so that it's self-documenting.

@tstromberg
Copy link
Contributor

Approved with minor nitpicks. Thanks!

@balopat balopat removed the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 18, 2018
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 18, 2018
@balopat balopat merged commit ae9f4b2 into kubernetes:master Oct 18, 2018
@balopat balopat deleted the tunnel branch October 18, 2018 18:01
@paultiplady
Copy link

Looking forward to giving this a spin, thanks for the hard work here!

@omeid
Copy link
Contributor

omeid commented Dec 1, 2018

This looks very interesting and I am keen to give it a crack, any chance this could be cut into a release?

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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants