Moving to scaleset client for the controller#4390
Moving to scaleset client for the controller#4390nikola-jokic wants to merge 2 commits intomasterfrom
Conversation
|
Hello! Thank you for your contribution. Please review our contribution guidelines to understand the project's testing and code conventions. |
nikola-jokic
requested review from
a team,
rentziass and
toast-gear
as code owners
nikola-jokic
added
the
gha-runner-scale-set
There was a problem hiding this comment.
Pull request overview
This PR migrates the actions.github.com controllers from the legacy github/actions client to the newer github.com/actions/scaleset client by introducing a scaleset-backed multi-client and updating secret resolution, controller logic, and tests accordingly.
Changes:
- Add a new
controllers/actions.github.com/multiclientscaleset-backed client cache and update controllers to use it viasecretresolver. - Refactor secret resolution into
controllers/actions.github.com/secretresolverand extract the sharedActionsGitHubObjectinterface intocontrollers/actions.github.com/object. - Update dependency
github.com/actions/scalesettov0.2.0and adjust controller/resource builder code + tests for renamed fields/types.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| main.go | Wire up the new scaleset multiclient + secretresolver and switch comma-slice parsing to strings.SplitSeq. |
| logging/logger.go | Minor formatting cleanup of zap options definition. |
| logger/logger.go | New helper for creating *slog.Logger from level/format. |
| go.mod / go.sum | Bump github.com/actions/scaleset to v0.2.0. |
| controllers/actions.github.com/secretresolver/secret_resolver.go | New secretresolver package; builds proxy + custom RootCAs options and requests a multiclient per config. |
| controllers/actions.github.com/multiclient/multi_client.go | New scaleset multiclient implementation with client caching and client construction. |
| controllers/actions.github.com/multiclient/fake/* | New fake multiclient/client implementations for tests. |
| controllers/actions.github.com/object/object.go | New shared ActionsGitHubObject interface extracted from old resolver. |
| controllers/actions.github.com/resourcebuilder.go | Update types/fields to scaleset equivalents and switch SecretResolver to an interface. |
| controllers/actions.github.com/_controller.go | Switch JIT config + client usage types to scaleset-backed interfaces. |
| controllers/actions.github.com/*_test.go | Update tests to use new secretresolver + multiclient fakes; adjust TLS/proxy tests accordingly. |
| cmd/ghalistener/* | Rename ConfigureUrl → ConfigureURL and update usages/tests; reuse new slog logger helper. |
| apis/actions.github.com/v1alpha1/ephemeralrunner_types.go | Rename RunnerScaleSetId → RunnerScaleSetID. |
| .mockery.yaml / controllers/actions.github.com/mocks_test.go | Update mockery config and add generated mocks for controller package. |
Comments suppressed due to low confidence (4)
controllers/actions.github.com/secretresolver/secret_resolver.go:60
- This returns an error using
%v, which prevents unwrapping witherrors.Is/As. Use%wwhen wrapping the underlying error so callers can detect specific failure causes.
controllers/actions.github.com/secretresolver/secret_resolver.go:28 SecretResolvernow has aloggerfield andWithLogger(...)option, but the logger is never read/used anywhere in this type, so the option currently has no effect. Either usesr.loggerfor resolver/client creation logging (and/or plumb it into the underlying scaleset client options) or remove the field/option to avoid misleading API surface.
controllers/actions.github.com/secretresolver/secret_resolver.go:79- This returns an error using
%v, which prevents unwrapping witherrors.Is/As. Use%wwhen wrapping the underlying error so callers can detect specific failure causes.
controllers/actions.github.com/secretresolver/secret_resolver.go:42 New(...)validatesk8sClientbut notscalesetMultiClient. IfscalesetMultiClientis nil,GetActionsServicewill panic later on first use. Consider validating it inNew(panic or return error) to fail fast with a clearer message.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (c *Config) Logger() (*slog.Logger, error) { | ||
| var lvl slog.Level | ||
| switch strings.ToLower(c.LogLevel) { | ||
| case "debug": | ||
| lvl = slog.LevelDebug | ||
| case "info": | ||
| lvl = slog.LevelInfo | ||
| case "warn": | ||
| lvl = slog.LevelWarn | ||
| case "error": | ||
| lvl = slog.LevelError | ||
| default: | ||
| return nil, fmt.Errorf("invalid log level: %s", c.LogLevel) | ||
| } | ||
|
|
||
| var logger *slog.Logger | ||
| switch c.LogFormat { | ||
| case "json": | ||
| logger = slog.New(slog.NewJSONHandler(os.Stdout, &slog.HandlerOptions{ | ||
| AddSource: true, | ||
| Level: lvl, | ||
| })) | ||
| case "text": | ||
| logger = slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{ | ||
| AddSource: true, | ||
| Level: lvl, | ||
| })) | ||
| default: | ||
| return nil, fmt.Errorf("invalid log format: %s", c.LogFormat) | ||
| } | ||
|
|
||
| return logger.With("app", appName), nil | ||
| return logger.New(c.LogLevel, c.LogFormat) | ||
| } |
There was a problem hiding this comment.
Config.Logger() used to add an app attribute (via logger.With("app", appName)), but now returns the base logger without that context. If log consumers rely on the app field, this is a behavior regression. Consider returning logger.New(...).With("app", appName) (or equivalent) to preserve the prior structured log field.
| entry, ok := m.clients[identifier] | ||
| if ok && entry.rootCAs.Equal(opts.RootCAs) { | ||
| return entry.client, nil |
There was a problem hiding this comment.
entry.rootCAs.Equal(opts.RootCAs) will panic when a cached entry has rootCAs == nil (the common case when no custom CA is configured). Handle nils explicitly (e.g., treat both nil as equal; otherwise compare non-nil pools) before calling Equal.
| identifier := fmt.Sprintf("configURL:%q,namespace:%q", o.GithubConfigURL, o.Namespace) | ||
|
|
||
| if o.AppConfig.Token != "" { | ||
| identifier += fmt.Sprintf("token:%q,", o.AppConfig.Token) | ||
| } else { |
There was a problem hiding this comment.
The identifier string is missing a delimiter after the namespace (fmt.Sprintf("configURL:%q,namespace:%q", ...)). Appending token:... / appID:... immediately after can create ambiguous identifiers and result in cache collisions. Add a trailing separator (e.g., a comma) after the namespace segment (and/or before appending auth fields).
| func (o *ClientForOptions) identifier() (string, error) { | ||
| if err := o.AppConfig.Validate(); err != nil { | ||
| return "", fmt.Errorf("failed to validate app config: %w", err) | ||
| } |
There was a problem hiding this comment.
identifier() assumes o.AppConfig is non-nil and calls o.AppConfig.Validate(). If a caller passes nil (or constructs ClientForOptions without AppConfig), this will panic. Consider validating o.AppConfig != nil and returning a descriptive error instead.
| type ClientForOptions struct { | ||
| GithubConfigURL string | ||
| AppConfig *appconfig.AppConfig | ||
| Namespace string | ||
| RootCAs *x509.CertPool | ||
| ProxyFunc func(*http.Request) (*url.URL, error) | ||
|
|
There was a problem hiding this comment.
ClientForOptions includes ProxyFunc, but proxy configuration is not included in identifier(). If two objects share config URL/namespace/creds but differ in proxy settings, the cached client can be reused with the wrong proxy transport. Include proxy configuration (or a stable hash) in the cache key/identifier.
| options := []scaleset.HTTPOption{ | ||
| scaleset.WithLogger(o.logger), | ||
| } |
There was a problem hiding this comment.
newClient() always passes scaleset.WithLogger(o.logger), but ClientForOptions.logger is unexported and never set by callers, so it will always be nil. If scaleset.WithLogger doesn’t tolerate nil this can panic; even if it does, it silently disables HTTP client logging. Consider defaulting to slog.New(slog.DiscardHandler) when o.logger is nil and/or provide an exported option to set the logger.
No description provided.