Introduce the Identity controller implementation by olix0r · Pull Request #2521 · linkerd/linkerd2

olix0r · 2019-03-18T23:17:05Z

This change introduces a new Identity service implementation for the
ioo.linkerd.proxy.identity.Identity gRPC service.

The pkg/identity contains a core, abstract implentation of the service
(generic over both the CA and (Kubernetes) Validator interfaces).

controller/identity inclues a concrete implementation that uses the
Kubernetes TokenReview API to validate serviceaccount tokens when
issuing certificates.

This change does NOT alter installation or runtime to include the
identity service. This will be included in a follow-up.

pkg/identity/service.go

pkg/tls/ca.go

controller/cmd/identity/main.go

This change introduces a new Identity service implementation for the `io.linkerd.proxy.identity.Identity` gRPC service. The `pkg/identity` contains a core, abstract implentation of the service (generic over both the CA and (Kubernetes) Validator interfaces). `controller/identity` inclues a concrete implementation that uses the Kubernetes TokenReview API to validate serviceaccount tokens when issuing certificates. This change does **NOT** alter installation or runtime to include the identity service. This will be included in a follow-up.

klingerf

Nice, seems like a good approach. I just had a bunch of misc feedback. It would also be good see some tests in the controller/identity package, but doesn't have to happen as part of this PR.

Gopkg.toml

controller/cmd/identity/main.go

pkg/identity/service.go

klingerf · 2019-03-19T05:00:06Z

pkg/identity/service.go

+// Register registers an identity service implementation in the provided gRPC
+// server.
+func Register(g *grpc.Server, s *Service) {
+	pb.RegisterIdentityServer(g, s)


This doesn't compile for me. Our pattern in most of our other gRPC servers is to include a NewServer initialization func that handles registration and returns *grpc.Server and net.Listener. For instance:

linkerd2/controller/tap/server.go

Lines 430 to 452 in 81f645d

func NewServer(

addr string,

tapPort uint,

controllerNamespace string,

k8sAPI *k8s.API,

) (*grpc.Server, net.Listener, error) {

k8sAPI.Pod().Informer().AddIndexers(cache.Indexers{podIPIndex: indexPodByIP})

lis, err := net.Listen("tcp", addr)

if err != nil {

return nil, nil, err

}

s := prometheus.NewGrpcServer()

srv := server{

tapPort: tapPort,

k8sAPI: k8sAPI,

controllerNamespace: controllerNamespace,

}

pb.RegisterTapServer(s, &srv)

return s, lis, nil

}

Can we do something like that here?

This pkg module is intentionally kubernetes-agnostic; and it exposes this helper so that the caller doesn't need to import the protobuf library just for this one call.

It would be a shame to make this module pull in a kubernetes dependency just for this builder. But perhaps the controller module should export a constructor like this?

The point about kubernetes isn't exactly relevant I guess.. but the larger point is that the pkg/ module is just a "library" and doesn't have any grpc.Server/net.Listener dependencies -- it only deals in the interfaces. I think the controller/identity package should be responsible for this, if we care to split out of the main...

Ah, sorry, I didn't even realize that this file was in pkg. I'd expect a file that implements a controller gRPC interface to be in controller, not pkg. How would you feel about moving this file to controller/identity? It would also be ok to leave it in pkg, if controller/identity provided a NewServer func like I suggested. But that said, it feels weird to have something that satisfies the IdentityServer interface defined outside of the controller directory.

Btw I totally agree that none of the pkg/... packages should depend on the controller/... packages, with the exception of controller/gen/... (which we should move out of the controller directory anyway). It looks like we abide by that for the most part, with a few lapses:

pkg/healthcheck/healthcheck.go: "github.com/linkerd/linkerd2/controller/api/public" pkg/profiles/tap.go: "github.com/linkerd/linkerd2/controller/api/util"

My thought process is the following: pkg/identity implements the grpc interface in terms of two other interfaces: Validator, and Issuer. The controller, which handles configuration and runtime logic, builds a validator and Issuer. I structured this specifically this way because I know of another user that wants to implement another identity service. Providing a clear library interface for implementing this seemed preferable to bundling the logical service and the concrete server.

Ok, that all makes sense. Since this implementation deviates from the implementation of the rest of our gRPC services in the project, it's adding complexity to the codebase. We can certainly justify the complexity, as it exists to satisfy some external use case, but I wanted to at least note it.

pkg/identity/service.go

controller/identity/domain.go

controller/identity/validator.go

olix0r · 2019-03-19T15:25:36Z

@klingerf I've addressed most of your feedback. I can't reproduce the compilation errors you're seeing, so I have not fixed those. And I would prefer to keep pkg/identity independent of application concerns like binding the service to a socket, prometheus, etc.

klingerf

⭐️ Thanks for making those updates, and sorry for the compilation comments -- my vendored deps had gotten into a weird state. After reinstalling them, everything builds as expected. I had one more comment about project organization, but would be fine addressing that in a separate branch. 🚢

klingerf · 2019-03-19T15:36:46Z

pkg/identity/service.go

+// Register registers an identity service implementation in the provided gRPC
+// server.
+func Register(g *grpc.Server, s *Service) {
+	pb.RegisterIdentityServer(g, s)


Ah, sorry, I didn't even realize that this file was in pkg. I'd expect a file that implements a controller gRPC interface to be in controller, not pkg. How would you feel about moving this file to controller/identity? It would also be ok to leave it in pkg, if controller/identity provided a NewServer func like I suggested. But that said, it feels weird to have something that satisfies the IdentityServer interface defined outside of the controller directory.

Btw I totally agree that none of the pkg/... packages should depend on the controller/... packages, with the exception of controller/gen/... (which we should move out of the controller directory anyway). It looks like we abide by that for the most part, with a few lapses:

pkg/healthcheck/healthcheck.go: "github.com/linkerd/linkerd2/controller/api/public" pkg/profiles/tap.go: "github.com/linkerd/linkerd2/controller/api/util"

klingerf

⭐️ Thanks for the additional info. Just had a few follow-up comments, but looks good to me.

pkg/identity/service.go

controller/identity/validator.go

klingerf · 2019-03-19T19:24:21Z

pkg/identity/service.go

+// Register registers an identity service implementation in the provided gRPC
+// server.
+func Register(g *grpc.Server, s *Service) {
+	pb.RegisterIdentityServer(g, s)


Ok, that all makes sense. Since this implementation deviates from the implementation of the rest of our gRPC services in the project, it's adding complexity to the codebase. We can certainly justify the complexity, as it exists to satisfy some external use case, but I wanted to at least note it.

#2521 introduces an "Identity" controller, but there is no way to include it in linkerd installation. This change alters the `install` flow as follows: - An Identity service is _always_ installed; - Issuer credentials may be specified via the CLI; - If no Issuer credentials are provided, they are generated each time `install` is called. - It's possible to override the credential generation logic, especially for tests; - Proxies are NOT configured to use the identity service.

alpeb

It all looked great to me 👍 I just added some nits below.

controller/identity/domain.go

controller/identity/validator.go

pkg/identity/service.go

klingerf

⭐️ Latest updates look great -- thanks for making those changes.

alpeb

Just one typo, and good to go IMO

pkg/identity/service.go

#2521 introduces an "Identity" controller, but there is no way to include it in linkerd installation. This change alters the `install` flow as follows: - An Identity service is _always_ installed; - Issuer credentials may be specified via the CLI; - If no Issuer credentials are provided, they are generated each time `install` is called. - Proxies are NOT configured to use the identity service. - It's possible to override the credential generation logic---especially for tests---via install options that can be configured via the CLI.

@cromulentbanana

## stable-2.14.5 This stable release fixes a proxy regression where bursts of TCP connections could result in EOF errors, due to an incorrect queue capacity. In addition, it includes fixes for the control plane, dependency upgrades, and support for image digests in Linkerd manifests. * Added a controlPlaneVersion override to the `linkerd-control-plane`` Helm chart to support including SHA256 image digests in Linkerd manifests (thanks @cromulentbanana!) ([#11406]; fixes [#11312]) * Added a `checksum/config `annotation to the destination and proxy injector deployment manifests, to force restarting those workloads whenever their webhook secrets change during upgrade (thanks @iAnomaly!) ([#11440]; fixes [#6940]) * Updated the Policy controller's OpenSSL dependency to v3, as OpenSSL 1.1.1 is EOL ([#11625]) * proxy: Increased `DEFAULT_OUTBOUND_TCP_QUEUE_CAPACITY` to prevent EOF errors during bursts of TCP connections (proxy PR [#2521][proxy-2521]) [#11406]: #11406 [#11312]: #11312 [#11440]: #11440 [#6940]: #6940 [#11625]: #11625 [proxy-2521]: linkerd/linkerd2-proxy#2521

@cromulentbanana

## stable-2.14.5 This stable release fixes a proxy regression where bursts of TCP connections could result in EOF errors, due to an incorrect queue capacity. In addition, it includes fixes for the control plane, dependency upgrades, and support for image digests in Linkerd manifests. * Added a controlPlaneVersion override to the `linkerd-control-plane`` Helm chart to support including SHA256 image digests in Linkerd manifests (thanks @cromulentbanana!) ([#11406]; fixes [#11312]) * Added a `checksum/config `annotation to the destination and proxy injector deployment manifests, to force restarting those workloads whenever their webhook secrets change during upgrade (thanks @iAnomaly!) ([#11440]; fixes [#6940]) * Updated the Policy controller's OpenSSL dependency to v3, as OpenSSL 1.1.1 is EOL ([#11625]) * proxy: Increased `DEFAULT_OUTBOUND_TCP_QUEUE_CAPACITY` to prevent EOF errors during bursts of TCP connections (proxy PR [#2521][proxy-2521]) [#11406]: #11406 [#11312]: #11312 [#11440]: #11440 [#6940]: #6940 [#11625]: #11625 [proxy-2521]: linkerd/linkerd2-proxy#2521

@cromulentbanana

## stable-2.14.5 This stable release fixes a proxy regression where bursts of TCP connections could result in EOF errors, due to an incorrect queue capacity. In addition, it includes fixes for the control plane, dependency upgrades, and support for image digests in Linkerd manifests. * Added a controlPlaneVersion override to the `linkerd-control-plane`` Helm chart to support including SHA256 image digests in Linkerd manifests (thanks @cromulentbanana!) ([#11406]; fixes [#11312]) * Added a `checksum/config `annotation to the destination and proxy injector deployment manifests, to force restarting those workloads whenever their webhook secrets change during upgrade (thanks @iAnomaly!) ([#11440]; fixes [#6940]) * Updated the Policy controller's OpenSSL dependency to v3, as OpenSSL 1.1.1 is EOL ([#11625]) * proxy: Increased `DEFAULT_OUTBOUND_TCP_QUEUE_CAPACITY` to prevent EOF errors during bursts of TCP connections (proxy PR [#2521][proxy-2521]) [#11406]: #11406 [#11312]: #11312 [#11440]: #11440 [#6940]: #6940 [#11625]: #11625 [proxy-2521]: linkerd/linkerd2-proxy#2521

olix0r self-assigned this Mar 18, 2019

olix0r changed the title ~~Inroduce the Identity controller implementation~~ Introduce the Identity controller implementation Mar 18, 2019

olix0r added area/controller area/identity Automatic transport identity labels Mar 18, 2019

olix0r requested review from alpeb, ihcsim and klingerf March 18, 2019 23:19

olix0r commented Mar 18, 2019

View reviewed changes

pkg/identity/service.go Outdated Show resolved Hide resolved

olix0r commented Mar 18, 2019

View reviewed changes

pkg/tls/ca.go Outdated Show resolved Hide resolved

olix0r commented Mar 18, 2019

View reviewed changes

controller/cmd/identity/main.go Outdated Show resolved Hide resolved

olix0r changed the base branch from ver/kill-ca to identity/master March 19, 2019 00:40

olix0r changed the base branch from identity/master to master March 19, 2019 00:41

olix0r force-pushed the ver/identity-service branch from 22c79bd to b4b3bbf Compare March 19, 2019 00:46

olix0r requested a review from siggy March 19, 2019 00:46

olix0r force-pushed the ver/identity-service branch from b4b3bbf to 00225b5 Compare March 19, 2019 01:11

olix0r marked this pull request as ready for review March 19, 2019 01:14

olix0r force-pushed the ver/identity-service branch from 00225b5 to 771e7a3 Compare March 19, 2019 01:18

klingerf reviewed Mar 19, 2019

View reviewed changes

olix0r added 5 commits March 19, 2019 06:10

update go deps shas

6e448cb

gopkg.lock

382bdbb

improve validation

f063d19

review fixups

c604a97

whitespace nits

7329a8e

klingerf approved these changes Mar 19, 2019

View reviewed changes

olix0r mentioned this pull request Mar 19, 2019

install: Introduce the Identity controller #2526

Merged

alpeb reviewed Mar 19, 2019

View reviewed changes

Review feedback; especially error messages

78360a3

klingerf approved these changes Mar 19, 2019

View reviewed changes

alpeb approved these changes Mar 19, 2019

View reviewed changes

pkg/identity/service.go Outdated Show resolved Hide resolved

typos

536c503

olix0r merged commit 790c13b into master Mar 19, 2019

admc mentioned this pull request Apr 4, 2019

Linkerd 2.3 release test plan #2459

Closed

25 tasks

olix0r deleted the ver/identity-service branch October 13, 2020 14:55

	func NewServer(
	addr string,
	tapPort uint,
	controllerNamespace string,
	k8sAPI *k8s.API,
	) (*grpc.Server, net.Listener, error) {
	k8sAPI.Pod().Informer().AddIndexers(cache.Indexers{podIPIndex: indexPodByIP})

	lis, err := net.Listen("tcp", addr)
	if err != nil {
	return nil, nil, err
	}

	s := prometheus.NewGrpcServer()
	srv := server{
	tapPort: tapPort,
	k8sAPI: k8sAPI,
	controllerNamespace: controllerNamespace,
	}
	pb.RegisterTapServer(s, &srv)

	return s, lis, nil
	}

Conversation

olix0r commented Mar 18, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

klingerf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

klingerf Mar 19, 2019

Choose a reason for hiding this comment

Uh oh!

olix0r Mar 19, 2019

Choose a reason for hiding this comment

Uh oh!

olix0r Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klingerf Mar 19, 2019

Choose a reason for hiding this comment

Uh oh!

olix0r Mar 19, 2019

Choose a reason for hiding this comment

Uh oh!

klingerf Mar 19, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olix0r commented Mar 19, 2019

Uh oh!

klingerf left a comment

Choose a reason for hiding this comment

Uh oh!

klingerf Mar 19, 2019

Choose a reason for hiding this comment

Uh oh!

klingerf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

klingerf Mar 19, 2019

Choose a reason for hiding this comment

Uh oh!

alpeb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

klingerf left a comment

Choose a reason for hiding this comment

Uh oh!

alpeb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

olix0r Mar 19, 2019 •

edited

Loading