feat: pre job fork check by yanksyoon · Pull Request #648 · canonical/github-runner-operator

yanksyoon · 2025-10-30T03:45:42Z

Applicable spec:

Overview

introduce feature to control workflow execution from forked repositories. New configuration option allow-external-contributor (default: True) allows operators to restrict workflow runs from external contributors who don't have COLLABORATOR, MEMBER, or OWNER status on the repository. Setting the option False would allow strict access control only for users with COLLABORATOR, MEMBER, or OWNER status, useful for running only trusted workflows for private-endpoint runners.
Changing the allow-external-contributor configuration will flush existing runners.
Marks repo-policy-compliance external service for deprecation.

Rationale

Reduce deployment complexity by deprecating repo-policy-compliance service dependency
Enforce repository workflow access control via GitHub native security mechanisms.

Juju Events Changes

Module Changes

Library Changes

Checklist

The charm style guide was applied.
The contributing guide was applied.
The changes are compliant with ISD054 - Managing Charm Complexity
The documentation for charmhub is updated.
The PR is tagged with appropriate label (urgent, trivial, complex).
The changelog is updated with changes that affects the users of the charm.
The application version number is updated in github-runner-manager/pyproject.toml.

…sitories

…tests

…lass

…ation

…t fixture

cbartz

Thank you! Reviewed up to the tests. Have one comment regarding the env mapping (missed it in the previous PR).

Copilot

Pull request overview

This PR introduces a new allow-external-contributor configuration option to control workflow execution from forked repositories, deprecating the external repo-policy-compliance service dependency. When set to False (non-default), only users with COLLABORATOR, MEMBER, or OWNER status can run workflows, providing strict access control for private-endpoint runners.

Key Changes:

Added allow-external-contributor boolean config (default: True) with pre-job script checks for author association
Marked repo-policy-compliance-url and repo-policy-compliance-token as deprecated
Updated version to 0.8.0 and added changelog entry

Reviewed changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
config.yaml	Added new `allow-external-contributor` config option and marked repo-policy-compliance options as deprecated
src/charm_state.py	Added config constant, deprecation warning, and CharmConfig field for allow_external_contributor
src/charm.py	Added config tracking to stored state to trigger runner flush on config change
src/factories.py	Pass allow_external_contributor to ApplicationConfiguration
github-runner-manager/src/github_runner_manager/configuration/base.py	Added allow_external_contributor field to ApplicationConfiguration
github-runner-manager/src/github_runner_manager/openstack_cloud/models.py	Added allow_external_contributor field to OpenStackRunnerManagerConfig
github-runner-manager/src/github_runner_manager/manager/runner_scaler.py	Pass allow_external_contributor when building OpenStackRunnerManagerConfig
github-runner-manager/src/github_runner_manager/templates/pre-job.j2	Added author association check logic and deprecation notice for repo-policy-check
github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py	Pass allow_external_contributor to pre-job template and fix proxy config null check
github-runner-manager/src/github_runner_manager/repo_policy_compliance_client.py	Added deprecation warning in init
github-runner-manager/tests/unit/test_pre_job_script.py	Added comprehensive unit tests for pre-job script security checks
github-runner-manager/tests/integration/test_external_contributor_security.py	Added integration tests for external contributor security feature
github-runner-manager/tests/integration/factories.py	Added factory functions and configuration classes for integration tests
github-runner-manager/tests/integration/conftest.py	Added pytest fixtures for GitHub, OpenStack, and test configurations
github-runner-manager/tests/integration/github_helpers.py	Added helper functions for GitHub API operations in tests
github-runner-manager/tests/integration/application.py	Added application lifecycle management for integration tests
tests/integration/conftest.py	Updated GitHub client initialization to use new PyGithub API
tests/unit/conftest.py	Added allow_external_contributor to test fixture
tests/unit/test_charm_state.py	Added test coverage for new config option
tests/unit/test_factories.py	Updated factory test with allow_external_contributor
github-runner-manager/pyproject.toml	Bumped version from 0.7.0 to 0.8.0
docs/changelog.md	Added changelog entry for new feature
.github/workflows/test_github_runner_manager.yaml	Added integration test job with OpenStack configuration

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

github-runner-manager/tests/integration/conftest.py:1

Replacing the generator expression with a list literal removes the all() check's intended behavior of verifying that all values are truthy. The list [auth_url, password, ...] will always be truthy (non-empty), so all([...]) will always return True even if the values inside are None or empty strings. The original code all(val for val in (...)) correctly checked if all values were truthy.

# Copyright 2025 Canonical Ltd.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

cbartz

+1 (assuming secret handling change will be done in a follow-up)

