feat: include crypto diagnostics in /debug/vars output
#23948
Conversation
Pulls `crypto` diagnostics and includes them in `/debug/vars` output. If no `crypto` diagnostics are available, then OSS crypto information will be shown instead. closes: #23947
|
See https://github.com/influxdata/plutonium/issues/3967 for more context. |
Removing the defaults when enterprise provides `crypto` information prevents us from lying to the user.
chore: improve logging
| } | ||
|
|
||
| if !first { | ||
| fmt.Fprintln(w, ",") |
There was a problem hiding this comment.
I'm still in favor of an h.Logger.Error(err) if Fprintln fails here.
| fmt.Fprintln(w, ",") | ||
| } | ||
| first = false | ||
| fmt.Fprintf(w, "\"crypto\": %s", data) |
There was a problem hiding this comment.
Same, let's log Fprintf errors, as unlikely as they may be.
There was a problem hiding this comment.
I'll go a step further and say the fact there are fmt.Printf calls to check when generating JSON is problematic. I don't understand why we don't build the sections into a map[string]interface{} and then serialize that. There's a lot more fmt.Printf's in there than just the ones I added.
There was a problem hiding this comment.
Absolutely agree. But I didn't want to generate more work for you rewriting everything.
| first = false | ||
| fmt.Fprintf(w, "\"crypto\": %s", data) | ||
| } | ||
|
|
||
| if val := expvar.Get("memstats"); val != nil { | ||
| if !first { | ||
| fmt.Fprintln(w, ",") |
There was a problem hiding this comment.
Same, let's log Fprintln errors.
| exp := map[string]interface{}{ | ||
| "crypto": map[string]interface{}{ | ||
| "FIPS": true, | ||
| "ensureFIPS": nil, |
There was a problem hiding this comment.
Do you want to omit this entirely, rather than having a nil entry?
There was a problem hiding this comment.
The nil is intentional. Here's my reasoning:
Assumptions:
- Seeing
nilvalues means enterprise was not updated to generate the appropriate diagnostic values. - You should not see
nilin release versions, because enterprise and OSS agree on the appropriate attributes. - Incomplete data is better than no data, as long as there's no misleading data.
Option 1: If enterprise doesn't provide an attribute value, GET /debug/vars will return a 500. This is what other sections like build do.
I don't like this option because OSS regression tests will pass, but enterprise currently has no tests for /debug/vars. Hopefully we'd catch this, but maybe not. We might have a situation where a customer can't pull /debug/vars, even though it has lots of other useful info.
Option 2: Leave the field out if enterprise doesn't set it.
Let's say in the future we add PAM support, and add PAMEnabled and PAMModules to OSS, but enterprise hasn't been updated yet. If we see /debug/vars without PAMEnabled and PAMModules, is that because the version is too old to have those fields, or does enterprise have a bug and isn't passing them through. If we see those fields as nil, we know something is wrong. This also applies to automated tests. If we forget to add fields to enterprise and hide them, our automated tests still pass so we don't realize we need to add something to enterprise.
Option 3: Missing fields are given a nil value.
Looks wrong because it is wrong, so a human will flag it when they see it. Once we add automated tests to enterprise, these tests will fail when suddenly there's a new field that enterprise doesn't set. Either way, hopefully we really would catch it during development time instead of RC or even release time.
Pulls
cryptodiagnostics and includes them in/debug/varsoutput. If nocryptodiagnostics are available, then OSS crypto information will be shown instead.closes: #23947
Context
Added to enhance supportability of Enterprise releases.
Affected areas (delete section if not relevant):
Extra
cryptosection is now included in/debug/varsSeverity (delete section if not relevant)
This is blocking the Enterprise FIPS release.
Note for reviewers:
Check the semantic commit type: