pkg/tlsutil: implement the case ca and key are in the cluster #412
Conversation
fanminshi
changed the title
|
cc/ @hasbro17 |
pkg/tlsutil/tls.go
Outdated
| return nil, nil, nil, err | ||
| } | ||
|
|
||
| secretName := strings.ToLower(k) + "-" + n + "-" + config.CertName |
There was a problem hiding this comment.
Verify that config.CertName exists.
pkg/tlsutil/tls.go
Outdated
| return nil, nil, nil, nil | ||
| } | ||
|
|
||
| func toSecret(kc *keyAndCert, name, namespace string) *v1.Secret { |
There was a problem hiding this comment.
toTLSSecret for a client/server TLS secret.
pkg/tlsutil/tls.go
Outdated
| } | ||
| } | ||
|
|
||
| func toSecretAndConfigmap(cakc *keyAndCert, name, namespace string) (*v1.ConfigMap, *v1.Secret) { |
pkg/tlsutil/tls.go
Outdated
| ns, err := a.Namespace(cr) | ||
| if err != nil { | ||
| return nil, nil, nil, err | ||
| } |
There was a problem hiding this comment.
Wrap the above in a helper to get name and namespace.
fanminshi
force-pushed
the
tls_impl
branch
2 times, most recently
from
422856b to
e5235b1
Compare
|
all fixed. PTAL cc/ @hasbro17 |
pkg/tlsutil/tls.go
Outdated
| // appKeyAndCertMap holds the application key and cert for a given cr + config.CertName. | ||
| appKeyAndCertMap sync.Map | ||
| // caKeyAndCertMap holds the CA key and cert for a given cr. | ||
| caKeyAndCertMap sync.Map |
There was a problem hiding this comment.
Why is a sync.Map needed here? I might not understand the interaction model so I am more asking for my understanding.
There was a problem hiding this comment.
I was thinking about making this function thread safe. On a second thought, sync.map is probably not enough. I'll just use a lock instead and use normal map.
pkg/tlsutil/tls.go
Outdated
| cakcv, hasCAKeyAndCert := scg.caKeyAndCertMap.Load(cascn + "-" + ns) | ||
| // TODO: handle passed in CA | ||
| if hasAppKeyAndCert && hasCAKeyAndCert { | ||
| s := toTLSSecret(appkcv.(*keyAndCert), asn, ns) |
There was a problem hiding this comment.
If the AppKeyAndCert and CAKeyAndCert are found then, the secrets and config map should be in the cluster. If that is true, I would expect that the secrets and config maps that are returned are the actual values from the cluster.
The reason I think this could be important is if for whatever reason a user wants to update those values, they will not be able to do, do to resource version mismatch I believe.
There was a problem hiding this comment.
@shawn-hurley that makes sense. Now if we want to maintain the cache with your suggestion, it can be tricky because once user updates the secret, the one in the cache is going to be outdated. Maintaining cache consistency is hard and error prone. Alternatively, CertGenerator always queries Kubernetes cluster to check for the existence of the Application Key and Cert and CA Key and Cert. However, this approach can leads to potential performance issue. I am fine going to the latter approach for the sake of simplicity. We can tackle the cache optimization later. Let's aim for correctness first.
cc/ @shawn-hurley and @hasbro17
fanminshi
changed the title
|
all fixed PTAL cc/ @hasbro17 @shawn-hurley |
|
We can add this back later, but we need to think about the case, where I have a custom CA, and I have not created the secret or config map in the cluster. This case will be implemented later, but I want to make sure this doesn't get lost. Other than that LGTM |
|
LGTM
@shawn-hurley right, we're tracking all the cases in #419 and the custom CA is part of that. |
Implement the case where the CA and encryption key and cert are in the cluster.
When both are in the cluster, we simply retrieve them and return them to the user.