Implement kubernetes_service registration and startup #4611

awly · 2020-10-21T19:08:10Z

The new service now starts, registers (locally or via a join token) and
heartbeats its presence to the auth server.

This service can handle k8s requests (like a proxy) but not to remote
teleport clusters. Proxies will be responsible for routing those.
The client (kubectl after tsh configures is) will not yet reach this service automatically, until proxy routing is
implemented. I manually tweaked server addres in kubeconfig to test it.

You can also run tctl get kube_service to list all registered
instances. The reported info is currently limited - only listening
address is set.

Existing k8s functionality in the proxy is not affected.

Updates #3952

lib/auth/auth_with_roles.go

lib/config/configuration.go

lib/config/fileconf.go

r0mant · 2020-10-27T03:50:08Z

lib/config/fileconf.go

+	if s.Configured() {
+		return !s.Enabled(def)
+	}
+	return !def


Hm, may be just me but it's a bit confusing - so the passed value def is in fact not the default but the inverse of the default? I.e. Disabled(true) will in fact return false if the service is not configured and vice versa?

This is more than a little confusing to me too.

Yeah, this is confusing.
def specifies whether a given service is enabled by default in teleport, not the default return value for this method if service isn't configured.

Changed this to set the default value as a field in Service. Hopefully it's less confusing.

r0mant · 2020-10-27T03:52:32Z

lib/kube/proxy/forwarder.go

