From 25b32797ea9e5f07c4c4c245a0770dc90b6cab5b Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Wed, 15 Jan 2025 11:58:58 +0100 Subject: [PATCH] Adding config read to the ghalistener --- .../v1alpha1/appconfig/appconfig.go | 86 ++++++ .../v1alpha1/appconfig/appconfig_test.go | 152 +++++++++++ .../v1alpha1/autoscalinglistener_types.go | 17 +- .../v1alpha1/autoscalingrunnerset_types.go | 16 ++ .../v1alpha1/ephemeralrunner_types.go | 16 ++ .../v1alpha1/ephemeralrunnerset_types.go | 17 +- .../templates/autoscalingrunnerset.yaml | 1 + .../tests/template_test.go | 40 +++ charts/gha-runner-scale-set/values.yaml | 9 +- cmd/ghalistener/app/app.go | 8 +- cmd/ghalistener/config/config.go | 59 ++++- cmd/ghalistener/config/config_client_test.go | 17 +- cmd/ghalistener/config/config_test.go | 31 ++- cmd/ghalistener/main.go | 11 +- .../actions.github.com/actions_client_pool.go | 242 +++++++++++++++++ .../actions_client_resolver.go | 248 ------------------ .../autoscalinglistener_controller.go | 25 +- .../autoscalinglistener_controller_test.go | 4 +- .../autoscalingrunnerset_controller.go | 16 +- .../autoscalingrunnerset_controller_test.go | 54 ++-- controllers/actions.github.com/constants.go | 2 + .../ephemeralrunner_controller.go | 78 +----- .../ephemeralrunner_controller_test.go | 84 ++++-- .../ephemeralrunnerset_controller.go | 4 +- .../ephemeralrunnerset_controller_test.go | 18 +- .../actions.github.com/resourcebuilder.go | 42 +-- .../resourcebuilder_test.go | 7 +- github/actions/fake/multi_client.go | 7 +- github/actions/multi_client.go | 62 +---- github/actions/multi_client_test.go | 37 +-- go.mod | 8 +- go.sum | 24 +- main.go | 53 ++-- proxyconfig/proxyconfig.go | 129 +++++++++ proxyconfig/proxyconfig_test.go | 104 ++++++++ test_e2e_arc/arc_jobs_test.go | 1 - vault/azurekeyvault/azurekeyvault.go | 64 +++-- vault/azurekeyvault/config.go | 119 +++++++++ vault/azurekeyvault/config_test.go | 159 +++++++++++ vault/vault.go | 46 +++- vault/vault_test.go | 34 +++ 41 files changed, 1503 insertions(+), 648 deletions(-) create mode 100644 apis/actions.github.com/v1alpha1/appconfig/appconfig.go create mode 100644 apis/actions.github.com/v1alpha1/appconfig/appconfig_test.go create mode 100644 controllers/actions.github.com/actions_client_pool.go delete mode 100644 controllers/actions.github.com/actions_client_resolver.go create mode 100644 proxyconfig/proxyconfig.go create mode 100644 proxyconfig/proxyconfig_test.go create mode 100644 vault/azurekeyvault/config.go create mode 100644 vault/azurekeyvault/config_test.go create mode 100644 vault/vault_test.go diff --git a/apis/actions.github.com/v1alpha1/appconfig/appconfig.go b/apis/actions.github.com/v1alpha1/appconfig/appconfig.go new file mode 100644 index 00000000..c6cceeda --- /dev/null +++ b/apis/actions.github.com/v1alpha1/appconfig/appconfig.go @@ -0,0 +1,86 @@ +package appconfig + +import ( + "bytes" + "encoding/json" + "fmt" + "strconv" + + corev1 "k8s.io/api/core/v1" +) + +type AppConfig struct { + AppID string `json:"github_app_id"` + AppInstallationID int64 `json:"github_app_installation_id"` + AppPrivateKey string `json:"github_app_private_key"` + + Token string `json:"github_token"` +} + +func (c *AppConfig) tidy() *AppConfig { + if len(c.Token) > 0 { + return &AppConfig{ + Token: c.Token, + } + } + + return &AppConfig{ + AppID: c.AppID, + AppInstallationID: c.AppInstallationID, + AppPrivateKey: c.AppPrivateKey, + } +} + +func (c *AppConfig) Validate() error { + hasToken := len(c.Token) > 0 + hasGitHubAppAuth := c.hasGitHubAppAuth() + if hasToken && hasGitHubAppAuth { + return fmt.Errorf("both PAT and GitHub App credentials provided. should only provide one") + } + if !hasToken && !hasGitHubAppAuth { + return fmt.Errorf("no credentials provided: either a PAT or GitHub App credentials should be provided") + } + + return nil +} + +func (c *AppConfig) hasGitHubAppAuth() bool { + return len(c.AppID) > 0 && c.AppInstallationID > 0 && len(c.AppPrivateKey) > 0 +} + +func FromSecret(secret *corev1.Secret) (*AppConfig, error) { + var appInstallationID int64 + if v := string(secret.Data["github_app_installation_id"]); v != "" { + val, err := strconv.ParseInt(v, 10, 64) + if err != nil { + return nil, err + } + appInstallationID = val + } + + cfg := &AppConfig{ + Token: string(secret.Data["github_token"]), + AppID: string(secret.Data["github_app_id"]), + AppInstallationID: appInstallationID, + AppPrivateKey: string(secret.Data["github_app_private_key"]), + } + + if err := cfg.Validate(); err != nil { + return nil, fmt.Errorf("failed to validate config: %v", err) + } + + return cfg.tidy(), nil +} + +func FromString(v string) (*AppConfig, error) { + var appConfig AppConfig + if err := json.NewDecoder(bytes.NewBufferString(v)).Decode(&appConfig); err != nil { + return nil, err + } + + if err := appConfig.Validate(); err != nil { + return nil, fmt.Errorf("failed to validate app config decoded from string: %w", err) + } + + return appConfig.tidy(), nil +} diff --git a/apis/actions.github.com/v1alpha1/appconfig/appconfig_test.go b/apis/actions.github.com/v1alpha1/appconfig/appconfig_test.go new file mode 100644 index 00000000..b410b16c --- /dev/null +++ b/apis/actions.github.com/v1alpha1/appconfig/appconfig_test.go @@ -0,0 +1,152 @@ +package appconfig + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" +) + +func TestAppConfigValidate_invalid(t *testing.T) { + tt := map[string]*AppConfig{ + "empty": {}, + "token and app config": { + AppID: "1", + AppInstallationID: 2, + AppPrivateKey: "private key", + Token: "token", + }, + "app id not set": { + AppInstallationID: 2, + AppPrivateKey: "private key", + }, + "app installation id not set": { + AppID: "2", + AppPrivateKey: "private key", + }, + "private key empty": { + AppID: "2", + AppInstallationID: 1, + AppPrivateKey: "", + }, + } + + for name, cfg := range tt { + t.Run(name, func(t *testing.T) { + err := cfg.Validate() + require.Error(t, err) + }) + } +} + +func TestAppConfigValidate_valid(t *testing.T) { + tt := map[string]*AppConfig{ + "token": { + Token: "token", + }, + "app ID": { + AppID: "1", + AppInstallationID: 2, + AppPrivateKey: "private key", + }, + } + + for name, cfg := range tt { + t.Run(name, func(t *testing.T) { + err := cfg.Validate() + require.NoError(t, err) + }) + } +} + +func TestAppConfigFromSecret_invalid(t *testing.T) { + tt := map[string]map[string]string{ + "empty": {}, + "token and app provided": { + "github_token": "token", + "github_app_id": "2", + "githu_app_installation_id": "3", + "github_app_private_key": "private key", + }, + "invalid app id": { + "github_app_id": "abc", + "githu_app_installation_id": "3", + "github_app_private_key": "private key", + }, + "invalid app installation_id": { + "github_app_id": "1", + "githu_app_installation_id": "abc", + "github_app_private_key": "private key", + }, + "empty private key": { + "github_app_id": "1", + "githu_app_installation_id": "2", + "github_app_private_key": "", + }, + } + + for name, data := range tt { + t.Run(name, func(t *testing.T) { + secret := &corev1.Secret{ + StringData: data, + } + + appConfig, err := FromSecret(secret) + assert.Error(t, err) + assert.Nil(t, appConfig) + }) + } +} + +func TestAppConfigFromSecret_valid(t *testing.T) { + tt := map[string]map[string]string{ + "with token": { + "github_token": "token", + }, + "app config": { + "github_app_id": "2", + "githu_app_installation_id": "3", + "github_app_private_key": "private key", + }, + } + + for name, data := range tt { + t.Run(name, func(t *testing.T) { + secret := &corev1.Secret{ + StringData: data, + } + + appConfig, err := FromSecret(secret) + assert.Error(t, err) + assert.Nil(t, appConfig) + }) + } +} + +func TestAppConfigFromString_valid(t *testing.T) { + tt := map[string]*AppConfig{ + "token": { + Token: "token", + }, + "app ID": { + AppID: "1", + AppInstallationID: 2, + AppPrivateKey: "private key", + }, + } + + for name, cfg := range tt { + t.Run(name, func(t *testing.T) { + bytes, err := json.Marshal(cfg) + require.NoError(t, err) + + got, err := FromString(string(bytes)) + require.NoError(t, err) + + want := cfg.tidy() + assert.Equal(t, want, got) + }) + } +} diff --git a/apis/actions.github.com/v1alpha1/autoscalinglistener_types.go b/apis/actions.github.com/v1alpha1/autoscalinglistener_types.go index 57363dba..c58e3854 100644 --- a/apis/actions.github.com/v1alpha1/autoscalinglistener_types.go +++ b/apis/actions.github.com/v1alpha1/autoscalinglistener_types.go @@ -86,8 +86,23 @@ type AutoscalingListener struct { Status AutoscalingListenerStatus `json:"status,omitempty"` } -// +kubebuilder:object:root=true +func (l *AutoscalingListener) GitHubConfigSecret() string { + return l.Spec.GitHubConfigSecret +} +func (l *AutoscalingListener) GitHubConfigUrl() string { + return l.Spec.GitHubConfigUrl +} + +func (l *AutoscalingListener) Proxy() *ProxyConfig { + return l.Spec.Proxy +} + +func (l *AutoscalingListener) GitHubServerTLS() *GitHubServerTLSConfig { + return l.Spec.GitHubServerTLS +} + +// +kubebuilder:object:root=true // AutoscalingListenerList contains a list of AutoscalingListener type AutoscalingListenerList struct { metav1.TypeMeta `json:",inline"` diff --git a/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go b/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go index 2d7946f9..1d00abe0 100644 --- a/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go +++ b/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go @@ -285,6 +285,22 @@ func (ars *AutoscalingRunnerSet) ListenerSpecHash() string { return hash.ComputeTemplateHash(&spec) } +func (ars *AutoscalingRunnerSet) GitHubConfigSecret() string { + return ars.Spec.GitHubConfigSecret +} + +func (ars *AutoscalingRunnerSet) GitHubConfigUrl() string { + return ars.Spec.GitHubConfigUrl +} + +func (ars *AutoscalingRunnerSet) Proxy() *ProxyConfig { + return ars.Spec.Proxy +} + +func (ars *AutoscalingRunnerSet) GitHubServerTLS() *GitHubServerTLSConfig { + return ars.Spec.GitHubServerTLS +} + func (ars *AutoscalingRunnerSet) RunnerSetSpecHash() string { type runnerSetSpec struct { GitHubConfigUrl string diff --git a/apis/actions.github.com/v1alpha1/ephemeralrunner_types.go b/apis/actions.github.com/v1alpha1/ephemeralrunner_types.go index 0c13bf7c..38067b34 100644 --- a/apis/actions.github.com/v1alpha1/ephemeralrunner_types.go +++ b/apis/actions.github.com/v1alpha1/ephemeralrunner_types.go @@ -67,6 +67,22 @@ func (er *EphemeralRunner) HasContainerHookConfigured() bool { return false } +func (er *EphemeralRunner) GitHubConfigSecret() string { + return er.Spec.GitHubConfigSecret +} + +func (er *EphemeralRunner) GitHubConfigUrl() string { + return er.Spec.GitHubConfigUrl +} + +func (er *EphemeralRunner) Proxy() *ProxyConfig { + return er.Spec.Proxy +} + +func (er *EphemeralRunner) GitHubServerTLS() *GitHubServerTLSConfig { + return er.Spec.GitHubServerTLS +} + // EphemeralRunnerSpec defines the desired state of EphemeralRunner type EphemeralRunnerSpec struct { // +required diff --git a/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go b/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go index f74edfb0..1b7e218e 100644 --- a/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go +++ b/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go @@ -60,9 +60,24 @@ type EphemeralRunnerSet struct { Status EphemeralRunnerSetStatus `json:"status,omitempty"` } -// +kubebuilder:object:root=true +func (ers *EphemeralRunnerSet) GitHubConfigSecret() string { + return ers.Spec.EphemeralRunnerSpec.GitHubConfigSecret +} + +func (ers *EphemeralRunnerSet) GitHubConfigUrl() string { + return ers.Spec.EphemeralRunnerSpec.GitHubConfigUrl +} + +func (ers *EphemeralRunnerSet) Proxy() *ProxyConfig { + return ers.Spec.EphemeralRunnerSpec.Proxy +} + +func (ers *EphemeralRunnerSet) GitHubServerTLS() *GitHubServerTLSConfig { + return ers.Spec.EphemeralRunnerSpec.GitHubServerTLS +} // EphemeralRunnerSetList contains a list of EphemeralRunnerSet +// +kubebuilder:object:root=true type EphemeralRunnerSetList struct { metav1.TypeMeta `json:",inline"` metav1.ListMeta `json:"metadata,omitempty"` diff --git a/charts/gha-runner-scale-set/templates/autoscalingrunnerset.yaml b/charts/gha-runner-scale-set/templates/autoscalingrunnerset.yaml index 3008b28c..4bae4b3b 100644 --- a/charts/gha-runner-scale-set/templates/autoscalingrunnerset.yaml +++ b/charts/gha-runner-scale-set/templates/autoscalingrunnerset.yaml @@ -45,6 +45,7 @@ metadata: {{- if and (ne $containerMode.type "kubernetes") (not .Values.template.spec.serviceAccountName) }} actions.github.com/cleanup-no-permission-service-account-name: {{ include "gha-runner-scale-set.noPermissionServiceAccountName" . }} {{- end }} + spec: githubConfigUrl: {{ required ".Values.githubConfigUrl is required" (trimSuffix "/" .Values.githubConfigUrl) }} githubConfigSecret: {{ include "gha-runner-scale-set.githubsecret" . }} diff --git a/charts/gha-runner-scale-set/tests/template_test.go b/charts/gha-runner-scale-set/tests/template_test.go index 72220866..050bded2 100644 --- a/charts/gha-runner-scale-set/tests/template_test.go +++ b/charts/gha-runner-scale-set/tests/template_test.go @@ -2468,3 +2468,43 @@ func TestNamespaceOverride(t *testing.T) { }) } } + +func TestAutoscalingRunnerSetCustomAnnotationsAndLabelsApplied(t *testing.T) { + t.Parallel() + + // Path to the helm chart we will test + helmChartPath, err := filepath.Abs("../../gha-runner-scale-set") + require.NoError(t, err) + + releaseName := "test-runners" + namespaceName := "test-" + strings.ToLower(random.UniqueId()) + + options := &helm.Options{ + Logger: logger.Discard, + SetValues: map[string]string{ + "githubConfigUrl": "https://github.com/actions", + "githubConfigSecret.github_token": "gh_token12345", + "controllerServiceAccount.name": "arc", + "controllerServiceAccount.namespace": "arc-system", + "annotations.actions\\.github\\.com/vault": "azure_key_vault", + "annotations.actions\\.github\\.com/cleanup-manager-role-name": "not-propagated", + "labels.custom": "custom", + "labels.app\\.kubernetes\\.io/component": "not-propagated", + }, + KubectlOptions: k8s.NewKubectlOptions("", "", namespaceName), + } + + output := helm.RenderTemplate(t, options, helmChartPath, releaseName, []string{"templates/autoscalingrunnerset.yaml"}) + + var autoscalingRunnerSet v1alpha1.AutoscalingRunnerSet + helm.UnmarshalK8SYaml(t, output, &autoscalingRunnerSet) + + vault := autoscalingRunnerSet.Annotations["actions.github.com/vault"] + assert.Equal(t, "azure_key_vault", vault) + + custom := autoscalingRunnerSet.Labels["custom"] + assert.Equal(t, "custom", custom) + + assert.NotEqual(t, "not-propagated", autoscalingRunnerSet.Annotations["actions.github.com/cleanup-manager-role-name"]) + assert.NotEqual(t, "not-propagated", autoscalingRunnerSet.Labels["app.kubernetes.io/component"]) +} diff --git a/charts/gha-runner-scale-set/values.yaml b/charts/gha-runner-scale-set/values.yaml index 35922f84..f3ad6f92 100644 --- a/charts/gha-runner-scale-set/values.yaml +++ b/charts/gha-runner-scale-set/values.yaml @@ -6,7 +6,7 @@ githubConfigUrl: "" ## You can choose to supply: ## A) a PAT token, ## B) a GitHub App, or -## C) a pre-defined Kubernetes secret. +## C) a pre-defined secret. ## The syntax for each of these variations is documented below. ## (Variation A) When using a PAT token, the syntax is as follows: githubConfigSecret: @@ -28,8 +28,11 @@ githubConfigSecret: # . # private key line N # -## (Variation C) When using a pre-defined Kubernetes secret in the same namespace that the gha-runner-scale-set is going to deploy, -## the syntax is as follows: +## (Variation C) When using a pre-defined secret. +## The secret can be pulled either directly from Kubernetes, or from the vault, depending on configuration. +## Kubernetes secret in the same namespace that the gha-runner-scale-set is going to deploy. +## On the other hand, if the vault is configured, secret name will be used to fetch the app configuration. +## The syntax is as follows: # githubConfigSecret: pre-defined-secret ## Notes on using pre-defined Kubernetes secrets: ## You need to make sure your predefined secret has all the required secret data set properly. diff --git a/cmd/ghalistener/app/app.go b/cmd/ghalistener/app/app.go index 529b5ba3..2b6fcfd5 100644 --- a/cmd/ghalistener/app/app.go +++ b/cmd/ghalistener/app/app.go @@ -17,7 +17,7 @@ import ( // App is responsible for initializing required components and running the app. type App struct { // configured fields - config config.Config + config *config.Config logger logr.Logger // initialized fields @@ -38,8 +38,12 @@ type Worker interface { } func New(config config.Config) (*App, error) { + if err := config.Validate(); err != nil { + return nil, fmt.Errorf("failed to validate config: %w", err) + } + app := &App{ - config: config, + config: &config, } ghConfig, err := actions.ParseGitHubConfigFromURL(config.ConfigureUrl) diff --git a/cmd/ghalistener/config/config.go b/cmd/ghalistener/config/config.go index 905249aa..846f39f0 100644 --- a/cmd/ghalistener/config/config.go +++ b/cmd/ghalistener/config/config.go @@ -1,6 +1,7 @@ package config import ( + "context" "crypto/x509" "encoding/json" "fmt" @@ -9,20 +10,20 @@ import ( "os" "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1/appconfig" "github.com/actions/actions-runner-controller/build" "github.com/actions/actions-runner-controller/github/actions" "github.com/actions/actions-runner-controller/logging" + "github.com/actions/actions-runner-controller/vault" "github.com/go-logr/logr" "golang.org/x/net/http/httpproxy" ) type Config struct { - ConfigureUrl string `json:"configure_url"` - // AppID can be an ID of the app or the client ID - AppID string `json:"app_id"` - AppInstallationID int64 `json:"app_installation_id"` - AppPrivateKey string `json:"app_private_key"` - Token string `json:"token"` + ConfigureUrl string `json:"configure_url"` + VaultType string `json:"vault_type"` + VaultLookupKey string `json:"vault_lookup_key"` + appconfig.AppConfig EphemeralRunnerSetNamespace string `json:"ephemeral_runner_set_namespace"` EphemeralRunnerSetName string `json:"ephemeral_runner_set_name"` MaxRunners int `json:"max_runners"` @@ -37,23 +38,57 @@ type Config struct { Metrics *v1alpha1.MetricsConfig `json:"metrics"` } -func Read(path string) (Config, error) { - f, err := os.Open(path) +func Read(ctx context.Context, configPath string) (*Config, error) { + f, err := os.Open(configPath) if err != nil { - return Config{}, err + return nil, err } defer f.Close() var config Config if err := json.NewDecoder(f).Decode(&config); err != nil { - return Config{}, fmt.Errorf("failed to decode config: %w", err) + return nil, fmt.Errorf("failed to decode config: %w", err) } + if config.VaultType == "" { + if err := config.Validate(); err != nil { + return nil, fmt.Errorf("failed to validate configuration: %v", err) + } + + return &config, nil + } + + if config.VaultLookupKey == "" { + panic(fmt.Errorf("vault type set to %q, but lookup key is empty", config.VaultType)) + } + + vaults, err := vault.InitAll("LISTENER_") + if err != nil { + return nil, fmt.Errorf("failed to initialize vaults: %v", err) + } + + vault, ok := vaults[config.VaultType] + if !ok { + return nil, fmt.Errorf("vault %q is not initialized", config.VaultType) + } + + appConfigRaw, err := vault.GetSecret(ctx, config.VaultLookupKey) + if err != nil { + return nil, err + } + + appConfig, err := appconfig.FromString(appConfigRaw) + if err != nil { + return nil, fmt.Errorf("failed to read app config from string: %v", err) + } + + config.AppConfig = *appConfig + if err := config.Validate(); err != nil { - return Config{}, fmt.Errorf("failed to validate config: %w", err) + return nil, fmt.Errorf("config validation failed: %w", err) } - return config, nil + return &config, ctx.Err() } // Validate checks the configuration for errors. diff --git a/cmd/ghalistener/config/config_client_test.go b/cmd/ghalistener/config/config_client_test.go index f2dfa3d9..8e8f4eca 100644 --- a/cmd/ghalistener/config/config_client_test.go +++ b/cmd/ghalistener/config/config_client_test.go @@ -9,6 +9,7 @@ import ( "path/filepath" "testing" + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1/appconfig" "github.com/actions/actions-runner-controller/cmd/ghalistener/config" "github.com/actions/actions-runner-controller/github/actions" "github.com/actions/actions-runner-controller/github/actions/testserver" @@ -53,7 +54,9 @@ func TestCustomerServerRootCA(t *testing.T) { config := config.Config{ ConfigureUrl: server.ConfigURLForOrg("myorg"), ServerRootCA: certsString, - Token: "token", + AppConfig: appconfig.AppConfig{ + Token: "token", + }, } client, err := config.ActionsClient(logr.Discard()) @@ -80,7 +83,9 @@ func TestProxySettings(t *testing.T) { config := config.Config{ ConfigureUrl: "https://github.com/org/repo", - Token: "token", + AppConfig: appconfig.AppConfig{ + Token: "token", + }, } client, err := config.ActionsClient(logr.Discard()) @@ -110,7 +115,9 @@ func TestProxySettings(t *testing.T) { config := config.Config{ ConfigureUrl: "https://github.com/org/repo", - Token: "token", + AppConfig: appconfig.AppConfig{ + Token: "token", + }, } client, err := config.ActionsClient(logr.Discard(), actions.WithRetryMax(0)) @@ -145,7 +152,9 @@ func TestProxySettings(t *testing.T) { config := config.Config{ ConfigureUrl: "https://github.com/org/repo", - Token: "token", + AppConfig: appconfig.AppConfig{ + Token: "token", + }, } client, err := config.ActionsClient(logr.Discard()) diff --git a/cmd/ghalistener/config/config_test.go b/cmd/ghalistener/config/config_test.go index f62b4b73..ecb711f9 100644 --- a/cmd/ghalistener/config/config_test.go +++ b/cmd/ghalistener/config/config_test.go @@ -4,6 +4,7 @@ import ( "fmt" "testing" + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1/appconfig" "github.com/stretchr/testify/assert" ) @@ -15,7 +16,9 @@ func TestConfigValidationMinMax(t *testing.T) { RunnerScaleSetId: 1, MinRunners: 5, MaxRunners: 2, - Token: "token", + AppConfig: appconfig.AppConfig{ + Token: "token", + }, } err := config.Validate() assert.ErrorContains(t, err, `MinRunners "5" cannot be greater than MaxRunners "2"`, "Expected error about MinRunners > MaxRunners") @@ -39,8 +42,10 @@ func TestConfigValidationAppKey(t *testing.T) { t.Run("app id integer", func(t *testing.T) { t.Parallel() config := &Config{ - AppID: "1", - AppInstallationID: 10, + AppConfig: appconfig.AppConfig{ + AppID: "1", + AppInstallationID: 10, + }, ConfigureUrl: "github.com/some_org/some_repo", EphemeralRunnerSetNamespace: "namespace", EphemeralRunnerSetName: "deployment", @@ -54,8 +59,10 @@ func TestConfigValidationAppKey(t *testing.T) { t.Run("app id as client id", func(t *testing.T) { t.Parallel() config := &Config{ - AppID: "Iv23f8doAlphaNumer1c", - AppInstallationID: 10, + AppConfig: appconfig.AppConfig{ + AppID: "Iv23f8doAlphaNumer1c", + AppInstallationID: 10, + }, ConfigureUrl: "github.com/some_org/some_repo", EphemeralRunnerSetNamespace: "namespace", EphemeralRunnerSetName: "deployment", @@ -69,10 +76,12 @@ func TestConfigValidationAppKey(t *testing.T) { func TestConfigValidationOnlyOneTypeOfCredentials(t *testing.T) { config := &Config{ - AppID: "1", - AppInstallationID: 10, - AppPrivateKey: "asdf", - Token: "asdf", + AppConfig: appconfig.AppConfig{ + AppID: "1", + AppInstallationID: 10, + AppPrivateKey: "asdf", + Token: "asdf", + }, ConfigureUrl: "github.com/some_org/some_repo", EphemeralRunnerSetNamespace: "namespace", EphemeralRunnerSetName: "deployment", @@ -91,7 +100,9 @@ func TestConfigValidation(t *testing.T) { RunnerScaleSetId: 1, MinRunners: 1, MaxRunners: 5, - Token: "asdf", + AppConfig: appconfig.AppConfig{ + Token: "asdf", + }, } err := config.Validate() diff --git a/cmd/ghalistener/main.go b/cmd/ghalistener/main.go index 10436b30..26fcb511 100644 --- a/cmd/ghalistener/main.go +++ b/cmd/ghalistener/main.go @@ -13,26 +13,27 @@ import ( ) func main() { + ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) + defer stop() + configPath, ok := os.LookupEnv("LISTENER_CONFIG_PATH") if !ok { fmt.Fprintf(os.Stderr, "Error: LISTENER_CONFIG_PATH environment variable is not set\n") os.Exit(1) } - config, err := config.Read(configPath) + + config, err := config.Read(ctx, configPath) if err != nil { log.Printf("Failed to read config: %v", err) os.Exit(1) } - app, err := app.New(config) + app, err := app.New(*config) if err != nil { log.Printf("Failed to initialize app: %v", err) os.Exit(1) } - ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) - defer stop() - if err := app.Run(ctx); err != nil { log.Printf("Application returned an error: %v", err) os.Exit(1) diff --git a/controllers/actions.github.com/actions_client_pool.go b/controllers/actions.github.com/actions_client_pool.go new file mode 100644 index 00000000..2be944a0 --- /dev/null +++ b/controllers/actions.github.com/actions_client_pool.go @@ -0,0 +1,242 @@ +package actionsgithubcom + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/url" + "strings" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1/appconfig" + "github.com/actions/actions-runner-controller/github/actions" + "github.com/actions/actions-runner-controller/vault" + "golang.org/x/net/http/httpproxy" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +type ActionsClientPool struct { + k8sClient client.Client + vaultResolvers map[string]resolver + multiClient actions.MultiClient +} + +type ActionsClientPoolOption func(*ActionsClientPool) + +func WithVault(ty string, vault vault.Vault) ActionsClientPoolOption { + return func(pool *ActionsClientPool) { + pool.vaultResolvers[ty] = &vaultResolver{vault} + } +} + +func NewActionsClientPool(k8sClient client.Client, multiClient actions.MultiClient, opts ...ActionsClientPoolOption) *ActionsClientPool { + if k8sClient == nil { + panic("k8sClient must not be nil") + } + + pool := &ActionsClientPool{ + k8sClient: k8sClient, + multiClient: multiClient, + vaultResolvers: make(map[string]resolver), + } + + for _, opt := range opts { + opt(pool) + } + + return pool +} + +type ActionsGitHubObject interface { + client.Object + GitHubConfigUrl() string + GitHubConfigSecret() string + Proxy() *v1alpha1.ProxyConfig + GitHubServerTLS() *v1alpha1.GitHubServerTLSConfig +} + +func (p *ActionsClientPool) Get(ctx context.Context, obj ActionsGitHubObject) (actions.ActionsService, error) { + resolver, err := p.resolverForObject(obj) + if err != nil { + return nil, fmt.Errorf("failed to get resolver for object: %v", err) + } + + appConfig, err := resolver.appConfig(ctx, obj.GitHubConfigSecret()) + if err != nil { + return nil, fmt.Errorf("failed to resolve app config: %v", err) + } + + var clientOptions []actions.ClientOption + if proxy := obj.Proxy(); proxy != nil { + config := &httpproxy.Config{ + NoProxy: strings.Join(proxy.NoProxy, ","), + } + + if proxy.HTTP != nil { + u, err := url.Parse(proxy.HTTP.Url) + if err != nil { + return nil, fmt.Errorf("failed to parse proxy http url %q: %w", proxy.HTTP.Url, err) + } + + if ref := proxy.HTTP.CredentialSecretRef; ref != "" { + u.User, err = resolver.proxyCredentials(ctx, ref) + if err != nil { + return nil, fmt.Errorf("failed to resolve proxy credentials: %v", err) + } + } + + config.HTTPProxy = u.String() + } + + if proxy.HTTPS != nil { + u, err := url.Parse(proxy.HTTPS.Url) + if err != nil { + return nil, fmt.Errorf("failed to parse proxy https url %q: %w", proxy.HTTPS.Url, err) + } + + if ref := proxy.HTTPS.CredentialSecretRef; ref != "" { + u.User, err = resolver.proxyCredentials(ctx, ref) + if err != nil { + return nil, fmt.Errorf("failed to resolve proxy credentials: %v", err) + } + } + + config.HTTPSProxy = u.String() + } + + proxyFunc := func(req *http.Request) (*url.URL, error) { + return config.ProxyFunc()(req.URL) + } + + clientOptions = append(clientOptions, actions.WithProxy(proxyFunc)) + } + + tlsConfig := obj.GitHubServerTLS() + if tlsConfig != nil { + pool, err := tlsConfig.ToCertPool(func(name, key string) ([]byte, error) { + var configmap corev1.ConfigMap + err := p.k8sClient.Get( + ctx, + types.NamespacedName{ + Namespace: obj.GetNamespace(), + Name: name, + }, + &configmap, + ) + if err != nil { + return nil, fmt.Errorf("failed to get configmap %s: %w", name, err) + } + + return []byte(configmap.Data[key]), nil + }) + if err != nil { + return nil, fmt.Errorf("failed to get tls config: %w", err) + } + + clientOptions = append(clientOptions, actions.WithRootCAs(pool)) + } + + return p.multiClient.GetClientFor( + ctx, + obj.GitHubConfigUrl(), + appConfig, + obj.GetNamespace(), + clientOptions..., + ) +} + +func (p *ActionsClientPool) resolverForObject(obj ActionsGitHubObject) (resolver, error) { + ty, ok := obj.GetAnnotations()[AnnotationKeyGitHubVaultType] + if !ok { + return &k8sResolver{ + namespace: obj.GetNamespace(), + client: p.k8sClient, + }, nil + } + + vault, ok := p.vaultResolvers[ty] + if !ok { + return nil, fmt.Errorf("unknown vault resolver %q", ty) + } + + return vault, nil +} + +type resolver interface { + appConfig(ctx context.Context, key string) (*appconfig.AppConfig, error) + proxyCredentials(ctx context.Context, key string) (*url.Userinfo, error) +} + +type k8sResolver struct { + namespace string + client client.Client +} + +func (r *k8sResolver) appConfig(ctx context.Context, key string) (*appconfig.AppConfig, error) { + nsName := types.NamespacedName{ + Namespace: r.namespace, + Name: key, + } + secret := new(corev1.Secret) + if err := r.client.Get( + ctx, + nsName, + secret, + ); err != nil { + return nil, fmt.Errorf("failed to get kubernetes secret: %q", nsName.String()) + } + + return appconfig.FromSecret(secret) +} + +func (r *k8sResolver) proxyCredentials(ctx context.Context, key string) (*url.Userinfo, error) { + nsName := types.NamespacedName{Namespace: r.namespace, Name: key} + secret := new(corev1.Secret) + if err := r.client.Get( + ctx, + nsName, + secret, + ); err != nil { + return nil, fmt.Errorf("failed to get kubernetes secret: %q", nsName.String()) + } + + return url.UserPassword( + string(secret.Data["username"]), + string(secret.Data["password"]), + ), nil +} + +type vaultResolver struct { + vault vault.Vault +} + +func (r *vaultResolver) appConfig(ctx context.Context, key string) (*appconfig.AppConfig, error) { + val, err := r.vault.GetSecret(ctx, key) + if err != nil { + return nil, fmt.Errorf("failed to resolve secret: %v", err) + } + + return appconfig.FromString(val) +} + +func (r *vaultResolver) proxyCredentials(ctx context.Context, key string) (*url.Userinfo, error) { + val, err := r.vault.GetSecret(ctx, key) + if err != nil { + return nil, fmt.Errorf("failed to resolve secret: %v", err) + } + + type info struct { + Username string `json:"username"` + Password string `json:"password"` + } + + var i info + if err := json.Unmarshal([]byte(val), &i); err != nil { + return nil, fmt.Errorf("failed to unmarshal info: %v", err) + } + + return url.UserPassword(i.Username, i.Password), nil +} diff --git a/controllers/actions.github.com/actions_client_resolver.go b/controllers/actions.github.com/actions_client_resolver.go deleted file mode 100644 index 5fdab8d2..00000000 --- a/controllers/actions.github.com/actions_client_resolver.go +++ /dev/null @@ -1,248 +0,0 @@ -package actionsgithubcom - -import ( - "context" - "fmt" - - "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" - "github.com/actions/actions-runner-controller/github/actions" - "github.com/actions/actions-runner-controller/vault" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/client" -) - -type ActionsClientGetter interface { - GetActionsClientForAutoscalingRunnerSet(ctx context.Context, ars *v1alpha1.AutoscalingRunnerSet) (actions.ActionsService, error) - GetActionsClientForEphemeralRunnerSet(ctx context.Context, ers *v1alpha1.EphemeralRunnerSet) (actions.ActionsService, error) - GetActionsClientForEphemeralRunner(ctx context.Context, er *v1alpha1.EphemeralRunner) (actions.ActionsService, error) -} - -var ( - _ ActionsClientGetter = (*ActionsClientSecretResolver)(nil) - _ ActionsClientGetter = (*ActionsClientVaultResolver)(nil) -) - -type ActionsClientSecretResolver struct { - client.Client - actions.MultiClient -} - -func (r *ActionsClientSecretResolver) GetActionsClientForAutoscalingRunnerSet(ctx context.Context, ars *v1alpha1.AutoscalingRunnerSet) (actions.ActionsService, error) { - var configSecret corev1.Secret - if err := r.Get(ctx, types.NamespacedName{Namespace: ars.Namespace, Name: ars.Spec.GitHubConfigSecret}, &configSecret); err != nil { - return nil, fmt.Errorf("failed to find GitHub config secret: %w", err) - } - - opts, err := r.actionsClientOptionsForAutoscalingRunnerSet(ctx, ars) - if err != nil { - return nil, fmt.Errorf("failed to get actions client options: %w", err) - } - - return r.MultiClient.GetClientFromSecret( - ctx, - ars.Spec.GitHubConfigUrl, - ars.Namespace, - configSecret.Data, - opts..., - ) -} - -func (r *ActionsClientSecretResolver) actionsClientOptionsForAutoscalingRunnerSet(ctx context.Context, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet) ([]actions.ClientOption, error) { - var options []actions.ClientOption - - if autoscalingRunnerSet.Spec.Proxy != nil { - proxyFunc, err := autoscalingRunnerSet.Spec.Proxy.ProxyFunc(func(s string) (*corev1.Secret, error) { - var secret corev1.Secret - err := r.Get(ctx, types.NamespacedName{Namespace: autoscalingRunnerSet.Namespace, Name: s}, &secret) - if err != nil { - return nil, fmt.Errorf("failed to get proxy secret %s: %w", s, err) - } - - return &secret, nil - }) - if err != nil { - return nil, fmt.Errorf("failed to get proxy func: %w", err) - } - - options = append(options, actions.WithProxy(proxyFunc)) - } - - tlsConfig := autoscalingRunnerSet.Spec.GitHubServerTLS - if tlsConfig != nil { - pool, err := tlsConfig.ToCertPool(func(name, key string) ([]byte, error) { - var configmap corev1.ConfigMap - err := r.Get( - ctx, - types.NamespacedName{ - Namespace: autoscalingRunnerSet.Namespace, - Name: name, - }, - &configmap, - ) - if err != nil { - return nil, fmt.Errorf("failed to get configmap %s: %w", name, err) - } - - return []byte(configmap.Data[key]), nil - }) - if err != nil { - return nil, fmt.Errorf("failed to get tls config: %w", err) - } - - options = append(options, actions.WithRootCAs(pool)) - } - - return options, nil -} - -func (r *ActionsClientSecretResolver) GetActionsClientForEphemeralRunnerSet(ctx context.Context, rs *v1alpha1.EphemeralRunnerSet) (actions.ActionsService, error) { - secret := new(corev1.Secret) - if err := r.Get(ctx, types.NamespacedName{Namespace: rs.Namespace, Name: rs.Spec.EphemeralRunnerSpec.GitHubConfigSecret}, secret); err != nil { - return nil, fmt.Errorf("failed to get secret: %w", err) - } - - opts, err := r.actionsClientOptionsForEphemeralRunnerSet(ctx, rs) - if err != nil { - return nil, fmt.Errorf("failed to get actions client options: %w", err) - } - - return r.MultiClient.GetClientFromSecret( - ctx, - rs.Spec.EphemeralRunnerSpec.GitHubConfigUrl, - rs.Namespace, - secret.Data, - opts..., - ) -} - -func (r *ActionsClientSecretResolver) actionsClientOptionsForEphemeralRunnerSet(ctx context.Context, rs *v1alpha1.EphemeralRunnerSet) ([]actions.ClientOption, error) { - var opts []actions.ClientOption - if rs.Spec.EphemeralRunnerSpec.Proxy != nil { - proxyFunc, err := rs.Spec.EphemeralRunnerSpec.Proxy.ProxyFunc(func(s string) (*corev1.Secret, error) { - var secret corev1.Secret - err := r.Get(ctx, types.NamespacedName{Namespace: rs.Namespace, Name: s}, &secret) - if err != nil { - return nil, fmt.Errorf("failed to get secret %s: %w", s, err) - } - - return &secret, nil - }) - if err != nil { - return nil, fmt.Errorf("failed to get proxy func: %w", err) - } - - opts = append(opts, actions.WithProxy(proxyFunc)) - } - - tlsConfig := rs.Spec.EphemeralRunnerSpec.GitHubServerTLS - if tlsConfig != nil { - pool, err := tlsConfig.ToCertPool(func(name, key string) ([]byte, error) { - var configmap corev1.ConfigMap - err := r.Get( - ctx, - types.NamespacedName{ - Namespace: rs.Namespace, - Name: name, - }, - &configmap, - ) - if err != nil { - return nil, fmt.Errorf("failed to get configmap %s: %w", name, err) - } - - return []byte(configmap.Data[key]), nil - }) - if err != nil { - return nil, fmt.Errorf("failed to get tls config: %w", err) - } - - opts = append(opts, actions.WithRootCAs(pool)) - } - - return opts, nil -} - -func (r *ActionsClientSecretResolver) GetActionsClientForEphemeralRunner(ctx context.Context, runner *v1alpha1.EphemeralRunner) (actions.ActionsService, error) { - secret := new(corev1.Secret) - if err := r.Get(ctx, types.NamespacedName{Namespace: runner.Namespace, Name: runner.Spec.GitHubConfigSecret}, secret); err != nil { - return nil, fmt.Errorf("failed to get secret: %w", err) - } - - opts, err := r.actionsClientOptionsForEphemeralRunner(ctx, runner) - if err != nil { - return nil, fmt.Errorf("failed to get actions client options: %w", err) - } - - return r.MultiClient.GetClientFromSecret( - ctx, - runner.Spec.GitHubConfigUrl, - runner.Namespace, - secret.Data, - opts..., - ) -} - -func (r *ActionsClientSecretResolver) actionsClientOptionsForEphemeralRunner(ctx context.Context, runner *v1alpha1.EphemeralRunner) ([]actions.ClientOption, error) { - var opts []actions.ClientOption - if runner.Spec.Proxy != nil { - proxyFunc, err := runner.Spec.Proxy.ProxyFunc(func(s string) (*corev1.Secret, error) { - var secret corev1.Secret - err := r.Get(ctx, types.NamespacedName{Namespace: runner.Namespace, Name: s}, &secret) - if err != nil { - return nil, fmt.Errorf("failed to get proxy secret %s: %w", s, err) - } - - return &secret, nil - }) - if err != nil { - return nil, fmt.Errorf("failed to get proxy func: %w", err) - } - - opts = append(opts, actions.WithProxy(proxyFunc)) - } - - tlsConfig := runner.Spec.GitHubServerTLS - if tlsConfig != nil { - pool, err := tlsConfig.ToCertPool(func(name, key string) ([]byte, error) { - var configmap corev1.ConfigMap - err := r.Get( - ctx, - types.NamespacedName{ - Namespace: runner.Namespace, - Name: name, - }, - &configmap, - ) - if err != nil { - return nil, fmt.Errorf("failed to get configmap %s: %w", name, err) - } - - return []byte(configmap.Data[key]), nil - }) - if err != nil { - return nil, fmt.Errorf("failed to get tls config: %w", err) - } - - opts = append(opts, actions.WithRootCAs(pool)) - } - - return opts, nil -} - -type ActionsClientVaultResolver struct { - vault.Vault - actions.MultiClient -} - -func (r *ActionsClientVaultResolver) GetActionsClientForAutoscalingRunnerSet(ctx context.Context, ars *v1alpha1.AutoscalingRunnerSet) (actions.ActionsService, error) { - panic("todo") -} - -func (r *ActionsClientVaultResolver) GetActionsClientForEphemeralRunnerSet(ctx context.Context, ers *v1alpha1.EphemeralRunnerSet) (actions.ActionsService, error) { - panic("todo") -} - -func (r *ActionsClientVaultResolver) GetActionsClientForEphemeralRunner(ctx context.Context, er *v1alpha1.EphemeralRunner) (actions.ActionsService, error) { - panic("todo") -} diff --git a/controllers/actions.github.com/autoscalinglistener_controller.go b/controllers/actions.github.com/autoscalinglistener_controller.go index cecfdfbf..b064648b 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller.go +++ b/controllers/actions.github.com/autoscalinglistener_controller.go @@ -137,27 +137,6 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, err } - // Create a mirror secret in the same namespace as the AutoscalingListener - mirrorSecret := new(corev1.Secret) - if err := r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Namespace, Name: autoscalingListener.Name}, mirrorSecret); err != nil { - if !kerrors.IsNotFound(err) { - log.Error(err, "Unable to get listener secret mirror", "namespace", autoscalingListener.Namespace, "name", autoscalingListener.Name) - return ctrl.Result{}, err - } - - // Create a mirror secret for the listener pod in the Controller namespace for listener pod to use - log.Info("Creating a mirror listener secret for the listener pod") - return r.createSecretsForListener(ctx, autoscalingListener, secret, log) - } - - // make sure the mirror secret is up to date - mirrorSecretDataHash := mirrorSecret.Labels["secret-data-hash"] - secretDataHash := hash.ComputeTemplateHash(secret.Data) - if mirrorSecretDataHash != secretDataHash { - log.Info("Updating mirror listener secret for the listener pod", "mirrorSecretDataHash", mirrorSecretDataHash, "secretDataHash", secretDataHash) - return r.updateSecretsForListener(ctx, secret, mirrorSecret, log) - } - // Make sure the runner scale set listener service account is created for the listener pod in the controller namespace serviceAccount := new(corev1.ServiceAccount) if err := r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Namespace, Name: autoscalingListener.Name}, serviceAccount); err != nil { @@ -239,7 +218,7 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. // Create a listener pod in the controller namespace log.Info("Creating a listener pod") - return r.createListenerPod(ctx, &autoscalingRunnerSet, autoscalingListener, serviceAccount, mirrorSecret, log) + return r.createListenerPod(ctx, &autoscalingRunnerSet, autoscalingListener, serviceAccount, secret, log) } cs := listenerContainerStatus(listenerPod) @@ -486,7 +465,7 @@ func (r *AutoscalingListenerReconciler) createListenerPod(ctx context.Context, a return ctrl.Result{Requeue: true}, nil } - newPod, err := r.newScaleSetListenerPod(autoscalingListener, &podConfig, serviceAccount, secret, metricsConfig, envs...) + newPod, err := r.newScaleSetListenerPod(autoscalingListener, &podConfig, serviceAccount, metricsConfig, envs...) if err != nil { logger.Error(err, "Failed to build listener pod") return ctrl.Result{}, err diff --git a/controllers/actions.github.com/autoscalinglistener_controller_test.go b/controllers/actions.github.com/autoscalinglistener_controller_test.go index 5ce5ee48..9466fed1 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller_test.go +++ b/controllers/actions.github.com/autoscalinglistener_controller_test.go @@ -14,7 +14,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" - listenerconfig "github.com/actions/actions-runner-controller/cmd/ghalistener/config" + ghalistenerconfig "github.com/actions/actions-runner-controller/cmd/ghalistener/config" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" kerrors "k8s.io/apimachinery/pkg/api/errors" @@ -1110,7 +1110,7 @@ var _ = Describe("Test GitHub Server TLS configuration", func() { g.Expect(config.Data["config.json"]).ToNot(BeEmpty(), "listener configuration file should not be empty") - var listenerConfig listenerconfig.Config + var listenerConfig ghalistenerconfig.Config err = json.Unmarshal(config.Data["config.json"], &listenerConfig) g.Expect(err).NotTo(HaveOccurred(), "failed to parse listener configuration file") diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index 3003a318..5f1462e3 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -207,14 +207,6 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return r.updateRunnerScaleSetName(ctx, autoscalingRunnerSet, log) } - secret := new(corev1.Secret) - if err := r.Get(ctx, types.NamespacedName{Namespace: autoscalingRunnerSet.Namespace, Name: autoscalingRunnerSet.Spec.GitHubConfigSecret}, secret); err != nil { - log.Error(err, "Failed to find GitHub config secret.", - "namespace", autoscalingRunnerSet.Namespace, - "name", autoscalingRunnerSet.Spec.GitHubConfigSecret) - return ctrl.Result{}, err - } - existingRunnerSets, err := r.listEphemeralRunnerSets(ctx, autoscalingRunnerSet) if err != nil { log.Error(err, "Failed to list existing ephemeral runner sets") @@ -402,7 +394,7 @@ func (r *AutoscalingRunnerSetReconciler) removeFinalizersFromDependentResources( func (r *AutoscalingRunnerSetReconciler) createRunnerScaleSet(ctx context.Context, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, logger logr.Logger) (ctrl.Result, error) { logger.Info("Creating a new runner scale set") - actionsClient, err := r.ActionsClientGetter.GetActionsClientForAutoscalingRunnerSet(ctx, autoscalingRunnerSet) + actionsClient, err := r.ActionsClientPool.Get(ctx, autoscalingRunnerSet) if len(autoscalingRunnerSet.Spec.RunnerScaleSetName) == 0 { autoscalingRunnerSet.Spec.RunnerScaleSetName = autoscalingRunnerSet.Name } @@ -498,7 +490,7 @@ func (r *AutoscalingRunnerSetReconciler) updateRunnerScaleSetRunnerGroup(ctx con return ctrl.Result{}, err } - actionsClient, err := r.ActionsClientGetter.GetActionsClientForAutoscalingRunnerSet(ctx, autoscalingRunnerSet) + actionsClient, err := r.ActionsClientPool.Get(ctx, autoscalingRunnerSet) if err != nil { logger.Error(err, "Failed to initialize Actions service client for updating a existing runner scale set") return ctrl.Result{}, err @@ -546,7 +538,7 @@ func (r *AutoscalingRunnerSetReconciler) updateRunnerScaleSetName(ctx context.Co return ctrl.Result{}, nil } - actionsClient, err := r.ActionsClientGetter.GetActionsClientForAutoscalingRunnerSet(ctx, autoscalingRunnerSet) + actionsClient, err := r.ActionsClientPool.Get(ctx, autoscalingRunnerSet) if err != nil { logger.Error(err, "Failed to initialize Actions service client for updating a existing runner scale set") return ctrl.Result{}, err @@ -597,7 +589,7 @@ func (r *AutoscalingRunnerSetReconciler) deleteRunnerScaleSet(ctx context.Contex return nil } - actionsClient, err := r.ActionsClientGetter.GetActionsClientForAutoscalingRunnerSet(ctx, autoscalingRunnerSet) + actionsClient, err := r.ActionsClientPool.Get(ctx, autoscalingRunnerSet) if err != nil { logger.Error(err, "Failed to initialize Actions service client for updating a existing runner scale set") return err diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go index 0f7c03fd..9b149447 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go @@ -71,9 +71,9 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", ResourceBuilder: ResourceBuilder{ - ActionsClientGetter: &ActionsClientSecretResolver{ - Client: k8sClient, - MultiClient: fake.NewMultiClient(), + ActionsClientPool: &ActionsClientPool{ + k8sClient: k8sClient, + multiClient: fake.NewMultiClient(), }, }, } @@ -711,9 +711,9 @@ var _ = Describe("Test AutoScalingController updates", Ordered, func() { ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", ResourceBuilder: ResourceBuilder{ - ActionsClientGetter: &ActionsClientSecretResolver{ - Client: k8sClient, - MultiClient: multiClient, + ActionsClientPool: &ActionsClientPool{ + k8sClient: k8sClient, + multiClient: multiClient, }, }, } @@ -831,9 +831,9 @@ var _ = Describe("Test AutoscalingController creation failures", Ordered, func() ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", ResourceBuilder: ResourceBuilder{ - ActionsClientGetter: &ActionsClientSecretResolver{ - Client: k8sClient, - MultiClient: fake.NewMultiClient(), + ActionsClientPool: &ActionsClientPool{ + k8sClient: k8sClient, + multiClient: fake.NewMultiClient(), }, }, } @@ -962,9 +962,9 @@ var _ = Describe("Test client optional configuration", Ordered, func() { ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", ResourceBuilder: ResourceBuilder{ - ActionsClientGetter: &ActionsClientSecretResolver{ - Client: k8sClient, - MultiClient: multiClient, + ActionsClientPool: &ActionsClientPool{ + k8sClient: k8sClient, + multiClient: multiClient, }, }, } @@ -1150,9 +1150,9 @@ var _ = Describe("Test client optional configuration", Ordered, func() { ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", ResourceBuilder: ResourceBuilder{ - ActionsClientGetter: &ActionsClientSecretResolver{ - Client: k8sClient, - MultiClient: fake.NewMultiClient(), + ActionsClientPool: &ActionsClientPool{ + k8sClient: k8sClient, + multiClient: fake.NewMultiClient(), }, }, } @@ -1163,9 +1163,9 @@ var _ = Describe("Test client optional configuration", Ordered, func() { }) It("should be able to make requests to a server using root CAs", func() { - controller.ResourceBuilder.ActionsClientGetter = &ActionsClientSecretResolver{ - Client: k8sClient, - MultiClient: actions.NewMultiClient(logr.Discard()), + controller.ActionsClientPool = &ActionsClientPool{ + k8sClient: k8sClient, + multiClient: actions.NewMultiClient(logr.Discard()), } certsFolder := filepath.Join( @@ -1392,9 +1392,9 @@ var _ = Describe("Test external permissions cleanup", Ordered, func() { ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", ResourceBuilder: ResourceBuilder{ - ActionsClientGetter: &ActionsClientSecretResolver{ - Client: k8sClient, - MultiClient: fake.NewMultiClient(), + ActionsClientPool: &ActionsClientPool{ + k8sClient: k8sClient, + multiClient: fake.NewMultiClient(), }, }, } @@ -1555,9 +1555,9 @@ var _ = Describe("Test external permissions cleanup", Ordered, func() { ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", ResourceBuilder: ResourceBuilder{ - ActionsClientGetter: &ActionsClientSecretResolver{ - Client: k8sClient, - MultiClient: fake.NewMultiClient(), + ActionsClientPool: &ActionsClientPool{ + k8sClient: k8sClient, + multiClient: fake.NewMultiClient(), }, }, } @@ -1768,9 +1768,9 @@ var _ = Describe("Test resource version and build version mismatch", func() { ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", ResourceBuilder: ResourceBuilder{ - ActionsClientGetter: &ActionsClientSecretResolver{ - Client: k8sClient, - MultiClient: fake.NewMultiClient(), + ActionsClientPool: &ActionsClientPool{ + k8sClient: k8sClient, + multiClient: fake.NewMultiClient(), }, }, } diff --git a/controllers/actions.github.com/constants.go b/controllers/actions.github.com/constants.go index cbf3309c..1a620906 100644 --- a/controllers/actions.github.com/constants.go +++ b/controllers/actions.github.com/constants.go @@ -43,6 +43,8 @@ const ( AnnotationKeyGitHubRunnerGroupName = "actions.github.com/runner-group-name" AnnotationKeyGitHubRunnerScaleSetName = "actions.github.com/runner-scale-set-name" AnnotationKeyPatchID = "actions.github.com/patch-id" + + AnnotationKeyGitHubVaultType = "actions.github.com/vault" ) // Labels applied to listener roles diff --git a/controllers/actions.github.com/ephemeralrunner_controller.go b/controllers/actions.github.com/ephemeralrunner_controller.go index 5ac80df7..e915220b 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller.go +++ b/controllers/actions.github.com/ephemeralrunner_controller.go @@ -45,9 +45,8 @@ const ( // EphemeralRunnerReconciler reconciles a EphemeralRunner object type EphemeralRunnerReconciler struct { client.Client - Log logr.Logger - Scheme *runtime.Scheme - ActionsClient actions.MultiClient + Log logr.Logger + Scheme *runtime.Scheme ResourceBuilder } @@ -529,7 +528,7 @@ func (r *EphemeralRunnerReconciler) deletePodAsFailed(ctx context.Context, ephem func (r *EphemeralRunnerReconciler) updateStatusWithRunnerConfig(ctx context.Context, ephemeralRunner *v1alpha1.EphemeralRunner, log logr.Logger) (*ctrl.Result, error) { // Runner is not registered with the service. We need to register it first log.Info("Creating ephemeral runner JIT config") - actionsClient, err := r.actionsClientFor(ctx, ephemeralRunner) + actionsClient, err := r.ActionsClientPool.Get(ctx, ephemeralRunner) if err != nil { return &ctrl.Result{}, fmt.Errorf("failed to get actions client for generating JIT config: %w", err) } @@ -753,77 +752,10 @@ func (r *EphemeralRunnerReconciler) updateRunStatusFromPod(ctx context.Context, return nil } -func (r *EphemeralRunnerReconciler) actionsClientFor(ctx context.Context, runner *v1alpha1.EphemeralRunner) (actions.ActionsService, error) { - secret := new(corev1.Secret) - if err := r.Get(ctx, types.NamespacedName{Namespace: runner.Namespace, Name: runner.Spec.GitHubConfigSecret}, secret); err != nil { - return nil, fmt.Errorf("failed to get secret: %w", err) - } - - opts, err := r.actionsClientOptionsFor(ctx, runner) - if err != nil { - return nil, fmt.Errorf("failed to get actions client options: %w", err) - } - - return r.ActionsClient.GetClientFromSecret( - ctx, - runner.Spec.GitHubConfigUrl, - runner.Namespace, - secret.Data, - opts..., - ) -} - -func (r *EphemeralRunnerReconciler) actionsClientOptionsFor(ctx context.Context, runner *v1alpha1.EphemeralRunner) ([]actions.ClientOption, error) { - var opts []actions.ClientOption - if runner.Spec.Proxy != nil { - proxyFunc, err := runner.Spec.Proxy.ProxyFunc(func(s string) (*corev1.Secret, error) { - var secret corev1.Secret - err := r.Get(ctx, types.NamespacedName{Namespace: runner.Namespace, Name: s}, &secret) - if err != nil { - return nil, fmt.Errorf("failed to get proxy secret %s: %w", s, err) - } - - return &secret, nil - }) - if err != nil { - return nil, fmt.Errorf("failed to get proxy func: %w", err) - } - - opts = append(opts, actions.WithProxy(proxyFunc)) - } - - tlsConfig := runner.Spec.GitHubServerTLS - if tlsConfig != nil { - pool, err := tlsConfig.ToCertPool(func(name, key string) ([]byte, error) { - var configmap corev1.ConfigMap - err := r.Get( - ctx, - types.NamespacedName{ - Namespace: runner.Namespace, - Name: name, - }, - &configmap, - ) - if err != nil { - return nil, fmt.Errorf("failed to get configmap %s: %w", name, err) - } - - return []byte(configmap.Data[key]), nil - }) - if err != nil { - return nil, fmt.Errorf("failed to get tls config: %w", err) - } - - opts = append(opts, actions.WithRootCAs(pool)) - } - - return opts, nil -} - // runnerRegisteredWithService checks if the runner is still registered with the service // Returns found=false and err=nil if ephemeral runner does not exist in GitHub service and should be deleted func (r EphemeralRunnerReconciler) runnerRegisteredWithService(ctx context.Context, runner *v1alpha1.EphemeralRunner, log logr.Logger) (found bool, err error) { - actionsClient, err := r.actionsClientFor(ctx, runner) + actionsClient, err := r.ActionsClientPool.Get(ctx, runner) if err != nil { return false, fmt.Errorf("failed to get Actions client for ScaleSet: %w", err) } @@ -850,7 +782,7 @@ func (r EphemeralRunnerReconciler) runnerRegisteredWithService(ctx context.Conte } func (r *EphemeralRunnerReconciler) deleteRunnerFromService(ctx context.Context, ephemeralRunner *v1alpha1.EphemeralRunner, log logr.Logger) error { - client, err := r.actionsClientFor(ctx, ephemeralRunner) + client, err := r.ActionsClientPool.Get(ctx, ephemeralRunner) if err != nil { return fmt.Errorf("failed to get actions client for runner: %w", err) } diff --git a/controllers/actions.github.com/ephemeralrunner_controller_test.go b/controllers/actions.github.com/ephemeralrunner_controller_test.go index 0b842d5c..c6a19142 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller_test.go +++ b/controllers/actions.github.com/ephemeralrunner_controller_test.go @@ -107,10 +107,15 @@ var _ = Describe("EphemeralRunner", func() { configSecret = createDefaultSecret(GinkgoT(), k8sClient, autoscalingNS.Name) controller = &EphemeralRunnerReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - Log: logf.Log, - ActionsClient: fake.NewMultiClient(), + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + ResourceBuilder: ResourceBuilder{ + ActionsClientPool: &ActionsClientPool{ + k8sClient: mgr.GetClient(), + multiClient: fake.NewMultiClient(), + }, + }, } err := controller.SetupWithManager(mgr) @@ -789,22 +794,27 @@ var _ = Describe("EphemeralRunner", func() { Client: mgr.GetClient(), Scheme: mgr.GetScheme(), Log: logf.Log, - ActionsClient: fake.NewMultiClient( - fake.WithDefaultClient( - fake.NewFakeClient( - fake.WithGetRunner( + ResourceBuilder: ResourceBuilder{ + ActionsClientPool: &ActionsClientPool{ + k8sClient: mgr.GetClient(), + multiClient: fake.NewMultiClient( + fake.WithDefaultClient( + fake.NewFakeClient( + fake.WithGetRunner( + nil, + &actions.ActionsError{ + StatusCode: http.StatusNotFound, + Err: &actions.ActionsExceptionError{ + ExceptionName: "AgentNotFoundException", + }, + }, + ), + ), nil, - &actions.ActionsError{ - StatusCode: http.StatusNotFound, - Err: &actions.ActionsExceptionError{ - ExceptionName: "AgentNotFoundException", - }, - }, ), ), - nil, - ), - ), + }, + }, } err := controller.SetupWithManager(mgr) Expect(err).To(BeNil(), "failed to setup controller") @@ -861,10 +871,15 @@ var _ = Describe("EphemeralRunner", func() { configSecret = createDefaultSecret(GinkgoT(), k8sClient, autoScalingNS.Name) controller = &EphemeralRunnerReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - Log: logf.Log, - ActionsClient: fake.NewMultiClient(), + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + ResourceBuilder: ResourceBuilder{ + ActionsClientPool: &ActionsClientPool{ + k8sClient: mgr.GetClient(), + multiClient: fake.NewMultiClient(), + }, + }, } err := controller.SetupWithManager(mgr) Expect(err).To(BeNil(), "failed to setup controller") @@ -874,7 +889,12 @@ var _ = Describe("EphemeralRunner", func() { It("uses an actions client with proxy transport", func() { // Use an actual client - controller.ActionsClient = actions.NewMultiClient(logr.Discard()) + controller.ResourceBuilder = ResourceBuilder{ + ActionsClientPool: &ActionsClientPool{ + k8sClient: mgr.GetClient(), + multiClient: actions.NewMultiClient(logr.Discard()), + }, + } proxySuccessfulllyCalled := false proxy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -1025,10 +1045,15 @@ var _ = Describe("EphemeralRunner", func() { Expect(err).NotTo(HaveOccurred(), "failed to create configmap with root CAs") controller = &EphemeralRunnerReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - Log: logf.Log, - ActionsClient: fake.NewMultiClient(), + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + ResourceBuilder: ResourceBuilder{ + ActionsClientPool: &ActionsClientPool{ + k8sClient: mgr.GetClient(), + multiClient: fake.NewMultiClient(), + }, + }, } err = controller.SetupWithManager(mgr) @@ -1059,7 +1084,12 @@ var _ = Describe("EphemeralRunner", func() { server.StartTLS() // Use an actual client - controller.ActionsClient = actions.NewMultiClient(logr.Discard()) + controller.ResourceBuilder = ResourceBuilder{ + ActionsClientPool: &ActionsClientPool{ + k8sClient: mgr.GetClient(), + multiClient: actions.NewMultiClient(logr.Discard()), + }, + } ephemeralRunner := newExampleRunner("test-runner", autoScalingNS.Name, configSecret.Name) ephemeralRunner.Spec.GitHubConfigUrl = server.ConfigURLForOrg("my-org") diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index d5975adc..2d2b8bcd 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller.go @@ -331,7 +331,7 @@ func (r *EphemeralRunnerSetReconciler) cleanUpEphemeralRunners(ctx context.Conte return false, nil } - actionsClient, err := r.ActionsClientGetter.GetActionsClientForEphemeralRunnerSet(ctx, ephemeralRunnerSet) + actionsClient, err := r.ActionsClientPool.Get(ctx, ephemeralRunnerSet) if err != nil { return false, err } @@ -439,7 +439,7 @@ func (r *EphemeralRunnerSetReconciler) deleteIdleEphemeralRunners(ctx context.Co log.Info("No pending or running ephemeral runners running at this time for scale down") return nil } - actionsClient, err := r.ActionsClientGetter.GetActionsClientForEphemeralRunnerSet(ctx, ephemeralRunnerSet) + actionsClient, err := r.ActionsClientPool.Get(ctx, ephemeralRunnerSet) if err != nil { return fmt.Errorf("failed to create actions client for ephemeral runner replica set: %w", err) } diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller_test.go b/controllers/actions.github.com/ephemeralrunnerset_controller_test.go index 32e2bb0a..a955ea8f 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller_test.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller_test.go @@ -58,9 +58,9 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { Scheme: mgr.GetScheme(), Log: logf.Log, ResourceBuilder: ResourceBuilder{ - ActionsClientGetter: &ActionsClientSecretResolver{ - Client: mgr.GetClient(), - MultiClient: fake.NewMultiClient(), + ActionsClientPool: &ActionsClientPool{ + k8sClient: mgr.GetClient(), + multiClient: fake.NewMultiClient(), }, }, } @@ -1113,9 +1113,9 @@ var _ = Describe("Test EphemeralRunnerSet controller with proxy settings", func( Scheme: mgr.GetScheme(), Log: logf.Log, ResourceBuilder: ResourceBuilder{ - ActionsClientGetter: &ActionsClientSecretResolver{ - Client: mgr.GetClient(), - MultiClient: actions.NewMultiClient(logr.Discard()), + ActionsClientPool: &ActionsClientPool{ + k8sClient: mgr.GetClient(), + multiClient: actions.NewMultiClient(logr.Discard()), }, }, } @@ -1417,9 +1417,9 @@ var _ = Describe("Test EphemeralRunnerSet controller with custom root CA", func( Scheme: mgr.GetScheme(), Log: logf.Log, ResourceBuilder: ResourceBuilder{ - ActionsClientGetter: &ActionsClientSecretResolver{ - Client: mgr.GetClient(), - MultiClient: actions.NewMultiClient(logr.Discard()), + ActionsClientPool: &ActionsClientPool{ + k8sClient: mgr.GetClient(), + multiClient: actions.NewMultiClient(logr.Discard()), }, }, } diff --git a/controllers/actions.github.com/resourcebuilder.go b/controllers/actions.github.com/resourcebuilder.go index 84158d06..dac23ac4 100644 --- a/controllers/actions.github.com/resourcebuilder.go +++ b/controllers/actions.github.com/resourcebuilder.go @@ -12,8 +12,9 @@ import ( "strings" "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1/appconfig" "github.com/actions/actions-runner-controller/build" - listenerconfig "github.com/actions/actions-runner-controller/cmd/ghalistener/config" + ghalistenerconfig "github.com/actions/actions-runner-controller/cmd/ghalistener/config" "github.com/actions/actions-runner-controller/github/actions" "github.com/actions/actions-runner-controller/hash" "github.com/actions/actions-runner-controller/logging" @@ -72,7 +73,7 @@ func SetListenerEntrypoint(entrypoint string) { type ResourceBuilder struct { ExcludeLabelPropagationPrefixes []string - ActionsClientGetter + *ActionsClientPool } // boolPtr returns a pointer to a bool value @@ -108,6 +109,10 @@ func (b *ResourceBuilder) newAutoScalingListener(autoscalingRunnerSet *v1alpha1. annotationKeyValuesHash: autoscalingRunnerSet.Annotations[annotationKeyValuesHash], } + if v, ok := autoscalingRunnerSet.Annotations[AnnotationKeyGitHubVaultType]; ok { + annotations[AnnotationKeyGitHubVaultType] = v + } + if err := applyGitHubURLLabels(autoscalingRunnerSet.Spec.GitHubConfigUrl, labels); err != nil { return nil, fmt.Errorf("failed to apply GitHub URL labels: %v", err) } @@ -171,21 +176,8 @@ func (b *ResourceBuilder) newScaleSetListenerConfig(autoscalingListener *v1alpha metricsEndpoint = metricsConfig.endpoint } - var appInstallationID int64 - if id, ok := secret.Data["github_app_installation_id"]; ok { - var err error - appInstallationID, err = strconv.ParseInt(string(id), 10, 64) - if err != nil { - return nil, fmt.Errorf("failed to convert github_app_installation_id to int: %v", err) - } - } - - config := listenerconfig.Config{ + config := ghalistenerconfig.Config{ ConfigureUrl: autoscalingListener.Spec.GitHubConfigUrl, - AppID: string(secret.Data["github_app_id"]), - AppInstallationID: appInstallationID, - AppPrivateKey: string(secret.Data["github_app_private_key"]), - Token: string(secret.Data["github_token"]), EphemeralRunnerSetNamespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, EphemeralRunnerSetName: autoscalingListener.Spec.EphemeralRunnerSetName, MaxRunners: autoscalingListener.Spec.MaxRunners, @@ -200,6 +192,17 @@ func (b *ResourceBuilder) newScaleSetListenerConfig(autoscalingListener *v1alpha Metrics: autoscalingListener.Spec.Metrics, } + if ty, ok := autoscalingListener.Annotations[AnnotationKeyGitHubVaultType]; !ok { + appConfig, err := appconfig.FromSecret(secret) + if err != nil { + return nil, fmt.Errorf("failed to parse appconfig from secret: %v", err) + } + config.AppConfig = *appConfig + } else { + config.VaultType = ty + config.VaultLookupKey = autoscalingListener.Spec.GitHubConfigSecret + } + if err := config.Validate(); err != nil { return nil, fmt.Errorf("invalid listener config: %w", err) } @@ -220,7 +223,7 @@ func (b *ResourceBuilder) newScaleSetListenerConfig(autoscalingListener *v1alpha }, nil } -func (b *ResourceBuilder) newScaleSetListenerPod(autoscalingListener *v1alpha1.AutoscalingListener, podConfig *corev1.Secret, serviceAccount *corev1.ServiceAccount, secret *corev1.Secret, metricsConfig *listenerMetricsServerConfig, envs ...corev1.EnvVar) (*corev1.Pod, error) { +func (b *ResourceBuilder) newScaleSetListenerPod(autoscalingListener *v1alpha1.AutoscalingListener, podConfig *corev1.Secret, serviceAccount *corev1.ServiceAccount, metricsConfig *listenerMetricsServerConfig, envs ...corev1.EnvVar) (*corev1.Pod, error) { listenerEnv := []corev1.EnvVar{ { Name: "LISTENER_CONFIG_PATH", @@ -535,6 +538,10 @@ func (b *ResourceBuilder) newEphemeralRunnerSet(autoscalingRunnerSet *v1alpha1.A annotationKeyRunnerSpecHash: runnerSpecHash, } + if v, ok := autoscalingRunnerSet.Annotations[AnnotationKeyGitHubVaultType]; ok { + newAnnotations[AnnotationKeyGitHubVaultType] = v + } + newEphemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ TypeMeta: metav1.TypeMeta{}, ObjectMeta: metav1.ObjectMeta{ @@ -583,6 +590,7 @@ func (b *ResourceBuilder) newEphemeralRunner(ephemeralRunnerSet *v1alpha1.Epheme for key, val := range ephemeralRunnerSet.Annotations { annotations[key] = val } + annotations[AnnotationKeyPatchID] = strconv.Itoa(ephemeralRunnerSet.Spec.PatchID) return &v1alpha1.EphemeralRunner{ TypeMeta: metav1.TypeMeta{}, diff --git a/controllers/actions.github.com/resourcebuilder_test.go b/controllers/actions.github.com/resourcebuilder_test.go index a7dcdf75..12e50533 100644 --- a/controllers/actions.github.com/resourcebuilder_test.go +++ b/controllers/actions.github.com/resourcebuilder_test.go @@ -82,12 +82,7 @@ func TestLabelPropagation(t *testing.T) { Name: "test", }, } - listenerSecret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - }, - } - listenerPod, err := b.newScaleSetListenerPod(listener, &corev1.Secret{}, listenerServiceAccount, listenerSecret, nil) + listenerPod, err := b.newScaleSetListenerPod(listener, &corev1.Secret{}, listenerServiceAccount, nil) require.NoError(t, err) assert.Equal(t, listenerPod.Labels, listener.Labels) diff --git a/github/actions/fake/multi_client.go b/github/actions/fake/multi_client.go index e6625102..62d6a0e5 100644 --- a/github/actions/fake/multi_client.go +++ b/github/actions/fake/multi_client.go @@ -3,6 +3,7 @@ package fake import ( "context" + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1/appconfig" "github.com/actions/actions-runner-controller/github/actions" ) @@ -34,10 +35,6 @@ func NewMultiClient(opts ...MultiClientOption) actions.MultiClient { return f } -func (f *fakeMultiClient) GetClientFor(ctx context.Context, githubConfigURL string, creds actions.ActionsAuth, namespace string, options ...actions.ClientOption) (actions.ActionsService, error) { - return f.defaultClient, f.defaultErr -} - -func (f *fakeMultiClient) GetClientFromSecret(ctx context.Context, githubConfigURL, namespace string, secretData actions.KubernetesSecretData, options ...actions.ClientOption) (actions.ActionsService, error) { +func (f *fakeMultiClient) GetClientFor(ctx context.Context, githubConfigURL string, appConfig *appconfig.AppConfig, namespace string, options ...actions.ClientOption) (actions.ActionsService, error) { return f.defaultClient, f.defaultErr } diff --git a/github/actions/multi_client.go b/github/actions/multi_client.go index f8e16071..cc81e045 100644 --- a/github/actions/multi_client.go +++ b/github/actions/multi_client.go @@ -2,16 +2,14 @@ package actions import ( "context" - "fmt" - "strconv" "sync" + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1/appconfig" "github.com/go-logr/logr" ) type MultiClient interface { - GetClientFor(ctx context.Context, githubConfigURL string, creds ActionsAuth, namespace string, options ...ClientOption) (ActionsService, error) - GetClientFromSecret(ctx context.Context, githubConfigURL, namespace string, secretData KubernetesSecretData, options ...ClientOption) (ActionsService, error) + GetClientFor(ctx context.Context, githubConfigURL string, appConfig *appconfig.AppConfig, namespace string, options ...ClientOption) (ActionsService, error) } type multiClient struct { @@ -50,15 +48,22 @@ func NewMultiClient(logger logr.Logger) MultiClient { } } -func (m *multiClient) GetClientFor(ctx context.Context, githubConfigURL string, creds ActionsAuth, namespace string, options ...ClientOption) (ActionsService, error) { +func (m *multiClient) GetClientFor(ctx context.Context, githubConfigURL string, appConfig *appconfig.AppConfig, namespace string, options ...ClientOption) (ActionsService, error) { m.logger.Info("retrieve actions client", "githubConfigURL", githubConfigURL, "namespace", namespace) - if creds.Token == "" && creds.AppCreds == nil { - return nil, fmt.Errorf("no credentials provided. either a PAT or GitHub App credentials should be provided") + if err := appConfig.Validate(); err != nil { + return nil, err } - if creds.Token != "" && creds.AppCreds != nil { - return nil, fmt.Errorf("both PAT and GitHub App credentials provided. should only provide one") + var creds ActionsAuth + if len(appConfig.Token) > 0 { + creds.Token = appConfig.Token + } else { + creds.AppCreds = &GitHubAppAuth{ + AppID: appConfig.AppID, + AppInstallationID: appConfig.AppInstallationID, + AppPrivateKey: appConfig.AppPrivateKey, + } } client, err := NewClient( @@ -94,42 +99,3 @@ func (m *multiClient) GetClientFor(ctx context.Context, githubConfigURL string, return client, nil } - -type KubernetesSecretData map[string][]byte - -func (m *multiClient) GetClientFromSecret(ctx context.Context, githubConfigURL, namespace string, secretData KubernetesSecretData, options ...ClientOption) (ActionsService, error) { - if len(secretData) == 0 { - return nil, fmt.Errorf("must provide secret data with either PAT or GitHub App Auth") - } - - token := string(secretData["github_token"]) - hasToken := len(token) > 0 - - appID := string(secretData["github_app_id"]) - appInstallationID := string(secretData["github_app_installation_id"]) - appPrivateKey := string(secretData["github_app_private_key"]) - hasGitHubAppAuth := len(appID) > 0 && len(appInstallationID) > 0 && len(appPrivateKey) > 0 - - if hasToken && hasGitHubAppAuth { - return nil, fmt.Errorf("must provide secret with only PAT or GitHub App Auth to avoid ambiguity in client behavior") - } - - if !hasToken && !hasGitHubAppAuth { - return nil, fmt.Errorf("neither PAT nor GitHub App Auth credentials provided in secret") - } - - auth := ActionsAuth{} - - if hasToken { - auth.Token = token - return m.GetClientFor(ctx, githubConfigURL, auth, namespace, options...) - } - - parsedAppInstallationID, err := strconv.ParseInt(appInstallationID, 10, 64) - if err != nil { - return nil, err - } - - auth.AppCreds = &GitHubAppAuth{AppID: appID, AppInstallationID: parsedAppInstallationID, AppPrivateKey: appPrivateKey} - return m.GetClientFor(ctx, githubConfigURL, auth, namespace, options...) -} diff --git a/github/actions/multi_client_test.go b/github/actions/multi_client_test.go index 57589857..95a65572 100644 --- a/github/actions/multi_client_test.go +++ b/github/actions/multi_client_test.go @@ -5,6 +5,7 @@ import ( "fmt" "testing" + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1/appconfig" "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -23,10 +24,13 @@ func TestMultiClientCaching(t *testing.T) { defaultNamespace := "default" defaultConfigURL := "https://github.com/org/repo" - defaultCreds := &ActionsAuth{ + defaultCreds := &appconfig.AppConfig{ Token: "token", } - client, err := NewClient(defaultConfigURL, defaultCreds) + defaultAuth := ActionsAuth{ + Token: defaultCreds.Token, + } + client, err := NewClient(defaultConfigURL, &defaultAuth) require.NoError(t, err) multiClient.clients[ActionsClientKey{client.Identifier(), defaultNamespace}] = client @@ -35,7 +39,7 @@ func TestMultiClientCaching(t *testing.T) { cachedClient, err := multiClient.GetClientFor( ctx, defaultConfigURL, - *defaultCreds, + defaultCreds, defaultNamespace, ) require.NoError(t, err) @@ -47,7 +51,7 @@ func TestMultiClientCaching(t *testing.T) { newClient, err := multiClient.GetClientFor( ctx, defaultConfigURL, - *defaultCreds, + defaultCreds, otherNamespace, ) require.NoError(t, err) @@ -63,7 +67,7 @@ func TestMultiClientOptions(t *testing.T) { defaultConfigURL := "https://github.com/org/repo" t.Run("GetClientFor", func(t *testing.T) { - defaultCreds := &ActionsAuth{ + defaultCreds := &appconfig.AppConfig{ Token: "token", } @@ -71,7 +75,7 @@ func TestMultiClientOptions(t *testing.T) { service, err := multiClient.GetClientFor( ctx, defaultConfigURL, - *defaultCreds, + defaultCreds, defaultNamespace, ) service.SetUserAgent(testUserAgent) @@ -83,27 +87,6 @@ func TestMultiClientOptions(t *testing.T) { require.NoError(t, err) assert.Equal(t, testUserAgent.String(), req.Header.Get("User-Agent")) }) - - t.Run("GetClientFromSecret", func(t *testing.T) { - secret := map[string][]byte{ - "github_token": []byte("token"), - } - - multiClient := NewMultiClient(logger) - service, err := multiClient.GetClientFromSecret( - ctx, - defaultConfigURL, - defaultNamespace, - secret, - ) - service.SetUserAgent(testUserAgent) - require.NoError(t, err) - - client := service.(*Client) - req, err := client.NewGitHubAPIRequest(ctx, "GET", "/test", nil) - require.NoError(t, err) - assert.Equal(t, testUserAgent.String(), req.Header.Get("User-Agent")) - }) } func TestCreateJWT(t *testing.T) { diff --git a/go.mod b/go.mod index 47da2893..42fbf620 100644 --- a/go.mod +++ b/go.mod @@ -4,8 +4,8 @@ go 1.24.3 require ( github.com/Azure/azure-sdk-for-go/sdk/azcore v1.17.0 - github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.8.0 - github.com/Azure/azure-sdk-for-go/sdk/keyvault/azsecrets v0.12.0 + github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.8.1 + github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets v1.3.0 github.com/bradleyfalzon/ghinstallation/v2 v2.14.0 github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc github.com/evanphx/json-patch v5.9.11+incompatible @@ -43,8 +43,8 @@ require ( require ( filippo.io/edwards25519 v1.1.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/internal v1.10.0 // indirect - github.com/Azure/azure-sdk-for-go/sdk/keyvault/internal v0.7.1 // indirect - github.com/AzureAD/microsoft-authentication-library-for-go v1.2.2 // indirect + github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/internal v1.1.0 // indirect + github.com/AzureAD/microsoft-authentication-library-for-go v1.3.2 // indirect github.com/BurntSushi/toml v1.4.0 // indirect github.com/ProtonMail/go-crypto v1.1.6 // indirect github.com/aws/aws-sdk-go-v2 v1.36.3 // indirect diff --git a/go.sum b/go.sum index c3e46c6c..f676aaa4 100644 --- a/go.sum +++ b/go.sum @@ -3,20 +3,20 @@ filippo.io/edwards25519 v1.1.0/go.mod h1:BxyFTGdWcka3PhytdK4V28tE5sGfRvvvRV7EaN4 github.com/Azure/azure-sdk-for-go v51.0.0+incompatible h1:p7blnyJSjJqf5jflHbSGhIhEpXIgIFmYZNg5uwqweso= github.com/Azure/azure-sdk-for-go/sdk/azcore v1.17.0 h1:g0EZJwz7xkXQiZAI5xi9f3WWFYBlX1CPTrR+NDToRkQ= github.com/Azure/azure-sdk-for-go/sdk/azcore v1.17.0/go.mod h1:XCW7KnZet0Opnr7HccfUw1PLc4CjHqpcaxW8DHklNkQ= -github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.8.0 h1:B/dfvscEQtew9dVuoxqxrUKKv8Ih2f55PydknDamU+g= -github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.8.0/go.mod h1:fiPSssYvltE08HJchL04dOy+RD4hgrjph0cwGGMntdI= -github.com/Azure/azure-sdk-for-go/sdk/azidentity/cache v0.3.0 h1:+m0M/LFxN43KvULkDNfdXOgrjtg6UYJPFBJyuEcRCAw= -github.com/Azure/azure-sdk-for-go/sdk/azidentity/cache v0.3.0/go.mod h1:PwOyop78lveYMRs6oCxjiVyBdyCgIYH6XHIVZO9/SFQ= +github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.8.1 h1:1mvYtZfWQAnwNah/C+Z+Jb9rQH95LPE2vlmMuWAHJk8= +github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.8.1/go.mod h1:75I/mXtme1JyWFtz8GocPHVFyH421IBoZErnO16dd0k= +github.com/Azure/azure-sdk-for-go/sdk/azidentity/cache v0.3.1 h1:Bk5uOhSAenHyR5P61D/NzeQCv+4fEVV8mOkJ82NqpWw= +github.com/Azure/azure-sdk-for-go/sdk/azidentity/cache v0.3.1/go.mod h1:QZ4pw3or1WPmRBxf0cHd1tknzrT54WPBOQoGutCPvSU= github.com/Azure/azure-sdk-for-go/sdk/internal v1.10.0 h1:ywEEhmNahHBihViHepv3xPBn1663uRv2t2q/ESv9seY= github.com/Azure/azure-sdk-for-go/sdk/internal v1.10.0/go.mod h1:iZDifYGJTIgIIkYRNWPENUnqx6bJ2xnSDFI2tjwZNuY= -github.com/Azure/azure-sdk-for-go/sdk/keyvault/azsecrets v0.12.0 h1:xnO4sFyG8UH2fElBkcqLTOZsAajvKfnSlgBBW8dXYjw= -github.com/Azure/azure-sdk-for-go/sdk/keyvault/azsecrets v0.12.0/go.mod h1:XD3DIOOVgBCO03OleB1fHjgktVRFxlT++KwKgIOewdM= -github.com/Azure/azure-sdk-for-go/sdk/keyvault/internal v0.7.1 h1:FbH3BbSb4bvGluTesZZ+ttN/MDsnMmQP36OSnDuSXqw= -github.com/Azure/azure-sdk-for-go/sdk/keyvault/internal v0.7.1/go.mod h1:9V2j0jn9jDEkCkv8w/bKTNppX/d0FVA1ud77xCIP4KA= +github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets v1.3.0 h1:WLUIpeyv04H0RCcQHaA4TNoyrQ39Ox7V+re+iaqzTe0= +github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets v1.3.0/go.mod h1:hd8hTTIY3VmUVPRHNH7GVCHO3SHgXkJKZHReby/bnUQ= +github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/internal v1.1.0 h1:eXnN9kaS8TiDwXjoie3hMRLuwdUBUMW9KRgOqB3mCaw= +github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/internal v1.1.0/go.mod h1:XIpam8wumeZ5rVMuhdDQLMfIPDf1WO3IzrCRO3e3e3o= github.com/AzureAD/microsoft-authentication-extensions-for-go/cache v0.1.1 h1:WJTmL004Abzc5wDB5VtZG2PJk5ndYDgVacGqfirKxjM= github.com/AzureAD/microsoft-authentication-extensions-for-go/cache v0.1.1/go.mod h1:tCcJZ0uHAmvjsVYzEFivsRTN00oz5BEsRgQHu5JZ9WE= -github.com/AzureAD/microsoft-authentication-library-for-go v1.2.2 h1:XHOnouVk1mxXfQidrMEnLlPk9UMeRtyBTnEFtxkV0kU= -github.com/AzureAD/microsoft-authentication-library-for-go v1.2.2/go.mod h1:wP83P5OoQ5p6ip3ScPr0BAq0BvuPAvacpEuSzyouqAI= +github.com/AzureAD/microsoft-authentication-library-for-go v1.3.2 h1:kYRSnvJju5gYVyhkij+RTJ/VR6QIUaCfWeaFm2ycsjQ= +github.com/AzureAD/microsoft-authentication-library-for-go v1.3.2/go.mod h1:wP83P5OoQ5p6ip3ScPr0BAq0BvuPAvacpEuSzyouqAI= github.com/BurntSushi/toml v1.4.0 h1:kuoIxZQy2WRRk1pttg9asf+WVv6tWQuBNVmK8+nqPr0= github.com/BurntSushi/toml v1.4.0/go.mod h1:ukJfTF/6rtPPRCnwkur4qwRxa8vTRFBF0uk2lLoLwho= github.com/ProtonMail/go-crypto v1.1.6 h1:ZcV+Ropw6Qn0AX9brlQLAUXfqLBc7Bl+f/DmNxpLfdw= @@ -309,8 +309,8 @@ github.com/prometheus/common v0.62.0 h1:xasJaQlnWAeyHdUBeGjXmutelfJHWMRr+Fg4QszZ github.com/prometheus/common v0.62.0/go.mod h1:vyBcEuLSvWos9B1+CyL7JZ2up+uFzXhkqml0W5zIY1I= github.com/prometheus/procfs v0.15.1 h1:YagwOFzUgYfKKHX6Dr+sHT7km/hxC76UB0learggepc= github.com/prometheus/procfs v0.15.1/go.mod h1:fB45yRUv8NstnjriLhBQLuOUt+WW4BsoGhij/e3PBqk= -github.com/redis/go-redis/v9 v9.6.1 h1:HHDteefn6ZkTtY5fGUE8tj8uy85AHk6zP7CpzIAM0y4= -github.com/redis/go-redis/v9 v9.6.1/go.mod h1:0C0c6ycQsdpVNQpxb1njEQIqkx5UcsM8FJCQLgE9+RA= +github.com/redis/go-redis/v9 v9.7.0 h1:HhLSs+B6O021gwzl+locl0zEDnyNkxMtf/Z3NNBMa9E= +github.com/redis/go-redis/v9 v9.7.0/go.mod h1:f6zhXITC7JUJIlPEiBOTXxJgPLdZcA93GewI7inzyWw= github.com/rogpeppe/go-internal v1.12.0 h1:exVL4IDcn6na9z1rAb56Vxr+CgyK3nn3O+epU5NdKM8= github.com/rogpeppe/go-internal v1.12.0/go.mod h1:E+RYuTGaKKdloAfM02xzb0FW3Paa99yedzYV+kq4uf4= github.com/russross/blackfriday/v2 v2.1.0 h1:JIOH55/0cWyOuilr9/qlrm0BSXldqnqwMsf35Ld67mk= diff --git a/main.go b/main.go index f30c71b5..7fb076c3 100644 --- a/main.go +++ b/main.go @@ -33,7 +33,6 @@ import ( "github.com/actions/actions-runner-controller/github/actions" "github.com/actions/actions-runner-controller/logging" "github.com/actions/actions-runner-controller/vault" - "github.com/actions/actions-runner-controller/vault/azurekeyvault" "github.com/kelseyhightower/envconfig" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" @@ -276,20 +275,30 @@ func main() { log.WithName("actions-clients"), ) - actionsClientGetter, err := newActionsClientGetter( - mgr.GetClient(), - actionsMultiClient, - ) + vaults, err := vault.InitAll("CONTROLLER_MANAGER_") if err != nil { - log.Error(err, "unable to create actions client resolver") + log.Error(err, "unable to read vaults") os.Exit(1) } + var poolOptions []actionsgithubcom.ActionsClientPoolOption + for name, vault := range vaults { + poolOptions = append(poolOptions, actionsgithubcom.WithVault(name, vault)) + } + + clientPool := actionsgithubcom.NewActionsClientPool( + mgr.GetClient(), + actionsMultiClient, + poolOptions..., + ) + rb := actionsgithubcom.ResourceBuilder{ ExcludeLabelPropagationPrefixes: excludeLabelPropagationPrefixes, - ActionsClientGetter: actionsClientGetter, + ActionsClientPool: clientPool, } + log.Info("Resource builder initializing") + if err = (&actionsgithubcom.AutoscalingRunnerSetReconciler{ Client: mgr.GetClient(), Log: log.WithName("AutoscalingRunnerSet").WithValues("version", build.Version), @@ -309,7 +318,6 @@ func main() { Client: mgr.GetClient(), Log: log.WithName("EphemeralRunner").WithValues("version", build.Version), Scheme: mgr.GetScheme(), - ActionsClient: actionsMultiClient, ResourceBuilder: rb, }).SetupWithManager(mgr, actionsgithubcom.WithMaxConcurrentReconciles(opts.RunnerMaxConcurrentReconciles)); err != nil { log.Error(err, "unable to create controller", "controller", "EphemeralRunner") @@ -503,32 +511,3 @@ func (s *commaSeparatedStringSlice) Set(value string) error { } return nil } - -func newActionsClientGetter(k8sClient client.Client, multiClient actions.MultiClient) (actionsgithubcom.ActionsClientGetter, error) { - vaultType := os.Getenv("CONTROLLER_MANAGER_VAULT_TYPE") - if vaultType == "" { - return &actionsgithubcom.ActionsClientSecretResolver{ - Client: k8sClient, - MultiClient: multiClient, - }, nil - } - - key := os.Getenv("CONTROLLER_MANAGER_VAULT_API_KEY") - var vault vault.Vault - switch vaultType { - case "azure": - v, err := azurekeyvault.New(azurekeyvault.Config{JWT: key}) - if err != nil { - return nil, fmt.Errorf("failed to create Azure Key Vault client: %w", err) - } - - vault = v - default: - return nil, fmt.Errorf("unsupported vault type: %q", vaultType) - } - - return &actionsgithubcom.ActionsClientVaultResolver{ - Vault: vault, - MultiClient: multiClient, - }, nil -} diff --git a/proxyconfig/proxyconfig.go b/proxyconfig/proxyconfig.go new file mode 100644 index 00000000..a2c93c0d --- /dev/null +++ b/proxyconfig/proxyconfig.go @@ -0,0 +1,129 @@ +package proxyconfig + +import ( + "fmt" + "net/url" + "os" + "strings" + + "golang.org/x/net/http/httpproxy" +) + +type ProxyConfig struct { + HTTP *ProxyServerConfig `json:"http,omitempty"` + HTTPS *ProxyServerConfig `json:"https,omitempty"` + NoProxy []string `json:"no_proxy,omitempty"` +} + +func (pc *ProxyConfig) Validate() error { + if pc == nil { + return nil + } + + if pc.HTTP != nil { + _, err := url.Parse(pc.HTTP.URL) + if err != nil { + return fmt.Errorf("proxy http set with invalid url: %v", err) + } + } + if pc.HTTPS != nil { + _, err := url.Parse(pc.HTTPS.URL) + if err != nil { + return fmt.Errorf("proxy https set with invalid url: %v", err) + } + } + + // TODO: maybe validate noproxy? + return nil +} + +func (c *ProxyConfig) ProxyConfig() (*httpproxy.Config, error) { + if c == nil { + return nil, nil + } + config := &httpproxy.Config{ + NoProxy: strings.Join(c.NoProxy, ","), + } + if c.HTTP != nil { + u, err := c.HTTP.proxyURL() + if err != nil { + return nil, fmt.Errorf("failed to create proxy http url: %w", err) + } + config.HTTPProxy = u.String() + } + + if c.HTTPS != nil { + u, err := c.HTTPS.proxyURL() + if err != nil { + return nil, fmt.Errorf("failed to create proxy https url: %w", err) + } + config.HTTPSProxy = u.String() + } + + return config, nil +} + +type ProxyServerConfig struct { + URL string `json:"url"` + Username string `json:"username"` + Password string `json:"password"` +} + +func (c *ProxyServerConfig) proxyURL() (*url.URL, error) { + u, err := url.Parse(c.URL) + if err != nil { + return nil, fmt.Errorf("failed to parse proxy url %q: %w", c.URL, err) + } + + u.User = url.UserPassword( + c.Username, + c.Password, + ) + + return u, nil +} + +func ReadFromEnv(prefix string) (*ProxyConfig, error) { + url := os.Getenv(prefix + "HTTP_URL") + username := os.Getenv(prefix + "HTTP_USERNAME") + password := os.Getenv(prefix + "HTTP_PASSWORD") + + var http *ProxyServerConfig + if url != "" || username != "" || password != "" { + http = &ProxyServerConfig{ + URL: url, + Username: username, + Password: password, + } + } + + url = os.Getenv(prefix + "HTTPS_URL") + username = os.Getenv(prefix + "HTTPS_USERNAME") + password = os.Getenv(prefix + "HTTPS_PASSWORD") + + var https *ProxyServerConfig + if url != "" || username != "" || password != "" { + https = &ProxyServerConfig{ + URL: url, + Username: username, + Password: password, + } + } + + noProxyRaw := os.Getenv(prefix + "NO_PROXY") + + if http == nil && https == nil && noProxyRaw == "" { + return nil, nil + } + + var noProxy []string + if noProxyRaw != "" { + noProxy = strings.Split(noProxyRaw, ",") + } + + return &ProxyConfig{ + HTTP: http, + HTTPS: https, + NoProxy: noProxy, + }, nil +} diff --git a/proxyconfig/proxyconfig_test.go b/proxyconfig/proxyconfig_test.go new file mode 100644 index 00000000..6d71c434 --- /dev/null +++ b/proxyconfig/proxyconfig_test.go @@ -0,0 +1,104 @@ +package proxyconfig + +import ( + "os" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type kv struct { + key string + value string +} + +func TestReadFromEnvNoPrefix(t *testing.T) { + var ( + url = "example.com" + username = "user" + password = "password" + + noProxy = "test.com,other.com" + ) + tt := map[string]struct { + envs []*kv + want *ProxyConfig + }{ + "no envs": {}, + "http only": { + envs: []*kv{ + {"HTTP_URL", url}, + {"HTTP_USERNAME", username}, + {"HTTP_PASSWORD", password}, + }, + want: &ProxyConfig{ + HTTP: &ProxyServerConfig{ + URL: url, + Username: username, + Password: password, + }, + }, + }, + "https only": { + envs: []*kv{ + {"HTTPS_URL", url}, + {"HTTPS_USERNAME", username}, + {"HTTPS_PASSWORD", password}, + }, + want: &ProxyConfig{ + HTTPS: &ProxyServerConfig{ + URL: url, + Username: username, + Password: password, + }, + }, + }, + "no proxy only": { + envs: []*kv{ + {"NO_PROXY", noProxy}, + }, + want: &ProxyConfig{ + NoProxy: strings.Split(noProxy, ","), + }, + }, + "all set": { + envs: []*kv{ + {"HTTP_URL", url}, + {"HTTP_USERNAME", username}, + {"HTTP_PASSWORD", password}, + {"HTTPS_URL", url}, + {"HTTPS_USERNAME", username}, + {"HTTPS_PASSWORD", password}, + {"NO_PROXY", noProxy}, + }, + want: &ProxyConfig{ + HTTP: &ProxyServerConfig{ + URL: url, + Username: username, + Password: password, + }, + HTTPS: &ProxyServerConfig{ + URL: url, + Username: username, + Password: password, + }, + NoProxy: strings.Split(noProxy, ","), + }, + }, + } + + for name, tc := range tt { + t.Run(name, func(t *testing.T) { + os.Clearenv() + for _, kv := range tc.envs { + os.Setenv(kv.key, kv.value) + } + + got, err := ReadFromEnv("") + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} diff --git a/test_e2e_arc/arc_jobs_test.go b/test_e2e_arc/arc_jobs_test.go index 39682c87..04991e44 100644 --- a/test_e2e_arc/arc_jobs_test.go +++ b/test_e2e_arc/arc_jobs_test.go @@ -126,7 +126,6 @@ func TestARCJobs(t *testing.T) { if !success { t.Fatal("Expected pods count did not match available pods count during job run.") } - }, ) t.Run("Get available pods after job run", func(t *testing.T) { diff --git a/vault/azurekeyvault/azurekeyvault.go b/vault/azurekeyvault/azurekeyvault.go index 02846780..ee162950 100644 --- a/vault/azurekeyvault/azurekeyvault.go +++ b/vault/azurekeyvault/azurekeyvault.go @@ -3,50 +3,58 @@ package azurekeyvault import ( "context" "fmt" + "os" - "github.com/Azure/azure-sdk-for-go/sdk/azcore" - "github.com/Azure/azure-sdk-for-go/sdk/azidentity" - "github.com/Azure/azure-sdk-for-go/sdk/keyvault/azsecrets" + "github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets" + "github.com/actions/actions-runner-controller/proxyconfig" ) +// AzureKeyVault is a struct that holds the Azure Key Vault client. type AzureKeyVault struct { client *azsecrets.Client } -type Config struct { - ClientID string - TenantID string - JWT string - URL string -} - -func (c *Config) getAssertion(ctx context.Context) (string, error) { - return c.JWT, nil -} - func New(cfg Config) (*AzureKeyVault, error) { - cred, err := azidentity.NewClientAssertionCredential( - cfg.TenantID, - cfg.ClientID, - cfg.getAssertion, - &azidentity.ClientAssertionCredentialOptions{ - ClientOptions: azcore.ClientOptions{}, - }, - ) - if err != nil { - return nil, fmt.Errorf("failed to create client assertion credential: %w", err) + if err := cfg.Validate(); err != nil { + return nil, fmt.Errorf("failed to validate config: %v", err) } - client, err := azsecrets.NewClient(cfg.URL, cred, nil) + client, err := cfg.Client() if err != nil { - return nil, fmt.Errorf("failed to initialize keyvault client: %w", err) + return nil, fmt.Errorf("failed to create azsecrets client from config: %v", err) } return &AzureKeyVault{client: client}, nil } -func (v *AzureKeyVault) GetSecret(ctx context.Context, name, version string) (string, error) { - secret, err := v.client.GetSecret(context.Background(), name, version, nil) +// FromEnv creates a new AzureKeyVault instance from environment variables. +// The environment variables should be prefixed with the provided prefix. +// For example, if the prefix is "AZURE_KEY_VAULT_", the environment variables should be: +// AZURE_KEY_VAULT_TENANT_ID, AZURE_KEY_VAULT_CLIENT_ID, AZURE_KEY_VAULT_URL, +// AZURE_KEY_VAULT_CERT_PATH, AZURE_KEY_VAULT_CERT_PASSWORD. +// The proxy configuration can be set using the environment variables prefixed with "PROXY_". +// For example, AZURE_KEY_VAULT_PROXY_HTTP_URL, AZURE_KEY_VAULT_PROXY_HTTP_USERNAME, etc. +func FromEnv(prefix string) (*AzureKeyVault, error) { + cfg := Config{ + TenantID: os.Getenv(prefix + "TENANT_ID"), + ClientID: os.Getenv(prefix + "CLIENT_ID"), + URL: os.Getenv(prefix + "URL"), + CertPath: os.Getenv(prefix + "CERT_PATH"), + CertPassword: os.Getenv(prefix + "CERT_PASSWORD"), + } + + proxyConfig, err := proxyconfig.ReadFromEnv(prefix + "PROXY_") + if err != nil { + return nil, fmt.Errorf("failed to read proxy config: %v", err) + } + cfg.Proxy = proxyConfig + + return New(cfg) +} + +// GetSecret retrieves a secret from Azure Key Vault. +func (v *AzureKeyVault) GetSecret(ctx context.Context, name string) (string, error) { + secret, err := v.client.GetSecret(ctx, name, "", nil) if err != nil { return "", fmt.Errorf("failed to get secret: %w", err) } diff --git a/vault/azurekeyvault/config.go b/vault/azurekeyvault/config.go new file mode 100644 index 00000000..8cbfc90a --- /dev/null +++ b/vault/azurekeyvault/config.go @@ -0,0 +1,119 @@ +package azurekeyvault + +import ( + "errors" + "fmt" + "net/http" + "net/url" + "os" + "time" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" + "github.com/Azure/azure-sdk-for-go/sdk/azidentity" + "github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets" + "github.com/actions/actions-runner-controller/proxyconfig" + "github.com/hashicorp/go-retryablehttp" +) + +// AzureKeyVault is a struct that holds the Azure Key Vault client. +type Config struct { + TenantID string `json:"tenant_id"` + ClientID string `json:"client_id"` + URL string `json:"url"` + CertPath string `json:"cert_path"` + CertPassword string `json:"cert_password"` // optional + Proxy *proxyconfig.ProxyConfig `json:"proxy,omitempty"` +} + +func (c *Config) Validate() error { + if c.TenantID == "" { + return errors.New("tenant_id is not set") + } + if c.ClientID == "" { + return errors.New("client_id is not set") + } + if _, err := url.Parse(c.URL); err != nil { + return fmt.Errorf("failed to parse url: %v", err) + } + + if c.CertPath != "" { + return errors.New("cert path must be provided") + } + + if err := c.Proxy.Validate(); err != nil { + return fmt.Errorf("proxy validation failed: %v", err) + } + + return nil +} + +// Client creates a new Azure Key Vault client using the provided configuration. +func (c *Config) Client() (*azsecrets.Client, error) { + return c.certClient() +} + +func (c *Config) certClient() (*azsecrets.Client, error) { + data, err := os.ReadFile(c.CertPath) + if err != nil { + return nil, fmt.Errorf("failed to read cert file from path %q: %v", c.CertPath, err) + } + + certs, key, err := azidentity.ParseCertificates(data, []byte(c.CertPassword)) + if err != nil { + return nil, fmt.Errorf("failed to parse certificates: %w", err) + } + + httpClient, err := c.httpClient() + if err != nil { + return nil, fmt.Errorf("failed to instantiate http client: %v", err) + } + + cred, err := azidentity.NewClientCertificateCredential( + c.TenantID, + c.ClientID, + certs, + key, + &azidentity.ClientCertificateCredentialOptions{ + ClientOptions: policy.ClientOptions{ + Transport: httpClient, + }, + }, + ) + if err != nil { + return nil, fmt.Errorf("failed to create client certificate credential: %v", err) + } + + client, err := azsecrets.NewClient(c.URL, cred, &azsecrets.ClientOptions{ + ClientOptions: policy.ClientOptions{ + Transport: httpClient, + }, + }) + if err != nil { + return nil, fmt.Errorf("failed to instantiate client for azsecrets: %v", err) + } + + return client, nil +} + +func (c *Config) httpClient() (*http.Client, error) { + retryClient := retryablehttp.NewClient() + retryClient.RetryMax = 4 + retryClient.RetryWaitMax = 30 * time.Second + retryClient.HTTPClient.Timeout = 5 * time.Minute + + transport, ok := retryClient.HTTPClient.Transport.(*http.Transport) + if !ok { + return nil, fmt.Errorf("failed to get http transport") + } + if c.Proxy != nil { + pc, err := c.Proxy.ProxyConfig() + if err != nil { + return nil, fmt.Errorf("failed to create proxy config: %v", err) + } + transport.Proxy = func(req *http.Request) (*url.URL, error) { + return pc.ProxyFunc()(req.URL) + } + } + + return retryClient.StandardClient(), nil +} diff --git a/vault/azurekeyvault/config_test.go b/vault/azurekeyvault/config_test.go new file mode 100644 index 00000000..bb7cd5db --- /dev/null +++ b/vault/azurekeyvault/config_test.go @@ -0,0 +1,159 @@ +package azurekeyvault + +import ( + "os" + "testing" + + "github.com/actions/actions-runner-controller/proxyconfig" + "github.com/stretchr/testify/require" +) + +func TestConfigValidate_invalid(t *testing.T) { + tenantID := "tenantID" + clientID := "clientID" + url := "https://example.com" + + cp, err := os.CreateTemp("", "") + require.NoError(t, err) + err = cp.Close() + require.NoError(t, err) + certPath := cp.Name() + + t.Cleanup(func() { + os.Remove(certPath) + }) + + proxy := &proxyconfig.ProxyConfig{ + HTTP: &proxyconfig.ProxyServerConfig{ + URL: "http://httpconfig.com", + Username: "user", + Password: "pass", + }, + HTTPS: &proxyconfig.ProxyServerConfig{ + URL: "https://httpsconfig.com", + Username: "user", + Password: "pass", + }, + NoProxy: []string{ + "http://noproxy.com", + }, + } + + tt := map[string]*Config{ + "empty": {}, + "no tenant id": { + TenantID: "", + ClientID: clientID, + URL: url, + CertPath: certPath, + CertPassword: "", + Proxy: proxy, + }, + "no client id": { + TenantID: tenantID, + ClientID: "", + URL: url, + CertPath: certPath, + CertPassword: "", + Proxy: proxy, + }, + "no url": { + TenantID: tenantID, + ClientID: clientID, + URL: "", + CertPath: certPath, + CertPassword: "", + Proxy: proxy, + }, + "no jwt and no cert path": { + TenantID: tenantID, + ClientID: clientID, + URL: url, + CertPath: "", + CertPassword: "", + Proxy: proxy, + }, + "invalid proxy": { + TenantID: tenantID, + ClientID: clientID, + URL: url, + CertPath: certPath, + CertPassword: "", + Proxy: &proxyconfig.ProxyConfig{ + HTTP: &proxyconfig.ProxyServerConfig{}, + }, + }, + } + + for name, cfg := range tt { + t.Run(name, func(t *testing.T) { + err := cfg.Validate() + require.Error(t, err) + }) + } +} + +func TestValidate_valid(t *testing.T) { + tenantID := "tenantID" + clientID := "clientID" + url := "https://example.com" + + cp, err := os.CreateTemp("", "") + require.NoError(t, err) + err = cp.Close() + require.NoError(t, err) + certPath := cp.Name() + + t.Cleanup(func() { + os.Remove(certPath) + }) + + proxy := &proxyconfig.ProxyConfig{ + HTTP: &proxyconfig.ProxyServerConfig{ + URL: "http://httpconfig.com", + Username: "user", + Password: "pass", + }, + HTTPS: &proxyconfig.ProxyServerConfig{ + URL: "https://httpsconfig.com", + Username: "user", + Password: "pass", + }, + NoProxy: []string{ + "http://noproxy.com", + }, + } + + tt := map[string]*Config{ + "with jwt": { + TenantID: tenantID, + ClientID: clientID, + URL: url, + CertPath: "", + CertPassword: "", + Proxy: proxy, + }, + "with cert": { + TenantID: tenantID, + ClientID: clientID, + URL: url, + CertPath: certPath, + CertPassword: "", + Proxy: proxy, + }, + "without proxy": { + TenantID: tenantID, + ClientID: clientID, + URL: url, + CertPath: certPath, + CertPassword: "", + }, + } + + for name, cfg := range tt { + t.Run(name, func(t *testing.T) { + err := cfg.Validate() + require.NoError(t, err) + }) + } +} diff --git a/vault/vault.go b/vault/vault.go index c0fc788e..c5f11c10 100644 --- a/vault/vault.go +++ b/vault/vault.go @@ -1,7 +1,49 @@ package vault -import "context" +import ( + "context" + "fmt" + "os" + "strings" + "github.com/actions/actions-runner-controller/vault/azurekeyvault" +) + +// Vault is the interface every vault implementation needs to adhere to type Vault interface { - GetSecret(ctx context.Context, name, version string) (string, error) + GetSecret(ctx context.Context, name string) (string, error) +} + +// VaultType is the type of vault supported +const ( + VaultTypeAzureKeyVault = "azure_key_vault" +) + +// Compile-time checks +var _ Vault = (*azurekeyvault.AzureKeyVault)(nil) + +// InitAll initializes all vaults based on the environment variables +// that start with the given prefix. It returns a map of vault types to their +// corresponding vault instances. +// +// Prefix is the namespace prefix used to filter environment variables. +// For example, the listener environment variable are prefixed with "LISTENER_", followed by the vault type, followed by the value. +// +// For example, listener has prefix "LISTENER_", has "AZURE_KEY_VAULT_" configured, +// and should read the vault URL. The environment variable will be "LISTENER_AZURE_KEY_VAULT_URL". +func InitAll(prefix string) (map[string]Vault, error) { + envs := os.Environ() + + result := make(map[string]Vault) + for _, env := range envs { + if strings.HasPrefix(env, prefix+"AZURE_KEY_VAULT_") { + akv, err := azurekeyvault.FromEnv(prefix + "AZURE_KEY_VAULT_") + if err != nil { + return nil, fmt.Errorf("failed to instantiate azure key vault from env: %v", err) + } + result[VaultTypeAzureKeyVault] = akv + } + } + + return result, nil } diff --git a/vault/vault_test.go b/vault/vault_test.go new file mode 100644 index 00000000..98d6c3fb --- /dev/null +++ b/vault/vault_test.go @@ -0,0 +1,34 @@ +package vault_test + +import ( + "os" + "testing" + + "github.com/actions/actions-runner-controller/vault" + "github.com/actions/actions-runner-controller/vault/azurekeyvault" + "github.com/stretchr/testify/require" +) + +func TestInitAll_AzureKeyVault(t *testing.T) { + os.Clearenv() + os.Setenv("LISTENER_AZURE_KEY_VAULT_TENANT_ID", "tenantID") + os.Setenv("LISTENER_AZURE_KEY_VAULT_CLIENT_ID", "clientID") + os.Setenv("LISTENER_AZURE_KEY_VAULT_URL", "https://example.com") + os.Setenv("LISTENER_AZURE_KEY_VAULT_CERT_PATH", "/path/to/cert") + os.Setenv("LISTENER_AZURE_KEY_VAULT_CERT_PASSWORD", "password") + os.Setenv("LISTENER_AZURE_KEY_VAULT_PROXY_HTTP_URL", "http://proxy.example.com") + os.Setenv("LISTENER_AZURE_KEY_VAULT_PROXY_HTTP_USERNAME", "username") + os.Setenv("LISTENER_AZURE_KEY_VAULT_PROXY_HTTP_PASSWORD", "password") + os.Setenv("LISTENER_AZURE_KEY_VAULT_PROXY_HTTPS_URL", "https://proxy.example.com") + os.Setenv("LISTENER_AZURE_KEY_VAULT_PROXY_HTTPS_USERNAME", "username") + os.Setenv("LISTENER_AZURE_KEY_VAULT_PROXY_HTTPS_PASSWORD", "password") + os.Setenv("LISTENER_AZURE_KEY_VAULT_PROXY_NO_PROXY", "temp.com") + + vaults, err := vault.InitAll("LISTENER_") + require.NoError(t, err) + require.Len(t, vaults, 1) + require.Contains(t, vaults, vault.VaultTypeAzureKeyVault) + akv, ok := vaults[vault.VaultTypeAzureKeyVault].(*azurekeyvault.AzureKeyVault) + require.True(t, ok) + require.NotNil(t, akv) +}