Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

Jenkins Configuration as Code #9057

Merged
merged 23 commits into from
Feb 8, 2019
Merged

Jenkins Configuration as Code #9057

merged 23 commits into from
Feb 8, 2019

Conversation

holmesb
Copy link
Contributor

@holmesb holmesb commented Nov 6, 2018

What this PR does / why we need it:

Implements Jenkins Configuration as Code. JCasC has been promoted as a top-level Jenkins project, and the corresponding Jenkins Enhancement Proposal has been accepted.

Which issue this PR fixes

Allows configuration of Jenkins using yaml configuration files that are concise and human-readable. The files also have user-friendly naming conventions making it easy for administrators to configure all Jenkins components.

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Chart Version bumped
  • Variables are documented in the README.md

@helm-bot helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). labels Nov 6, 2018
@k8s-ci-robot
Copy link
Contributor

Hi @holmesb. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 6, 2018
@holmesb
Copy link
Contributor Author

holmesb commented Nov 6, 2018

@davidkarlsen - passing lint now.

@helm-bot helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 7, 2018
Signed-off-by: Brendan Holmes <[email protected]>
@helm-bot helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 7, 2018
Signed-off-by: Brendan Holmes <[email protected]>
@helm-bot helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 7, 2018
@holmesb
Copy link
Contributor Author

holmesb commented Nov 13, 2018

@davidkarlsen, did you get a chance to review this at all?

@holmesb
Copy link
Contributor Author

holmesb commented Nov 24, 2018

Note: Currently this PR only loads Jenkins config at startup. As an option, should introduce a Jenkins "sidecar" - another container running inside the pod that monitors the JCasC configmap, creates the yaml files on the file-system, then executes the Jenkins CLI command to reload the config. The Grafana Helm chart is a good example of how "sidecar" functionality can be added. Would need to ensure this real-time CasC approach won't bork Jenkins if bad config is introduced. But this is something for a future PR. Startup-only is good enough for now.

@stale
Copy link

stale bot commented Dec 24, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

@stale stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 24, 2018
@helm-bot helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 6, 2019
@stale stale bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 6, 2019
@maorfr
Copy link
Member

maorfr commented Jan 8, 2019

/assign @maorfr

@maorfr
Copy link
Member

maorfr commented Jan 8, 2019

Hey @holmesb,

Thanks for contributing!
Can we get the lint-charts tests passing?

@torstenwalter
Copy link
Collaborator

Looks like the lint test was failing because the chart version was not incremented.

@holmesb I created a branch which fixes that I just don't know how to send a PR to your fork.
https://github.com/torstenwalter/charts/tree/feature/PR-9057-casc-bump-version

I'd also suggest to increase the version of these plugins to 1.4:
- configuration-as-code:1.3
- configuration-as-code-support:1.3

Signed-off-by: Brendan Holmes <[email protected]>
@holmesb
Copy link
Contributor Author

holmesb commented Feb 7, 2019

Hi @maorfr, good points. I've documented my sidecar image on dockerhub and included a link to the github repo. I'll also start getting into the habit of adding proper change logs to each version.

I've replaced a few if conditions with simpler else statements in config.yaml as you suggested. Let me know if it isn't what you had in mind. We've been happily doing on-the-fly config reloads for over two weeks now in our busy Jenkins, so hopefully this is in a good enough state to merge.

@helm-bot helm-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 7, 2019
@maorfr
Copy link
Member

maorfr commented Feb 7, 2019

/lgtm

Good job on this PR!

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: holmesb, maorfr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2019
@k8s-ci-robot k8s-ci-robot merged commit c3e8c0b into helm:master Feb 8, 2019
@JeanMertz
Copy link

I've been looking into JCasC, and the Helm chart. We've been using a fork of 0.6.0 since two years ago, and are looking into moving back to the latest upstream version. I had some thoughts yesterday that I wanted to post here, but I see the PR has since been merged.

Either way, here's a quick summary, feel free to ignore it if you don't think it's relevant.

