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

[stable/jenkins] Added backup service account annotations and pod securitycontext #22730

Closed
wants to merge 13 commits into from
Closed

Conversation

KrzesloSzatan
Copy link

@KrzesloSzatan KrzesloSzatan commented Jun 10, 2020

Ive checked this on my local project and it works fine, adds annotation to the service account Jenkins-backup and sets proper securitycontext to the backup pod.

What this PR does / why we need it: Adds annotations to the jenkins-backup service account and securitycontext runAsUser and fsGroup to the pod generated by backup Cronjob

Which issue this PR fixes

  • fixes 22670

Special notes for your reviewer:

This is my first PR so dunno if ive done everything right, please guide me if there is something wrong.

Checklist

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

  • Variables are documented in the README.md
  • Title of the PR starts with chart name (e.g. [stable/mychartname])

@helm-bot helm-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 10, 2020
@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 Jun 10, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @KrzesloSzatan. 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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@KrzesloSzatan KrzesloSzatan changed the title [stable/jenkins] Added backup serbice account annotations and pod securitycontext [stable/jenkins] Added backup service account annotations and pod securitycontext Jun 10, 2020
Comment on lines +102 to +105
runAsUser: {{ default 0 .Values.backup.runAsUser }}
{{- if and (.Values.backup.runAsUser) (.Values.backup.fsGroup) }}
{{- if not (eq (int .Values.backup.runAsUser) 0) }}
fsGroup: {{ .Values.backup.fsGroup }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you see a use case where runAsUser for backup and Jenkins would be different? Otherwise we could re-use the existing value master.runAdUser. Then we only need to configure the user in one place.

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately, i have absolutely no idea atm. I have never needed that so cant tell if it is crucial to have them separated. For my case now, there shouldnt be any difference, but didnt check what will happen if i set the runAsUser and fsGroup in master values, untill few days ago i was working with older version ;) If there is no big problem i would leave 2 separate options, but probably either way works for me. Dunno about others.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest to use master.runAsUser

Could you change it and merge changes from Master into your branch?

@torstenwalter
Copy link
Collaborator

@KrzesloSzatan please also look into the DCO check. That needs to be fixed before it can be merged.

@torstenwalter
Copy link
Collaborator

@KrzesloSzatan any update here?

@KrzesloSzatan
Copy link
Author

@KrzesloSzatan please also look into the DCO check. That needs to be fixed before it can be merged.

I dont know what is wrong, ive done what is written in the guide like 15 times already and its always the same. Could you please tell me what im doing wrong ?
Ive done

git commit --amend --signoff
then
git push --force-with-lease origin master

But there is still a problem i dont know how to resolve :/

@torstenwalter
Copy link
Collaborator

@KrzesloSzatan your PR has three commits. One of them (85d941e) is not signed.

You could rebase your changes on master interactively (git rebase -i master) and squash those commits into one and sign it.

@helm-bot helm-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 16, 2020
jordanabderrachid and others added 13 commits July 16, 2020 11:15
)

* [stable/stackdriver-exporter] update k8s deployement api version

As of v1.16, k8s won't accept a deploymement with apps/v1beta1
or apps/v1beta2 apiVersion. The accepted apiVersion is apps/v1
that is available since k8s v1.9.

See https://kubernetes.io/blog/2019/07/18/api-deprecations-in-1-16

Signed-off-by: Jordan Abderrachid <[email protected]>

* bump chart version to 1.2.3

Signed-off-by: Jordan Abderrachid <[email protected]>
Signed-off-by: Maciej <[email protected]>
* Add support for GCP BackendConfig.

Signed-off-by: Anas <[email protected]>

* Bump chart version.

Signed-off-by: Anas <[email protected]>

* Document new variable.

Signed-off-by: Anas <[email protected]>
Signed-off-by: Maciej <[email protected]>
* [stable/unifi] adding functionality to mount extra volumes

This change adds possibility to specify additional volumes
when deploying Unifi controller.

Signed-off-by: Marcin Iwinski <[email protected]>

* fixing defaults in README.md

Signed-off-by: Marcin Iwinski <[email protected]>

* [stable/unifi] bumping version to 0.9.0

Signed-off-by: Marcin Iwinski <[email protected]>
Signed-off-by: Maciej <[email protected]>
Signed-off-by: William Hutcheson <[email protected]>

Co-authored-by: William Hutcheson <[email protected]>
Signed-off-by: Maciej <[email protected]>
… the Cluster Agent Pod(s) (#22715)

Signed-off-by: Matheus Weber da Conceição <[email protected]>
Signed-off-by: Maciej <[email protected]>
* Revert "[stable/fluentd] Fix missing serviceName in HPA (#21202)"

This reverts commit 1a877e5.

Signed-off-by: Jeremy Mathevet <[email protected]>

* Remove trailing tab

Signed-off-by: Jeremy Mathevet <[email protected]>

* newline at EOF

Signed-off-by: Jeremy Mathevet <[email protected]>
Signed-off-by: Maciej <[email protected]>
…rvice. (#22703)

Fixes #21887

Bump version to 0.10.0

Signed-off-by: Stephen Liang <[email protected]>
Signed-off-by: Maciej <[email protected]>
* update rabbitm-ha chart version from 1.46.1 to 1.46.2

Signed-off-by: yj.zeng <[email protected]>

* rabbitmq readiness probe fails in some cases.

Signed-off-by: yj.zeng <[email protected]>
Signed-off-by: Maciej <[email protected]>
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: KrzesloSzatan
To complete the pull request process, please assign mattfarina
You can assign the PR to them by writing /assign @mattfarina in a comment when ready.

The full list of commands accepted by this bot can be found 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

@KrzesloSzatan
Copy link
Author

@KrzesloSzatan your PR has three commits. One of them (85d941e) is not signed.

You could rebase your changes on master interactively (git rebase -i master) and squash those commits into one and sign it.

Well, ive done something, all commits are signed off. But dunno if i havent done more mess than it was before.

@torstenwalter
Copy link
Collaborator

@KrzesloSzatan DCO check passes, but we have quite some conflicts...

@KrzesloSzatan
Copy link
Author

@KrzesloSzatan DCO check passes, but we have quite some conflicts...

Maybe i should just clone repo once again and do my changes, then just push it again and create another PR ?

@torstenwalter
Copy link
Collaborator

That's also ok. Looking forward to your PR.

@stale
Copy link

stale bot commented Aug 22, 2020

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 Aug 22, 2020
@torstenwalter
Copy link
Collaborator

/hold This chart will be moved to https://github.com/jenkinsci/helm-charts see for details #23562
Would be great if you could create the PR in the new location.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2020
@stale stale bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 24, 2020
@scottrigby
Copy link
Member

👍

@scottrigby scottrigby closed this Sep 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe 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.