Skip to content
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

[CRITICAL] self-hosted label is missing from all scale-set (AutoscalingRunnerSet) runners #3330

Closed
3 of 4 tasks
thesuperzapper opened this issue Mar 7, 2024 · 19 comments
Closed
3 of 4 tasks
Labels
bug Something isn't working gha-runner-scale-set Related to the gha-runner-scale-set mode needs triage Requires review from the maintainers

Comments

@thesuperzapper
Copy link

Checks

Controller Version

0.8.3

Deployment Method

Other

Checks

  • This isn't a question or user support case (For Q&A and community support, go to Discussions).
  • I've read the Changelog before submitting this issue and I'm sure it's not due to any recently-introduced backward-incompatible changes

To Reproduce

N/A

Describe the bug

Currently, runners managed by the new AutoscalingRunnerSet do not have the self-hosted label, this is an important security measure that prevents accidentally selecting public GitHub action runners (if I happen to use the same name as an upstream one, e.g. ubuntu-latest)

Additionally, this will make it impossible for many organizations to update to use AutoscalingRunnerSet, as they probably have thousands of workflows that select runs_on: ["self-hosted", "CUSTOM_LABEL"] given that this is explicitly recommended in the GitHub docs, which say:

All self-hosted runners have the self-hosted label. Using only this label will select any self-hosted runner. To select runners that meet certain criteria, such as operating system or architecture, we recommend providing an array of labels that begins with self-hosted (this must be listed first) and then includes additional labels as needed. When you specify an array of labels, jobs will be queued on runners that have all the labels that you specify.


Furthermore, because AutoscalingRunnerSet don't support custom labels, we can't even manually add self-hosted:

PS: separate from this issue, is there a particular reason for the regression in the ability to have multiple custom labels?

Describe the expected behavior

Every runner managed by ScaleSets have the self-hosted label, in addition to whatever their custom label it has.

Additional Context

N/A

Controller Logs

N/A

Runner Pod Logs

N/A
@thesuperzapper thesuperzapper added bug Something isn't working gha-runner-scale-set Related to the gha-runner-scale-set mode needs triage Requires review from the maintainers labels Mar 7, 2024
Copy link
Contributor

github-actions bot commented Mar 7, 2024

Hello! Thank you for filing an issue.

The maintainers will triage your issue shortly.

In the meantime, please take a look at the troubleshooting guide for bug reports.

If this is a feature request, please review our contribution guidelines.

@thesuperzapper
Copy link
Author

@nikola-jokic @Link- this is really a critical issue, and I would like to know your plan to resolve it.

@nikola-jokic
Copy link
Contributor

Hey @thesuperzapper,

This limitation is by design. Please read this discussion post and feel free to comment on it. The more feedback, the better ☺️.

@thesuperzapper
Copy link
Author

This limitation is by design. Please read this discussion post and feel free to comment on it. The more feedback, the better ☺️.

@nikola-jokic I think you may have misunderstood my request, I am not asking for arbitrary labels, just the addition of a common self-hosted label across all runners. Because the self-hosted label is "special", it can be treated separately and included across all runners, while being ignored for scaling purposes.

If not, you really need to update the GitHub docs, as they are currently incorrect to say "All self-hosted runners have the self-hosted label." and suggest people select runners using an array like runs_on: [self-hosted, MY_LABEL], and this is not possible with the latest actions-runner-controller.

@nikola-jokic
Copy link
Contributor

Hey @thesuperzapper,

I knew what you meant ☺️ but allowing self-hosted and scale-set name to both be matched is effectively allowing multiple labels. That is why I asked you to post your feedback on the discussion if you disagree.

@thesuperzapper
Copy link
Author

@nikola-jokic that discussion is more generic than this issue, it was about allowing multiple arbitrary labels.

Can we please reopen this issue, so people can specifically discuss the idea of re-adding self-hosted to all self-hosted runners?

@nikola-jokic
Copy link
Contributor

Hey @thesuperzapper,

I mean this scale set label is by design. It is not a bug. If the reason to re-open the issue is to have a discussion, please use discussions for that.

I want to point out that it is not more generic. There is a big difference between a single label and multiple labels. If the combination of two labels (self-hosted + scale-set-name) is allowed, then there would be no reason not to allow multiple labels. But please, use this discussion, it is the best place to get feedback about this problem ☺️

@danmanners
Copy link

danmanners commented Mar 12, 2024

I'm with @thesuperzapper on this. The idea that the self-hosted label has been removed from Self Hosted runners is insane, and they're completely correct. This is not a request to support multiple labels, it's to identify that the runners are in fact self hosted.

See here: @tmehlinger called it out really well here, and frankly this thread is incredibly dismissive of understanding WHY this is critical.

@nebu, is this something you folks can look into?

Also: @nikola-jokic I'd highly recommend a different choice of emote. It truly reads quite patronizing to customers who are actively bringing attention to issues, not making requests.

@nikola-jokic
Copy link
Contributor

