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

feat(*) adding kumactl install transparent-proxy #1321

Merged
merged 20 commits into from
Dec 21, 2020

Conversation

nickolaev
Copy link
Contributor

@nickolaev nickolaev commented Dec 16, 2020

Summary

Expanding kumactl with new commands to manage host setting and enable Transparent Proxy mode:

kumactl install transparent-proxy --kuma-dp-user kuma-dp --kuma-cp-ip 1.2.3.4

The kuma-dp user should be pre-created on the host and the kuma-dp process should be started as this user. One option is to use runuser

The typical Dataplane resource to be used with transparent proxy looks like this:

type: Dataplane
mesh: default
name: {{ name }}
networking:
  address: {{ address }}
  inbound:
  - port: {{ port }}
    tags:
      kuma.io/service: demo-client
  transparentProxying:
    redirectPortInbound: 15006
    redirectPortOutbound: 15001

Where {{ name }}, {{ address }} and {{ port }} can be command line substituted variables. The ports 15001 and 15006 are the defaults set in kumactl install transparent-proxy. The additional flags --redirect-inbound-port and --redirect-outbound-port will set these to whatever values are needed.

A full command that runs kuma-dp process as kuma-dp user and substitutes the variables in the yaml:

runuser -u kuma-dp -- /usr/bin/kuma-dp run --cp-address=https://172.19.0.2:5678 --dataplane-token-file=/kuma/token-demo --dataplane-file=/kuma/dpyaml-demo --dataplane-var name=dp-demo --dataplane-var address=172.19.0.4 --dataplane-var port=80  --binary-path /usr/local/bin/envoy

Reverting the transparent proxy changes made on the host:

kumactl uninstall transparent-proxy

Documentation

Nikolay Nikolaev added 7 commits December 11, 2020 10:55
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
…y' into feat/kumactl_install_transparent_proxy
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
@nickolaev nickolaev force-pushed the feat/kumactl_install_transparent_proxy branch from 1bd8362 to 8233e25 Compare December 16, 2020 10:51
Nikolay Nikolaev added 3 commits December 16, 2020 17:16
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
@subnetmarco
Copy link
Contributor

Even if the user installs transparent proxying in some hosts, they may still be setting up outbound sections in their data plane proxy specifications.

  • Do we need to show a warning when that happens?
  • Do we keep it working?
  • Will it work in conjunction with transparent proxying?

Nikolay Nikolaev added 3 commits December 17, 2020 11:09
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
@nickolaev
Copy link
Contributor Author

nickolaev commented Dec 17, 2020

  • Do we need to show a warning when that happens?

We do have an explicit check for this already:

		// this part of validation works only for Universal scenarios with TransparentProxying
		if dp.Spec.Networking.TransparentProxying != nil && len(dp.Spec.Networking.Outbound) != 0 {
			var err validators.ValidationError
			err.AddViolation("outbound", "should be empty since dataplane is in Transparent Proxying mode")
			return nil, err.OrNil()
		}

This happens late in the generation process, so this message is seen on the envoy console, it may make sense to actually check this early when the Dataplane is instantiated.

  • Do we keep it working?

The iptables rules we deploy have an explicit rule that does not redirect any traffic that is towards localhost. This essentially prevent us from having localhost:4000 listeners. Well we can have these, but they will never get any traffic when the iptables rules are deployed.

  • Will it work in conjunction with transparent proxying?

It technically can be made to work with some patching, but then we have the iptables rules not forwarding the traffic. Not saying it is impossible, it just will make things harder to maintain in production, and I personally don't see any particular benefit.

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
@nickolaev nickolaev force-pushed the feat/kumactl_install_transparent_proxy branch from 37376da to e60bab8 Compare December 17, 2020 15:27
@nickolaev nickolaev marked this pull request as ready for review December 17, 2020 15:30
@nickolaev nickolaev requested a review from a team as a code owner December 17, 2020 15:30
Nikolay Nikolaev added 2 commits December 17, 2020 18:10
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Copy link
Contributor

@jakubdyszkiewicz jakubdyszkiewicz left a comment

Choose a reason for hiding this comment

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

Some nits here and there but overall nice 👏


