Skip to content

Commit 1dbb88c

Browse files
nikola-jokicnikola-jokic
authored
Allow use of client id as an app id (#4057)
1 parent 43f1cd0 commit 1dbb88c

File tree

8 files changed

+59
-47
lines changed

8 files changed

+59
-47
lines changed

‎charts/gha-runner-scale-set/values.yaml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ githubConfigSecret:
1717
## (Variation B) When using a GitHub App, the syntax is as follows:
1818
# githubConfigSecret:
1919
# # NOTE: IDs MUST be strings, use quotes
20+
# # The github_app_id can be an app_id or the client_id
2021
# github_app_id: ""
2122
# github_app_installation_id: ""
2223
# github_app_private_key: |

‎cmd/ghalistener/config/config.go‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,9 @@ import (
1717
)
1818

1919
type Config struct {
20-
ConfigureUrl string `json:"configure_url"`
21-
AppID int64 `json:"app_id"`
20+
ConfigureUrl string `json:"configure_url"`
21+
// AppID can be an ID of the app or the client ID
22+
AppID string `json:"app_id"`
2223
AppInstallationID int64 `json:"app_installation_id"`
2324
AppPrivateKey string `json:"app_private_key"`
2425
Token string `json:"token"`
@@ -62,26 +63,26 @@ func (c *Config) Validate() error {
6263
}
6364

6465
if len(c.EphemeralRunnerSetNamespace) == 0 || len(c.EphemeralRunnerSetName) == 0 {
65-
return fmt.Errorf("EphemeralRunnerSetNamespace '%s' or EphemeralRunnerSetName '%s' is missing", c.EphemeralRunnerSetNamespace, c.EphemeralRunnerSetName)
66+
return fmt.Errorf("EphemeralRunnerSetNamespace %q or EphemeralRunnerSetName %q is missing", c.EphemeralRunnerSetNamespace, c.EphemeralRunnerSetName)
6667
}
6768

6869
if c.RunnerScaleSetId == 0 {
69-
return fmt.Errorf("RunnerScaleSetId '%d' is missing", c.RunnerScaleSetId)
70+
return fmt.Errorf(`RunnerScaleSetId "%d" is missing`, c.RunnerScaleSetId)
7071
}
7172

7273
if c.MaxRunners < c.MinRunners {
73-
return fmt.Errorf("MinRunners '%d' cannot be greater than MaxRunners '%d'", c.MinRunners, c.MaxRunners)
74+
return fmt.Errorf(`MinRunners "%d" cannot be greater than MaxRunners "%d"`, c.MinRunners, c.MaxRunners)
7475
}
7576

7677
hasToken := len(c.Token) > 0
77-
hasPrivateKeyConfig := c.AppID > 0 && c.AppPrivateKey != ""
78+
hasPrivateKeyConfig := len(c.AppID) > 0 && c.AppPrivateKey != ""
7879

7980
if !hasToken && !hasPrivateKeyConfig {
80-
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))
81+
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))
8182
}
8283

8384
if hasToken && hasPrivateKeyConfig {
84-
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))
85+
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))
8586
}
8687

8788
return nil

‎cmd/ghalistener/config/config_test.go‎

Lines changed: 35 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ func TestConfigValidationMinMax(t *testing.T) {
1818
Token: "token",
1919
}
2020
err := config.Validate()
21-
assert.ErrorContains(t, err, "MinRunners '5' cannot be greater than MaxRunners '2", "Expected error about MinRunners > MaxRunners")
21+
assert.ErrorContains(t, err, `MinRunners "5" cannot be greater than MaxRunners "2"`, "Expected error about MinRunners > MaxRunners")
2222
}
2323

2424
func TestConfigValidationMissingToken(t *testing.T) {
@@ -29,27 +29,47 @@ func TestConfigValidationMissingToken(t *testing.T) {
2929
RunnerScaleSetId: 1,
3030
}
3131
err := config.Validate()
32-
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))
32+
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))
3333
assert.ErrorContains(t, err, expectedError, "Expected error about missing auth")
3434
}
3535

