Add service commands to system api group #97

jmpfar · 2020-10-18T11:36:38Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
Adding methods that can start and stop services as well as query some general service information.
This is needed for the iSCSI implementation as iSCSI commands depend on the MSiSCSI service running, and it is stopped by default.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
Adding the methods to the still unreleased system/v1alpha1 . If this is an issue this can be moved to a different version.

Does this PR introduce a user-facing change?:

Adding commands that can start/stop/query services to the system group

Adding methods that can start and stop services as well as query some general service information. Adding the methods to the still unreleased v1alpha1 Change-Id: I7fc8b15712fe37095ccf246b847042659d963be4

k8s-ci-robot · 2020-10-18T11:36:46Z

Hi @jmpfar. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

a0f195c Merge pull request #106 from msau42/fix-canary 7100c12 Only set staging registry when running canary job b3c65f9 Merge pull request #99 from msau42/add-release-process e53f3e8 Merge pull request #103 from msau42/fix-canary d129462 Document new method for adding CI jobs are new K8s versions e73c2ce Use staging registry for canary tests 2c09846 Add cleanup instructions to release-notes generation 60e1cd3 Merge pull request #98 from pohly/kubernetes-1-19-fixes 0979c09 prow.sh: fix E2E suite for Kubernetes >= 1.18 3b4a2f1 prow.sh: fix installing Go for Kubernetes 1.19.0 1fbb636 Merge pull request #97 from pohly/go-1.15 82d108a switch to Go 1.15 d8a2530 Merge pull request #95 from msau42/add-release-process 843bddc Add steps on promoting release images 0345a83 Merge pull request #94 from linux-on-ibm-z/bump-timeout 1fdf2d5 cloud build: bump timeout in Prow job 41ec6d1 Merge pull request #93 from animeshk08/patch-1 5a54e67 filter-junit: Fix gofmt error 0676fcb Merge pull request #92 from animeshk08/patch-1 36ea4ff filter-junit: Fix golint error f5a4203 Merge pull request #91 from cyb70289/arm64 43e50d6 prow.sh: enable building arm64 image 0d5bd84 Merge pull request #90 from pohly/k8s-staging-sig-storage 3df86b7 cloud build: k8s-staging-sig-storage c5fd961 Merge pull request #89 from pohly/cloud-build-binfmt db0c2a7 cloud build: initialize support for running commands in Dockerfile be902f4 Merge pull request #88 from pohly/multiarch-windows-fix 340e082 build.make: optional inclusion of Windows in multiarch images 5231f05 build.make: properly declare push-multiarch 4569f27 build.make: fix push-multiarch ambiguity 17dde9e Merge pull request #87 from pohly/cloud-build bd41690 cloud build: initial set of shared files 9084fec Merge pull request #81 from msau42/add-release-process 6f2322e Update patch release notes generation command 0fcc3b1 Merge pull request #78 from ggriffiths/fix_csi_snapshotter_rbac_version_set d8c76fe Support local snapshot RBAC for pull jobs c1bdf5b Merge pull request #80 from msau42/add-release-process ea1f94a update release tools instructions git-subtree-dir: release-tools git-subtree-split: a0f195cc2ddc2a1f07d4d3e46fc08187db358f94

ddebroy

Looks great! Couple of comments on why environment variables are used to configure the parameters to the cmdlets rather than sprintf directly when setting up the command line. If it can be simplified to not use env vars, please address it in a follow-up PR.

ddebroy · 2020-10-19T06:33:59Z

internal/os/system/api.go

+}
+
+func (APIImplementor) StartService(name string) error {
+	script := `Start-Service -Name $env:ServiceName`


Is there a reason to set the servicename parameter through an environment variable here? Why not use sprintf directly here rather than a couple of lines below?

Hey, thanks for the review! :)

I've built this upon the comments that were left in the smb api group:

csi-proxy/internal/os/smb/api.go

Lines 55 to 60 in 6966676

