check the ingress for tls and compare to secrets #1874
Conversation
|
@microsoft-github-policy-service agree |
nquandt
force-pushed
the
feature/tls-from-ingress
branch
from
b4abbd6 to
3916c9d
Compare
It seems like the reasonable thing to do to me. @dpbevin did you have a different approach in mind here? |
| @@ -136,7 +153,15 @@ public void Update(WatchEventType eventType, V1Secret secret) | |||
|
|
|||
| if (eventType == WatchEventType.Added || eventType == WatchEventType.Modified) | |||
| { | |||
| _certificateSelector.AddCertificate(namespacedName, certificate); | |||
There was a problem hiding this comment.
My thinking was that only the default certificate would be added directly to the selector here because there should only be one of them.
In theory, there could be many secrets used for TLS in a system and we may want to update all referenced certificates in one go. My thinking is that this would be part of the Reconciler since this updates the Yarp routing in one go too. Drip-feeding in certificates one-by-one feels "chatty" to me.
| using System.Security.Cryptography.X509Certificates; | ||
| using Microsoft.AspNetCore.Connections; | ||
|
|
||
| namespace Yarp.Kubernetes.Controller.Certificates; | ||
|
|
||
| internal class ServerCertificateSelector : IServerCertificateSelector | ||
| { | ||
| private X509Certificate2 _defaultCertificate; | ||
| private const string FALLBACK_DOMAIN = "_default"; | ||
| private Dictionary<NamespacedName, X509Certificate2> _defaultCertificate = new Dictionary<NamespacedName, X509Certificate2>(); |
There was a problem hiding this comment.
There would only ever be one default certificate, so I'm not fully understanding why this needs to be a dictionary.
| AddCertificate(certificateName, certificate, null); | ||
| } | ||
|
|
||
| public void AddCertificate(NamespacedName certificateName, X509Certificate2 certificate, string domainName) |
There was a problem hiding this comment.
Following on from my comment above about updating certs all in one go, I'd envisaged there would be an UpdateCertificates method that processes all non-default certificates.
While using the host name from the Ingress resource, the certificates themselves might contain wildcards and/or Subject Alternate Names. I think it's important to validate the certificate before adding it to the dictionary. I found that the Service Fabric version of Yarp appears to do a really good job of this. See https://github.com/microsoft/service-fabric-yarp/blob/main/src/YarpProxy.Service/Service/Security/ServerCertificateBinding/SniServerCertificateSelector.cs
| return _defaultCertificate[_mapping[FALLBACK_DOMAIN]]; | ||
| } | ||
|
|
||
| return _defaultCertificate[mapp]; |
There was a problem hiding this comment.
If _defaultCertificate was empty, wouldn't this throw? Returning null is valid but throwing isn't.
| } | ||
|
|
||
| public void RemoveCertificate(NamespacedName certificateName) | ||
| { | ||
| _defaultCertificate = null; | ||
| var keys = _mapping.Keys.Where(x => _mapping[x] == certificateName); |
There was a problem hiding this comment.
Definitely need good testing around removal because of the one-to-many relationship between certificates and ingresses.
|
At the time of submitting this PR I was not aware of the Reconciler strategy... Closing this, thanks again for reviewing |
#1254 TLS from ingress..
I'm not sure if there was a different plan, but I just used the .GetIngresses() inside of the Update() method for secrets and compared...
Unit Tests would need to be made.. Feel free to scrap this