Skip to content

feat: support self-signed certificates #584

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

Merged
merged 9 commits into from
Mar 2, 2023
Merged

Conversation

baolsen
Copy link
Contributor

@baolsen baolsen commented Nov 30, 2022

Description

This change simply allows the Gitlab runner to register and run with a self-signed gitlab instance. The runner helper image can access such an instance.

Closes #563

Migrations required

NO

Verification

I've verified that the iniital change works in my own environment where I have self-signed certificates in use.
I have not verified that the changes after enabling pre-commit and tf docs etc work, but they should...

@baolsen
Copy link
Contributor Author

baolsen commented Nov 30, 2022

@kayman-mk please take a look.
I closed the linked PR as it was easier to create a new branch.

@npalm npalm self-requested a review November 30, 2022 22:35
@baolsen
Copy link
Contributor Author

baolsen commented Dec 1, 2022

Just a note, I could only get this to work with the docker executor.

With docker+machine I tried but could not find a way to install the certs on the child runners, I think it is not natively supported and no longer maintained either.

Perhaps if someone else in future wants this functionality they can provide a PR for it.

@baolsen baolsen force-pushed the cacert2 branch 2 times, most recently from fbf8a48 to 8e6244e Compare December 5, 2022 07:34
@baolsen baolsen requested review from kayman-mk and removed request for npalm December 9, 2022 06:13
@kayman-mk
Copy link
Collaborator

I will do a quick check in my environment. Code looks good.

@kayman-mk
Copy link
Collaborator

Verified that my runners in a complex scenario are still running (without using the new feature): Success

kayman-mk
kayman-mk previously approved these changes Dec 10, 2022
@npalm npalm self-requested a review December 11, 2022 13:48
Copy link
Collaborator

@npalm npalm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the PR on the default branch, docker machines are not starting healthy. Build are not starting.

NAME                                        ACTIVE   DRIVER      STATE     URL                    SWARM   DOCKER    ERRORS
runner-gksonazu-npalm-1670767825-0f2da2d9   -        amazonec2   Running   tcp://10.0.1.93:2376           Unknown   Unable to query dockerversion: Cannot connect to the docker engine endpoint

So seems the change in the start scripts breaks the runners.

@npalm
Copy link
Collaborator

npalm commented Dec 11, 2022

Tested the PR on the default branch, docker machines are not starting healthy. Build are not starting.

NAME                                        ACTIVE   DRIVER      STATE     URL                    SWARM   DOCKER    ERRORS
runner-gksonazu-npalm-1670767825-0f2da2d9   -        amazonec2   Running   tcp://10.0.1.93:2376           Unknown   Unable to query dockerversion: Cannot connect to the docker engine endpoint

So seems the change in the start scripts breaks the runners.

Another run, now it seems working. Would like to check the PR in a bit more detail

@npalm npalm changed the base branch from develop to main December 11, 2022 15:17
@npalm npalm dismissed kayman-mk’s stale review December 11, 2022 15:17

The base branch was changed.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@jpooler
Copy link

jpooler commented Feb 14, 2023

@npalm any chance this one can get merged in? being able to customize certs makes dind possible, among other things.

@kayman-mk kayman-mk changed the title feat: Support self-signed certificates feat: support self-signed certificates Feb 27, 2023
@kayman-mk
Copy link
Collaborator

kayman-mk commented Feb 27, 2023

@baolsen @npalm Same here. Merged the current main in, but the pipelines are no longer starting. Seems to be a problem with the cloud init script.

EDIT: Sorry, my fault. Merge conflict not solved.

@kayman-mk
Copy link
Collaborator

@npalm Could you please recheck? Looks good here. Tested the "no own certificate" path only. So nobody should be affected by this change.

@kayman-mk kayman-mk self-requested a review March 2, 2023 10:56
kayman-mk
kayman-mk previously approved these changes Mar 2, 2023
@kayman-mk kayman-mk requested a review from npalm March 2, 2023 10:57
kayman-mk and others added 2 commits March 2, 2023 11:58

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Collaborator

@npalm npalm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@baolsen sorry for the late response. But it seems the PR is not merged correctly. I made on one place a comment, but it seems there are multiple places where the reabase went wrong. Please can you fix the rebase?

kayman-mk and others added 2 commits March 2, 2023 15:22

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@npalm
Copy link
Collaborator

npalm commented Mar 2, 2023

@kayman-mk I also checked the default example. For me the PR is OK. But the only thing that is strange is. When I do a local rebase with main, I got merge conflicts. But form reason they are not shown here.

@npalm
Copy link
Collaborator

npalm commented Mar 2, 2023

@baolsen thank you for you contribution!

@npalm npalm merged commit 6c1180e into cattle-ops:main Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for self-signed certs on self-hosted gitlab instance
4 participants