func (APIImplementor) NewSmbGlobalMapping(remotePath, username, password string) error {

// use PowerShell Environment Variables to store user input string to prevent command line injection

// https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_environment_variables?view=powershell-5.1

cmdLine := fmt.Sprintf(`$PWord = ConvertTo-SecureString -String $Env:smbpassword -AsPlainText -Force` +

`;$Credential = New-Object -TypeName System.Management.Automation.PSCredential -ArgumentList $Env:smbuser, $PWord` +

`;New-SmbGlobalMapping -RemotePath $Env:smbremotepath -Credential $Credential`)

I tried to find more data online about Powershell command injection, but I haven't found anyone referring to env variables. I did find this link which might indicate that env variables could prevent injection.
Anyway, if this really solves injection then it's a good idea to add this in all csi-proxy commands

Thanks for pointing out the link! Makes complete sense to use this protection in case the csi-proxy is being used to invoke arbitrary commands in a malicious context on the host.

ddebroy · 2020-10-19T06:35:12Z

internal/os/system/api.go

+}
+
+func (APIImplementor) StopService(name string, force bool) error {
+	script := `Stop-Service -Name $env:ServiceName -Force:$([System.Convert]::ToBoolean($env:Force))`


Same comment as before: Is there a reason to set the servicename and force parameters through environment variables here? Why not use sprintf directly rather than a couple of lines below?

see above, tell me what you think

ddebroy · 2020-10-19T06:37:40Z

/lgtm
/approve

k8s-ci-robot · 2020-10-19T06:37:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ddebroy, jmpfar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ddebroy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ddebroy · 2020-10-19T06:38:50Z

/ok-to-test

ddebroy · 2020-10-19T06:44:02Z

@jmpfar can you also fix update the vendoring and proto newline issue please as noted here?

Change-Id: I4d3c80b8866683c6d7abb8384d589801b38d20cf

ddebroy · 2020-10-20T05:43:51Z

/lgtm

Add service commands to system api group

ea935da

Adding methods that can start and stop services as well as query some general service information. Adding the methods to the still unreleased v1alpha1 Change-Id: I7fc8b15712fe37095ccf246b847042659d963be4

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 18, 2020

k8s-ci-robot requested review from ddebroy and msau42 October 18, 2020 11:36

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 18, 2020

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 18, 2020

pohly mentioned this pull request Oct 19, 2020

master: update release-tools #88

Merged

ddebroy reviewed Oct 19, 2020

View reviewed changes

k8s-ci-robot assigned ddebroy Oct 19, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 19, 2020

Fix vendor/protobuf issue

e68936e

Change-Id: I4d3c80b8866683c6d7abb8384d589801b38d20cf

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 20, 2020

k8s-ci-robot merged commit fcfdb01 into kubernetes-csi:master Oct 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add service commands to system api group #97

Add service commands to system api group #97

jmpfar commented Oct 18, 2020

k8s-ci-robot commented Oct 18, 2020

ddebroy left a comment

ddebroy Oct 19, 2020

jmpfar Oct 19, 2020

ddebroy Oct 20, 2020

ddebroy Oct 19, 2020

jmpfar Oct 19, 2020

ddebroy commented Oct 19, 2020

k8s-ci-robot commented Oct 19, 2020

ddebroy commented Oct 19, 2020

ddebroy commented Oct 19, 2020 •

edited

Loading

ddebroy commented Oct 20, 2020

	func (APIImplementor) NewSmbGlobalMapping(remotePath, username, password string) error {
	// use PowerShell Environment Variables to store user input string to prevent command line injection
	// https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_environment_variables?view=powershell-5.1
	cmdLine := fmt.Sprintf(`$PWord = ConvertTo-SecureString -String $Env:smbpassword -AsPlainText -Force` +
	`;$Credential = New-Object -TypeName System.Management.Automation.PSCredential -ArgumentList $Env:smbuser, $PWord` +
	`;New-SmbGlobalMapping -RemotePath $Env:smbremotepath -Credential $Credential`)

Add service commands to system api group #97

Add service commands to system api group #97

Conversation

jmpfar commented Oct 18, 2020

k8s-ci-robot commented Oct 18, 2020

ddebroy left a comment

Choose a reason for hiding this comment

ddebroy Oct 19, 2020

Choose a reason for hiding this comment

jmpfar Oct 19, 2020

Choose a reason for hiding this comment

ddebroy Oct 20, 2020

Choose a reason for hiding this comment

ddebroy Oct 19, 2020

Choose a reason for hiding this comment

jmpfar Oct 19, 2020

Choose a reason for hiding this comment

ddebroy commented Oct 19, 2020

k8s-ci-robot commented Oct 19, 2020

ddebroy commented Oct 19, 2020

ddebroy commented Oct 19, 2020 • edited Loading

ddebroy commented Oct 20, 2020

ddebroy commented Oct 19, 2020 •

edited

Loading