kubernetes-sigs / kubernetes-sigs/controller-runtime

ManagerOptions#CertDir default is confusing

Open
#900 18 comments 3 reactions 1 assignee Claimed by @didiberman View on GitHub
good first issue help wanted kind/bug lifecycle/frozen priority/important-longterm
Dominant language
Go
Stars
3k
Forks
1.3k
Avg merge
2d 5h
Merged PRs (30d)
14

Description

# Problem Description

The default for the `CertDir` configuration option is presently nonsensical. I expect because it is out of date with other changes in the expected workflow for a developer to configure TLS. A prior PR #300 removed the cert provisioners that would create local credentials at `{TempDir}/k8s-webhook-server/serving-certs/tls.key` and `{TempDir}/k8s-webhook-server/serving-certs/tls.crt` This default was likely missed in the refactor due to some unexpected tight coupling to the prior implementation. However, it creates some confusion for a new developer when trying to run [the examples](https://github.com/kubernetes-sigs/controller-runtime/tree/master/examples).

[CertDir has the comment](https://github.com/kubernetes-sigs/controller-runtime/blob/a7c8a93c1cf395974911ef0ece977a7d27b2bf4b/pkg/manager/manager.go#L175)
```
// CertDir is the directory that contains the server key and certificate.
// if not set, webhook server would look up the server key and certificate in
// {TempDir}/k8s-webhook-server/serving-certs. The server key and certificate
// must be named tls.key and tls.crt, respectively.
CertDir string
```

In the logs running the example you will get:
```
unable to run manager {"error": "open /var/folders/kc/7wscczc57v15s84cy1nx0d3h0000gn/T/k8s-webhook-server/serving-certs/tls.crt: no such file or directory"}
```

# Developer Experience

A new developer is likely to run through the examples in the repository. This is what I was doing. The `CertDir` default sort of served as a red herring, masking the problem for awhile. The default value is so specific it seemed like something else was broken and the examples should work as written. However, in the current implementation it seems they require additional configuration.

# Possible Solution

Would love some feedback on the proper updates, but my inclination is to:
1. Remove the outdated default for `CertDir`
2. Update the comment on the type definition
3. Add a helpful logging message on receiving a `nil` value for `CertDir`
4. Update the examples to setup and reference local certs using `mkcert`
5. (maybe?) Change the signature for `manager.New` to reflect that option is not optional

# Additional Context

```
$ kc version
Client Version: version.Info{Major:"1", Minor:"16", GitVersion:"v1.16.3", GitCommit:"b3cbbae08ec52a7fc73d334838e18d17e8512749", GitTreeState:"clean", BuildDate:"2019-11-14T04:24:29Z", GoVersion:"go1.12.13", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"15", GitVersion:"v1.15.5", GitCommit:"20c265fef0741dd71a66480e35bd69f18351daea", GitTreeState:"clean", BuildDate:"2019-10-15T19:07:57Z", GoVersion:"go1.12.10", Compiler:"gc", Platform:"linux/amd64"}
```

```
$ go version
go version go1.13.8 darwin/amd64
```

# Related Prior Contributions
@mengqiy authored PR #300 and it was reviewed by @droot and @DirectXMan12 and all of them may likely have superior context to the past, present, and future state. Would love to have y'alls input and thank you for your contributions ❤️

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.