From 2117fd18923aa0c37369426713f0169bb6b2ccf6 Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Thu, 19 Oct 2023 12:29:32 +0200 Subject: [PATCH] Configure listener pod with the secret instead of env (#2965) Co-authored-by: Bassem Dghaidi <568794+Link-@users.noreply.github.com> --- .../autoScalerService.go | 3 +- .../config/config.go | 76 ++++++ .../config/config_test.go | 92 +++++++ cmd/githubrunnerscalesetlistener/main.go | 76 +----- cmd/githubrunnerscalesetlistener/main_test.go | 94 +------ .../autoscalinglistener_controller.go | 66 ++++- .../autoscalinglistener_controller_test.go | 62 +++-- .../actions.github.com/resourcebuilder.go | 251 ++++++++---------- .../resourcebuilder_test.go | 2 +- 9 files changed, 401 insertions(+), 321 deletions(-) create mode 100644 cmd/githubrunnerscalesetlistener/config/config.go create mode 100644 cmd/githubrunnerscalesetlistener/config/config_test.go diff --git a/cmd/githubrunnerscalesetlistener/autoScalerService.go b/cmd/githubrunnerscalesetlistener/autoScalerService.go index f7db1b62..201e280b 100644 --- a/cmd/githubrunnerscalesetlistener/autoScalerService.go +++ b/cmd/githubrunnerscalesetlistener/autoScalerService.go @@ -8,6 +8,7 @@ import ( "math" "strings" + "github.com/actions/actions-runner-controller/cmd/githubrunnerscalesetlistener/config" "github.com/actions/actions-runner-controller/github/actions" "github.com/go-logr/logr" ) @@ -30,7 +31,7 @@ type Service struct { errs []error } -func WithPrometheusMetrics(conf RunnerScaleSetListenerConfig) func(*Service) { +func WithPrometheusMetrics(conf config.Config) func(*Service) { return func(svc *Service) { parsedURL, err := actions.ParseGitHubConfigFromURL(conf.ConfigureUrl) if err != nil { diff --git a/cmd/githubrunnerscalesetlistener/config/config.go b/cmd/githubrunnerscalesetlistener/config/config.go new file mode 100644 index 00000000..3a977a22 --- /dev/null +++ b/cmd/githubrunnerscalesetlistener/config/config.go @@ -0,0 +1,76 @@ +package config + +import ( + "encoding/json" + "fmt" + "os" +) + +type Config struct { + ConfigureUrl string `json:"configureUrl"` + AppID int64 `json:"appID"` + AppInstallationID int64 `json:"appInstallationID"` + AppPrivateKey string `json:"appPrivateKey"` + Token string `json:"token"` + EphemeralRunnerSetNamespace string `json:"ephemeralRunnerSetNamespace"` + EphemeralRunnerSetName string `json:"ephemeralRunnerSetName"` + MaxRunners int `json:"maxRunners"` + MinRunners int `json:"minRunners"` + RunnerScaleSetId int `json:"runnerScaleSetId"` + RunnerScaleSetName string `json:"runnerScaleSetName"` + ServerRootCA string `json:"serverRootCA"` + LogLevel string `json:"logLevel"` + LogFormat string `json:"logFormat"` + MetricsAddr string `json:"metricsAddr"` + MetricsEndpoint string `json:"metricsEndpoint"` +} + +func Read(path string) (Config, error) { + f, err := os.Open(path) + if err != nil { + return Config{}, 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) + } + + if err := config.validate(); err != nil { + return Config{}, fmt.Errorf("failed to validate config: %w", err) + } + + return config, nil +} + +func (c *Config) validate() error { + if len(c.ConfigureUrl) == 0 { + return fmt.Errorf("GitHubConfigUrl is not provided") + } + + if len(c.EphemeralRunnerSetNamespace) == 0 || len(c.EphemeralRunnerSetName) == 0 { + return fmt.Errorf("EphemeralRunnerSetNamespace '%s' or EphemeralRunnerSetName '%s' is missing", c.EphemeralRunnerSetNamespace, c.EphemeralRunnerSetName) + } + + if c.RunnerScaleSetId == 0 { + return fmt.Errorf("RunnerScaleSetId '%d' is missing", c.RunnerScaleSetId) + } + + if c.MaxRunners < c.MinRunners { + return fmt.Errorf("MinRunners '%d' cannot be greater than MaxRunners '%d'", c.MinRunners, c.MaxRunners) + } + + hasToken := len(c.Token) > 0 + hasPrivateKeyConfig := c.AppID > 0 && c.AppPrivateKey != "" + + if !hasToken && !hasPrivateKeyConfig { + return fmt.Errorf("GitHub auth credential is missing, token length: '%d', appId: '%d', installationId: '%d', private key length: '%d", len(c.Token), c.AppID, c.AppInstallationID, len(c.AppPrivateKey)) + } + + if hasToken && hasPrivateKeyConfig { + return fmt.Errorf("only one GitHub auth method supported at a time. Have both PAT and App auth: token length: '%d', appId: '%d', installationId: '%d', private key length: '%d", len(c.Token), c.AppID, c.AppInstallationID, len(c.AppPrivateKey)) + } + + return nil +} diff --git a/cmd/githubrunnerscalesetlistener/config/config_test.go b/cmd/githubrunnerscalesetlistener/config/config_test.go new file mode 100644 index 00000000..99e6ac99 --- /dev/null +++ b/cmd/githubrunnerscalesetlistener/config/config_test.go @@ -0,0 +1,92 @@ +package config + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestConfigValidationMinMax(t *testing.T) { + config := &Config{ + ConfigureUrl: "github.com/some_org/some_repo", + EphemeralRunnerSetNamespace: "namespace", + EphemeralRunnerSetName: "deployment", + RunnerScaleSetId: 1, + MinRunners: 5, + MaxRunners: 2, + Token: "token", + } + err := config.validate() + assert.ErrorContains(t, err, "MinRunners '5' cannot be greater than MaxRunners '2", "Expected error about MinRunners > MaxRunners") +} + +func TestConfigValidationMissingToken(t *testing.T) { + config := &Config{ + ConfigureUrl: "github.com/some_org/some_repo", + EphemeralRunnerSetNamespace: "namespace", + EphemeralRunnerSetName: "deployment", + RunnerScaleSetId: 1, + } + err := config.validate() + expectedError := fmt.Sprintf("GitHub auth credential is missing, token length: '%d', appId: '%d', installationId: '%d', private key length: '%d", len(config.Token), config.AppID, config.AppInstallationID, len(config.AppPrivateKey)) + assert.ErrorContains(t, err, expectedError, "Expected error about missing auth") +} + +func TestConfigValidationAppKey(t *testing.T) { + config := &Config{ + AppID: 1, + AppInstallationID: 10, + ConfigureUrl: "github.com/some_org/some_repo", + EphemeralRunnerSetNamespace: "namespace", + EphemeralRunnerSetName: "deployment", + RunnerScaleSetId: 1, + } + err := config.validate() + expectedError := fmt.Sprintf("GitHub auth credential is missing, token length: '%d', appId: '%d', installationId: '%d', private key length: '%d", len(config.Token), config.AppID, config.AppInstallationID, len(config.AppPrivateKey)) + assert.ErrorContains(t, err, expectedError, "Expected error about missing auth") +} + +func TestConfigValidationOnlyOneTypeOfCredentials(t *testing.T) { + config := &Config{ + AppID: 1, + AppInstallationID: 10, + AppPrivateKey: "asdf", + Token: "asdf", + ConfigureUrl: "github.com/some_org/some_repo", + EphemeralRunnerSetNamespace: "namespace", + EphemeralRunnerSetName: "deployment", + RunnerScaleSetId: 1, + } + err := config.validate() + expectedError := fmt.Sprintf("only one GitHub auth method supported at a time. Have both PAT and App auth: token length: '%d', appId: '%d', installationId: '%d', private key length: '%d", len(config.Token), config.AppID, config.AppInstallationID, len(config.AppPrivateKey)) + assert.ErrorContains(t, err, expectedError, "Expected error about missing auth") +} + +func TestConfigValidation(t *testing.T) { + config := &Config{ + ConfigureUrl: "https://github.com/actions", + EphemeralRunnerSetNamespace: "namespace", + EphemeralRunnerSetName: "deployment", + RunnerScaleSetId: 1, + MinRunners: 1, + MaxRunners: 5, + Token: "asdf", + } + + err := config.validate() + + assert.NoError(t, err, "Expected no error") +} + +func TestConfigValidationConfigUrl(t *testing.T) { + config := &Config{ + EphemeralRunnerSetNamespace: "namespace", + EphemeralRunnerSetName: "deployment", + RunnerScaleSetId: 1, + } + + err := config.validate() + + assert.ErrorContains(t, err, "GitHubConfigUrl is not provided", "Expected error about missing ConfigureUrl") +} diff --git a/cmd/githubrunnerscalesetlistener/main.go b/cmd/githubrunnerscalesetlistener/main.go index c27443d9..15b3b8a0 100644 --- a/cmd/githubrunnerscalesetlistener/main.go +++ b/cmd/githubrunnerscalesetlistener/main.go @@ -28,39 +28,26 @@ import ( "time" "github.com/actions/actions-runner-controller/build" + "github.com/actions/actions-runner-controller/cmd/githubrunnerscalesetlistener/config" "github.com/actions/actions-runner-controller/github/actions" "github.com/actions/actions-runner-controller/logging" "github.com/go-logr/logr" - "github.com/kelseyhightower/envconfig" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promhttp" "golang.org/x/net/http/httpproxy" "golang.org/x/sync/errgroup" ) -type RunnerScaleSetListenerConfig struct { - ConfigureUrl string `split_words:"true"` - AppID int64 `split_words:"true"` - AppInstallationID int64 `split_words:"true"` - AppPrivateKey string `split_words:"true"` - Token string `split_words:"true"` - EphemeralRunnerSetNamespace string `split_words:"true"` - EphemeralRunnerSetName string `split_words:"true"` - MaxRunners int `split_words:"true"` - MinRunners int `split_words:"true"` - RunnerScaleSetId int `split_words:"true"` - RunnerScaleSetName string `split_words:"true"` - ServerRootCA string `split_words:"true"` - LogLevel string `split_words:"true"` - LogFormat string `split_words:"true"` - MetricsAddr string `split_words:"true"` - MetricsEndpoint string `split_words:"true"` -} - func main() { - var rc RunnerScaleSetListenerConfig - if err := envconfig.Process("github", &rc); err != nil { - fmt.Fprintf(os.Stderr, "Error: processing environment variables for RunnerScaleSetListenerConfig: %v\n", err) + 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) + } + + rc, err := config.Read(configPath) + if err != nil { + fmt.Fprintf(os.Stderr, "Error: reading config from path(%q): %v\n", configPath, err) os.Exit(1) } @@ -80,12 +67,6 @@ func main() { os.Exit(1) } - // Validate all inputs - if err := validateConfig(&rc); err != nil { - logger.Error(err, "Inputs validation failed") - os.Exit(1) - } - ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) defer stop() @@ -123,7 +104,7 @@ func main() { } type metricsServer struct { - rc RunnerScaleSetListenerConfig + rc config.Config logger logr.Logger srv *http.Server } @@ -173,7 +154,7 @@ type runOptions struct { serviceOptions []func(*Service) } -func run(ctx context.Context, rc RunnerScaleSetListenerConfig, logger logr.Logger, opts runOptions) error { +func run(ctx context.Context, rc config.Config, logger logr.Logger, opts runOptions) error { // Create root context and hook with sigint and sigterm creds := &actions.ActionsAuth{} if rc.Token != "" { @@ -232,38 +213,7 @@ func run(ctx context.Context, rc RunnerScaleSetListenerConfig, logger logr.Logge return nil } -func validateConfig(config *RunnerScaleSetListenerConfig) error { - if len(config.ConfigureUrl) == 0 { - return fmt.Errorf("GitHubConfigUrl is not provided") - } - - if len(config.EphemeralRunnerSetNamespace) == 0 || len(config.EphemeralRunnerSetName) == 0 { - return fmt.Errorf("EphemeralRunnerSetNamespace '%s' or EphemeralRunnerSetName '%s' is missing", config.EphemeralRunnerSetNamespace, config.EphemeralRunnerSetName) - } - - if config.RunnerScaleSetId == 0 { - return fmt.Errorf("RunnerScaleSetId '%d' is missing", config.RunnerScaleSetId) - } - - if config.MaxRunners < config.MinRunners { - return fmt.Errorf("MinRunners '%d' cannot be greater than MaxRunners '%d'", config.MinRunners, config.MaxRunners) - } - - hasToken := len(config.Token) > 0 - hasPrivateKeyConfig := config.AppID > 0 && config.AppPrivateKey != "" - - if !hasToken && !hasPrivateKeyConfig { - return fmt.Errorf("GitHub auth credential is missing, token length: '%d', appId: '%d', installationId: '%d', private key length: '%d", len(config.Token), config.AppID, config.AppInstallationID, len(config.AppPrivateKey)) - } - - if hasToken && hasPrivateKeyConfig { - return fmt.Errorf("only one GitHub auth method supported at a time. Have both PAT and App auth: token length: '%d', appId: '%d', installationId: '%d', private key length: '%d", len(config.Token), config.AppID, config.AppInstallationID, len(config.AppPrivateKey)) - } - - return nil -} - -func newActionsClientFromConfig(config RunnerScaleSetListenerConfig, creds *actions.ActionsAuth, options ...actions.ClientOption) (*actions.Client, error) { +func newActionsClientFromConfig(config config.Config, creds *actions.ActionsAuth, options ...actions.ClientOption) (*actions.Client, error) { if config.ServerRootCA != "" { systemPool, err := x509.SystemCertPool() if err != nil { diff --git a/cmd/githubrunnerscalesetlistener/main_test.go b/cmd/githubrunnerscalesetlistener/main_test.go index 6d11194f..9cd9302c 100644 --- a/cmd/githubrunnerscalesetlistener/main_test.go +++ b/cmd/githubrunnerscalesetlistener/main_test.go @@ -3,7 +3,6 @@ package main import ( "context" "crypto/tls" - "fmt" "net/http" "net/http/httptest" "os" @@ -13,94 +12,11 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/actions/actions-runner-controller/cmd/githubrunnerscalesetlistener/config" "github.com/actions/actions-runner-controller/github/actions" "github.com/actions/actions-runner-controller/github/actions/testserver" ) -func TestConfigValidationMinMax(t *testing.T) { - config := &RunnerScaleSetListenerConfig{ - ConfigureUrl: "github.com/some_org/some_repo", - EphemeralRunnerSetNamespace: "namespace", - EphemeralRunnerSetName: "deployment", - RunnerScaleSetId: 1, - MinRunners: 5, - MaxRunners: 2, - Token: "token", - } - err := validateConfig(config) - assert.ErrorContains(t, err, "MinRunners '5' cannot be greater than MaxRunners '2", "Expected error about MinRunners > MaxRunners") -} - -func TestConfigValidationMissingToken(t *testing.T) { - config := &RunnerScaleSetListenerConfig{ - ConfigureUrl: "github.com/some_org/some_repo", - EphemeralRunnerSetNamespace: "namespace", - EphemeralRunnerSetName: "deployment", - RunnerScaleSetId: 1, - } - err := validateConfig(config) - expectedError := fmt.Sprintf("GitHub auth credential is missing, token length: '%d', appId: '%d', installationId: '%d', private key length: '%d", len(config.Token), config.AppID, config.AppInstallationID, len(config.AppPrivateKey)) - assert.ErrorContains(t, err, expectedError, "Expected error about missing auth") -} - -func TestConfigValidationAppKey(t *testing.T) { - config := &RunnerScaleSetListenerConfig{ - AppID: 1, - AppInstallationID: 10, - ConfigureUrl: "github.com/some_org/some_repo", - EphemeralRunnerSetNamespace: "namespace", - EphemeralRunnerSetName: "deployment", - RunnerScaleSetId: 1, - } - err := validateConfig(config) - expectedError := fmt.Sprintf("GitHub auth credential is missing, token length: '%d', appId: '%d', installationId: '%d', private key length: '%d", len(config.Token), config.AppID, config.AppInstallationID, len(config.AppPrivateKey)) - assert.ErrorContains(t, err, expectedError, "Expected error about missing auth") -} - -func TestConfigValidationOnlyOneTypeOfCredentials(t *testing.T) { - config := &RunnerScaleSetListenerConfig{ - AppID: 1, - AppInstallationID: 10, - AppPrivateKey: "asdf", - Token: "asdf", - ConfigureUrl: "github.com/some_org/some_repo", - EphemeralRunnerSetNamespace: "namespace", - EphemeralRunnerSetName: "deployment", - RunnerScaleSetId: 1, - } - err := validateConfig(config) - expectedError := fmt.Sprintf("only one GitHub auth method supported at a time. Have both PAT and App auth: token length: '%d', appId: '%d', installationId: '%d', private key length: '%d", len(config.Token), config.AppID, config.AppInstallationID, len(config.AppPrivateKey)) - assert.ErrorContains(t, err, expectedError, "Expected error about missing auth") -} - -func TestConfigValidation(t *testing.T) { - config := &RunnerScaleSetListenerConfig{ - ConfigureUrl: "https://github.com/actions", - EphemeralRunnerSetNamespace: "namespace", - EphemeralRunnerSetName: "deployment", - RunnerScaleSetId: 1, - MinRunners: 1, - MaxRunners: 5, - Token: "asdf", - } - - err := validateConfig(config) - - assert.NoError(t, err, "Expected no error") -} - -func TestConfigValidationConfigUrl(t *testing.T) { - config := &RunnerScaleSetListenerConfig{ - EphemeralRunnerSetNamespace: "namespace", - EphemeralRunnerSetName: "deployment", - RunnerScaleSetId: 1, - } - - err := validateConfig(config) - - assert.ErrorContains(t, err, "GitHubConfigUrl is not provided", "Expected error about missing ConfigureUrl") -} - func TestCustomerServerRootCA(t *testing.T) { ctx := context.Background() certsFolder := filepath.Join( @@ -134,7 +50,7 @@ func TestCustomerServerRootCA(t *testing.T) { require.NoError(t, err) certsString = certsString + string(intermediate) - config := RunnerScaleSetListenerConfig{ + config := config.Config{ ConfigureUrl: server.ConfigURLForOrg("myorg"), ServerRootCA: certsString, } @@ -164,7 +80,7 @@ func TestProxySettings(t *testing.T) { os.Setenv("http_proxy", proxy.URL) defer os.Setenv("http_proxy", prevProxy) - config := RunnerScaleSetListenerConfig{ + config := config.Config{ ConfigureUrl: "https://github.com/org/repo", } creds := &actions.ActionsAuth{ @@ -196,7 +112,7 @@ func TestProxySettings(t *testing.T) { os.Setenv("https_proxy", proxy.URL) defer os.Setenv("https_proxy", prevProxy) - config := RunnerScaleSetListenerConfig{ + config := config.Config{ ConfigureUrl: "https://github.com/org/repo", } creds := &actions.ActionsAuth{ @@ -233,7 +149,7 @@ func TestProxySettings(t *testing.T) { os.Setenv("no_proxy", "example.com") defer os.Setenv("no_proxy", prevNoProxy) - config := RunnerScaleSetListenerConfig{ + config := config.Config{ ConfigureUrl: "https://github.com/org/repo", } creds := &actions.ActionsAuth{ diff --git a/controllers/actions.github.com/autoscalinglistener_controller.go b/controllers/actions.github.com/autoscalinglistener_controller.go index d3caecfd..dd4e8fcf 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller.go +++ b/controllers/actions.github.com/autoscalinglistener_controller.go @@ -287,6 +287,21 @@ func (r *AutoscalingListenerReconciler) cleanupResources(ctx context.Context, au } logger.Info("Listener pod is deleted") + var secret corev1.Secret + err = r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Namespace, Name: scaleSetListenerConfigName(autoscalingListener)}, &secret) + switch { + case err == nil: + if secret.ObjectMeta.DeletionTimestamp.IsZero() { + logger.Info("Deleting the listener config secret") + if err := r.Delete(ctx, &secret); err != nil { + return false, fmt.Errorf("failed to delete listener config secret: %v", err) + } + } + return false, nil + case err != nil && !kerrors.IsNotFound(err): + return false, fmt.Errorf("failed to get listener config secret: %v", err) + } + if autoscalingListener.Spec.Proxy != nil { logger.Info("Cleaning up the listener proxy secret") proxySecret := new(corev1.Secret) @@ -418,13 +433,13 @@ func (r *AutoscalingListenerReconciler) createListenerPod(ctx context.Context, a } } + cert := "" if autoscalingListener.Spec.GitHubServerTLS != nil { - env, err := r.certificateEnvVarForListener(ctx, autoscalingRunnerSet, autoscalingListener) + var err error + cert, err = r.certificate(ctx, autoscalingRunnerSet, autoscalingListener) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to create certificate env var for listener: %v", err) } - - envs = append(envs, env) } var metricsConfig *listenerMetricsServerConfig @@ -435,7 +450,35 @@ func (r *AutoscalingListenerReconciler) createListenerPod(ctx context.Context, a } } - newPod, err := r.resourceBuilder.newScaleSetListenerPod(autoscalingListener, serviceAccount, secret, metricsConfig, envs...) + var podConfig corev1.Secret + if err := r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Namespace, Name: scaleSetListenerConfigName(autoscalingListener)}, &podConfig); err != nil { + if !kerrors.IsNotFound(err) { + logger.Error(err, "Unable to get listener config secret", "namespace", autoscalingListener.Namespace, "name", scaleSetListenerConfigName(autoscalingListener)) + return ctrl.Result{Requeue: true}, err + } + + logger.Info("Creating listener config secret") + + podConfig, err := r.resourceBuilder.newScaleSetListenerConfig(autoscalingListener, secret, metricsConfig, cert) + if err != nil { + logger.Error(err, "Failed to build listener config secret") + return ctrl.Result{}, err + } + + if err := ctrl.SetControllerReference(autoscalingListener, podConfig, r.Scheme); err != nil { + logger.Error(err, "Failed to set controller reference") + return ctrl.Result{}, err + } + + if err := r.Create(ctx, podConfig); err != nil { + logger.Error(err, "Unable to create listener config secret", "namespace", podConfig.Namespace, "name", podConfig.Name) + return ctrl.Result{}, err + } + + return ctrl.Result{Requeue: true}, nil + } + + newPod, err := r.resourceBuilder.newScaleSetListenerPod(autoscalingListener, &podConfig, serviceAccount, secret, metricsConfig, envs...) if err != nil { logger.Error(err, "Failed to build listener pod") return ctrl.Result{}, err @@ -456,13 +499,13 @@ func (r *AutoscalingListenerReconciler) createListenerPod(ctx context.Context, a return ctrl.Result{}, nil } -func (r *AutoscalingListenerReconciler) certificateEnvVarForListener(ctx context.Context, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, autoscalingListener *v1alpha1.AutoscalingListener) (corev1.EnvVar, error) { +func (r *AutoscalingListenerReconciler) certificate(ctx context.Context, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, autoscalingListener *v1alpha1.AutoscalingListener) (string, error) { if autoscalingListener.Spec.GitHubServerTLS.CertificateFrom == nil { - return corev1.EnvVar{}, fmt.Errorf("githubServerTLS.certificateFrom is not specified") + return "", fmt.Errorf("githubServerTLS.certificateFrom is not specified") } if autoscalingListener.Spec.GitHubServerTLS.CertificateFrom.ConfigMapKeyRef == nil { - return corev1.EnvVar{}, fmt.Errorf("githubServerTLS.certificateFrom.configMapKeyRef is not specified") + return "", fmt.Errorf("githubServerTLS.certificateFrom.configMapKeyRef is not specified") } var configmap corev1.ConfigMap @@ -475,7 +518,7 @@ func (r *AutoscalingListenerReconciler) certificateEnvVarForListener(ctx context &configmap, ) if err != nil { - return corev1.EnvVar{}, fmt.Errorf( + return "", fmt.Errorf( "failed to get configmap %s: %w", autoscalingListener.Spec.GitHubServerTLS.CertificateFrom.ConfigMapKeyRef.Name, err, @@ -484,17 +527,14 @@ func (r *AutoscalingListenerReconciler) certificateEnvVarForListener(ctx context certificate, ok := configmap.Data[autoscalingListener.Spec.GitHubServerTLS.CertificateFrom.ConfigMapKeyRef.Key] if !ok { - return corev1.EnvVar{}, fmt.Errorf( + return "", fmt.Errorf( "key %s is not found in configmap %s", autoscalingListener.Spec.GitHubServerTLS.CertificateFrom.ConfigMapKeyRef.Key, autoscalingListener.Spec.GitHubServerTLS.CertificateFrom.ConfigMapKeyRef.Name, ) } - return corev1.EnvVar{ - Name: "GITHUB_SERVER_ROOT_CA", - Value: certificate, - }, nil + return certificate, nil } func (r *AutoscalingListenerReconciler) createSecretsForListener(ctx context.Context, autoscalingListener *v1alpha1.AutoscalingListener, secret *corev1.Secret, logger logr.Logger) (ctrl.Result, error) { diff --git a/controllers/actions.github.com/autoscalinglistener_controller_test.go b/controllers/actions.github.com/autoscalinglistener_controller_test.go index 97603529..5327c541 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller_test.go +++ b/controllers/actions.github.com/autoscalinglistener_controller_test.go @@ -2,6 +2,7 @@ package actionsgithubcom import ( "context" + "encoding/json" "fmt" "os" "path/filepath" @@ -13,6 +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/githubrunnerscalesetlistener/config" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" kerrors "k8s.io/apimachinery/pkg/api/errors" @@ -103,6 +105,19 @@ var _ = Describe("Test AutoScalingListener controller", func() { Context("When creating a new AutoScalingListener", func() { It("It should create/add all required resources for a new AutoScalingListener (finalizer, secret, service account, role, rolebinding, pod)", func() { + config := new(corev1.Secret) + Eventually( + func() error { + err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerConfigName(autoscalingListener), Namespace: configSecret.Namespace}, config) + if err != nil { + return err + } + return nil + }, + autoscalingListenerTestTimeout, + autoscalingListenerTestInterval, + ).Should(Succeed(), "Config secret should be created") + // Check if finalizer is added created := new(actionsv1alpha1.AutoscalingListener) Eventually( @@ -251,6 +266,17 @@ var _ = Describe("Test AutoScalingListener controller", func() { autoscalingListenerTestInterval, ).Should(BeTrue(), "failed to delete role") + // Cleanup the listener config + Eventually( + func() bool { + config := new(corev1.Secret) + err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerConfigName(autoscalingListener), Namespace: autoscalingListener.Namespace}, config) + return kerrors.IsNotFound(err) + }, + autoscalingListenerTestTimeout, + autoscalingListenerTestInterval, + ).Should(BeTrue(), "failed to delete config secret") + // Cleanup the listener service account Eventually( func() error { @@ -501,6 +527,17 @@ var _ = Describe("Test AutoScalingListener customization", func() { autoscalingListenerTestTimeout, autoscalingListenerTestInterval).Should(BeEquivalentTo(autoscalingListenerFinalizerName), "AutoScalingListener should have a finalizer") + // Check if config is created + config := new(corev1.Secret) + Eventually( + func() error { + err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerConfigName(autoscalingListener), Namespace: autoscalingListener.Namespace}, config) + return err + }, + autoscalingListenerTestTimeout, + autoscalingListenerTestInterval, + ).Should(Succeed(), "Config secret should be created") + // Check if pod is created pod := new(corev1.Pod) Eventually( @@ -989,31 +1026,26 @@ var _ = Describe("Test GitHub Server TLS configuration", func() { }) Context("When creating a new AutoScalingListener", func() { - It("It should set the certificates as an environment variable on the pod", func() { - pod := new(corev1.Pod) + It("It should set the certificates in the config of the pod", func() { + config := new(corev1.Secret) Eventually( func(g Gomega) { err := k8sClient.Get( ctx, client.ObjectKey{ - Name: autoscalingListener.Name, + Name: scaleSetListenerConfigName(autoscalingListener), Namespace: autoscalingListener.Namespace, }, - pod, + config, ) g.Expect(err).NotTo(HaveOccurred(), "failed to get pod") - g.Expect(pod.Spec.Containers).NotTo(BeEmpty(), "pod should have containers") - g.Expect(pod.Spec.Containers[0].Env).NotTo(BeEmpty(), "pod should have env variables") - var env *corev1.EnvVar - for _, e := range pod.Spec.Containers[0].Env { - if e.Name == "GITHUB_SERVER_ROOT_CA" { - env = &e - break - } - } - g.Expect(env).NotTo(BeNil(), "pod should have an env variable named GITHUB_SERVER_ROOT_CA_PATH") + g.Expect(config.Data["config.json"]).ToNot(BeEmpty(), "listener configuration file should not be empty") + + var listenerConfig listenerconfig.Config + err = json.Unmarshal(config.Data["config.json"], &listenerConfig) + g.Expect(err).NotTo(HaveOccurred(), "failed to parse listener configuration file") cert, err := os.ReadFile(filepath.Join( "../../", @@ -1024,7 +1056,7 @@ var _ = Describe("Test GitHub Server TLS configuration", func() { )) g.Expect(err).NotTo(HaveOccurred(), "failed to read rootCA.crt") - g.Expect(env.Value).To( + g.Expect(listenerConfig.ServerRootCA).To( BeEquivalentTo(string(cert)), "GITHUB_SERVER_ROOT_CA should be the rootCA.crt", ) diff --git a/controllers/actions.github.com/resourcebuilder.go b/controllers/actions.github.com/resourcebuilder.go index 2b69a8df..e42f3a60 100644 --- a/controllers/actions.github.com/resourcebuilder.go +++ b/controllers/actions.github.com/resourcebuilder.go @@ -1,7 +1,9 @@ package actionsgithubcom import ( + "bytes" "context" + "encoding/json" "fmt" "math" "net" @@ -9,6 +11,7 @@ import ( "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" "github.com/actions/actions-runner-controller/build" + listenerconfig "github.com/actions/actions-runner-controller/cmd/githubrunnerscalesetlistener/config" "github.com/actions/actions-runner-controller/github/actions" "github.com/actions/actions-runner-controller/hash" "github.com/actions/actions-runner-controller/logging" @@ -33,21 +36,6 @@ var commonLabelKeys = [...]string{ LabelKeyGitHubRepository, } -var reservedListenerContainerEnvKeys = map[string]struct{}{ - "GITHUB_CONFIGURE_URL": {}, - "GITHUB_EPHEMERAL_RUNNER_SET_NAMESPACE": {}, - "GITHUB_EPHEMERAL_RUNNER_SET_NAME": {}, - "GITHUB_MAX_RUNNERS": {}, - "GITHUB_MIN_RUNNERS": {}, - "GITHUB_RUNNER_SCALE_SET_ID": {}, - "GITHUB_RUNNER_LOG_LEVEL": {}, - "GITHUB_RUNNER_LOG_FORMAT": {}, - "GITHUB_TOKEN": {}, - "GITHUB_APP_ID": {}, - "GITHUB_APP_INSTALLATION_ID": {}, - "GITHUB_APP_PRIVATE_KEY": {}, -} - const labelValueKubernetesPartOf = "gha-runner-scale-set" var scaleSetListenerLogLevel = DefaultScaleSetListenerLogLevel @@ -144,133 +132,101 @@ type listenerMetricsServerConfig struct { endpoint string } -func (b *resourceBuilder) newScaleSetListenerPod(autoscalingListener *v1alpha1.AutoscalingListener, serviceAccount *corev1.ServiceAccount, secret *corev1.Secret, metricsConfig *listenerMetricsServerConfig, envs ...corev1.EnvVar) (*corev1.Pod, error) { +func (lm *listenerMetricsServerConfig) containerPort() (corev1.ContainerPort, error) { + _, portStr, err := net.SplitHostPort(lm.addr) + if err != nil { + return corev1.ContainerPort{}, err + } + port, err := strconv.ParseInt(portStr, 10, 32) + if err != nil { + return corev1.ContainerPort{}, err + } + return corev1.ContainerPort{ + ContainerPort: int32(port), + Protocol: corev1.ProtocolTCP, + Name: "metrics", + }, nil +} + +func (b *resourceBuilder) newScaleSetListenerConfig(autoscalingListener *v1alpha1.AutoscalingListener, secret *corev1.Secret, metricsConfig *listenerMetricsServerConfig, cert string) (*corev1.Secret, error) { + var ( + metricsAddr = "" + metricsEndpoint = "" + ) + if metricsConfig != nil { + metricsAddr = metricsConfig.addr + metricsEndpoint = metricsConfig.endpoint + } + + var appID int64 + if id, ok := secret.Data["github_app_id"]; ok { + var err error + appID, err = strconv.ParseInt(string(id), 10, 64) + if err != nil { + return nil, fmt.Errorf("failed to convert github_app_id to int: %v", err) + } + } + + 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{ + ConfigureUrl: autoscalingListener.Spec.GitHubConfigUrl, + AppID: appID, + 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, + MinRunners: autoscalingListener.Spec.MinRunners, + RunnerScaleSetId: autoscalingListener.Spec.RunnerScaleSetId, + RunnerScaleSetName: autoscalingListener.Spec.AutoscalingRunnerSetName, + ServerRootCA: cert, + LogLevel: scaleSetListenerLogLevel, + LogFormat: scaleSetListenerLogFormat, + MetricsAddr: metricsAddr, + MetricsEndpoint: metricsEndpoint, + } + + var buf bytes.Buffer + if err := json.NewEncoder(&buf).Encode(config); err != nil { + return nil, fmt.Errorf("failed to encode config: %w", err) + } + + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: scaleSetListenerConfigName(autoscalingListener), + Namespace: autoscalingListener.Namespace, + }, + Data: map[string][]byte{ + "config.json": buf.Bytes(), + }, + }, 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) { listenerEnv := []corev1.EnvVar{ { - Name: "GITHUB_CONFIGURE_URL", - Value: autoscalingListener.Spec.GitHubConfigUrl, - }, - { - Name: "GITHUB_EPHEMERAL_RUNNER_SET_NAMESPACE", - Value: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, - }, - { - Name: "GITHUB_EPHEMERAL_RUNNER_SET_NAME", - Value: autoscalingListener.Spec.EphemeralRunnerSetName, - }, - { - Name: "GITHUB_MAX_RUNNERS", - Value: strconv.Itoa(autoscalingListener.Spec.MaxRunners), - }, - { - Name: "GITHUB_MIN_RUNNERS", - Value: strconv.Itoa(autoscalingListener.Spec.MinRunners), - }, - { - Name: "GITHUB_RUNNER_SCALE_SET_ID", - Value: strconv.Itoa(autoscalingListener.Spec.RunnerScaleSetId), - }, - { - Name: "GITHUB_RUNNER_SCALE_SET_NAME", - Value: autoscalingListener.Spec.AutoscalingRunnerSetName, - }, - { - Name: "GITHUB_RUNNER_LOG_LEVEL", - Value: scaleSetListenerLogLevel, - }, - { - Name: "GITHUB_RUNNER_LOG_FORMAT", - Value: scaleSetListenerLogFormat, + Name: "LISTENER_CONFIG_PATH", + Value: "/etc/gha-listener/config.json", }, } listenerEnv = append(listenerEnv, envs...) - if _, ok := secret.Data["github_token"]; ok { - listenerEnv = append(listenerEnv, corev1.EnvVar{ - Name: "GITHUB_TOKEN", - ValueFrom: &corev1.EnvVarSource{ - SecretKeyRef: &corev1.SecretKeySelector{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: secret.Name, - }, - Key: "github_token", - }, - }, - }) - } - - if _, ok := secret.Data["github_app_id"]; ok { - listenerEnv = append(listenerEnv, corev1.EnvVar{ - Name: "GITHUB_APP_ID", - ValueFrom: &corev1.EnvVarSource{ - SecretKeyRef: &corev1.SecretKeySelector{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: secret.Name, - }, - Key: "github_app_id", - }, - }, - }) - } - - if _, ok := secret.Data["github_app_installation_id"]; ok { - listenerEnv = append(listenerEnv, corev1.EnvVar{ - Name: "GITHUB_APP_INSTALLATION_ID", - ValueFrom: &corev1.EnvVarSource{ - SecretKeyRef: &corev1.SecretKeySelector{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: secret.Name, - }, - Key: "github_app_installation_id", - }, - }, - }) - } - - if _, ok := secret.Data["github_app_private_key"]; ok { - listenerEnv = append(listenerEnv, corev1.EnvVar{ - Name: "GITHUB_APP_PRIVATE_KEY", - ValueFrom: &corev1.EnvVarSource{ - SecretKeyRef: &corev1.SecretKeySelector{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: secret.Name, - }, - Key: "github_app_private_key", - }, - }, - }) - } - var ports []corev1.ContainerPort if metricsConfig != nil && len(metricsConfig.addr) != 0 { - listenerEnv = append( - listenerEnv, - corev1.EnvVar{ - Name: "GITHUB_METRICS_ADDR", - Value: metricsConfig.addr, - }, - corev1.EnvVar{ - Name: "GITHUB_METRICS_ENDPOINT", - Value: metricsConfig.endpoint, - }, - ) - - _, portStr, err := net.SplitHostPort(metricsConfig.addr) + port, err := metricsConfig.containerPort() if err != nil { - return nil, fmt.Errorf("failed to split host:port for metrics address: %v", err) + return nil, fmt.Errorf("failed to convert metrics server address to container port: %v", err) } - port, err := strconv.ParseInt(portStr, 10, 32) - if err != nil { - return nil, fmt.Errorf("failed to convert port %q to int32: %v", portStr, err) - } - ports = append( - ports, - corev1.ContainerPort{ - ContainerPort: int32(port), - Protocol: corev1.ProtocolTCP, - Name: "metrics", - }, - ) + ports = append(ports, port) } podSpec := corev1.PodSpec{ @@ -285,6 +241,23 @@ func (b *resourceBuilder) newScaleSetListenerPod(autoscalingListener *v1alpha1.A "/github-runnerscaleset-listener", }, Ports: ports, + VolumeMounts: []corev1.VolumeMount{ + { + Name: "listener-config", + MountPath: "/etc/gha-listener", + ReadOnly: true, + }, + }, + }, + }, + Volumes: []corev1.Volume{ + { + Name: "listener-config", + VolumeSource: corev1.VolumeSource{ + Secret: &corev1.SecretVolumeSource{ + SecretName: podConfig.Name, + }, + }, }, }, ImagePullSecrets: autoscalingListener.Spec.ImagePullSecrets, @@ -348,7 +321,7 @@ func mergeListenerPodWithTemplate(pod *corev1.Pod, tmpl *corev1.PodTemplateSpec) } // apply pod related spec - // NOTE: fields that should be gnored + // NOTE: fields that should be ignored // - service account based fields if tmpl.Spec.RestartPolicy != "" { @@ -359,7 +332,7 @@ func mergeListenerPodWithTemplate(pod *corev1.Pod, tmpl *corev1.PodTemplateSpec) pod.Spec.ImagePullSecrets = tmpl.Spec.ImagePullSecrets } - pod.Spec.Volumes = tmpl.Spec.Volumes + pod.Spec.Volumes = append(pod.Spec.Volumes, tmpl.Spec.Volumes...) pod.Spec.InitContainers = tmpl.Spec.InitContainers pod.Spec.EphemeralContainers = tmpl.Spec.EphemeralContainers pod.Spec.TerminationGracePeriodSeconds = tmpl.Spec.TerminationGracePeriodSeconds @@ -405,11 +378,7 @@ func mergeListenerContainer(base, from *corev1.Container) { base.Command = from.Command } - for _, v := range from.Env { - if _, ok := reservedListenerContainerEnvKeys[v.Name]; !ok { - base.Env = append(base.Env, v) - } - } + base.Env = append(base.Env, from.Env...) if from.ImagePullPolicy != "" { base.ImagePullPolicy = from.ImagePullPolicy @@ -682,6 +651,10 @@ func (b *resourceBuilder) newEphemeralRunnerJitSecret(ephemeralRunner *v1alpha1. } } +func scaleSetListenerConfigName(autoscalingListener *v1alpha1.AutoscalingListener) string { + return fmt.Sprintf("%s-config", autoscalingListener.Name) +} + func scaleSetListenerName(autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet) string { namespaceHash := hash.FNVHashString(autoscalingRunnerSet.Namespace) if len(namespaceHash) > 8 { diff --git a/controllers/actions.github.com/resourcebuilder_test.go b/controllers/actions.github.com/resourcebuilder_test.go index 7f0257c2..8b98b891 100644 --- a/controllers/actions.github.com/resourcebuilder_test.go +++ b/controllers/actions.github.com/resourcebuilder_test.go @@ -68,7 +68,7 @@ func TestLabelPropagation(t *testing.T) { Name: "test", }, } - listenerPod, err := b.newScaleSetListenerPod(listener, listenerServiceAccount, listenerSecret, nil) + listenerPod, err := b.newScaleSetListenerPod(listener, &corev1.Secret{}, listenerServiceAccount, listenerSecret, nil) require.NoError(t, err) assert.Equal(t, listenerPod.Labels, listener.Labels)