Fix (Flux)HelmRelease cluster lookups

[2opremio]

Fixes #2011 (Introduced in #1111)

The value obtained by _, value := range slice is overwritten on every iteration. Thus, saving a pointer to value is dangerous since its content will be overwritten in the next iteration.

From https://golang.org/ref/spec#For_range :

For each entry it assigns iteration values to corresponding
iteration variables if present and then executes the block.

But the iteration variables are the same on each iteration.

More explicitly, from the gccgo source code:

  // The loop we generate:
  //   len_temp := len(range)
  //   range_temp := range
  //   for index_temp = 0; index_temp < len_temp; index_temp++ {
  //           value_temp = range_temp[index_temp]
  //           index = index_temp
  //           value = value_temp
  //           original body
  //   }

[hiddeco]

Thanks Fons 👑

Fix looks good to go to me (and looking for this must have been fun).

I am now wondering if we do this in other places though, guess it is a matter of time before we find out.

[2opremio]

Fix looks good to go to me (and looking for this must have been fun).

It didn't take that long, I had a similar problem in #1848 and went directly to the loop.

I am now wondering if we do this in other places though, guess it is a matter of time before we find out.

I checked the other resources and are ok. I wonder if go vet and friends check this kind of stuff (it's similar to the loopclosure check).

[squaremo]

I agree that using the address of a loopvar is often going to lead to problems. Can you explain what was the problem in this specific case? It looks to me like the make<Whatever>Workload procedures don't hold on to the pointer, they just extract what they need from it before returning -- so presumably the intent was to avoid passing a large struct by value (and the safest fix would be to pass them by value).

[squaremo]

Can you explain what was the problem in this specific case?

Ah, the intermediate workload struct has an embedded field, and that holds on to the pointer. Ugh

[hiddeco]

Looks like this did not fix the issue described in #2011. Output from fluxctl build fresh from current master:

$ fluxctl list-workloads
WORKLOAD                               CONTAINER           IMAGE                               RELEASE  POLICY
<snip>
default:helmrelease/faulty2                                                                             
default:helmrelease/faulty2
$ kubectl get helmrelease
NAME      AGE
faulty    5d21h
faulty2   5d19h

[2opremio]

Interesting, I did test the fix and fluxctl list-worloads showed the correct IDs. @hiddeco can you double-check?

[2opremio]

Ah, the intermediate workload struct has an embedded field, and that holds on to the pointer. Ugh

Exactly, I should probably had explained that instead of elaborating on why you should save a pointer to a loop variable.

[squaremo]

I had a look at some git blame and this is the only change I thought might be relevant: 2opremio@abe9db8#diff-18f3f72b1b5287be3ecab6e5884622b3R191 (but even so, I think it's actually OK).

[hiddeco]

@2opremio see below

❯ which fluxctl
/home/hidde/go/bin/fluxctl

~/go/src/github.com/weaveworks/flux master
❯ rm -rf /home/hidde/go/bin/fluxctl

~/go/src/github.com/weaveworks/flux master
❯ make clean realclean
go clean
rm -rf ./build
rm -f test/bin/kubectl test/bin/helm
rm -rf ./cache

~/go/src/github.com/weaveworks/flux master
❯ which fluxctl
fluxctl not found

~/go/src/github.com/weaveworks/flux master
❯ git pull
Already up to date.

~/go/src/github.com/weaveworks/flux master
❯ make
go install ./cmd/fluxctl
<snip>

~/go/src/github.com/weaveworks/flux master
❯ fluxctl list-workloads
WORKLOAD                               CONTAINER           IMAGE                               RELEASE  POLICY
default:helmrelease/faulty2                                                                             
default:helmrelease/faulty2   

~/go/src/github.com/weaveworks/flux master
❯ kubectl get hr
NAME      AGE
faulty    5d21h
faulty2   5d19h

[2opremio]

@hiddeco can you triple-check that you are running the image from master?

[hiddeco]

@2opremio err, crap, you are right, my image was fixed on something else. Nevermind.

[2opremio]

No worries, it happens. Thanks for checking that the fix works!