I think using the JCasC plugin makes a lot of sense. At the same time, I also think we should take this opportunity to either 1) kill off 90% of the current configuration flags that can just as easily be defined using the JCasC yaml-based set-up (and do a backward breaking release), or 2) create a new chart with only the following features:

  • Persistence support (but slightly tweaked to support a useable ephemeral set-up, see [stable/jenkins] Allow disabling persistence of home path #11225)
  • Plugin installation support (since the JCasC plugin installation support is still considered beta)
  • All the relevant Kubernetes tweakables (RBAC, annotations, Ingress, ports, etc)
  • All the relevant flags that you need to set before Jenkins starts (JVM arguments, user to run Jenkins as, etc)
  • JCasC support
  • Init scripts (for those who want full access to the Jenkins API, for the things that JCasC does not yet support)

With this, you can eliminate a lot of extra configs, while still being able to achieve the exact same set-up as before, using JCasC. It would reduce the complexity of this chart, and we'd be able to point to the proper JCasC documentation for people to read, instead of having to read this Chart's documentation to be able to understand what you need to do to get the config you want.

As it currently stands, this PR (and the entire Jenkins chart) has so much complexity and hidden details based on the combination of configurables you set, that I'm almost inclined to ditch the chart and use a more simple set-up. With the above list of features "simple" would not mean "only useful to us", it would just mean less mental overhead when working with the chart, and moving to a set-up that is more in-line with the entire Jenkins ecosystem.

I've been experimenting locally with the JCasC setup, and the official Jenkins Docker image, and with a few volumes, JCasC, and some init scripts, you can get the exact set-up you want. Add to that the regular Kubernetes tweakables, and you end up where this chart is, without all the added complexity.

(note that I'm not criticising the current state, as I know how we got here, and a lot of the things that Jenkins properly supports these days (blueocean, kubernetes, JCasC) where just not easy to do one or two years ago, but given the current state, I think they are, and I think this chart would be better off if it reduced its complexity and off loaded the heavy lifting back to the official plugins)

@maorfr
Copy link
Member

maorfr commented Feb 8, 2019

I will prioritize any PR that simplifies this chart. Assign me to any such PR!

@JeanMertz
Copy link

I will prioritize any PR that simplifies this chart.

That's good to know, but I'm sure there are qualifiers. Do we want to make backward breaking changes, if so do we want to do them all in one big overhaul PR, and if not, do we see any simplifications we can make without breaking backward compatibility?


I'm in the process of reconfiguring our CI right now, starting from scratch. I plan on taking the current chart, stripping it of everything except the things I mentioned above, and see if that works.

I can create a PR with those changes once done (sometime next week), and we can see if anything can be upstreamed, or not.

@maorfr
Copy link
Member

maorfr commented Feb 8, 2019

My guidelines are:

  • as few breaking changes
  • PRs should be as small as possible

Maybe we can open an issue to follow this process. Each PR can reference this issue (but not close it). Once all the work is done, the issue can be closed. It will also be a way to involve additional community members!

What do you think? Would you like to lead this effort?

@holmesb
Copy link
Contributor Author

holmesb commented Feb 8, 2019

Simple is good. But I'd prefer not to break existing users who use alternate (non JCasC) methods of configuring Jenkins. Rather than be too brutal by discarding without warning, I'd prefer marking all non-JCasC configuration methods as deprecated for a period to nudge users off them and onto JCasC. Breakages will dissuade people from using and relying on this chart.

I suggest moving non-JCasC configuration to the bottom of the Readme table and as low as poss in the values. Mark them clearly as "deprecated" and add comments that these options will be removed shortly. Also warn the user in Notes.yaml output if they're using these options.

@JeanMertz, pls can you clarify what "proper JCasC documentation... [not] this Chart's documentation" means? In values.yaml and notes.yaml, I link clearly to the most authoritative JCasC reference: https://<jenkins_url>/configuration-as-code/reference, which contains info for the user's specific Jenkins implementation (their unique set of plugins, etc). Also to the CasC demos on github which give practical examples that users can copy and paste. Perhaps these links are needed more prominently in the Readme?

@maorfr
Copy link
Member

maorfr commented Feb 8, 2019

Would you prefer to discuss this in an issue instead of a merged PR? ;)

@holmesb
Copy link
Contributor Author

holmesb commented Feb 8, 2019

Would you prefer to discuss this in an issue instead of a merged PR? ;)

Done.

tbuchier pushed a commit to tbuchier/charts that referenced this pull request Feb 14, 2019
* Includes Jenkins Configuration as Code

Signed-off-by: Brendan Holmes <[email protected]>

* Minor README.md changes

Signed-off-by: Brendan Holmes <[email protected]>

* bumped CasC plugin version

Signed-off-by: Brendan Holmes <[email protected]>

* Adding auto config reload

Signed-off-by: Brendan Holmes <[email protected]>

* Needed to exclude some deployment config in the without-jcasc case.  Fixed tabulation and simplified jcasc example in values.yaml.  Tweaked documentation.

Signed-off-by: Brendan Holmes <[email protected]>

* Disabling auto-reload by default.

Signed-off-by: Brendan Holmes <[email protected]>

* Fixes error in corner-case where user has disabled config as code, but enabled auto-config.

Signed-off-by: Brendan Holmes <[email protected]>

* Expanded auto-config info in readme and added guidance for using non-internal identity db.
Doubled master memory limit since entered OOM restart loop when using config-as-code plugin.
Fixed missing casc_configs dir (in config.yaml & deployment.yaml) when not using auto-reload

Signed-off-by: Brendan Holmes <[email protected]>

* Sidecar was reloading once per key in the configmap when any single key had changed.  Resolved by creating separate configmaps, one for each key under ConfigScripts.
I was mistaken above that users only need Overall\Read rights to auto-reload.  Seems JCasC has higher privilege requirements than the CLI\API generally.  I've amended the Readme accordingly.  Process for enabling for LDAP\other ID store is still simple.
Fixed connectivity from sidecar when enabling non-root privileges by using a TCP port > 1024 (1044)
Bumped the sidecar image from 0.0.1 to 0.0.2 which a few improvements: faster, less error-prone startup by testing the Jenkins container's avaibility using SSH port instead of the main jenkins port.  This removes the need for an arbitary wait.  Also fixed "access denied" when enabling non-root privileges by creating the same jenkins 1000 user in the sidecar.