if args.DryRun {
_, _ = cmd.OutOrStdout().Write([]byte(output))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

else: print something like iptables modified.

What happens if I run this command twice?

Copy link
Contributor

Choose a reason for hiding this comment

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

can you maybe extract this to method modifyIptables()? and below to modifyResolvConf()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolv.conf will not be modified for sure. I think iptables will not too.

cmd.Flags().StringVar(&args.User, "kuma-dp-user", args.UID, "the user that will run kuma-dp")
cmd.Flags().StringVar(&args.UID, "kuma-dp-uid", args.UID, "the UID of the user that will run kuma-dp")
cmd.Flags().BoolVar(&args.ModifyResolvConf, "modify-resolv-conf", args.ModifyResolvConf, "modify the host `/etc/resolv.conf` to allow `.mesh` resolution through kuma-cp")
cmd.Flags().IPVar(&args.KumaCpIP, "kuma-cp-ip", args.KumaCpIP, "the ")
Copy link
Contributor

Choose a reason for hiding this comment

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

the ... end? :D
btw. is it IP only without port? can this be a hostname?

return errors.Wrap(err, "uanble to open /etc/resolv.conf")
}
_, _ = cmd.OutOrStdout().Write(content)
_, _ = cmd.OutOrStdout().Write([]byte("\n"))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think instead of printing resolv.conf I'd print something like:

/etc/resolv.conf modified. The previous version of the file backed up to /etc/resolv.conf.kuma

nit: how about instead of /etc/resolv.conf.kuma call it /etc/resolv.conf.kuma-backup or /etc/resolv.conf.backup-by-kuma. Also it would be nice to add as a first line a comment to this file:

# This file was backed up by kumactl install transparent proxy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All applied except for modifying the file, as it will make restoring it slightly harder. But meanwhile, its name is still saying enough (kuma-backup).

Cleanup(dryRun bool) (string, error)
}

func GetDefaultTransparentProxy() TransparentProxy {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: just DefaultTransparentProxy() without Get, to follow our convention in components.go files.


It("should access the service using .mesh", func() {

for i := 0; i < iterations; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need iterations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, just to have more tries and see it is consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

but we do have DoWithRetry in the iterations. Even if this is not consistent (example - every other request works), it will be retried in the loop and the test will pass.

test/framework/universal_app.go Outdated Show resolved Hide resolved
os.Args = append(os.Args, savedArgs...)
}()

if err := install.GetCommand().Execute(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of messing around with global os.Args and os.Stdout, can we do it like with tests?

cmd := install.GetCommand()

cmd.SetArgs(...)

stdout := &bytes.Buffer{}
stderr := &bytes.Buffer{}
cmd.SetOut(stdout)
cmd.SetErr(stderr)

err := cmd.Execute()

stdout.Bytes()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately no, because:

  • viper validates that everything in os.Args is a valid flag, and it does not expect to find there our own flags
  • the execute functions in istio-iptabes are directly writing to os.Stdout, there is no way around this

Nikolay Nikolaev added 3 commits December 21, 2020 09:53
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

It("should access the service using .mesh", func() {

for i := 0; i < iterations; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

but we do have DoWithRetry in the iterations. Even if this is not consistent (example - every other request works), it will be retried in the loop and the test will pass.

@nickolaev nickolaev merged commit 5dcd5ee into master Dec 21, 2020
@nickolaev nickolaev deleted the feat/kumactl_install_transparent_proxy branch December 21, 2020 12:12
mergify bot pushed a commit that referenced this pull request Dec 21, 2020
* chore(*) add kuma-iptables to universal

* test(e2e) add universal transparent proxy

* test(e2e) transparent proxy cleanup

* feat(*) add Istio transparent proxy

* test(*) e2e uses kumactl install transparent-proxy

* chore(*) flexible configuration

* chore(*) modify resolv conf

* tests(*) transparent proxy

* fix(*) new error in golang-migrate/migrate v4.14.1

* test(*) install the transparent proxy

* docs(*) more extensive help for transparent proxy

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
(cherry picked from commit 5dcd5ee)

# Conflicts:
#	go.mod
#	go.sum
nickolaev pushed a commit that referenced this pull request Dec 21, 2020
* feat(*) adding kumactl install transparent-proxy (#1321)

* chore(*) add kuma-iptables to universal

* test(e2e) add universal transparent proxy

* test(e2e) transparent proxy cleanup

* feat(*) add Istio transparent proxy

* test(*) e2e uses kumactl install transparent-proxy

* chore(*) flexible configuration

* chore(*) modify resolv conf

* tests(*) transparent proxy

* fix(*) new error in golang-migrate/migrate v4.14.1

* test(*) install the transparent proxy

* docs(*) more extensive help for transparent proxy


Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

Co-authored-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
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.

None yet

3 participants