Adds new preRegistrationScript to run commands before the registration happens

What does this MR do?

Adds new preRegistrationScript to run commands before the registration happens

Why was this MR needed?

When using a secret manager like vault that will directly inject tokens into the pod, such as the runner token. We want to be able to run some commands to change the CI_SERVER_TOKEN environment variable before the runner registration happens.

This also solves #462 (closed)

What's the best way to test this MR?

What are the relevant issue numbers?

Hey @javion3! 👋

Welcome to our community! We're excited to have you here, and can't wait to review this first MR with you!

Thank you for your contribution to GitLab. Please refer to the contribution documentation for an overview of the process.

Did you know about our community forks? Working from there will make your contribution process easier. Please check it out!

When you're ready for a first review, post @gitlab-bot ready. If you know a relevant reviewer(s) (for example, someone that was involved in a related issue), you can also assign them directly with @gitlab-bot ready @user1 @user2.

At any time, if you need help, feel free to post @gitlab-bot help or initiate a mentor session on Discord. Read more on how to get help.

This message was generated automatically. You're welcome to improve it.

added 1st contribution Community contribution workflowin dev labels

assigned to @javion3

added Category:Runner Core devopsverify grouprunner labels

added sectionci label

mentioned in issue gitlab-org/quality/triage-reports#13172 (closed)

added maintenancepipelines typemaintenance labels

@gitlab-bot ready

Thanks @javion3 🙇

I see that CI pipeline is failing. For example: 🔴 https://gitlab.com/javion3/gitlab-runner/-/jobs/4597849260

Merging configuration from template file "/configmaps/config.template.toml" 
PANIC: The registration-token needs to be entered  
Registration attempt 24 of 30
Runtime platform                                    arch=amd64 os=linux pid=265 revision=503bdad3 version=16.2.0~beta.30.g503bdad3

Do you mind checking?

Hi @splattael Thanks for the review.

I'm not sure how I should go about fixing this. The cause of this test failure seems to be because the test wants to assert that the token for the registration token exists.

However, my PR aims to achieve the opposite of that, to allow no registration token to be supplied. As in my system, token are supplied through init containers, which meant that I do not need any secret volumes mounted to the container.

What should we do?

Hi @splattael I've re-done this PR into something else that will both solve my issue and another issue #462 (closed)

This new way of solving the problem should be better and less intrusive

Thanks @javion3 🙇

This looks good to me 👍

Since I am not a domain expert happy to pass this MR to one.

👋 @tmaczukin

Can you review this MR? 🙏

added workflowready for review label and removed workflowin dev label

@splattael, this Community contribution is ready for review.

• Do you have capacity and domain expertise to review this? We are mindful of your time, so if you are not able to take this on, please re-assign to one or more other reviewers.
• Add the workflowin dev label if the merge request needs action from the author.

This message was generated automatically. You're welcome to improve it.

requested review from @splattael

added workflowin dev label and removed workflowready for review label

added 1 commit

• d8cf03b4 - Removed optional secrets, added preRegistrationScript

Compare with previous version

changed title from Changed runner token secret mount to be conditional to Adds new preRegistrationScript to run commands before the registration happens

changed the description

mentioned in issue #462 (closed)

added 1 commit

• a0856fe1 - Update file values.yaml

Compare with previous version

@javion3 ,

#462 (closed) fails due to missing CI_SERVER_TOKEN at https://gitlab.com/gitlab-org/charts/gitlab-runner/-/blob/main/templates/configmap.yaml#L59, which is unset due to 1password connect behavior, as detailed in the issue.

if [ -z "${CI_SERVER_TOKEN+1}" ]
then
  # https://gitlab.com/gitlab-org/charts/gitlab-runner/-/issues/462
  echo "Setting CI_SERVER_TOKEN to empty string"
  CI_SERVER_TOKEN=""
fi

in preRegistrationScript should fix #462 (closed). Pipeline failure in this MR seems due to some other reasons.

Sorry for my previous comment that might suggested the wrong solution.

Sweeet, awesome!

changed this line in version 5 of the diff

added 1 commit

• b41eeaa4 - Update file values.yaml

