flags: Clone to avoid modifying os.Args #2195

mqasimsarfraz · 2023-11-03T11:52:18Z

If we try to shrink args (to drop '--sort ...') e.g from top file --sort -writes -m 10 --timeout 10 --interval 10 we end up reusing the underlying array and hence the original os.Args end up getting modified resulting in top file -m 10 --timeout 10 --interval 10 --interval 10. The commit clones the slice to ensure os.Args stay untouched and evaluated correctly later on.

Testing done

before the PR

→ ./kubectl-gadget  snapshot process --sort pid  --all-namespaces
K8S.NODE                                   K8S.NAMESPACE                             K8S.POD                                   K8S.CONTAINER                             COMM             PID                  UID        GID       
minikube-docker                            gadget                                    gadget-76f66                              gadget                                    gadgettracerman  3591435              0          0         
minikube-docker                            kube-system                               storage-provisioner                       storage-provisioner                       storage-provisi  3269857              0          0         
minikube-docker                            kube-system                               coredns-6d4b75cb6d-9dsjr                  coredns                                   coredns          3269733              0          0         
minikube-docker                            kube-system                               kube-proxy-swnds                          kube-proxy                                kube-proxy       3269478              0          0         
minikube-docker                            kube-system                               kube-apiserver-minikube-docker            kube-apiserver                            kube-apiserver   3268122              0          0         
minikube-docker                            kube-system                               kube-controller-manager-minikube-docker   kube-controller-manager                   kube-controller  3268080              0          0         
minikube-docker                            kube-system                               etcd-minikube-docker                      etcd                                      etcd             3268070              0          0         
minikube-docker                            kube-system                               kube-scheduler-minikube-docker            kube-scheduler                            kube-scheduler   3268062              0          0

After the PR

→ ./kubectl-gadget  snapshot process --sort pid  --all-namespaces
K8S.NODE                                   K8S.NAMESPACE                             K8S.POD                                   K8S.CONTAINER                             COMM             PID                  UID        GID       
minikube-docker                            kube-system                               kube-scheduler-minikube-docker            kube-scheduler                            kube-scheduler   3268062              0          0         
minikube-docker                            kube-system                               etcd-minikube-docker                      etcd                                      etcd             3268070              0          0         
minikube-docker                            kube-system                               kube-controller-manager-minikube-docker   kube-controller-manager                   kube-controller  3268080              0          0         
minikube-docker                            kube-system                               kube-apiserver-minikube-docker            kube-apiserver                            kube-apiserver   3268122              0          0         
minikube-docker                            kube-system                               kube-proxy-swnds                          kube-proxy                                kube-proxy       3269478              0          0         
minikube-docker                            kube-system                               coredns-6d4b75cb6d-9dsjr                  coredns                                   coredns          3269733              0          0         
minikube-docker                            kube-system                               storage-provisioner                       storage-provisioner                       storage-provisi  3269857              0          0         
minikube-docker                            gadget                                    gadget-76f66                              gadget                                    gadgettracerman  3591435              0          0

Note: You need sort before --all-namespace to ensure while shrinking the args we modify the order of items in removeSplitSortArgs to reproduce the issue.

blanquicet · 2023-11-03T14:39:31Z

.github/workflows/inspektor-gadget.yml

@@ -2,7 +2,7 @@ name: Inspektor Gadget CI
 env:
  REGISTRY: ghcr.io
  CONTAINER_REPO: ${{ github.repository }}
-  GO_VERSION: 1.20.10


Why is this upgrade necessary for this PR?

To have slices.Clone(), I mean looking at what slices.Clone() does, we can also do (using append):

args := append([]string{}, os.Args[1:]...) // clone to avoid modifying the original os.Args

But should be fine to go to 1.21. What do you say?

IMO, I would avoid upgrading it unless it is strictly necessary or it provides a big advantage. And, I consider this doesn't seem the case.

However, let's see what other maintainers think.

I think 1.21 with its maps and slices packages and the new min() and max() funcs alone are worth the upgrade as we use those things constantly. And this is timed just after a release, so there's plenty of time ahead of us to make sure it's running well. So my opinion is: go for it.

The release notes state that Go 1.21 no longer runs on Win 7 & 8 and older MacOS versions, but I don't know if the binaries it produces are affected as well - we should check for that and maybe add that to the release notes.

(But maybe this should get its own PR prior to this one)

(But maybe this should get its own PR prior to this one)

Yeah, maybe it is a better idea to open a separate PR for golang 1.21 and gather more use-cases.

args := append([]string{}, os.Args[1:]...) // clone to avoid modifying the original os.Args

If the above approach looks good, I can unblock this fix and we can continue with golang 1.21 separately?

I'll suggest not to tie these two things together. Unblock this PR by using that approach and add a comment saying it should be changed once we use go1.21, then in another PR we can discuss the go1.21 implications.

I am fine having golang version bumped but I would rather have a dedicated PR and a more detailed commit message.

Makes sense. I would open a separate PR for bumping golang version. For now I switched to different approach with a comment.

I opened #2206 to update golang version.

blanquicet · 2023-11-03T14:41:20Z

cmd/common/utils/flags.go

+	args := slices.Clone(os.Args) // clone to avoid modifying the original os.Args
+	args = removeSplitSortArgs(args)


Good catch!

This is another solution but I think your proposal is cleaner:

// removeSplitSortArgs removes the --sort flag with its arg, if it isn't in the // merged form of --sort=foo func removeSplitSortArgs(args []string) []string { for i := 0; i < len(args); i++ { if args[i] == "--sort" { // Remove also the next element as it is the arg of --sort ret := make([]string, 0, len(args)-2) ret = append(ret, args[:i]...) return append(ret, args[i+2:]...) } } return args } // removeHelpArg removes the --help flag func removeHelpArg(args []string) []string { for i := 0; i < len(args); i++ { if args[i] == "--help" || args[i] == "-h" { ret := make([]string, 0, len(args)-1) ret = append(ret, args[:i]...) return append(ret, args[i+1:]...) } } return args }

This is another solution but I think your proposal is cleaner

Yeah, Also cloning one level above ensure it is done for all following function.

mauriciovasquezbernal · 2023-11-03T19:22:17Z

Perhaps you can also include the commit from #2192, so we get all the fixes in the same PR?

eiffel-fl · 2023-11-06T09:12:40Z

.github/workflows/inspektor-gadget.yml

@@ -2,7 +2,7 @@ name: Inspektor Gadget CI
 env:
  REGISTRY: ghcr.io
  CONTAINER_REPO: ${{ github.repository }}
-  GO_VERSION: 1.20.10


I am fine having golang version bumped but I would rather have a dedicated PR and a more detailed commit message.

eiffel-fl · 2023-11-06T09:14:00Z

cmd/common/utils/flags.go

@@ -187,7 +188,8 @@ func ParseEarlyFlags(cmd *cobra.Command) error {
 		cmd.FParseErrWhitelist.UnknownFlags = false
 	}()
 	// temporary workaround for https://github.com/inspektor-gadget/inspektor-gadget/pull/2174#issuecomment-1780923952
-	args := removeSplitSortArgs(os.Args[1:])
+	args := slices.Clone(os.Args) // clone to avoid modifying the original os.Args


Rather than golang 1.21 Clone() would a copy() do the trick?

Yep, that should also do the trick. I read this to confuse myself more :D and then decided to go for whatever slices will give us in future.

If we try to shrink args (to drop '--sort ...') we end up reusing the underlying array and hence the original os.Args end up getting modified. The commit clones the slice to ensure os.Args stay untouched and evaluated correctly later on. Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>

Use ParseEarlyFlags() on main to be sure flags from the host package are parsed when host.Init() is called. Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>

flyth

LGTM, thanks!

mqasimsarfraz · 2023-11-06T16:48:58Z

Thanks for reviews!

mqasimsarfraz requested a review from flyth as a code owner November 3, 2023 11:52

mqasimsarfraz force-pushed the qasim/clone-os-args branch from ccf6e64 to 613a19f Compare November 3, 2023 12:07

mqasimsarfraz requested review from alban, mauriciovasquezbernal and eiffel-fl as code owners November 3, 2023 12:07

blanquicet reviewed Nov 3, 2023

View reviewed changes

mauriciovasquezbernal mentioned this pull request Nov 3, 2023

ig: Fix host.Init() workaround flags #2192

Merged

eiffel-fl reviewed Nov 6, 2023

View reviewed changes

mqasimsarfraz and others added 2 commits November 6, 2023 11:40

ig: Fix host.Init() workaround flags

e988658

Use ParseEarlyFlags() on main to be sure flags from the host package are parsed when host.Init() is called. Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>

mqasimsarfraz force-pushed the qasim/clone-os-args branch from 613a19f to e988658 Compare November 6, 2023 10:42

flyth approved these changes Nov 6, 2023

View reviewed changes

mqasimsarfraz merged commit 7d12644 into main Nov 6, 2023
50 checks passed

mqasimsarfraz deleted the qasim/clone-os-args branch November 6, 2023 16:49

mqasimsarfraz mentioned this pull request Nov 6, 2023

treewide: Update to golang 1.21 #2206

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

flags: Clone to avoid modifying os.Args #2195

flags: Clone to avoid modifying os.Args #2195

mqasimsarfraz commented Nov 3, 2023 •

edited

blanquicet Nov 3, 2023

mqasimsarfraz Nov 3, 2023 •

edited

blanquicet Nov 3, 2023

flyth Nov 3, 2023 •

edited

mqasimsarfraz Nov 3, 2023

mauriciovasquezbernal Nov 3, 2023

eiffel-fl Nov 6, 2023

mqasimsarfraz Nov 6, 2023

mqasimsarfraz Nov 6, 2023

blanquicet Nov 3, 2023

mqasimsarfraz Nov 3, 2023

mauriciovasquezbernal commented Nov 3, 2023

eiffel-fl Nov 6, 2023

eiffel-fl Nov 6, 2023

mqasimsarfraz Nov 6, 2023

flyth left a comment

mqasimsarfraz commented Nov 6, 2023

		args := slices.Clone(os.Args) // clone to avoid modifying the original os.Args
		args = removeSplitSortArgs(args)

flags: Clone to avoid modifying os.Args #2195

flags: Clone to avoid modifying os.Args #2195

Conversation

mqasimsarfraz commented Nov 3, 2023 • edited

Testing done

before the PR

After the PR

Choose a reason for hiding this comment

mqasimsarfraz Nov 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flyth Nov 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciovasquezbernal commented Nov 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flyth left a comment

Choose a reason for hiding this comment

mqasimsarfraz commented Nov 6, 2023

mqasimsarfraz commented Nov 3, 2023 •

edited

mqasimsarfraz Nov 3, 2023 •

edited

flyth Nov 3, 2023 •

edited