[Istio/Security]: Flex volume driver#3370
[Istio/Security]: Flex volume driver#3370wattli merged 12 commits intoistio:masterfrom colabsaumoh:fvolume-driver-review
Conversation
|
@saumoh PR needs rebase |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Assign the PR to them by writing The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:You can indicate your approval by writing |
security/cmd/flexvolume/main.go
Outdated
| mountCmd = &cobra.Command{ | ||
| Use: "mount", | ||
| Short: "Flex volume unmount command.", | ||
| Long: "Flex volume unmount command.", |
|
Just taking a look at this PR to keep up to speed with changes to Istio-CA/security/how istio works. Is there a corresponding design doc for the flexvolume? From what I understand, this will eventually mean that the nodeagent (usually used for bare metal/VMs) will run even for Kubernetes workloads, and a flex volume can then be used to provide certificates etc.? Or perhaps I've completely misunderstood 😄 |
|
@munnerz |
| return failure("mount", inp, sErr) | ||
| } | ||
| } else { | ||
| if err := addCredentialFile(ninputs); err != nil { |
There was a problem hiding this comment.
i think we can combine this else with previous if.
There was a problem hiding this comment.
@wattli Thanks for the review. I've pushed a commit to address it.
| @@ -0,0 +1,515 @@ | |||
| // Copyright 2018 Istio Authors | |||
There was a problem hiding this comment.
I noticed this PR puts flexvolume under cmd/, which is reasonable. I will remove the one under node_agent_k8s once this one is submitted.
| if err != nil { | ||
| return err | ||
| } | ||
| fmt.Println(string(resp)) |
|
|
||
| // checkValidMountOpts checks if there are sufficient inputs to | ||
| // call node agent. | ||
| func checkValidMountOpts(opts string) (*pb.WorkloadInfo, bool) { |
There was a problem hiding this comment.
Instead of returning bool, you can just return the error.
There was a problem hiding this comment.
This function converts the options string to WorkloadInfo proto message. Can you change the function name to something like produceWorkloadInfo?
| err := cmd.Run() | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Actually you can avoid this function. It is equivalent to:
getExecCmd("/bin/umount", dir).Run()
| var emsgs []string | ||
| // Stop the listener. | ||
| // /var/lib/kubelet/pods/20154c76-bf4e-11e7-8a7e-080027631ab3/volumes/nodeagent~uds/test-volume/ | ||
| // /var/lib/kubelet/pods/2dc75e9a-cbec-11e7-b158-0800270da466/volumes/nodeagent~uds/test-volume |
| } | ||
|
|
||
| // logToSys is to write to syslog. | ||
| func logToSys(caller, inp, opts string) { |
There was a problem hiding this comment.
A general question: can we use golang "log" here?
There was a problem hiding this comment.
@myidpt. Thanks for the review.
The reason to use fmt.Print is because the interface between the kubelet and flexvolume drivers is standard output stdout. The kubelet expects the flexvolume driver to put json string as output, without it being prefixed by date/time.
Hope that clarifies.
There was a problem hiding this comment.
Thanks for the clarification. That makes sense!
There was a problem hiding this comment.
@myidpt Thanks for the review comments. I've incorporated them and pushed the changes.
| "path/filepath" | ||
| "strings" | ||
|
|
||
| nagent "istio.io/istio/security/cmd/node_agent_k8s/nodeagentmgmt" |
There was a problem hiding this comment.
seems like this should be moved up in a pkg/
Add the flex volume driver that will be used by node agent in k8s environment to get notifications from kubelet about workloads getting added/deleted.
This version supports: