internal/dag: update TLS fallbackCertificate logic to require tlscertificatedelegation #2528
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2528 +/- ##
==========================================
+ Coverage 76.78% 76.84% +0.06%
==========================================
Files 72 72
Lines 5889 5892 +3
==========================================
+ Hits 4522 4528 +6
+ Misses 1268 1264 -4
- Partials 99 100 +1
|
internal/dag/builder.go
Outdated
| if err != nil { | ||
| sw.SetInvalid("Spec.Virtualhost.TLS fallback certificate Secret %q is invalid: %s", b.FallbackCertificate, err) | ||
| return | ||
| } | ||
|
|
||
| if !b.delegationPermitted(*b.FallbackCertificate, proxy.Namespace) { | ||
| sw.SetInvalid("Spec.VirtualHost.TLS fallback secret %s is not configured for certificate delegation to namespace %s", b.FallbackCertificate.Name, b.FallbackCertificate.Namespace) |
There was a problem hiding this comment.
| sw.SetInvalid("Spec.VirtualHost.TLS fallback secret %s is not configured for certificate delegation to namespace %s", b.FallbackCertificate.Name, b.FallbackCertificate.Namespace) | |
| sw.SetInvalid("Spec.VirtualHost.TLS fallback Secret %q certificate delegation not permitted", b.FallbackCertificate.Name) |
Suggest this status message, which is consistent with the certificate delegation status message for the non-fallback case.
There was a problem hiding this comment.
The problem is that the certificate isn't delegated, not that it's not permitted to be delegated. So the suggestion doesn't read properly as to what's wrong.
There was a problem hiding this comment.
Since the problem here is identical to the delegation check for regular certificates, we should align the status message. If you prefer your more specific message, then I'd suggest updating both occurrences. Just please use %q (which clarifies the distinction between object names and message test), and check that we are consistently capitalizing Kubernetes kind names. I know this is super picky, but I think that it's worth trying to make the error status message as clear and consistent as we can.
There was a problem hiding this comment.
I dislike %q because you get \"log messages\" like \"this\".
There was a problem hiding this comment.
Because it quotes in the JSON output? In general, JSON logs work better when tooling consumes them I think :)
In this case, it's a status field. The %q makes it clearer to the person reading this that its refers to a object name and that it's not part of the message text. If the string is ever empty, %q makes it clear that there is an empty string. So I get that the quoting looks awkward in JSON, but because this is the only place where we tell the user about errors, I'd argue that reducing any ambiguity as much as possible makes the quoting worth it.
There was a problem hiding this comment.
I like the use of %q because of how it shows the empty string case, although I agree the quoting is awkward when it's JSON. This should be mitigated as we move to Conditions anyway.
There was a problem hiding this comment.
cool cool I updated to use the %q
stevesloka
force-pushed
the
fallbackCertTLSDelegation
branch
3 times, most recently
from
d803875 to
fe6ee4a
Compare
…ion to each namespace where a root HTTPProxy enables fallbackCertificate for a fqdn. Signed-off-by: Steve Sloka <slokas@vmware.com>
stevesloka
force-pushed
the
fallbackCertTLSDelegation
branch
from
6bcea3e to
d3c914d
Compare
Updates #1503 by requiring TLSCertificateDelegation to each namespace where a root HTTPProxy enables tls fallback certificate.
Signed-off-by: Steve Sloka slokas@vmware.com