-
Notifications
You must be signed in to change notification settings - Fork 103
[ALI] adds AUTH_GH_ORG and AUTH_GH_REPO to set/restrict authentication and management for a specific org/repo #7152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,8 @@ import { getBoolean, shuffleArrayInPlace } from './utils'; | |
export class Config { | ||
private static _instance: Config | undefined; | ||
|
||
readonly authGHOrg: string | undefined; | ||
readonly authGHRepo: string | undefined; | ||
readonly awsRegion: string; | ||
readonly awsRegionInstances: string[]; | ||
readonly awsRegionsToVpcIds: Map<string, Array<string>>; | ||
|
@@ -39,8 +41,8 @@ export class Config { | |
readonly scaleConfigOrg: string; | ||
readonly scaleConfigRepo: string; | ||
readonly scaleConfigRepoPath: string; | ||
readonly scaleUpMaxQueueTimeMinutes: number; | ||
readonly scaleUpChronRecordQueueUrl: string | undefined; | ||
readonly scaleUpMaxQueueTimeMinutes: number; | ||
readonly secretsManagerSecretsId: string | undefined; | ||
readonly sSMParamCleanupAgeDays: number; | ||
readonly sSMParamMaxCleanupAllowance: number; | ||
|
@@ -50,6 +52,10 @@ export class Config { | |
readonly vpcIdToSubnetIds: Map<string, Array<string>>; | ||
|
||
protected constructor() { | ||
/* istanbul ignore next */ | ||
this.authGHOrg = process.env.AUTH_GH_ORG; | ||
/* istanbul ignore next */ | ||
this.authGHRepo = process.env.AUTH_GH_REPO; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is the env var you're setting in terraform? |
||
this.awsRegion = process.env.AWS_REGION || 'us-east-1'; | ||
/* istanbul ignore next */ | ||
this.awsRegionInstances = process.env.AWS_REGION_INSTANCES?.split(',').filter((w) => w.length > 0) || []; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -263,14 +263,21 @@ export function backwardCompatibleGetRepoForgetRunnerTypes(ec2runner: RunnerInfo | |
return getRepo(ec2runner.repo as string); | ||
} | ||
|
||
export function backwardCompatibleGetAuthRepoForGetRunnerTypes(ec2runner: RunnerInfo): Repo { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit on the term "auth" in AuthRepo (see comment on variables.tf) |
||
if (Config.Instance.authGHOrg) { | ||
return getRepo(Config.Instance.authGHOrg, Config.Instance.authGHRepo); | ||
} | ||
return ec2runner.repo ? getRepo(ec2runner.repo as string) : { owner: ec2runner.org as string, repo: '' }; | ||
} | ||
|
||
export async function isEphemeralRunner(ec2runner: RunnerInfo, metrics: ScaleDownMetrics): Promise<boolean> { | ||
if (ec2runner.runnerType === undefined) { | ||
return false; | ||
} | ||
|
||
const runnerTypes = await getRunnerTypes( | ||
backwardCompatibleGetRepoForgetRunnerTypes(ec2runner), | ||
ec2runner.repo ? getRepo(ec2runner.repo as string) : { owner: ec2runner.org as string, repo: '' }, | ||
backwardCompatibleGetAuthRepoForGetRunnerTypes(ec2runner), | ||
metrics, | ||
); | ||
return runnerTypes.get(ec2runner.runnerType)?.is_ephemeral ?? false; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,12 +14,16 @@ export async function scaleUpChron(metrics: ScaleUpChronMetrics): Promise<void> | |
// 3. For each runner queued for longer than the minimum delay, try to scale it up | ||
|
||
try { | ||
const repo = getRepo(Config.Instance.scaleConfigOrg, Config.Instance.scaleConfigRepo); | ||
const authRepo = getRepo( | ||
Config.Instance.authGHOrg || Config.Instance.scaleConfigOrg, | ||
Config.Instance.authGHRepo || Config.Instance.scaleConfigRepo, | ||
); | ||
const scaleConfigRepo = getRepo(Config.Instance.scaleConfigOrg, Config.Instance.scaleConfigRepo); | ||
const validRunnerTypes = await getRunnerTypes( | ||
// For scaleUpChron, we don't have a situation where the auth repo is different from the config repo | ||
// so we can just pass the same repo for both parameters | ||
repo, | ||
repo, | ||
scaleConfigRepo, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the comment up above this line should be updated |
||
authRepo, | ||
metrics, | ||
Config.Instance.scaleConfigRepoPath, | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ import { SQSRecord } from 'aws-lambda'; | |
import { expBackOff } from './utils'; | ||
import { Metrics } from './metrics'; | ||
|
||
function getQueueUrl(evt: SQSRecord, sqs: SQS) { | ||
function getQueueUrl(evt: SQSRecord) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: There seem to be a lot of lint changes here. This PR would be way easier to review if split into a "lint only" changes PR and a "new functionality" PR |
||
const splitARN = evt.eventSourceARN.split(':'); | ||
// arn:aws:sqs:region:account-id:queue-name | ||
const region = splitARN[3]; | ||
|
@@ -44,7 +44,7 @@ export async function sqsChangeMessageVisibilityBatch( | |
) { | ||
const sqs: SQS = new SQS(); | ||
|
||
const queueUrl = getQueueUrl(events[0], sqs); | ||
const queueUrl = getQueueUrl(events[0]); | ||
const parameters = { | ||
Entries: events.map((evt) => { | ||
return { | ||
|
@@ -72,7 +72,7 @@ export async function sqsChangeMessageVisibilityBatch( | |
export async function sqsDeleteMessageBatch(metrics: Metrics, events: Array<SQSRecord>) { | ||
const sqs: SQS = new SQS(); | ||
|
||
const queueUrl = getQueueUrl(events[0], sqs); | ||
const queueUrl = getQueueUrl(events[0]); | ||
const parameters = { | ||
Entries: events.map((evt) => { | ||
return { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,9 +94,6 @@ export function getRepo(repoDef: string, repoName?: string): Repo { | |
} | ||
|
||
const repoArr = repoDef.split('/'); | ||
if (repoArr.length != 2) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this no longer needed? |
||
throw Error('getRepo: repoDef string must be in the format "owner/repo_name"'); | ||
} | ||
return { owner: repoArr[0], repo: repoArr[1] }; | ||
} catch (e) { | ||
console.error(`[getRepo]: ${e}`); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,8 @@ resource "aws_lambda_function" "scale_up_chron" { | |
# changes should reflect the changes in scale-up.tf | ||
environment { | ||
variables = { | ||
AUTH_GH_APP = var.auth_gh_app | ||
AUTH_GH_ORG = var.auth_gh_org | ||
CANT_HAVE_ISSUES_LABELS = join(",", var.cant_have_issues_labels) | ||
DATETIME_DEPLOY = local.datetime_deploy | ||
ENABLE_ORGANIZATION_RUNNERS = var.enable_organization_runners | ||
|
@@ -53,16 +55,16 @@ resource "aws_lambda_function" "scale_up_chron" { | |
MUST_HAVE_ISSUES_LABELS = join(",", var.must_have_issues_labels) | ||
REDIS_ENDPOINT = var.redis_endpoint | ||
REDIS_LOGIN = var.redis_login | ||
RETRY_SCALE_UP_CHRON_RECORD_QUEUE_URL = var.sqs_build_queue_retry.url | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. random: were these automated changes via some lint tool? Or did you change it manaully? Total side comment, but it would be nice if the automation was part of CI! |
||
RETRY_SCALE_UP_RECORD_DELAY_S = "60" | ||
RETRY_SCALE_UP_RECORD_JITTER_PCT = "0.5" | ||
RETRY_SCALE_UP_CHRON_RECORD_QUEUE_URL = var.sqs_build_queue_retry.url | ||
RUNNER_EXTRA_LABELS = var.runner_extra_labels | ||
SCALE_CONFIG_ORG = var.scale_config_org | ||
SCALE_CONFIG_REPO = var.scale_config_repo | ||
SCALE_CONFIG_REPO_PATH = var.scale_config_repo_path | ||
SECRETSMANAGER_SECRETS_ID = var.secretsmanager_secrets_id | ||
SCALE_UP_CHRON_HUD_QUERY_URL = var.retry_scale_up_chron_hud_query_url | ||
SCALE_UP_MAX_QUEUE_TIME_MINUTES = 30 | ||
SECRETSMANAGER_SECRETS_ID = var.secretsmanager_secrets_id | ||
|
||
AWS_REGIONS_TO_VPC_IDS = join( | ||
",", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,18 @@ variable "aws_region_instances" { | |
type = list(string) | ||
} | ||
|
||
variable "auth_gh_app" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this var needed? Don't we already get this token somehow? |
||
description = "GitHub App authentication token." | ||
type = string | ||
default = "" | ||
} | ||
|
||
variable "auth_gh_org" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. naming nit: can we have a name without "auth" in it? Maybe "ci_github_org" or just "github_org" The auth part is rather incidental here (a gh app can be authorized to affect multiple github orgs). The key thing we want to know is which github org is this particular autoscaler fleet supposed to be managing. |
||
description = "GitHub organization for the runners." | ||
type = string | ||
default = "" | ||
} | ||
|
||
variable "vpc_ids" { | ||
description = "The list of vpc_id for aws_region. keys; 'vpc' 'region'" | ||
type = list(map(string)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised this was needed