Compare with previous version

added 1 commit

• 2840c08b - Update file values.yaml

Compare with previous version

added 1 commit

• ee6c498c - Update file values.yaml

Compare with previous version

requested review from @tmaczukin and removed review request for @splattael

added 5 commits

• ee6c498c...48b699c7 - 4 commits from branch gitlab-org/charts:main
• c74823f1 - Merge branch gitlab-runner:main into javion3-main-patch-94666

Compare with previous version

added 1 commit

• dabca2d4 - Update file values.yaml

Compare with previous version

added 1 commit

• d6c8e7ae - Update values.yaml

Compare with previous version

Hi @splattael @tmaczukin Can I please get some help on why the pipeline is failing? 🙏 Thank you

in templates/secrets.yaml

{{- if or .Values.runnerRegistrationToken .Values.runnerToken -}}

expects at least one of the tokens to be set.

However,

$ grep 'Token' values.yaml 
## DEPRECATED: The Registration Token for adding new Runners to the GitLab Server.
# runnerRegistrationToken: ""
## The Runner Token for adding new Runners to the GitLab Server. This must
# runnerToken: ""

means this secret is not created in the integration job.

I infer this from

MountVolume.SetUp failed for volume "projected-secrets" :
...
  projected-secrets:
    Type:                Projected (a volume that contains injected data from multiple sources)
    SecretName:          integration-test-d6c8e7ae-gitlab-runner
    SecretOptionalName:  <nil>
...

options could be to

• make this secret optional
• integration job creates this secret

I dont know which one is right way to go forward. I might start with making projected-secrets volume as optional as specified in https://kubernetes.io/docs/concepts/configuration/secret/#restriction-secret-must-exist.

So i've tried making the token secret inside the project-secrets volume optional. However, the tests doesn't seem to be picking up the conditional?

https://gitlab.com/javion3/gitlab-runner/-/jobs/4660222747

@at-platform24 Help please!

@tmaczukin Hi, can I please get some help on this. Not really sure why the pipeline is failing

@javion3 ,

sorry for the late reply. I had a look at the failed integration job in detail. Here is a successful job in Gitlab.com/gitlab-org group, which has line 100 as

+ helm install -f values.yaml --timeout 5m --wait --set gitlabUrl=https://gitlab.com,runnerRegistrationToken=[MASKED] integration-test-c93ebe73 .

which is executed with ${REGISTRATION_TOKEN}, a group/ or project variable.

When integration test is running in your fork, this variable is not available and thus integration test fails. I would take this test should succeed when run in upstream project.

to make this work in your fork, set

• $CI_SERVER_URL
• $REGISTRATION_TOKEN

variables to something valid, and then this test should pass. I have tested this in a personal fork and worked for me with a minor fix.

Please remove any leftover runner from this CI job from your project/ group runners, after the test is successful.

Also, you may want to revert changes from the unit test.

What do you mean by

to make this work in your fork, set

• $CI_SERVER_URL
• $REGISTRATION_TOKEN

I should set this variables in my fork? What should I be setting them to?

Hi @pslaughter, can I please get some extra help here. The changes are really small, but I don't understand why its causing the unit test and integration test jobs to fail?

Thanks for working on this @javion3! This project is a bit outside my wheelhouse, but looking at the CI errors, I'm assuming that adding the if condition in templates/deployment.yaml is causing both unit and integration tests to fail that are expecting this secret: name: items: ... to be rendered, but are no longer being rendered now that they have a more restrictive condition around them.

Either the behavior is correct and we need to update the tests to pass the condition, or the tests are correct and we need to make the if condition more permissive.

I'm not familiar with this project at all, so let me loop someone who might be more helpful 😄

@mnielsen you might be familiar with these changes. Could you please review and help out this Community contribution? Thanks!

@javion3

• set REGISTRATION_TOKEN with value from from https://gitlab.com/javion3/gitlab-runner -> settings -> CI/CD -> runners -> new project runner/ runner token
• set CI_SERVER_URL with value gitlab.com

in https://gitlab.com/javion3/gitlab-runner -> settings -> CI/CD -> variables.