Signed-off-by: Brendan Holmes <[email protected]>

* Minor fix: configmap names now include the release name

Signed-off-by: Brendan Holmes <[email protected]>

* Update stable/jenkins/templates/config.yaml

Co-Authored-By: holmesb <[email protected]>
Signed-off-by: Brendan Holmes <[email protected]>

* Update stable/jenkins/templates/jcasc_config.yaml

Co-Authored-By: holmesb <[email protected]>
Signed-off-by: Brendan Holmes <[email protected]>

* Replaced a few if conditions with simpler else statements in config.yaml

Signed-off-by: Brendan Holmes <[email protected]>

* Bumping to the latest Config as Code plugin version.

Signed-off-by: Brendan Holmes <[email protected]>
@ricardompcarvalho
Copy link

@holmesb can you help me, please? How can i run the sidecar configuration to autoreload the casc configuration? I do it like that
image
but give me error, miss something?

@torstenwalter
Copy link
Collaborator

@ricardompcarvalho It would help if you could provide more details about the error.

@ricardompcarvalho
Copy link

The configuration that i show in image above is correct? I still need someone else configuration to do an auto-reload? @torstenwalter

@ricardompcarvalho
Copy link

@torstenwalter @holmesb gives me this
image

@holmesb
Copy link
Contributor Author

holmesb commented Feb 20, 2019

@ricardompcarvalho
Pls see latest values.yaml. Should be:

Sidecars:
    configAutoReload:
        enabled: true
        image: shadwell/k8s-sidecar:0.0.2
        imagePullPolicy: IfNotPresent
        resoureces:

etc. Sorry, we changed this in the latest version to accommodate multiple sidecars.

@ricardompcarvalho
Copy link

Yes, i already see that and change. Gives the error that i send above your answer @holmesb

@holmesb
Copy link
Contributor Author

holmesb commented Feb 20, 2019

This is a 403 Forbidden error. Are you using the internal jenkins user database, or an external one such as LDAP? The admin user will no longer exist if the later, hence you must change the Master.adminUser property to one that has Overall\Administer perms.

Anyway, this discussion doesn't belong in this merged PR. Please raise an issue.

@ricardompcarvalho
Copy link

@holmesb thank you

wmcdona89 pushed a commit to wmcdona89/charts that referenced this pull request Aug 30, 2020
* Includes Jenkins Configuration as Code

Signed-off-by: Brendan Holmes <[email protected]>

* Minor README.md changes

Signed-off-by: Brendan Holmes <[email protected]>

* bumped CasC plugin version

Signed-off-by: Brendan Holmes <[email protected]>

* Adding auto config reload

Signed-off-by: Brendan Holmes <[email protected]>

* Needed to exclude some deployment config in the without-jcasc case.  Fixed tabulation and simplified jcasc example in values.yaml.  Tweaked documentation.

Signed-off-by: Brendan Holmes <[email protected]>

* Disabling auto-reload by default.

Signed-off-by: Brendan Holmes <[email protected]>

* Fixes error in corner-case where user has disabled config as code, but enabled auto-config.

Signed-off-by: Brendan Holmes <[email protected]>

* Expanded auto-config info in readme and added guidance for using non-internal identity db.
Doubled master memory limit since entered OOM restart loop when using config-as-code plugin.
Fixed missing casc_configs dir (in config.yaml & deployment.yaml) when not using auto-reload

Signed-off-by: Brendan Holmes <[email protected]>

* Sidecar was reloading once per key in the configmap when any single key had changed.  Resolved by creating separate configmaps, one for each key under ConfigScripts.
I was mistaken above that users only need Overall\Read rights to auto-reload.  Seems JCasC has higher privilege requirements than the CLI\API generally.  I've amended the Readme accordingly.  Process for enabling for LDAP\other ID store is still simple.
Fixed connectivity from sidecar when enabling non-root privileges by using a TCP port > 1024 (1044)
Bumped the sidecar image from 0.0.1 to 0.0.2 which a few improvements: faster, less error-prone startup by testing the Jenkins container's avaibility using SSH port instead of the main jenkins port.  This removes the need for an arbitary wait.  Also fixed "access denied" when enabling non-root privileges by creating the same jenkins 1000 user in the sidecar.

Signed-off-by: Brendan Holmes <[email protected]>

* Minor fix: configmap names now include the release name

Signed-off-by: Brendan Holmes <[email protected]>

* Update stable/jenkins/templates/config.yaml

Co-Authored-By: holmesb <[email protected]>
Signed-off-by: Brendan Holmes <[email protected]>

* Update stable/jenkins/templates/jcasc_config.yaml

Co-Authored-By: holmesb <[email protected]>
Signed-off-by: Brendan Holmes <[email protected]>

* Replaced a few if conditions with simpler else statements in config.yaml

Signed-off-by: Brendan Holmes <[email protected]>

* Bumping to the latest Config as Code plugin version.

Signed-off-by: Brendan Holmes <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). lgtm Indicates that a PR is ready to be merged. ok-to-test size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants