Allow reading AutoscalingRunnerSet githubConfigSecret from controller namespace #3714
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lacarvalho91
requested review from
mumoshu,
toast-gear,
rentziass and
a team
as code owners
lacarvalho91
commented
Aug 19, 2024
Comment on lines
+181
to
+219
| It("It should read GitHub config secret from controller namespace if ReadGitHubConfigSecretFromControllerNamespace is enabled", func() { | ||
| err := k8sClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: configSecret.Name, Namespace: autoscalingNS.Name}}) | ||
| Expect(err).ToNot(HaveOccurred(), "failed to delete github-config-secret") | ||
|
|
||
| controller2 := controller | ||
| controller2.ReadGitHubConfigSecretFromControllerNamespace = true | ||
| controller2.ControllerNamespace = controllerNS.Name | ||
|
|
||
| err = controller2.SetupWithManager(mgr) | ||
| Expect(err).NotTo(HaveOccurred(), "failed to setup controller") | ||
|
|
||
| controllerNamespaceConfigSecret := createDefaultSecret(GinkgoT(), k8sClient, controller2.ControllerNamespace) | ||
| runner := createAutoscalingRunnerSet("secret-test-asrs", autoscalingNS, buildVersion, controllerNamespaceConfigSecret.Name, 0, 1) | ||
| err = k8sClient.Create(ctx, runner) | ||
| Expect(err).NotTo(HaveOccurred(), "failed to create AutoscalingRunnerSet") | ||
|
|
||
| created := new(v1alpha1.AutoscalingRunnerSet) | ||
| // Check if runner scale set is created on service | ||
| Eventually( | ||
| func() (string, error) { | ||
| err := k8sClient.Get(ctx, client.ObjectKey{Name: runner.Name, Namespace: autoscalingRunnerSet.Namespace}, created) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| if _, ok := created.Annotations[runnerScaleSetIdAnnotationKey]; !ok { | ||
| return "", nil | ||
| } | ||
|
|
||
| if _, ok := created.Annotations[AnnotationKeyGitHubRunnerGroupName]; !ok { | ||
| return "", nil | ||
| } | ||
|
|
||
| return fmt.Sprintf("%s_%s", created.Annotations[runnerScaleSetIdAnnotationKey], created.Annotations[AnnotationKeyGitHubRunnerGroupName]), nil | ||
| }, | ||
| autoscalingRunnerSetTestTimeout, | ||
| autoscalingRunnerSetTestInterval).Should(BeEquivalentTo("1_testgroup"), "RunnerScaleSet should be created/fetched and update the AutoScalingRunnerSet's annotation") | ||
|
|
||
| }) |
There was a problem hiding this comment.
i've only added this test on the autoscalingrunnerset controller so far because I wanted to get some feedback before writing similar tests on the other controllers. I struggled to find a better way to write this test, it doesn't seem ideal that I'm setting up a second controller in this test but one is already setup in the BeforeEach but it isn't configurable. Any thoughts on this would be appreciated.
|
+1 to adding something like this. I have a similar usecase and not having the secret in the runner's namespace would be a good win towards securing the api secret. |
lacarvalho91
force-pushed
the
github-config-secret-controller-ns
branch
from
281095c
to
a86e981
Compare
… controller namespace
lacarvalho91
force-pushed
the
github-config-secret-controller-ns
branch
from
a86e981
to
0cde472
Compare
|
@mumoshu @toast-gear @rentziass hey all, just wondering if theres anything that can be done to progress this 🙂 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There isn't an open issue for this because the issue templates state that features for
actions.github.comcontrollers should be raised elsewhere, which I have done here but I haven't had a response yet. I also raised a discussion on this repo here.The reason I am raising this PR is because in my setup the
AutoscalingRunnerSets we provide are specific to users, so users can mount their own config maps and secrets. This is a problem because it means they can see the secret referenced ingithubConfigSecretsince it must currently be in the same namespace as the runner itself. We are configuring ARC to register at the organisation level so this means that users will have access to secrets for a GitHub App with "Self-hosted runners: Read and write" org level permissions, which we do not want.To resolve this problem, this PR introduces a flag for the controller that, when enabled, tells the controller to read the secret referenced by
githubConfigSecretfrom the same namespace as the controller. This is disabled by default.I spent some time looking into the code and I couldn't actually see a technical reason for the runner secret to be in the same namespace as the runner, because the secret that gets read by the runner pod is a completely different secret - that one is the jitconfig secret that is created on the fly by the controller.
I have run this on a local kind cluster and verified that it works, from what I can see the e2e tests are only for the legacy controllers so I haven't added any coverage there. If there are e2e tests for the new controllers that I should add to please let me know.