feat: Add feature to enable dynamic ec2 config via workflow labels#5003
feat: Add feature to enable dynamic ec2 config via workflow labels#5003edersonbrilhante wants to merge 25 commits intogithub-aws-runners:mainfrom
Conversation
|
@edersonbrilhante great to see this PR. |
edersonbrilhante
force-pushed
the
feat-ec2-dynamic-config
branch
3 times, most recently
from
e9c175c to
3dc5d6d
Compare
edersonbrilhante
force-pushed
the
feat-ec2-dynamic-config
branch
from
7ea3dc1 to
5dab5e4
Compare
edersonbrilhante
force-pushed
the
feat-ec2-dynamic-config
branch
from
5dab5e4 to
afdfb36
Compare
|
This is a really interesting feature! Just one suggestion from my side: would it be possible to support a whitelist of allowed instance types? Also, it could be really powerful to have some kind of feature-flag / policy control over which parts of the configuration are allowed to be dynamic. For example, in my org we don’t want developers to be able to select arbitrary AMIs (only a pre-approved set), but it would be awesome to still allow them to choose the instance type for workflow jobs, as long as it’s constrained to an allowed list. Maybe the "feature flag" is not even necessary as long as we could define the "allowed values" for each configuration, with this we could list only the pre-approved AMIs. |
|
@andrecastro I liked and makes a lot of sense to me. I just need more time to think about the implementation. And tbis PR is already big enough XD. I could create a following for adding this restricted values feature |
stuartp44
left a comment
stuartp44
left a comment
There was a problem hiding this comment.
I am happy to approve, but I do have a statement about incorrect labels and the effect on the process.
| expect(canRunJob(workflowLabels, runnerLabels, false)).toBe(true); | ||
| expect(canRunJob(workflowLabels, runnerLabels, false, false)).toBe(true); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Reference my previous point, what do I feel is valid here?
| }), | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
I think these tests work with all good values, but because we are in the user space, what about bad values and their effects? Is it maybe worth extending the tests to not trust the user data? I am not sure how the behaviour will be if someone makes a mistake, does it take the whole batch/process out?
There was a problem hiding this comment.
An example would be m5.large, could be m5,large. How will this change behaviour?
|
I also agree with what was previously mentioned; we probably need a safelist, as we don't want lateral movement when a compromised pipeline is used, especially with the VPC setting. Maybe worth some "allowed_instance_type" setting or something to that effect that can be checked against, and if not in the list, ignored. |
4 participants
Summary
This PR resumes and completes the work started in #4529.
It also allows to use any other dynamic labels with prefix
ghr-. Giving support for unique labels per job or per group of jobsIt ensures that EC2-specific config can be defined via
run-onsHow to test:
Use your regular labels, and add ghr-ec2-instance-type and ghr-ec2-image-id
In this case:
<regular-labels>