3636
func TestConfigValidationAppKey(t *testing.T) {
37-
config := &Config{
38-
AppID: 1,
39-
AppInstallationID: 10,
40-
ConfigureUrl: "github.com/some_org/some_repo",
41-
EphemeralRunnerSetNamespace: "namespace",
42-
EphemeralRunnerSetName: "deployment",
43-
RunnerScaleSetId: 1,
44-
}
45-
err := config.Validate()
46-
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))
47-
assert.ErrorContains(t, err, expectedError, "Expected error about missing auth")
37+
t.Parallel()
38+
39+
t.Run("app id integer", func(t *testing.T) {
40+
t.Parallel()
41+
config := &Config{
42+
AppID: "1",
43+
AppInstallationID: 10,
44+
ConfigureUrl: "github.com/some_org/some_repo",
45+
EphemeralRunnerSetNamespace: "namespace",
46+
EphemeralRunnerSetName: "deployment",
47+
RunnerScaleSetId: 1,
48+
}
49+
err := config.Validate()
50+
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))
51+
assert.ErrorContains(t, err, expectedError, "Expected error about missing auth")
52+
})
53+
54+
t.Run("app id as client id", func(t *testing.T) {
55+
t.Parallel()
56+
config := &Config{
57+
AppID: "Iv23f8doAlphaNumer1c",
58+
AppInstallationID: 10,
59+
ConfigureUrl: "github.com/some_org/some_repo",
60+
EphemeralRunnerSetNamespace: "namespace",
61+
EphemeralRunnerSetName: "deployment",
62+
RunnerScaleSetId: 1,
63+
}
64+
err := config.Validate()
65+
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))
66+
assert.ErrorContains(t, err, expectedError, "Expected error about missing auth")
67+
})
4868
}
4969

5070
func TestConfigValidationOnlyOneTypeOfCredentials(t *testing.T) {
5171
config := &Config{
52-
AppID: 1,
72+
AppID: "1",
5373
AppInstallationID: 10,
5474
AppPrivateKey: "asdf",
5575
Token: "asdf",
@@ -59,7 +79,7 @@ func TestConfigValidationOnlyOneTypeOfCredentials(t *testing.T) {
5979
RunnerScaleSetId: 1,
6080
}
6181
err := config.Validate()
62-
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))
82+
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))
6383
assert.ErrorContains(t, err, expectedError, "Expected error about missing auth")
6484
}
6585

‎controllers/actions.github.com/resourcebuilder.go‎

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"context"
66
"encoding/json"
77
"fmt"
8+
"maps"
89
"math"
910
"net"
1011
"strconv"
@@ -169,15 +170,6 @@ func (b *ResourceBuilder) newScaleSetListenerConfig(autoscalingListener *v1alpha
169170
metricsEndpoint = metricsConfig.endpoint
170171
}
171172

172-
var appID int64
173-
if id, ok := secret.Data["github_app_id"]; ok {
174-
var err error
175-
appID, err = strconv.ParseInt(string(id), 10, 64)
176-
if err != nil {
177-
return nil, fmt.Errorf("failed to convert github_app_id to int: %v", err)
178-
}
179-
}
180-
181173
var appInstallationID int64
182174
if id, ok := secret.Data["github_app_installation_id"]; ok {
183175
var err error
@@ -189,7 +181,7 @@ func (b *ResourceBuilder) newScaleSetListenerConfig(autoscalingListener *v1alpha
189181

190182
config := listenerconfig.Config{
191183
ConfigureUrl: autoscalingListener.Spec.GitHubConfigUrl,
192-
AppID: appID,
184+
AppID: string(secret.Data["github_app_id"]),
193185
AppInstallationID: appInstallationID,
194186
AppPrivateKey: string(secret.Data["github_app_private_key"]),
195187
Token: string(secret.Data["github_token"]),
@@ -207,6 +199,10 @@ func (b *ResourceBuilder) newScaleSetListenerConfig(autoscalingListener *v1alpha
207199
Metrics: autoscalingListener.Spec.Metrics,
208200
}
209201

202+
if err := config.Validate(); err != nil {
203+
return nil, fmt.Errorf("invalid listener config: %w", err)
204+
}
205+
210206
var buf bytes.Buffer
211207
if err := json.NewEncoder(&buf).Encode(config); err != nil {
212208
return nil, fmt.Errorf("failed to encode config: %w", err)
@@ -278,9 +274,7 @@ func (b *ResourceBuilder) newScaleSetListenerPod(autoscalingListener *v1alpha1.A
278274
}
279275

280276
labels := make(map[string]string, len(autoscalingListener.Labels))
281-
for key, val := range autoscalingListener.Labels {
282-
labels[key] = val
283-
}
277+
maps.Copy(labels, autoscalingListener.Labels)
284278

285279
newRunnerScaleSetListenerPod := &corev1.Pod{
286280
TypeMeta: metav1.TypeMeta{

‎github/actions/client.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1212,7 +1212,7 @@ func createJWTForGitHubApp(appAuth *GitHubAppAuth) (string, error) {
12121212
claims := &jwt.RegisteredClaims{
12131213
IssuedAt: jwt.NewNumericDate(issuedAt),
12141214
ExpiresAt: jwt.NewNumericDate(expiresAt),
1215-
Issuer: strconv.FormatInt(appAuth.AppID, 10),
1215+
Issuer: appAuth.AppID,
12161216
}
12171217

12181218
token := jwt.NewWithClaims(jwt.SigningMethodRS256, claims)

‎github/actions/identifier_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ func TestClient_Identifier(t *testing.T) {
5757
}
5858
defaultAppCreds := &actions.ActionsAuth{
5959
AppCreds: &actions.GitHubAppAuth{
60-
AppID: 123,
60+
AppID: "123",
6161
AppInstallationID: 123,
6262
AppPrivateKey: "private key",
6363
},
@@ -90,7 +90,7 @@ func TestClient_Identifier(t *testing.T) {
9090
old: defaultAppCreds,
9191
new: &actions.ActionsAuth{
9292
AppCreds: &actions.GitHubAppAuth{
93-
AppID: 456,
93+
AppID: "456",
9494
AppInstallationID: 456,
9595
AppPrivateKey: "new private key",
9696
},

‎github/actions/multi_client.go‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,8 @@ type multiClient struct {
2323
}
2424

2525
type GitHubAppAuth struct {
26-
AppID int64
26+
// AppID is the ID or the Client ID of the application
27+
AppID string
2728
AppInstallationID int64
2829
AppPrivateKey string
2930
}
@@ -124,16 +125,11 @@ func (m *multiClient) GetClientFromSecret(ctx context.Context, githubConfigURL,
124125
return m.GetClientFor(ctx, githubConfigURL, auth, namespace, options...)
125126
}
126127

127-
parsedAppID, err := strconv.ParseInt(appID, 10, 64)
128-
if err != nil {
129-
return nil, err
130-
}
131-
132128
parsedAppInstallationID, err := strconv.ParseInt(appInstallationID, 10, 64)
133129
if err != nil {
134130
return nil, err
135131
}
136132

137-
auth.AppCreds = &GitHubAppAuth{AppID: parsedAppID, AppInstallationID: parsedAppInstallationID, AppPrivateKey: appPrivateKey}
133+
auth.AppCreds = &GitHubAppAuth{AppID: appID, AppInstallationID: parsedAppInstallationID, AppPrivateKey: appPrivateKey}
138134
return m.GetClientFor(ctx, githubConfigURL, auth, namespace, options...)
139135
}

‎github/actions/multi_client_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ etFcaQuTHEZyRhhJ4BU=
137137
-----END PRIVATE KEY-----`
138138

139139
auth := &GitHubAppAuth{
140-
AppID: 123,
140+
AppID: "123",
141141
AppPrivateKey: key,
142142
}
143143
jwt, err := createJWTForGitHubApp(auth)

0 commit comments

Comments
 (0)