Hey @danmanners,

Sorry if it feels that way, I'm not trying to be patronizing. I'm just explaining the current reasoning and saying that the feedback would be much more valuable if it were submitted on the discussion post. I simply kindly asked @thesuperzapper to submit it there.

@thesuperzapper
Copy link
Author

Personally, I view this as a bug.

So I did not think discussions were the appropriate place to raise it.

@gcaracuel
Copy link

Personally, I view this as a bug.

Totally agree with this. Adding self-hosted does not break the motivation of not to support labels as stated in the docs:

Runner scale sets is a group of homogeneous runners that can be assigned jobs from GitHub Actions. The number of active runners owned by a runner scale set can be controlled by auto-scaling runner solutions such as Actions Runner Controller (ARC).

Having self-hosted in addition to current label does not break the fact that runners are homogeneous in the set and does add a very useful and well spread (it was actually recommended by Github) usage pattern.

@thesuperzapper
Copy link
Author

I see no other option than to raise this with higher-level people at GitHub, there is clearly some kind of issue here with the GitHub Actions team.

@ashtom @kdaigle @mph4 I would like to bring this issue to your attention. The GHA team is refusing to resolve a critical security issue with GHA which could result in self-hosted customer jobs running on public runners.

Separately, I find it strange that labels for GitHub Actions Runners have effectively been deprecated without so much as a blog post or migration plan for your enterprise customers, see the comment from @Link- in #2921 (comment).

PS: Please understand that I love GitHub, and I only raise this issue because there has clearly been some kind of breakdown within the GHA team, and I would love to continue recommending GHA to my customers.

@nebuk89
Copy link

nebuk89 commented Mar 21, 2024

Hey!

As far as a security issue, if you need to scope down to ensure there is no conflict on jobs - you can use the runner group scoping which will achieve exactly the behavior you have highlighted as missing here.

i.e. if you create a self-hosted runner in a runner-group called ubuntu-latest, you could add scope for the runner group it's in to the same effect.

I will add some feedback onto the other threads relating to the wider ask/other needs to support this by the end of the week!

@jeffmccune
Copy link

jeffmccune commented Mar 21, 2024

@nebuk89 runner groups are an enterprise only feature, so not an option in many cases.

Even orgs that pay for this enterprise feature often have related projects contained in a separate org which do not have groups enabled. For example, my org is paid but we have a related open source project which is not enterprise.

See #2921 (reply in thread) for more info on how this doesn't just impact security, it also impacts availability and reliability.

@thesuperzapper
Copy link
Author

@nebuk89 separate to the security issue, it's also just kind of crazy to remove self-hosted labels (especially given it is still recommended to be selected on your docs.

I have many customers who have literally thousands of GHA jobs which now need to be updated and re-architected. Even in the simplest case (where they were previously only selecting a single label), they need to update the runs_on field to remove self-hosted.


Also, because you have deprecated runner labels, potentially thousands of GHA jobs need to be re-architected (suboptimally). It's no longer possible to achieve a scoped approach to selecting the kind of runner you want.

Previously you could define a scoped list of labels:

  • x86_64
  • Linux
  • Ubuntu
  • Ubuntu_22.04
  • Java
  • Java_8
  • ....

And only select the actual criteria that your job needs (which also acts as a form of documentation for the job).

Are you really expecting your customers to do a bunch of work because you don't want to implement a feature?


Finally, when you make a breaking change to something, you write a blog post, update the documentation, send an email, or really document the fact that there is now a new limitation literally ANYWHERE permanent (more than just a comment on a discussion thread).

This is completely aside to the fact that trying to replace the concept of labels with literally a "name" (which is what a single label really is), is frankly ridiculous.

@8BallDuVal
Copy link

I 100% agree with the comments in this post. GitHub really needs to be made aware of this and the impact it is causing.

See my reply on 2921.

I offered some solutions that could be used as a workaround. It's really unfortunate that the early adopters of github actions are getting punished for literally following github's own documentation about how you SHOULD use runner labels in an array.

This is a massive breaking change that serves no clear benefit as far as I can tell, and only causes problems for enterprise customers who have been using github actions for several years.

@juliandunn
Copy link

Hi all: I lead the Actions product management team at GitHub. We appreciate you raising awareness around the functionality of self-hosted labels for GitHub Actions Runner Controller. Our team is taking a deeper look into your feedback around this issue to determine its risk and severity, and we will follow up once we have an update to share.

As a note: we also encourage submissions to our bug bounty program if you believe there is a serious security issue in any of our products.

@8BallDuVal
Copy link

@juliandunn thank you for taking a closer look!

@nebuk89
Copy link

nebuk89 commented Apr 2, 2024

I have dropped a follow-up post discussing with @juliandunn here: #3340 so we can start to reduce the number of places we are discussing the same items.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working gha-runner-scale-set Related to the gha-runner-scale-set mode needs triage Requires review from the maintainers
Projects
None yet
Development

No branches or pull requests

8 participants