* Initial plan * Update Crashed runner alert to reduce false positives - Change sum_over_time window from [1h] to [10m] - Increase threshold from > 0 to > 2 - Increase for duration from 0s to 15m This ensures we only alert if there are at least 3 crashes in a 10-minute window, and this state persists for 15 minutes. Co-authored-by: cbartz <4182921+cbartz@users.noreply.github.com> * ci: apply dockerhub mirror e2e (#661) * ci: microk8s use dockerhub mirror * ci: conditionally apply mirror if set * ci: use dockerhub mirror for microk8s in setup (#662) * ci: use dockerhub mirror for microk8s in setup * restart microk8s * chore(deps): update all non-major dependencies (#627) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix: Add missing timeouts to GitHub, OpenStack, and repo policy compliance API calls (#659) * Initial plan * Add timeouts to GitHub, OpenStack, and repo policy compliance API calls - Added timeout parameter to missing GitHub API calls (list_self_hosted_runners_for_repo, list_self_hosted_runners_for_org) - Replaced hardcoded timeout value (60) with TIMEOUT_IN_SECS constant in list_jobs_for_workflow_run - Added OPENSTACK_API_TIMEOUT constant (5 minutes) to openstack_cloud/constants.py - Added timeout to OpenStack session.get call in _determine_max_compute_api_version_by_cloud - Added REPO_POLICY_COMPLIANCE_TIMEOUT constant (5 minutes) to repo_policy_compliance_client.py - Added timeout to repo policy compliance service API call All timeouts are configured as constants at the top of files for easy maintenance as requested. Co-authored-by: cbartz <4182921+cbartz@users.noreply.github.com> * Fix test mock to support timeout parameter in OpenStack session.get Updated FakeOpenstackCloud.session to return self instead of dict, and added get() method that accepts timeout parameter. This allows the timeout parameter to be properly passed through in tests. Co-authored-by: cbartz <4182921+cbartz@users.noreply.github.com> * Apply black formatting to fake_runner_managers.py Co-authored-by: cbartz <4182921+cbartz@users.noreply.github.com> * Increase GitHub API timeout from 60 seconds to 5 minutes Changed TIMEOUT_IN_SECS from 60 to 300 (5 minutes) to be consistent with OpenStack and repo policy compliance timeouts, and to address the problem statement suggestion of using 5-minute timeouts for API calls. Co-authored-by: cbartz <4182921+cbartz@users.noreply.github.com> * Improve test mock implementation for consistency - Store session response mock as instance variable for consistent behavior - Rename 'endpoint' parameter to 'url' for clarity - Address code review feedback Co-authored-by: cbartz <4182921+cbartz@users.noreply.github.com> * ci: apply dockerhub mirror e2e (#661) * ci: microk8s use dockerhub mirror * ci: conditionally apply mirror if set * ci: use dockerhub mirror for microk8s in setup (#662) * ci: use dockerhub mirror for microk8s in setup * restart microk8s * chore(deps): update all non-major dependencies (#627) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Update changelog --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: cbartz <4182921+cbartz@users.noreply.github.com> Co-authored-by: Yanks Yoon <37652070+yanksyoon@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Christopher Bartz <christopher.bartz@canonical.com> * test: fix jubilant controller prefix issue (#666) * test: fix jubilant controller prefix issue * test: update latest jubilant native syntax * test: use model prefixed offer * test: remove str type casting * test: add controller name * test: try again, unintuitive lib API design * test: try again * fix: Remove -v in socat as in can incur in performance issues (#641) Co-authored-by: Christopher Bartz <christopher.bartz@canonical.com> * feat: pre job fork check (#648) * ci: organize secrets (#667) * ci: use secrets from env var mapping * test: disable github integration tests (preserve code as framework) * ci: map secret values to env vars * test: unify testing interface * test: unify testing interface * chore(deps): update all non-major dependencies (#664) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: cbartz <4182921+cbartz@users.noreply.github.com> Co-authored-by: Yanks Yoon <37652070+yanksyoon@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Christopher Bartz <christopher.bartz@canonical.com> Co-authored-by: javierdelapuente <javier.delapuente@canonical.com>

yanksyoon added 24 commits October 28, 2025 05:42

mark external repo-policy-compliance configurations as deprecated

2c94bc1

add allow-forked-repository charm configuration option

7ba3881

add deprecated configuration option warning

f19bbe7

use type casting for bool charm configs

8134a51

change config option name to allow-external-contributor

ef9b73c

parse allow-external-contributor charm config option

d753dc7

test: configuration parsing unit tests

89f2bad

fix: add use_aproxy default value

4ffd599

add allow_external_contributor configuration to application config

4ecc739

add deprecation warning for repo-policy-compliance-client

2498169

add allow_external_contributor option to application

dc3f022

test: initial forked repo testing

1e10fff

debug

d1d4c52

debug: ping me when test done

953fcb5

Merge remote-tracking branch 'origin' into feat/pre-job-fork-check

dc7c75d

feat: enhance integration tests for trusted and untrusted forked repo…

f2c7378

…sitories

feat: update GitHub client initialization to use Token authentication

e2350d0

chore: revert charm integration tests

32f9818

chore: revert charm integration tests

1eea0f7

chore: merge pre_job_contents_dict generation

56a182e

feat: add GitHub and OpenStack configuration options for integration …

596a559

…tests

refactor: simplify configuration file reading in RunningApplication c…

1e796ff

…lass

chore: remove deprecated OpenStack ARM64 options from pytest configur…

9200a9b

…ation

Merge remote-tracking branch 'origin' into feat/pre-job-fork-check

81fa6ec

yanksyoon changed the title ~~[WIP] feat: pre job fork check~~ feat: pre job fork check Nov 20, 2025

yanksyoon added 5 commits November 20, 2025 17:26

feat: add integration tests configuration to GitHub Actions and tox

d4a3453

chore: lint fixes

7134402

refactor: streamline OpenStack options and remove deprecated token_al…

a850b00

…t fixture

chore: update integration test environment variables for OpenStack

3ef26d0

Merge branch 'chore/cleanup-tests' into feat/pre-job-fork-check

5c3934b

yanksyoon requested a review from javierdelapuente as a code owner November 28, 2025 08:36

Merge remote-tracking branch 'origin' into feat/pre-job-fork-check

ee17ea1

yanksyoon requested a review from cbartz November 30, 2025 15:05

yanksyoon added 4 commits November 30, 2025 23:16

feat: flush runners on external contributor config change

461744d

feat: add allow_external_contributor to stored state

62ea95a

chore: increment app version & docs: update changelog

33d49e6

fix: use stored state

c323b96

cbartz reviewed Dec 1, 2025

View reviewed changes

Comment thread .github/workflows/test_github_runner_manager.yaml

cbartz requested a review from Copilot December 1, 2025 06:22

Copilot AI reviewed Dec 1, 2025

View reviewed changes

Comment thread tests/integration/conftest.py

Comment thread github-runner-manager/src/github_runner_manager/templates/pre-job.j2 Outdated

Comment thread github-runner-manager/src/github_runner_manager/repo_policy_compliance_client.py Outdated

weiiwang01 reviewed Dec 1, 2025

View reviewed changes

Comment thread github-runner-manager/src/github_runner_manager/templates/pre-job.j2 Outdated

yanksyoon and others added 4 commits December 1, 2025 06:52

fix: only check user association for pull_request events

30dc913

chore: whitespace for concatenated string

1a1a037

revert removed whitespace from test fixture docstring

dadbf41

Merge branch 'main' into feat/pre-job-fork-check

9886120

weiiwang01 reviewed Dec 1, 2025

View reviewed changes

Comment thread github-runner-manager/src/github_runner_manager/templates/pre-job.j2 Outdated

feat: check all events that can be triggered by external contributors

f51a1f4

weiiwang01 approved these changes Dec 1, 2025

View reviewed changes

yanksyoon and others added 3 commits December 3, 2025 15:30

Merge branch 'main' into feat/pre-job-fork-check

5697288

Merge branch 'main' into feat/pre-job-fork-check

b62b44f

test: cover different event types

f3ecd9f

yanksyoon requested a review from Copilot December 3, 2025 10:59

Copilot AI reviewed Dec 3, 2025

View reviewed changes

Comment thread config.yaml

yanksyoon requested a review from cbartz December 3, 2025 12:59

cbartz approved these changes Dec 3, 2025

View reviewed changes

yanksyoon enabled auto-merge (squash) December 3, 2025 13:49

yanksyoon merged commit 9564ae8 into main Dec 3, 2025
52 of 53 checks passed

yanksyoon deleted the feat/pre-job-fork-check branch December 3, 2025 13:50

cbartz pushed a commit that referenced this pull request Dec 5, 2025

feat: pre job fork check (#648)

98935f3

Navigation Menu

Repository navigation

feat: pre job fork check - #648

feat: pre job fork check#648
yanksyoon merged 95 commits into
mainfrom
feat/pre-job-fork-check

yanksyoon commented Oct 30, 2025 •

edited

Loading

Uh oh!

cbartz left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

cbartz left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

yanksyoon commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Rationale

Juju Events Changes

Module Changes

Library Changes

Checklist

Uh oh!

cbartz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

cbartz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yanksyoon commented Oct 30, 2025 •

edited

Loading