-
Notifications
You must be signed in to change notification settings - Fork 211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Encryption status sub command. #2212
Conversation
fd862b1
to
3d2d4bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this addition.
I have concerns building this feature by parsing the raw text output of the command cilium-dbg encrypt status
. IMO we have to improve the "API" first: at least by introducing a structured form that we can rely on a little bit more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for the CLI aspect (i only tested that the command works - but didn't verified the output from a IPSec & Wireguard perspective)
nit: would be nice if you could reduce the amount of commits and squash the following commits into the first one.
Alias for the encryption command
Status structure and initTargetCiliumPods method simplified.
I think it's OK to have the last commit (JSON) as dedicated commit.
But i'll leave that up to the maintainer that will merge this PR.
Thanks Viktor!
77d7ada
to
8be60d8
Compare
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, just a small nit to simplify the concurrent status fetch if possible.
Signed-off-by: viktor-kurchenko <[email protected]>
Signed-off-by: viktor-kurchenko <[email protected]>
ea6faa4
to
e851321
Compare
Cluster wide encryption status command:
Output examples: