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

Remove the support for register the runner outside the module #186

Closed
npalm opened this issue Feb 25, 2020 · 10 comments · May be fixed by #388
Closed

Remove the support for register the runner outside the module #186

npalm opened this issue Feb 25, 2020 · 10 comments · May be fixed by #388
Assignees
Labels
work-in-progress Issue/PR is worked, should not become stale
Milestone

Comments

@npalm
Copy link
Collaborator

npalm commented Feb 25, 2020

Due to backwards compatibly the module still supports the scenario to register a runner manual and pass the token. To reduce the complexity and number of scenarios this option will be removed.

@npalm npalm self-assigned this Mar 9, 2020
@trallnag
Copy link

A major version bump that throws out a bunch of deprecated stuff would be great

@npalm
Copy link
Collaborator Author

npalm commented Mar 3, 2021

fully agree, feel free to create a PR

@kayman-mk
Copy link
Collaborator

Ok, here we go. I will remove everything marked as deprecated. Let's clean up the stuff.

@kayman-mk
Copy link
Collaborator

@npalm Anything else I can remove which is not marked as deprectated?

@kayman-mk
Copy link
Collaborator

kayman-mk commented Oct 14, 2021

@npalm As we always have to store the token somewhere: Which option should be removed (you are referring in your first post)? EDIT: secure_parameter_store_runner_token_key?

I guess you point to gitlab_runner_registration_config["registration_token"]. But we need it to place the token from Gitlab somewhere. I usually store that token in a SSM parameter and reference it in the module configuration. Otherwise the token is visible in the code and checked in to the source control which is not a good idea at all.

@github-actions github-actions bot added the stale Issue/PR is stale and closed automatically label Jan 2, 2023
@kayman-mk kayman-mk removed the stale Issue/PR is stale and closed automatically label Jan 3, 2023
@github-actions github-actions bot added the stale Issue/PR is stale and closed automatically label Mar 19, 2023
@kayman-mk kayman-mk removed the stale Issue/PR is stale and closed automatically label Mar 19, 2023
kayman-mk added a commit that referenced this issue May 3, 2023
)

## Description

Adds the ability to read the Gitlab registration token from SSM. If no
registration token is passed in, it will look in SSM to find the token
to use. This prevents the token from being leaked as part of the
user_data.

```hcl
module "gitlab_runner" {
  # ...
  gitlab_runner_registration_config = {
    registration_token = "" # this is the default value too
    # ...
  }

  secure_parameter_store_gitlab_runner_registration_token_name = "name-of-ssm-parameter-holding-the-registration-token"
```

Closes #776
Precondition for #186 to get rid of pre-registered runners.

## Migrations required

NO

## Verification

I modified the runner-default example to not pass in a registration
token and added the token to SSM instead. Then I started up the runner
and confirmed that it successfully registered with Gitlab.

---------

Co-authored-by: Matthias Kay <github-public@matthiaskay.de>
Co-authored-by: Matthias Kay <matthias.kay@hlag.com>
@github-actions github-actions bot added the stale Issue/PR is stale and closed automatically label May 19, 2023
@kayman-mk kayman-mk removed the stale Issue/PR is stale and closed automatically label May 22, 2023
@github-actions github-actions bot added the stale Issue/PR is stale and closed automatically label Jul 22, 2023
@kayman-mk kayman-mk removed the stale Issue/PR is stale and closed automatically label Jul 27, 2023
@dsalaza4
Copy link

dsalaza4 commented Sep 23, 2023

We're currently having issues with the parameter store after upgrading from 6.3.1 to 6.5.2.

This only happens when we run a terraform plan with a read-only role. A terraform deploy works fine.

Logs say:

│ Error: reading SSM Parameter (ci-common-large-0-sentry-dsn): AccessDeniedException: User: arn:aws:sts::205810638802:assumed-role/dev/gitlab-20741933-1014135500-5149570679 is not authorized to perform: kms:Decrypt on resource: arn:aws:kms:us-east-1:205810638802:key/a4f312cd-f0bf-4701-9cbe-d044996161ec because no identity-based policy allows the kms:Decrypt action (Service: AWSKMS; Status Code: 400; Error Code: AccessDeniedException; Request ID: 0a802a13-e844-4f89-9d8f-9c31880dff35; Proxy: null)
│ 	status code: 400, request id: 7de4acec-8047-4ef5-a14d-d042971c1375

Keys went to a pending deletion state after making enable_kms = false, but ssm parameters kept pointing to them:

│ Error: reading SSM Parameter (ci-common-large-x86-0-sentry-dsn): InvalidKeyId: arn:aws:kms:us-east-1:205810638802:key/b1bd2030-d46d-41b7-89ef-3ed07a9181fb is pending deletion. (Service: AWSKMS; Status Code: 400; Error Code: KMSInvalidStateException; Request ID: 81ba4e7e-f4b0-4f76-b1c3-9f6df3eac93f; Proxy: null)
│ 
│   with module.runners["common-large-x86-0"].aws_ssm_parameter.runner_sentry_dsn,
│   on .terraform/modules/runners/main.tf line 27, in resource "aws_ssm_parameter" "runner_sentry_dsn":
│   27: resource "aws_ssm_parameter" "runner_sentry_dsn" {

When inspecting the parameter store I see this:

image

I will try removing all parameters and letting the module recreate them to see what happens.

@dsalaza4
Copy link

dsalaza4 commented Sep 23, 2023

We're currently having issues with the parameter store after upgrading from 6.3.1 to 6.5.2.

This only happens when we run a terraform plan with a read-only role. A terraform deploy works fine.

Logs say:

│ Error: reading SSM Parameter (ci-common-large-0-sentry-dsn): AccessDeniedException: User: arn:aws:sts::205810638802:assumed-role/dev/gitlab-20741933-1014135500-5149570679 is not authorized to perform: kms:Decrypt on resource: arn:aws:kms:us-east-1:205810638802:key/a4f312cd-f0bf-4701-9cbe-d044996161ec because no identity-based policy allows the kms:Decrypt action (Service: AWSKMS; Status Code: 400; Error Code: AccessDeniedException; Request ID: 0a802a13-e844-4f89-9d8f-9c31880dff35; Proxy: null)
│ 	status code: 400, request id: 7de4acec-8047-4ef5-a14d-d042971c1375

Keys went to a pending deletion state after making enable_kms = false, but ssm parameters kept pointing to them:

│ Error: reading SSM Parameter (ci-common-large-x86-0-sentry-dsn): InvalidKeyId: arn:aws:kms:us-east-1:205810638802:key/b1bd2030-d46d-41b7-89ef-3ed07a9181fb is pending deletion. (Service: AWSKMS; Status Code: 400; Error Code: KMSInvalidStateException; Request ID: 81ba4e7e-f4b0-4f76-b1c3-9f6df3eac93f; Proxy: null)
│ 
│   with module.runners["common-large-x86-0"].aws_ssm_parameter.runner_sentry_dsn,
│   on .terraform/modules/runners/main.tf line 27, in resource "aws_ssm_parameter" "runner_sentry_dsn":
│   27: resource "aws_ssm_parameter" "runner_sentry_dsn" {

When inspecting the parameter store I see this:

image I will try removing all parameters and letting the module recreate them to see what happens.

Manually removing all parameters and running a deploy with enable_kms = false recreated them with alias/aws/ssm. At this point running a terraform test with the read-only role worked fine.

If we try to set enable_kms = true again, terraform states that it will change the kms key for all parameter stores to the custom key again.

I would rather use aws owned keys for this (/alias/aws/ssm) for simplicity, does enable_kms = false mean that things are not being encrypted or that things are being encrypted using aws owned keys?

Would it make sense to separate these concepts so users can either set

enable_kms_aws_owned = true or enable_kms_aws_managed = true?

@kayman-mk
Copy link
Collaborator

This seems to be a valid point, yes. Could you please create a separate issue for this, @dsalaza4 ?

@kayman-mk kayman-mk added this to the Release v8 milestone Sep 30, 2023
@kayman-mk kayman-mk added the work-in-progress Issue/PR is worked, should not become stale label Nov 9, 2023
@kayman-mk
Copy link
Collaborator

kayman-mk commented Nov 9, 2023

Planned for the v8 release end of 2024. https://docs.gitlab.com/ee/architecture/blueprints/runner_tokens/index.html#proposal

@cattle-ops cattle-ops deleted a comment from github-actions bot Apr 20, 2024
@cattle-ops cattle-ops deleted a comment from github-actions bot Apr 20, 2024
@cattle-ops cattle-ops deleted a comment from github-actions bot Apr 20, 2024
@cattle-ops cattle-ops deleted a comment from github-actions bot Apr 20, 2024
@kayman-mk
Copy link
Collaborator

Will close this now as this is the official strategy from GitLab to handle Runners. Register them manually and go with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work-in-progress Issue/PR is worked, should not become stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants