Pointer issue in ListProbesResponse() causes incorrect values in ListProbesResponse.

[evanSpendlove]

Issue Summary

There is a pointer issue in the ListProbes() RPC. The ListProbesResponse object contains a list of structs, each containing a string and ProbeInfo object. If there are multiple probes added, the strings will all be the same as the last item added to this list.

Example:
3 probes (probe0, probe1, probe2) are added.
ListProbes() is called.

Desired Response: ["probe0" , "probe1" , "probe2" ]
Received Response: ["probe2" , "probe2" , "probe2" ]

There is also a minor bug in the serviceimpl_test.go file.

cloudprober/prober/serviceimpl_test.go

Lines 185 to 187 in 4cbdccb

	if reflect.DeepEqual(respProbeNames, testProbes) {
	t.Errorf("Probes in ListProbes() response: %v, expected: %s", respProbeNames, testProbes)
	}

There is a missing negation (!) in the if statement above, it should be:

if !reflect.DeepEqual(respProbeNames, testProbes) {
		t.Errorf("Probes in ListProbes() response: %v, expected: %s", respProbeNames, testProbes)
	}

Without the negation, this will only throw an error when these are the same, rather than different.

I have made a PR, with the proposed solution detailed below, that fixes this issue.

Explanation

The issue occurs in lines 72-77 in serviceimpl.go in the prober package.

cloudprober/prober/serviceimpl.go

Lines 72 to 77 in 4cbdccb

	for name, p := range pr.Probes {
	resp.Probe = append(resp.Probe, &pb.Probe{
	Name: &name,
	Config: p.ProbeDef,
	})
	}

The name and p variables in the range loop are local variables, so there can be issues when passing their addresses to objects/variables. This works for the p variable as it is itself a pointer in the pr.Probes map. However, the name variable is a string variable, not a pointer to a string.

If the address of name is passed, then the value associated with this address will be the same for all instances; the last value that p holds.

Proposed Solution

Since the issue is with using the address of a local variable when iterating over a map, a possible solution is to use the addresses of the actual values in the map.

This would look like:

names := make([]string, 0)                                                                                                                                   
         for n, _ := range pr.Probes {                                                                                                                               
                 names = append(names, n)                                                                                                                              
         }                                                                                                                                                           
                                                                                                                                                                     
         for i := 0; i < len(keys); i++ {                                                                                                                            
                 resp.Probe = append(resp.Probe, &pb.Probe{                                                                                                          
                         Name:   &names[i],                                                                                                                           
                         Config: pr.Probes[names[i]].ProbeDef,                                                                                                        
                 })                                                                                                          
         } 

As this passes the addresses of the actual values in the map, the pointer issue is eliminated.

I have also added the negation to the serviceimp_test.go file as discussed above, so it now reads:

if !reflect.DeepEqual(respProbeNames, testProbes) {
		t.Errorf("Probes in ListProbes() response: %v, expected: %s", respProbeNames, testProbes)
	}

I have made a PR, #459, that makes this change.

[manugarg]

Fixed by #461. Thanks again for the bug report and the fix @evanSpendlove.