Also amit-public/git-lab-runner-javion@62a930bc is needed in your branch.

here in my personal project pipelines (https://gitlab.com/amit-public/git-lab-runner-javion/-/pipelines) you can see working pipelines.

Sorry, I just still don't understand.

I've added the 2 variables in my personal repo, but the unit test stage is still failing? https://gitlab.com/javion3/gitlab-runner/-/jobs/4823678122

you will need to run a fresh pipeline. retrying jobs in existing pipelines does not get newly added variables.

just click run pipeline button at the top ( also make sure variable is not protected, and readable on MRs )

I don't maintain this project (I work on https://gitlab.com/gitlab-org/charts/gitlab), so I'll tag in the folks who were involved in !407 (merged) as that seems like the related component here.

cc @vshushlin @ratchade @ajwalker

in https://gitlab.com/javion3/gitlab-runner/-/jobs/4824497896

ERROR: Verifying runner... error                    runner=jetGB53UW status=only http or https scheme supported
PANIC: Failed to verify the runner.    

here gitlabUrl=gitlab.com should be gitlabUrl=https://gitlab.com

Thank you, @mnielsen for pinging me! 💚

@at-platform24 I replied to you on #462 (comment 1504938652)

If the only reason for adding this new script is to fix the "empty secret value" issue, then I think we should close this:

1. you can update to GitLab 16.0 and use new way of generating runner tokens, which will fix the issue
2. this MR will add more complexity to the project
3. and I think adding script is always a double-edged pattern. It allows users to do all sorts of things and fix all sorts of bugs, but it also makes the whole system very fragile - we can't guarantee compatibility with those user fixes.

cc @ratchade

Hi @vshushlin, this is a different use case.

When using a secret manager like vault that will directly inject tokens into the pod, such as the runner token. We want to be able to run some commands to change the CI_SERVER_TOKEN environment variable before the runner registration happens.

@at-platform24 Still no luck after changing the variable to https://gitlab.com 😞

@javion3 ,

integration test passed in latest pipeline, its unit test that is failing. perhaps time to revert the unit-test changes.

@javion3 @at-platform24 the unit tests fail because of this line: !413 (diffs)

The test expects volume to be created, and it isn't because of the if.

We want to be able to run some commands to change the CI_SERVER_TOKEN environment variable before the runner registration happens.

Can you tell what problem are trying to solve and what script do you want to run?

The script is just env variable exports, something like

export RUNNER_TOKEN=xxxxxx

@vshushlin That makes sense, that the pipeline fails because of the if but how does @at-platform24's pipeline succeed then?

@javion3

The script is just env variable exports, something like

Why can't you use secret or just the helm variable(which sets secret itself)?

@vshushlin

When using a secret manager like vault that will directly inject tokens into the pod, such as the runner token. We want to be able to run some commands to change the CI_SERVER_TOKEN environment variable before the runner registration happens.

☝ Because of this. The secrets gets directly injected into the pod without being present in any CI/CD or build environment.

Ok, @javion3! I see your current approach. 👍

Why you don't create the k8s secret using vault instead of injecting values to the pod directly?

The recommended approach is to have secrets directly injected into the pods. This is because the sidecar injector will also monitor the secret values for changes and can run commands inside the pod to restart any processes in case the secret values change.

If the secrets were created as k8s secrets, then you would need another tool that would keep the k8s secret in sync with vault secrets and re create pods when those secrets change.

@vshushlin I'm still confused on how to fix the unit tests 😞 Are you able to help me understand what I'm doing wrong?

The recommended approach is to have secrets directly injected into the pods. This is because the sidecar injector will also monitor the secret values for changes and can run commands inside the pod to restart any processes in case the secret values change.

If the secrets were created as k8s secrets, then you would need another tool that would keep the k8s secret in sync with vault secrets and re create pods when those secrets change.

I'm not an expert in k8s. @ratchade @ggeorgiev_gitlab do you know how this can be solved using our current secrets approach, or if it's already solved?

I'm still confused on how to fix the unit tests 😞 Are you able to help me understand what I'm doing wrong?

Let's first decide if that's the right, approach, and if yes, we can help you with tests 👍

Hi @vshushlin @ratchade @ggeorgiev_gitlab any updates?

Hi @vshushlin @ratchade @ggeorgiev_gitlab, just following up again

Hey, @javion3! I asked more people internally who have more k8s knowledge, waiting for the answer 🤞

Hi @vshushlin Any updates on this?

Hi @vshushlin just following up on this again

Hey, @javion3 ! Sorry for the delay...

Yeah, it's complicated. Maybe adding the preRegistrationScript is an ok idea 🤷

Adding `{{ if or .Values.runnerRegistrationToken .Values.runnerToken -}}` definitely won't work because some people set secrets directly, not through values file and we need to support this.

Can you help me to reproduce this whole setup with Vault? Just to describe all the steps I need to take to set it up(you can point me to the docs).

If the secrets were created as k8s secrets, then you would need another tool that would keep the k8s secret in sync with vault secrets and re create pods when those secrets change.

I asked our k8s experts, and the most promising answer is:

Not directly, you could for example hash the secret into an annotation of the deployment. Thus, whatever changes the secret would also have to change that hash. If you use kustomize or Helm that’s usually not a huge problem. We do something like this for the agent, here: https://gitlab.com/gitlab-org/charts/gitlab-agent/-/blob/main/templates/_helpers.tpl#L83

So maybe we should just implement such annotation, and then you use Vault to set the secrets? 🤔

Hashing the secret into the annotation of the deployment, means that it only needs readonly access of the k8s deployment to see the secret.

IMO that defeats the purpose of secrets 😂

The other way, with the script, to see the secret in the pod means that you would need exec access to the pod, which is much higher priveledged.

I do recognize that the runner token is a not so secret, secret.

Buuuut, I would still like to treat it as a secret nevertheless if possible

@javion3 👋

Ok, let's add the pre-registration-script 👍 I can't come up with the better solution, so I don't want to hold you any more

We need to remove the if though: !413 (diffs), it simply won't work for people who have secrets set-up in a different way.

cc @ratchade (I plan to send it to you for the maintainer review, so are you ok with the pre-registration-script idea?)

Hmmm, well inside the secret https://gitlab.com/javion3/gitlab-runner/-/blob/javion3-main-patch-94666/templates/secrets.yaml it uses the same condition

If we want to remove that if then we need a separate flag to specify whether or not secret is mounted.

Like a BYOS (bring your own secret) flag

@javion3

Hmmm, well inside the secret https://gitlab.com/javion3/gitlab-runner/-/blob/javion3-main-patch-94666/templates/secrets.yaml it uses the same condition

Yes, but there we create the secret. Basically, right now support only 2 modes:

1. set secrets in the values.yaml
2. BYOS

But we mount it anyway.

If I understand it correctly, it's not necessary for your case to not mount secrets, since you'll be able to override with your script anyway. You'll have to put something in these secrets though 🤷

The mounting is problematic because secret mounts become read-only file permissions. The directories basically become unwritable 😞

So, for my case, i can't have any mounts to that location.

Hi @vshushlin 👋 🙂 This is just a friendly reminder to make sure this is still on your tracks 🙂 Also, I don't have the full context but if there are changes needed would it be worth moving this one back to workflowin dev? 🙂

Hi @vshushlin 👋 This is just a friendly reminder to make sure this is still on your tracks 🙂

Another little nudge if you please @vshushlin 🏓

Hi @vshushlin 👋 🙂 Just another friendly nudge 🙏 🙂

@ratchade can I ask you to take over this and the Add an option to disable mounting of token secrets (!437) ? 💚

I've been struggling to find time for this, and I don't think I will be able to in the near future.

Hi @ratchade 👋 wanted to check in and see if this was something you might be able to take a look at. It popped up as idle on our community triage report.

Hi @ratchade 👋 🙂 This is just another friendly nudge to make sure this is still on your tracks 🙂

added 1 commit

• 29f6f1b3 - Update file deployment.yaml

Compare with previous version

added 3 commits

• 29f6f1b3...21dbccf3 - 2 commits from branch gitlab-org/charts:main
• 676848e0 - Merge branch gitlab-runner:main into javion3-main-patch-94666

Compare with previous version

added 1 commit

• 304e6691 - Update file deployment_test.yaml

Compare with previous version

added 1 commit

• bfdcdf8c - Update file deployment_test.yaml

Compare with previous version

added 1 commit

• 58f983ca - Update file deployment_test.yaml

Compare with previous version

added 1 commit

• 4c1323a9 - Update file deployment.yaml

Compare with previous version

added 1 commit

• 79870d38 - Update file deployment.yaml

Compare with previous version

added 1 commit

• c2bea301 - Update file deployment_test.yaml

Compare with previous version

added 1 commit

• e52f14f6 - Update deployment_test.yaml

Compare with previous version

added 13 commits

• e52f14f6...c93ebe73 - 12 commits from branch gitlab-org/charts:main
• 36d13eeb - Merge branch gitlab-runner:main into javion3-main-patch-94666

Compare with previous version

added 1 commit

• 07a90bd7 - Update deployment.yaml

Compare with previous version

@gitlab-bot ready

added workflowready for review label and removed workflowin dev label

@tmaczukin, this Community contribution is ready for review.

• Do you have capacity and domain expertise to review this? We are mindful of your time, so if you are not able to take this on, please re-assign to one or more other reviewers.
• Add the workflowin dev label if the merge request needs action from the author.

This message was generated automatically. You're welcome to improve it.

@gitlab-bot help

Hey there @pslaughter, could you please help @javion3 out? This message was generated automatically. You're welcome to improve it.

requested review from @pslaughter

requested review from @mnielsen and removed review request for @tmaczukin

removed review request for @pslaughter

removed review request for @mnielsen

Hi @mnielsen 👋

We noticed this MR is marked as workflowready for review but no reviewer is assigned. workflowin dev has automatically been applied to this MR based on the likelihood the review is finished. If additional reviews are still required, please assign a reviewer and reapply workflowready for review.

@javion3 you may also request a review by commenting @gitlab-bot ready. You can also assign reviewers directly using @gitlab-bot ready @user1 @user2 if you know the relevant reviewer(s), such as those who were involved in a related issue.

This message was generated automatically. You're welcome to improve it.

added workflowin dev label and removed workflowready for review label

@gitlab-bot ready

added workflowready for review label and removed workflowin dev label

@justin_ho, this Community contribution is ready for review.

• Do you have capacity and domain expertise to review this? We are mindful of your time, so if you are not able to take this on, please re-assign to one or more other reviewers.
• Add the workflowin dev label if the merge request needs action from the author.

This message was generated automatically. You're welcome to improve it.

@vshushlin Can you continue with the review since you already have context?

@javion3 next time please tag the existing reviewers if you are asking for new rounds of the same review. As per the bot:

You can also assign reviewers directly using @gitlab-bot ready @user1 @user2 if you know the relevant reviewer(s), such as those who were involved in a related issue.

requested review from @justin_ho

requested review from @vshushlin and removed review request for @justin_ho

@vshushlin, this Community contribution was recently assigned to you for review.

• Do you still have capacity to review this? We are mindful of your time, so if you are not able to take this on, please re-assign to one or more other reviewers.
• Add the workflowin dev label if the merge request needs action from the author.

This message was generated automatically. You're welcome to improve it.

added automation:reviewers-reminded label

@gitlab-bot ready @vshushlin

mentioned in merge request !389 (closed)

added 32 commits

• 07a90bd7...493c3410 - 31 commits from branch gitlab-org/charts:main
• dff42437 - Merge branch gitlab-runner:main into javion3-main-patch-94666

Compare with previous version

mentioned in merge request !437

added idle label

added linked-issue label

mentioned in issue gitlab-org/quality/triage-reports#14606 (closed)

removed idle label

added idle label

mentioned in issue gitlab-org/quality/triage-reports#14956 (closed)

mentioned in issue gitlab-org/quality/triage-reports#15052 (closed)

mentioned in issue gitlab-org/quality/triage-reports#15166 (closed)

removed idle label

added idle label

mentioned in issue gitlab-org/quality/triage-reports#15546 (closed)

mentioned in issue gitlab-org/quality/triage-reports#15811 (closed)

mentioned in issue gitlab-org/quality/triage-reports#15884 (closed)

removed idle label

added idle label

mentioned in issue gitlab-org/quality/triage-reports#16347 (closed)

mentioned in issue gitlab-org/quality/triage-reports#16454 (closed)

removed idle label

added cicomponents label

requested review from @ratchade and removed review request for @vshushlin

removed cicomponents label

added cicomponents label

removed cicomponents label

added cicomponents label

added idle label

mentioned in issue gitlab-org/quality/triage-reports#17340 (closed)

removed idle label

added idle label

mentioned in issue gitlab-org/quality/triage-reports#17790 (closed)

mentioned in issue gitlab-org/quality/triage-reports#17896 (closed)

removed idle label

added idle label

mentioned in issue gitlab-org/quality/triage-reports#18434 (closed)

mentioned in issue gitlab-org/quality/triage-reports#18630 (closed)

mentioned in issue gitlab-org/quality/triage-reports#18717 (closed)

Hi @ratchade 👋 Just another friendly reminder to make sure this is on your tracks 🙂

removed idle label

added idle label

Security policy violations have been resolved.

mentioned in issue gitlab-org/quality/triage-reports#19195 (closed)

mentioned in issue gitlab-org/quality/triage-reports#19280 (closed)

mentioned in issue gitlab-org/quality/triage-reports#19365 (closed)

mentioned in issue gitlab-org/quality/triage-reports#19535 (closed)

mentioned in issue gitlab-org/quality/triage-reports#19650 (closed)

mentioned in issue gitlab-org/quality/triage-reports#19662 (closed)

mentioned in issue gitlab-org/quality/triage-reports#19799 (closed)

mentioned in issue gitlab-org/quality/triage-reports#19909 (closed)

mentioned in issue gitlab-org/quality/triage-reports#19991 (closed)

mentioned in issue gitlab-org/quality/triage-reports#20126 (closed)

mentioned in issue gitlab-org/quality/triage-reports#20240 (closed)

mentioned in issue gitlab-org/quality/triage-reports#20322 (closed)

mentioned in issue gitlab-org/quality/triage-reports#20438 (closed)

mentioned in issue gitlab-org/quality/triage-reports#20575 (closed)

mentioned in issue gitlab-org/quality/triage-reports#20743 (closed)

mentioned in issue gitlab-org/quality/triage-reports#20823 (closed)

mentioned in issue gitlab-org/quality/triage-reports#20942 (closed)

mentioned in issue gitlab-org/quality/triage-reports#21132 (closed)

added stale label and removed idle label

Hi @nicolewilliams! 👋

To provide a better contribution experience, we're identifying older merge requests with no human activity in the past 120 days. Our goal is to bring these merge requests to completion or close them, enabling us to spend more time on active merge requests.

Review this merge request and determine if you should:

• Work on the provided feedback. Add @gitlab-bot ready when you need a review or are looking for more feedback.
• Don't know how to proceed? Ask for help from a merge request coach by adding @gitlab-bot help in a comment.
• Close the merge request.

Please see the handbook for additional details on this

(Improve this message?)

added automation:stale-reminded label

Closing due to inactivity. Please reopen if desired.

closed

Hi @javion3 👋

How was your code review experience with this merge request? Please tell us how we can continue to iterate and improve:

1. React with a 👍 or a 👎 on this comment to describe your experience.
2. Create a new comment starting with @gitlab-bot feedback below, and leave any additional feedback you have for us in the comment.

Request access to our community forks to receive complimentary access to GitLab Duo, our AI-powered features. With Code Suggestions, Chat, Root Cause Analysis and more AI-powered features, GitLab Duo helps to boost your efficiency and effectiveness by reducing the time required to write and understand code and pipelines. Visit the GitLab Duo documentation to learn more about the benefits.

🎉 See where you rank! Check your spot on the contributor leaderboard and unlock rewards.

Thanks for your help! ❤️

This message was generated automatically. Improve it or delete it.

removed stale label