scalesets runner group check incorrectly encodes query parameters

[Hdom]

Hello @gabriel-samfira I am in the midst of testing the scaleset feature and ran into this issue.

In GetRunnerGroupByName the way that the query parameter is set into the path is breaking once the path is passed into newActionsRequest

I added some debugging and what I am experiencing is the following:

• Path generated by GetRunnerGroupByName: _apis/runtime/runnergroups/?groupName=<runnergroup>
• newActionsRequest acquires url from s.actionsServiceInfo.GetURL(): https://pipelinesghubeus12.actions.githubusercontent.com/<token>/
• newActionsRequest joins URL with Path uri := actionsURI.JoinPath(path). At this point I added a debug and the path looks like so: https://pipelinesghubeus12.actions.githubusercontent.com/<token>/_apis/runtime/runnergroups/%3FgroupName=<runnergroup>

Hopefully that makes sense, it looks like the ampersand in the path is being url encoded.

This results in the query returning all runners groups which breaks the condition at this line and I get error multiple runner groups exist with the same name (<runnergroup>)

[gabriel-samfira]

yeah, looks like JoinPath() escapes query args. Will fix in the morning.

[Hdom]

I did some testing here and and I believe you can use actionsURI.Parse or actionsURI.ResolveReference to combine the actionsURI with the path.

I tested the actionsURI.Parse option and it works fine but I don't know what unforeseen side effects it might have if you were relying on the automatic url encoding.

[gabriel-samfira]

You can try this:

func (s *ScaleSetClient) newActionsRequest(ctx context.Context, method, uriPath string, body io.Reader) (*http.Request, error) {
	if err := s.ensureAdminInfo(ctx); err != nil {
		return nil, fmt.Errorf("failed to update token: %w", err)
	}

	actionsURI, err := s.actionsServiceInfo.GetURL()
	if err != nil {
		return nil, fmt.Errorf("failed to get pipeline URL: %w", err)
	}

	pathURI, err := url.Parse(uriPath)
	if err != nil {
		return nil, fmt.Errorf("failed to parse path: %w", err)
	}
	pathQuery := pathURI.Query()
	baseQuery := actionsURI.Query()
	for k, values := range pathQuery {
		if baseQuery.Get(k) == "" {
			for _, val := range values {
				baseQuery.Add(k, val)
			}
		}
	}
	if baseQuery.Get("api-version") == "" {
		baseQuery.Set("api-version", "6.0-preview")
	}

	actionsURI.Path = path.Join(actionsURI.Path, pathURI.Path)
	actionsURI.RawQuery = baseQuery.Encode()

	req, err := http.NewRequestWithContext(ctx, method, actionsURI.String(), body)
	if err != nil {
		return nil, err
	}

	req.Header.Set("Content-Type", "application/json")
	req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", s.actionsServiceInfo.Token))

	return req, nil
}

Until I get a chance to put a PR together. The scale set APIs are undocumented. I do not know if the pipeline URI will at some point include some query args as well. So let's attempt to combine query args from the path we send in and any potential args from what we get from github.

Will test something in the morning.

[gabriel-samfira]

Would you mind trying out:

• Handle query args #454

I need to write a bunch of unit tests for the new functionality, once I free up some time.