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

Transparent proxy injection #1

Merged
merged 11 commits into from
Jul 8, 2017
Merged

Transparent proxy injection #1

merged 11 commits into from
Jul 8, 2017

Conversation

rmars
Copy link

@rmars rmars commented Jul 3, 2017

Transparently proxy all outgoing requests to a daemonset linkerd via iptables rules (supplied by an init container).

This only looks big because of /vendor. Relevant code is in ce4887e.

This is based on istioctl's proxy injection (they run a sidecar proxy and do transparent proxying via iptables rules).

  • Adds a Dockerfile to create an init-container image that we can use to apply iptables rules to the pod. These rules forward outgoing pod traffic to a daemonset linkerd
  • Add prepare_proxy.sh which sets up these rules
  • Add an inject.go which will add the initContainer to the user's resource file

All this is based heavily on istio's existing code and ideally this would go with that code somewhere, but I think it is being moved around right now. Would be nice to delete our inject.go in favour of something in that repo after it has settled.

Testing:
Ran inject.go against yamls in https://github.com/istio/pilot/tree/master/platform/kube/testdata.
Used our hello world example, ran it in minikube and in a 3 node GKE cluster.

Notes:

Fixes linkerd#1415

Risha Mars added 2 commits July 3, 2017 16:41
* Add script which generates iptables rules to forward traffic to a daemonset linkerd
* Add Dockerfile to build this initContainer
* Add go script to inject this initContainer into the user's config
@rmars rmars self-assigned this Jul 3, 2017
Copy link
Member

@klingerf klingerf left a comment

Choose a reason for hiding this comment

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

⭐️ This looks good to me -- nice work!

README.md Outdated
```

It is based on Istio's method of
[injecting sidecars](https://github.com/istio/pilot/blob/master/doc/proxy-injection.md),
Copy link
Member

Choose a reason for hiding this comment

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

Super minor, but since this code is in flux right now, maybe consider pinning this to a specific release tag, rather than master. How about https://github.com/istio/pilot/blob/stable-a632657/doc/proxy-injection.md?

Copy link
Author

Choose a reason for hiding this comment

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

ooh, yeah that's a good idea! thanks!

README.md Outdated
more directly, but this seems to be in transit right now. This `prepare_proxy.sh`
sets up iptables rules for transparently proxying requests to a Daemonset linkerd
(rather than a sidecar proxy, which Istio
[currently uses](https://github.com/istio/pilot/blob/master/docker/prepare_proxy.sh)).
Copy link
Member

Choose a reason for hiding this comment

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

Same comment goes for here (and the rest of the github.com/istio/pilot links in this diff): https://github.com/istio/pilot/blob/stable-a632657/docker/prepare_proxy.sh

Copy link

@esbie esbie left a comment

Choose a reason for hiding this comment

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

This is looking very nice ^_^

@@ -0,0 +1,88 @@
---
Copy link

Choose a reason for hiding this comment

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

maybe name this hello-world.yml to match the linkerd-examples examples

Copy link
Author

Choose a reason for hiding this comment

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

oops, yes, thanks!

},
},
},
},
Copy link

Choose a reason for hiding this comment

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

Could you add this env section to the comment at the top of the file?

I'm a little concerned that this means this doesn't work in minikube? It's not a blocker for this initial PR, but I definitely am interested in finding a way to support minikube.

Copy link
Author

Choose a reason for hiding this comment

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

Yeahhhh agreed. Since minikube has one node, maybe we could use the l5d service VIP in the iptables rules rather than the host ip (which we can't get in minikube)?

So I can get this to run in minikube by doing the following, though it's a bit much to ask a user to do this:

Modify prepare_proxy.sh to use DNAT to l5d service VIP:

iptables -t nat -A OUTPUT -p tcp -j DNAT --to ${L5D_SERVICE_HOST}:${LINKERD_PORT}                  -m comment --comment "istio/dnat-to-daemonset-l5d"

Build a linkerd/istio-init docker image locally and use that.
Use inject.go or manually insert the initContainer section in your pod spec. Make sure to remove imagePullPolicy: Always from this section though.

Deploy your modified yaml. Then:

L5D_PORT=$(kubectl get svc l5d -o jsonpath="{.spec.ports[0].nodePort}")
curl -v $(minikube ip):$L5D_PORT -H "Host: hello"

I was just talking to @adleong about this and maybe we can add a flag to inject.go so that people can run in minikube without having to build their own image for this.

Copy link
Author

Choose a reason for hiding this comment

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

Okay added a -runInMinikube in 2a128ea

# Build linkerd inject image
#
# docker build -t buoyantio/istio-init:v1 .
# docker push buoyantio/istio-init:v1
Copy link

Choose a reason for hiding this comment

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

How about putting this at linkerd/istio-init instead?

Risha Mars added 4 commits July 5, 2017 14:40

if [ "$RUN_IN_MINIKUBE" = true ]; then
# Forward traffic to the linkerd service VIP on the single minikube node
iptables -t nat -A OUTPUT -p tcp -j DNAT --to ${L5D_SERVICE_HOST}:${LINKERD_PORT} -m comment --comment "istio/dnat-to-minikube-l5d"
Copy link

Choose a reason for hiding this comment

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

I'm a little confused, where is L5D_SERVICE_HOST set?

Copy link
Author

@rmars rmars Jul 6, 2017

Choose a reason for hiding this comment

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

Kubernetes creates these env variables (https://kubernetes.io/docs/concepts/containers/container-environment-variables/)
This does assume we've named the linkerd service "l5d" - so I should probably explicitly pass in the service name

@esbie
Copy link

esbie commented Jul 6, 2017

-runInMinikube is straightforward but not descriptive. Would prefer --use-vip or --single-node or something, because minikube isn't the only kubernetes installation without downward api access. In fact I was just helping someone with that kind of setup yesterday in linkerd/linkerd#1464

@adleong
Copy link
Member

adleong commented Jul 6, 2017

or maybe --l5d-host

Copy link

@esbie esbie left a comment

Choose a reason for hiding this comment

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

the changes look great! 💡 🎥 🎬

@rmars rmars merged commit b811d93 into master Jul 8, 2017
@rmars rmars deleted the rmars/inject branch July 8, 2017 00:24
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants