Abstract the secret key names for Pinniped-generated certificates held in K8s secrets #2025
Conversation
| corev1 "k8s.io/api/core/v1" | ||
| ) | ||
|
|
||
| type RetrieveFromSecretFunc func(secret *corev1.Secret) ([]byte, []byte) |
There was a problem hiding this comment.
Should this be split into separate types? Confirm that the call sites only ever want a CA or a serving cert.
| @@ -56,7 +62,7 @@ func NewCertsManagerController( | |||
| return controllerlib.New( | |||
| controllerlib.Config{ | |||
| Name: "certs-manager-controller", | |||
There was a problem hiding this comment.
Missed renaming controller name string.
There was a problem hiding this comment.
Will be fixed with the next push
| @@ -164,6 +162,7 @@ func PrepareControllers(c *Config) (controllerinit.RunnerBuilder, error) { //nol | |||
| apicerts.NewAPIServiceUpdaterController( | |||
| c.ServerInstallationInfo.Namespace, | |||
| c.NamesConfig.ServingCertificateSecret, | |||
| apicerts.RetrieveCAFromSecret, | |||
There was a problem hiding this comment.
Not sure I understand why the controllers need these new dependencies injected?
There was a problem hiding this comment.
They are exported, global functions, so theoretically they could just be called inline.
The reason they are injected as dependencies is just so the unit tests of these controllers can provide their own implementation of this function, which means that it does not need to be aware of the specific keys used in the secret (it can make up its own keys).
| @@ -42,6 +42,9 @@ func TestAPIServiceUpdaterControllerOptions(t *testing.T) { | |||
| _ = NewAPIServiceUpdaterController( | |||
| installedInNamespace, | |||
| certsSecretResourceName, | |||
| func(secret *corev1.Secret) ([]byte, []byte) { | |||
There was a problem hiding this comment.
Why is this being injected here?
joshuatcasey
force-pushed
the
jtc/externally-configured-serving-certs
branch
from
f42af41 to
bb4ef17
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2025 +/- ##
==========================================
+ Coverage 31.94% 31.96% +0.02%
==========================================
Files 379 380 +1
Lines 62087 62111 +24
==========================================
+ Hits 19835 19856 +21
- Misses 41723 41726 +3
Partials 529 529 ☔ View full report in Codecov by Sentry. |
joshuatcasey
force-pushed
the
jtc/externally-configured-serving-certs
branch
from
bb4ef17 to
7bf0bd2
Compare
|
Marking this as draft until we have a path forward for #2027 |
joshuatcasey
force-pushed
the
jtc/externally-configured-serving-certs
branch
from
7bf0bd2 to
c9b7e49
Compare
joshuatcasey
force-pushed
the
jtc/externally-configured-serving-certs
branch
from
c9b7e49 to
7e433a2
Compare
✅ Deploy Preview for pinniped-dev canceled.
|
…ted certificate secrets
joshuatcasey
force-pushed
the
jtc/externally-configured-serving-certs
branch
from
7e433a2 to
1958bb8
Compare
Abstract the secret key names for Pinniped-generated certificates held in K8s secrets.
This does change production code, but should essentially be a refactor (no change in observable behavior). It's a refactor in preparation for any changes made for #1238 .