support gen-manifests and disable-installer #158
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mengqiy If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
mengqiy
force-pushed
the
gen_manifests
branch
from
36660e1 to
5cd64e2
Compare
mengqiy
force-pushed
the
gen_manifests
branch
from
5cd64e2 to
402440c
Compare
example/main.go
Outdated
| @@ -39,18 +39,26 @@ import ( | |||
| var log = logf.Log.WithName("example-controller") | |||
|
|
|||
| func main() { | |||
| var genManifests, disableInstaller bool | |||
| flag.BoolVar(&genManifests, "generate-manifests", false, | |||
There was a problem hiding this comment.
Do we want to be more specific here ? generate-webhook-manifests ?
pkg/webhook/bootstrap.go
Outdated
| @@ -129,6 +135,14 @@ func (s *Server) installWebhookConfig() error { | |||
| return s.err | |||
| } | |||
|
|
|||
| if !s.GenerateManifests && s.DisableInstaller { | |||
| log.Info("webhook installer is disabled") | |||
There was a problem hiding this comment.
log seems to be incorrect. may be add "and generating webhook manifests is also disabled"
| }, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
I was thinking about just printing out the webhook config only. Outputting other manifests means, now we have two different ways to generate the manifests for controller ?
There was a problem hiding this comment.
IMO using generated Deployment (can also be a StatefulSet with a headless Service) is less error-prone.
There are quite some places need to be set correctly in order to make the webhook server binary work correctly.
We can discuss in person tomorrow :)
| type SelfSignedCertGenerator struct{} | ||
| type SelfSignedCertGenerator struct { | ||
| CAKey []byte | ||
| CACert []byte |
There was a problem hiding this comment.
why are they public ? (if they are public then probably we don't need the SetCA method ?
| var valid bool | ||
| var err error | ||
|
|
||
| valid, signingKey, signingCert = cp.validCACert() |
There was a problem hiding this comment.
Can these become invalid due to cert expiry ? Do we need to differentiate between expiry from other scenario ? (in case of expiry, we probably don't need to re-generate the CA ? )
There was a problem hiding this comment.
The generated CA is valid for 10 years.
I added some check to ensure it won't expire in 1 year.
| @@ -39,7 +41,7 @@ const ( | |||
| // CertWriter provides method to handle webhooks. | |||
| type CertWriter interface { | |||
| // EnsureCert provisions the cert for the webhookClientConfig. | |||
| EnsureCert(dnsName string, dryrun bool) (*generator.Artifacts, bool, error) | |||
| EnsureCert(dnsName string) (*generator.Artifacts, bool, error) | |||
There was a problem hiding this comment.
Making these pkgs internal is helping us today :)
| certs := secretToCerts(secret) | ||
| if certs != nil { | ||
| // Store the CA for next usage. | ||
| s.CertGenerator.SetCA(certs.CAKey, certs.CACert) |
There was a problem hiding this comment.
I am assuming secret will be updated only if installer is enabled ?
There was a problem hiding this comment.
Not always true.
Disabling the installer only skip installation during bootstrapping.
The webhook server will refresh its cert if the the cert is expiring.
IMO installer only cover bootstrapping time, after that the server may try to rotate its cert if necessary. If it doesn't have permission it will fail.
We probably want to reconsider if it should fail or just log a warning.
|
What's the main reason that we've added install-webhook-config to controller-runtime, but we removed the equivalent install-crds? Is there any reason not to have the manifest generation in controller-tools like for CRDs? |
|
It makes more sense to have the generator in controller-tools. |
Documentation for creating events
Some test changes make the PR looks big, but it's actually not that big :)