-		From: &c.remoteAddr,
-		To:   &utils.NetAddr{AddrNetwork: "tcp", Addr: c.targetAddr},
-	})
+func (c *tpClusterClient) Dial(network, addr string) (net.Conn, error) {


Nit: Missing godocs on Dial and DialWithContext.

these methods are on an unexported struct and won't appear in godoc output.

lib/kube/proxy/forwarder.go

lib/service/kubernetes.go

webvictim · 2020-10-27T15:00:31Z

lib/config/fileconf.go

+	if s.Configured() {
+		return !s.Enabled(def)
+	}
+	return !def


This is more than a little confusing to me too.

lib/service/service.go

lib/auth/permissions.go

a-palchikov · 2020-10-29T12:54:48Z

lib/config/configuration_test.go

-	c.Assert(conf.Proxy.WebAddr, check.Equals, "tcp://web_addr")
-	c.Assert(conf.Proxy.TunAddr, check.Equals, "reverse_tunnel_address:3311")
+	conf, err = ReadFromFile(testConfigs.configFile)
+	require.NoError(t, err)


Have you considered asserting the value as a whole instead of individual attributes?
I'm not too familiar with the require package and not sure how one goes about that but it is generally less verbose and more consistent since it avoids unexpected changes to attributes that aren't validated.

Yep, I was too lazy when refactoring this test the first time.
Changed to do a full struct compare, plus testing the Service helpers

lib/config/fileconf.go

a-palchikov · 2020-10-29T13:10:51Z

lib/service/connect.go

-		// Only attempt to connect through the proxy for nodes.
-		if identity.ID.Role != teleport.RoleNode {
+		// Don't attempt to connect through a tunnel as a proxy or auth server.
+		if identity.ID.Role == teleport.RoleAuth || identity.ID.Role == teleport.RoleProxy {
 			return nil, trace.Unwrap(err)


I've never seen the error being actually unwrapped on return and just wanted to make sure it is intentional.

Yeah, this looks like a typo.
If it's not, there should've been a comment explaining it.
Changed to trace.Wrap

lib/service/kubernetes.go

a-palchikov

lgtm although I'd need more time for it to sink in in terms of the bigger picture.

lib/auth/apiserver.go

lib/cache/cache.go

lib/service/kubernetes.go

The new service now starts, registers (locally or via a join token) and heartbeats its presence to the auth server. This service can handle k8s requests (like a proxy) but not to remote teleport clusters. Proxies will be responsible for routing those. The client (tsh) will not yet go to this service, until proxy routing is implemented. I manually tweaked server addres in kubeconfig to test it. You can also run `tctl get kube_service` to list all registered instances. The self-reported info is currently limited - only listening address is set.

r0mant · 2020-11-09T17:14:29Z

lib/service/kubernetes.go

+		if payload != nil {
+			// Graceful shutdown.
+			warnOnErr(kubeServer.Shutdown(payloadContext(payload)))
+			agentPool.Stop()


@awly Just noticed something in this old PR as I'm working on a similar part in db access. Looks like in case of direct dial (!conn.UseTunnel() case above), agentPool will stay uninitialized so these stop/wait calls will probably panic in that case?

I made stop/wait nil-safe: https://github.com/gravitational/teleport/pull/4611/files#diff-b9f7168b21e0fd7edd940ce7547de3171339012b8d4064d135e50340c6790c9fR171-R185

awly requested review from fspmarshall, klizhentas, russjones and webvictim as code owners October 21, 2020 19:08

awly changed the title ~~Implement kubernetes_service registration and sratup~~ Implement kubernetes_service registration and startup Oct 21, 2020

awly force-pushed the andrew/kubernetes_service_core branch from da90a79 to a1f2b4d Compare October 22, 2020 17:43

awly mentioned this pull request Oct 22, 2020

Make k8s permissions test optional #4618

Merged

awly force-pushed the andrew/kubernetes_service_core branch 2 times, most recently from 79e595a to 40be491 Compare October 26, 2020 16:15

r0mant reviewed Oct 27, 2020

View reviewed changes

webvictim reviewed Oct 27, 2020

View reviewed changes

awly force-pushed the andrew/kubernetes_service_core branch from 40be491 to 3e486ee Compare October 28, 2020 21:59

awly requested a review from a-palchikov as a code owner October 28, 2020 21:59

awly force-pushed the andrew/kubernetes_service_core branch 2 times, most recently from 4e81998 to 258c282 Compare October 28, 2020 22:17

awly requested review from r0mant and webvictim October 28, 2020 22:24

a-palchikov reviewed Oct 29, 2020

View reviewed changes

awly force-pushed the andrew/kubernetes_service_core branch from 258c282 to 6b035f6 Compare October 29, 2020 16:59

awly requested a review from a-palchikov October 29, 2020 17:00

a-palchikov approved these changes Oct 29, 2020

View reviewed changes

r0mant approved these changes Oct 29, 2020

View reviewed changes

lib/auth/apiserver.go Show resolved Hide resolved

lib/cache/cache.go Show resolved Hide resolved

lib/service/kubernetes.go Outdated Show resolved Hide resolved

lib/service/kubernetes.go Outdated Show resolved Hide resolved

Andrew Lytvynov added 2 commits October 30, 2020 09:54

Address review feedback

7a52a6d

awly force-pushed the andrew/kubernetes_service_core branch from 6b035f6 to 7a52a6d Compare October 30, 2020 17:01

awly merged commit 5ec194c into master Oct 30, 2020

awly deleted the andrew/kubernetes_service_core branch October 30, 2020 17:19

awly mentioned this pull request Nov 5, 2020

Kubernetes request routing and cluster registration #4670

Merged

r0mant reviewed Nov 9, 2020

View reviewed changes

awly mentioned this pull request Nov 10, 2020

Support k8s clusters behind firewall/NAT using a single teleport cluster #3667

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement kubernetes_service registration and startup #4611

Implement kubernetes_service registration and startup #4611

awly commented Oct 21, 2020

r0mant Oct 27, 2020

webvictim Oct 27, 2020

awly Oct 28, 2020

r0mant Oct 27, 2020

awly Oct 28, 2020

webvictim Oct 27, 2020

a-palchikov Oct 29, 2020

awly Oct 29, 2020

a-palchikov Oct 29, 2020

awly Oct 29, 2020

a-palchikov left a comment

r0mant Nov 9, 2020

awly Nov 9, 2020

Implement kubernetes_service registration and startup #4611

Implement kubernetes_service registration and startup #4611

Conversation

awly commented Oct 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-palchikov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment