Allow use of client id as an app id (#4057)
This commit is contained in:
		
							parent
							
								
									43f1cd0dac
								
							
						
					
					
						commit
						1dbb88cb9e
					
				|  | @ -17,6 +17,7 @@ githubConfigSecret: | ||||||
| ## (Variation B) When using a GitHub App, the syntax is as follows: | ## (Variation B) When using a GitHub App, the syntax is as follows: | ||||||
| # githubConfigSecret: | # githubConfigSecret: | ||||||
| #   # NOTE: IDs MUST be strings, use quotes | #   # NOTE: IDs MUST be strings, use quotes | ||||||
|  | #   # The github_app_id can be an app_id or the client_id | ||||||
| #   github_app_id: "" | #   github_app_id: "" | ||||||
| #   github_app_installation_id: "" | #   github_app_installation_id: "" | ||||||
| #   github_app_private_key: | | #   github_app_private_key: | | ||||||
|  |  | ||||||
|  | @ -17,8 +17,9 @@ import ( | ||||||
| ) | ) | ||||||
| 
 | 
 | ||||||
| type Config struct { | type Config struct { | ||||||
| 	ConfigureUrl                string                  `json:"configure_url"` | 	ConfigureUrl string `json:"configure_url"` | ||||||
| 	AppID                       int64                   `json:"app_id"` | 	// AppID can be an ID of the app or the client ID
 | ||||||
|  | 	AppID                       string                  `json:"app_id"` | ||||||
| 	AppInstallationID           int64                   `json:"app_installation_id"` | 	AppInstallationID           int64                   `json:"app_installation_id"` | ||||||
| 	AppPrivateKey               string                  `json:"app_private_key"` | 	AppPrivateKey               string                  `json:"app_private_key"` | ||||||
| 	Token                       string                  `json:"token"` | 	Token                       string                  `json:"token"` | ||||||
|  | @ -62,26 +63,26 @@ func (c *Config) Validate() error { | ||||||
| 	} | 	} | ||||||
| 
 | 
 | ||||||
| 	if len(c.EphemeralRunnerSetNamespace) == 0 || len(c.EphemeralRunnerSetName) == 0 { | 	if len(c.EphemeralRunnerSetNamespace) == 0 || len(c.EphemeralRunnerSetName) == 0 { | ||||||
| 		return fmt.Errorf("EphemeralRunnerSetNamespace '%s' or EphemeralRunnerSetName '%s' is missing", c.EphemeralRunnerSetNamespace, c.EphemeralRunnerSetName) | 		return fmt.Errorf("EphemeralRunnerSetNamespace %q or EphemeralRunnerSetName %q is missing", c.EphemeralRunnerSetNamespace, c.EphemeralRunnerSetName) | ||||||
| 	} | 	} | ||||||
| 
 | 
 | ||||||
| 	if c.RunnerScaleSetId == 0 { | 	if c.RunnerScaleSetId == 0 { | ||||||
| 		return fmt.Errorf("RunnerScaleSetId '%d' is missing", c.RunnerScaleSetId) | 		return fmt.Errorf(`RunnerScaleSetId "%d" is missing`, c.RunnerScaleSetId) | ||||||
| 	} | 	} | ||||||
| 
 | 
 | ||||||
| 	if c.MaxRunners < c.MinRunners { | 	if c.MaxRunners < c.MinRunners { | ||||||
| 		return fmt.Errorf("MinRunners '%d' cannot be greater than MaxRunners '%d'", c.MinRunners, c.MaxRunners) | 		return fmt.Errorf(`MinRunners "%d" cannot be greater than MaxRunners "%d"`, c.MinRunners, c.MaxRunners) | ||||||
| 	} | 	} | ||||||
| 
 | 
 | ||||||
| 	hasToken := len(c.Token) > 0 | 	hasToken := len(c.Token) > 0 | ||||||
| 	hasPrivateKeyConfig := c.AppID > 0 && c.AppPrivateKey != "" | 	hasPrivateKeyConfig := len(c.AppID) > 0 && c.AppPrivateKey != "" | ||||||
| 
 | 
 | ||||||
| 	if !hasToken && !hasPrivateKeyConfig { | 	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)) | 		return fmt.Errorf(`GitHub auth credential is missing, token length: "%d", appId: %q, installationId: "%d", private key length: "%d"`, len(c.Token), c.AppID, c.AppInstallationID, len(c.AppPrivateKey)) | ||||||
| 	} | 	} | ||||||
| 
 | 
 | ||||||
| 	if hasToken && hasPrivateKeyConfig { | 	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 fmt.Errorf(`only one GitHub auth method supported at a time. Have both PAT and App auth: token length: "%d", appId: %q, installationId: "%d", private key length: "%d"`, len(c.Token), c.AppID, c.AppInstallationID, len(c.AppPrivateKey)) | ||||||
| 	} | 	} | ||||||
| 
 | 
 | ||||||
| 	return nil | 	return nil | ||||||
|  |  | ||||||
|  | @ -18,7 +18,7 @@ func TestConfigValidationMinMax(t *testing.T) { | ||||||
| 		Token:                       "token", | 		Token:                       "token", | ||||||
| 	} | 	} | ||||||
| 	err := config.Validate() | 	err := config.Validate() | ||||||
| 	assert.ErrorContains(t, err, "MinRunners '5' cannot be greater than MaxRunners '2", "Expected error about MinRunners > MaxRunners") | 	assert.ErrorContains(t, err, `MinRunners "5" cannot be greater than MaxRunners "2"`, "Expected error about MinRunners > MaxRunners") | ||||||
| } | } | ||||||
| 
 | 
 | ||||||
| func TestConfigValidationMissingToken(t *testing.T) { | func TestConfigValidationMissingToken(t *testing.T) { | ||||||
|  | @ -29,27 +29,47 @@ func TestConfigValidationMissingToken(t *testing.T) { | ||||||
| 		RunnerScaleSetId:            1, | 		RunnerScaleSetId:            1, | ||||||
| 	} | 	} | ||||||
| 	err := config.Validate() | 	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)) | 	expectedError := fmt.Sprintf(`GitHub auth credential is missing, token length: "%d", appId: %q, 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") | 	assert.ErrorContains(t, err, expectedError, "Expected error about missing auth") | ||||||
| } | } | ||||||
| 
 | 
 | ||||||
| func TestConfigValidationAppKey(t *testing.T) { | func TestConfigValidationAppKey(t *testing.T) { | ||||||
| 	config := &Config{ | 	t.Parallel() | ||||||
| 		AppID:                       1, | 
 | ||||||
| 		AppInstallationID:           10, | 	t.Run("app id integer", func(t *testing.T) { | ||||||
| 		ConfigureUrl:                "github.com/some_org/some_repo", | 		t.Parallel() | ||||||
| 		EphemeralRunnerSetNamespace: "namespace", | 		config := &Config{ | ||||||
| 		EphemeralRunnerSetName:      "deployment", | 			AppID:                       "1", | ||||||
| 		RunnerScaleSetId:            1, | 			AppInstallationID:           10, | ||||||
| 	} | 			ConfigureUrl:                "github.com/some_org/some_repo", | ||||||
| 	err := config.Validate() | 			EphemeralRunnerSetNamespace: "namespace", | ||||||
| 	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)) | 			EphemeralRunnerSetName:      "deployment", | ||||||
| 	assert.ErrorContains(t, err, expectedError, "Expected error about missing auth") | 			RunnerScaleSetId:            1, | ||||||
|  | 		} | ||||||
|  | 		err := config.Validate() | ||||||
|  | 		expectedError := fmt.Sprintf(`GitHub auth credential is missing, token length: "%d", appId: %q, 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") | ||||||
|  | 	}) | ||||||
|  | 
 | ||||||
|  | 	t.Run("app id as client id", func(t *testing.T) { | ||||||
|  | 		t.Parallel() | ||||||
|  | 		config := &Config{ | ||||||
|  | 			AppID:                       "Iv23f8doAlphaNumer1c", | ||||||
|  | 			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: %q, 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) { | func TestConfigValidationOnlyOneTypeOfCredentials(t *testing.T) { | ||||||
| 	config := &Config{ | 	config := &Config{ | ||||||
| 		AppID:                       1, | 		AppID:                       "1", | ||||||
| 		AppInstallationID:           10, | 		AppInstallationID:           10, | ||||||
| 		AppPrivateKey:               "asdf", | 		AppPrivateKey:               "asdf", | ||||||
| 		Token:                       "asdf", | 		Token:                       "asdf", | ||||||
|  | @ -59,7 +79,7 @@ func TestConfigValidationOnlyOneTypeOfCredentials(t *testing.T) { | ||||||
| 		RunnerScaleSetId:            1, | 		RunnerScaleSetId:            1, | ||||||
| 	} | 	} | ||||||
| 	err := config.Validate() | 	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)) | 	expectedError := fmt.Sprintf(`only one GitHub auth method supported at a time. Have both PAT and App auth: token length: "%d", appId: %q, 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") | 	assert.ErrorContains(t, err, expectedError, "Expected error about missing auth") | ||||||
| } | } | ||||||
| 
 | 
 | ||||||
|  |  | ||||||
|  | @ -5,6 +5,7 @@ import ( | ||||||
| 	"context" | 	"context" | ||||||
| 	"encoding/json" | 	"encoding/json" | ||||||
| 	"fmt" | 	"fmt" | ||||||
|  | 	"maps" | ||||||
| 	"math" | 	"math" | ||||||
| 	"net" | 	"net" | ||||||
| 	"strconv" | 	"strconv" | ||||||
|  | @ -169,15 +170,6 @@ func (b *ResourceBuilder) newScaleSetListenerConfig(autoscalingListener *v1alpha | ||||||
| 		metricsEndpoint = metricsConfig.endpoint | 		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 | 	var appInstallationID int64 | ||||||
| 	if id, ok := secret.Data["github_app_installation_id"]; ok { | 	if id, ok := secret.Data["github_app_installation_id"]; ok { | ||||||
| 		var err error | 		var err error | ||||||
|  | @ -189,7 +181,7 @@ func (b *ResourceBuilder) newScaleSetListenerConfig(autoscalingListener *v1alpha | ||||||
| 
 | 
 | ||||||
| 	config := listenerconfig.Config{ | 	config := listenerconfig.Config{ | ||||||
| 		ConfigureUrl:                autoscalingListener.Spec.GitHubConfigUrl, | 		ConfigureUrl:                autoscalingListener.Spec.GitHubConfigUrl, | ||||||
| 		AppID:                       appID, | 		AppID:                       string(secret.Data["github_app_id"]), | ||||||
| 		AppInstallationID:           appInstallationID, | 		AppInstallationID:           appInstallationID, | ||||||
| 		AppPrivateKey:               string(secret.Data["github_app_private_key"]), | 		AppPrivateKey:               string(secret.Data["github_app_private_key"]), | ||||||
| 		Token:                       string(secret.Data["github_token"]), | 		Token:                       string(secret.Data["github_token"]), | ||||||
|  | @ -207,6 +199,10 @@ func (b *ResourceBuilder) newScaleSetListenerConfig(autoscalingListener *v1alpha | ||||||
| 		Metrics:                     autoscalingListener.Spec.Metrics, | 		Metrics:                     autoscalingListener.Spec.Metrics, | ||||||
| 	} | 	} | ||||||
| 
 | 
 | ||||||
|  | 	if err := config.Validate(); err != nil { | ||||||
|  | 		return nil, fmt.Errorf("invalid listener config: %w", err) | ||||||
|  | 	} | ||||||
|  | 
 | ||||||
| 	var buf bytes.Buffer | 	var buf bytes.Buffer | ||||||
| 	if err := json.NewEncoder(&buf).Encode(config); err != nil { | 	if err := json.NewEncoder(&buf).Encode(config); err != nil { | ||||||
| 		return nil, fmt.Errorf("failed to encode config: %w", err) | 		return nil, fmt.Errorf("failed to encode config: %w", err) | ||||||
|  | @ -278,9 +274,7 @@ func (b *ResourceBuilder) newScaleSetListenerPod(autoscalingListener *v1alpha1.A | ||||||
| 	} | 	} | ||||||
| 
 | 
 | ||||||
| 	labels := make(map[string]string, len(autoscalingListener.Labels)) | 	labels := make(map[string]string, len(autoscalingListener.Labels)) | ||||||
| 	for key, val := range autoscalingListener.Labels { | 	maps.Copy(labels, autoscalingListener.Labels) | ||||||
| 		labels[key] = val |  | ||||||
| 	} |  | ||||||
| 
 | 
 | ||||||
| 	newRunnerScaleSetListenerPod := &corev1.Pod{ | 	newRunnerScaleSetListenerPod := &corev1.Pod{ | ||||||
| 		TypeMeta: metav1.TypeMeta{ | 		TypeMeta: metav1.TypeMeta{ | ||||||
|  |  | ||||||
|  | @ -1212,7 +1212,7 @@ func createJWTForGitHubApp(appAuth *GitHubAppAuth) (string, error) { | ||||||
| 	claims := &jwt.RegisteredClaims{ | 	claims := &jwt.RegisteredClaims{ | ||||||
| 		IssuedAt:  jwt.NewNumericDate(issuedAt), | 		IssuedAt:  jwt.NewNumericDate(issuedAt), | ||||||
| 		ExpiresAt: jwt.NewNumericDate(expiresAt), | 		ExpiresAt: jwt.NewNumericDate(expiresAt), | ||||||
| 		Issuer:    strconv.FormatInt(appAuth.AppID, 10), | 		Issuer:    appAuth.AppID, | ||||||
| 	} | 	} | ||||||
| 
 | 
 | ||||||
| 	token := jwt.NewWithClaims(jwt.SigningMethodRS256, claims) | 	token := jwt.NewWithClaims(jwt.SigningMethodRS256, claims) | ||||||
|  |  | ||||||
|  | @ -57,7 +57,7 @@ func TestClient_Identifier(t *testing.T) { | ||||||
| 		} | 		} | ||||||
| 		defaultAppCreds := &actions.ActionsAuth{ | 		defaultAppCreds := &actions.ActionsAuth{ | ||||||
| 			AppCreds: &actions.GitHubAppAuth{ | 			AppCreds: &actions.GitHubAppAuth{ | ||||||
| 				AppID:             123, | 				AppID:             "123", | ||||||
| 				AppInstallationID: 123, | 				AppInstallationID: 123, | ||||||
| 				AppPrivateKey:     "private key", | 				AppPrivateKey:     "private key", | ||||||
| 			}, | 			}, | ||||||
|  | @ -90,7 +90,7 @@ func TestClient_Identifier(t *testing.T) { | ||||||
| 				old:  defaultAppCreds, | 				old:  defaultAppCreds, | ||||||
| 				new: &actions.ActionsAuth{ | 				new: &actions.ActionsAuth{ | ||||||
| 					AppCreds: &actions.GitHubAppAuth{ | 					AppCreds: &actions.GitHubAppAuth{ | ||||||
| 						AppID:             456, | 						AppID:             "456", | ||||||
| 						AppInstallationID: 456, | 						AppInstallationID: 456, | ||||||
| 						AppPrivateKey:     "new private key", | 						AppPrivateKey:     "new private key", | ||||||
| 					}, | 					}, | ||||||
|  |  | ||||||
|  | @ -23,7 +23,8 @@ type multiClient struct { | ||||||
| } | } | ||||||
| 
 | 
 | ||||||
| type GitHubAppAuth struct { | type GitHubAppAuth struct { | ||||||
| 	AppID             int64 | 	// AppID is the ID or the Client ID of the application
 | ||||||
|  | 	AppID             string | ||||||
| 	AppInstallationID int64 | 	AppInstallationID int64 | ||||||
| 	AppPrivateKey     string | 	AppPrivateKey     string | ||||||
| } | } | ||||||
|  | @ -124,16 +125,11 @@ func (m *multiClient) GetClientFromSecret(ctx context.Context, githubConfigURL, | ||||||
| 		return m.GetClientFor(ctx, githubConfigURL, auth, namespace, options...) | 		return m.GetClientFor(ctx, githubConfigURL, auth, namespace, options...) | ||||||
| 	} | 	} | ||||||
| 
 | 
 | ||||||
| 	parsedAppID, err := strconv.ParseInt(appID, 10, 64) |  | ||||||
| 	if err != nil { |  | ||||||
| 		return nil, err |  | ||||||
| 	} |  | ||||||
| 
 |  | ||||||
| 	parsedAppInstallationID, err := strconv.ParseInt(appInstallationID, 10, 64) | 	parsedAppInstallationID, err := strconv.ParseInt(appInstallationID, 10, 64) | ||||||
| 	if err != nil { | 	if err != nil { | ||||||
| 		return nil, err | 		return nil, err | ||||||
| 	} | 	} | ||||||
| 
 | 
 | ||||||
| 	auth.AppCreds = &GitHubAppAuth{AppID: parsedAppID, AppInstallationID: parsedAppInstallationID, AppPrivateKey: appPrivateKey} | 	auth.AppCreds = &GitHubAppAuth{AppID: appID, AppInstallationID: parsedAppInstallationID, AppPrivateKey: appPrivateKey} | ||||||
| 	return m.GetClientFor(ctx, githubConfigURL, auth, namespace, options...) | 	return m.GetClientFor(ctx, githubConfigURL, auth, namespace, options...) | ||||||
| } | } | ||||||
|  |  | ||||||
|  | @ -137,7 +137,7 @@ etFcaQuTHEZyRhhJ4BU= | ||||||
| -----END PRIVATE KEY-----` | -----END PRIVATE KEY-----` | ||||||
| 
 | 
 | ||||||
| 	auth := &GitHubAppAuth{ | 	auth := &GitHubAppAuth{ | ||||||
| 		AppID:         123, | 		AppID:         "123", | ||||||
| 		AppPrivateKey: key, | 		AppPrivateKey: key, | ||||||
| 	} | 	} | ||||||
| 	jwt, err := createJWTForGitHubApp(auth) | 	jwt, err := createJWTForGitHubApp(auth) | ||||||
|  |  | ||||||
		Loading…
	
		Reference in New Issue