Encryption status output enriched with IPsec details #2454
Conversation
encrypt/model.go
Outdated
| XfrmErrorNodeCount map[string]int64 `json:"xfrm-error-node-count,omitempty"` | ||
| } | ||
|
|
||
| type encryptionStatus struct { |
There was a problem hiding this comment.
Rather than calculate these additional fields per node, they should be calculated once for the entire cluster, and thus shouldn't appear in this type (this new type is not needed).
encrypt/status.go
Outdated
| @@ -184,6 +249,9 @@ func clusterNodeStatus(res map[string]models.EncryptionStatus) (clusterStatus, e | |||
| } | |||
| if v.Mode == "IPsec" { | |||
| cs.EncIPsecNodeCount++ | |||
| cs.IPsecExpectedKeyCount = v.IPsecExpectedKeyCount | |||
There was a problem hiding this comment.
This silently uses the value of these fields from the last encryptionStatus.
Rather than store these fields on each encryptionStatus, they should be calculated once for the entire cluster (as mentioned above), by using the values from the first models.EncryptionStatus with mode "IPSec".
This eliminates some anti-patterns:
- Copying fields into each element of a collection before they are presented
- Storing overlapping data in a structure that could diverge over time
There was a problem hiding this comment.
Yeah, I agree it looks ugly.
But if I remove the helper model I won't be able to inject IPsec details into per-node output:
https://github.com/cilium/cilium-cli/pull/2454/files#diff-6d172ad17f4677d4b688b3202dda47a51a3a73196c7e0b4fca422f5f53faa83cR314
I can see two options:
- implement this logic in Cilium and extend
EncryptionStatusmodel instead - ignore per-node details output (looks inconsistent and might be confusing for user)
WDYT?
There was a problem hiding this comment.
Viktor: how about making clusterStatus as a return value of enrichWithIPsecDetails(), where we can store expectedKeyCount in clusterStatus. In this way, we don't have to store expectedKeyCount in nodes' encryptionStatus and read from there later.
There was a problem hiding this comment.
@asauber @jschwinger233 I've refactored IPsec status but still have no idea how to get rid of nodeStatus struct.
Could you please review one more time?
Thanks!
viktor-kurchenko
force-pushed
the
pr/vk/ipsec/status/details
branch
from
3736b65 to
8841e61
Compare
@jschwinger233 I've refactored |
encrypt/status.go
Outdated
| @@ -184,6 +249,9 @@ func clusterNodeStatus(res map[string]models.EncryptionStatus) (clusterStatus, e | |||
| } | |||
| if v.Mode == "IPsec" { | |||
| cs.EncIPsecNodeCount++ | |||
| cs.IPsecExpectedKeyCount = v.IPsecExpectedKeyCount | |||
There was a problem hiding this comment.
Viktor: how about making clusterStatus as a return value of enrichWithIPsecDetails(), where we can store expectedKeyCount in clusterStatus. In this way, we don't have to store expectedKeyCount in nodes' encryptionStatus and read from there later.
|
@brb kind ping ) |
Expected IPsec key count, IPsec key type and IPsec key rotation in progress status added to the encrypt output. Signed-off-by: viktor-kurchenko <[email protected]> Please enter the commit message for your changes. Lines starting
viktor-kurchenko
force-pushed
the
pr/vk/ipsec/status/details
branch
from
7c7dfd5 to
0a2dc09
Compare
Expected IPsec key count, IPsec key type and IPsec key rotation in progress status added to the encrypt output.