Skip to content
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

Fix (Flux)HelmRelease cluster lookups #2018

Merged
merged 1 commit into from May 7, 2019

Conversation

@2opremio
Copy link
Collaborator

commented May 7, 2019

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
  //   }
@2opremio 2opremio requested a review from hiddeco May 7, 2019
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](https://github.com/golang/gofrontend/blob/e387439bfd24d5e142874b8e68e7039f74c744d7/go/statements.cc#L5593-L5604):

```
  // 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
  //   }
```
@2opremio 2opremio force-pushed the 2opremio:2011-fix-helmrelease-lookup branch from 815b94b to 7183f13 May 7, 2019
@hiddeco
hiddeco approved these changes May 7, 2019
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
Collaborator Author

commented May 7, 2019

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).

@2opremio 2opremio merged commit b01a2bf into fluxcd:master May 7, 2019
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: e2e-testing Your tests passed on CircleCI!
Details
@2opremio 2opremio deleted the 2opremio:2011-fix-helmrelease-lookup branch May 7, 2019
@hiddeco hiddeco added this to the v1.12.2 milestone May 7, 2019
@squaremo

This comment has been minimized.

Copy link
Member

commented May 8, 2019

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

This comment has been minimized.

Copy link
Member

commented May 8, 2019

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

This comment has been minimized.

Copy link
Member

commented May 8, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

commented May 8, 2019

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

@2opremio

This comment has been minimized.

Copy link
Collaborator Author

commented May 8, 2019

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

This comment has been minimized.

Copy link
Member

commented May 8, 2019

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

This comment has been minimized.

Copy link
Member

commented May 8, 2019

@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

This comment has been minimized.

Copy link
Collaborator Author

commented May 8, 2019

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

@hiddeco

This comment has been minimized.

Copy link
Member

commented May 8, 2019

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

@2opremio

This comment has been minimized.

Copy link
Collaborator Author

commented May 8, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.