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

feat(dockerExecute): Infer Kubernetes securityContext from dockerOptions #4557

Merged
merged 4 commits into from
Sep 18, 2023

Conversation

pbusko
Copy link
Member

@pbusko pbusko commented Sep 7, 2023

Changes

The dockerExecute step will take securityContext relevant arguments from dockerOptions and pass it to the dockerExecuteOnKubernetes

  • Tests
  • Documentation

@pbusko pbusko requested a review from a team as a code owner September 7, 2023 15:10
@pbusko pbusko marked this pull request as draft September 7, 2023 15:13
@pbusko pbusko force-pushed the custom-kubernetes-user branch from c54563e to 85a9a43 Compare September 8, 2023 08:41
@pbusko pbusko marked this pull request as ready for review September 8, 2023 08:41
@pbusko
Copy link
Member Author

pbusko commented Sep 8, 2023

/it

Copy link
Member

@anilkeshav27 anilkeshav27 left a comment

Choose a reason for hiding this comment

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

if i understand the PR correctly, we introduce a new step level configuration at the container level to have securitContext which then be passed to dockerExecute -> dockerExecuteOnKuberenetes as a example:

cnbBuild:
     containers:
          securityContext : 

however this configuration is only jenkins specific since its related to dockerExecuteOnKubernetes and will confuse end users who might add the same configuration and expect it to run on azure / github actions

rather than that, we can have a common solutions since we already have the possibility to provide the user / group via the dockerOptions .

dockerExecute is aware of dockerOptions and we can choose security context related metadata and pass it to dockerExecuteOnKubernetes since securityContext is a parameter for dockerExecuteOnKubernetes .

this will simplify the location of adding root users and will be a common solution for jenkins/github actions / azure task

for e.g in azure task we already pass the dockerOptions to the underlying azure task and we could do same for jenkins as well i.e

Decide (pick and choose only security context related params) the security context related parameters here

and pass the selected security context when calling dockerExecuteOnKubernetes here

@c0d1ngm0nk3y
Copy link
Member

and will confuse end users who might add the same configuration and expect it to run on azure / github actions

But why would this be any less confusing? Most of the dockerOptions have no effect, but some would get parsed and have magically meaning.

@pbusko pbusko force-pushed the custom-kubernetes-user branch 2 times, most recently from 2ce4282 to a26b3ce Compare September 12, 2023 13:59
@pbusko pbusko changed the title Allow running as different user on Kubernetes feat(dockerExecute): Infer Kubernetes securityContext from dockerOptions Sep 12, 2023
@pbusko pbusko requested a review from anilkeshav27 September 12, 2023 14:01
@pbusko
Copy link
Member Author

pbusko commented Sep 12, 2023

@anilkeshav27 could you please review the PR? We implemented changes as requested.

Copy link
Member

@CCFenner CCFenner left a comment

Choose a reason for hiding this comment

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

Thanks, looks good so far 👍

vars/dockerExecute.groovy Outdated Show resolved Hide resolved
@pbusko pbusko force-pushed the custom-kubernetes-user branch from a26b3ce to 609c1a5 Compare September 13, 2023 07:35
@pbusko
Copy link
Member Author

pbusko commented Sep 13, 2023

/it

@pbusko pbusko requested a review from CCFenner September 13, 2023 07:37
modulo11 and others added 3 commits September 14, 2023 12:39
Co-authored-by: Ralf Pannemans <[email protected]>
Co-authored-by: Johannes Dillmann <[email protected]>
Co-authored-by: Pavel Busko <[email protected]>
Co-authored-by: Ralf Pannemans <[email protected]>
Co-authored-by: Pavel Busko <[email protected]>
@pbusko pbusko force-pushed the custom-kubernetes-user branch from 609c1a5 to 0c09e16 Compare September 14, 2023 10:39
@pbusko
Copy link
Member Author

pbusko commented Sep 14, 2023

/it

1 similar comment
@loewenstein
Copy link

/it

Copy link
Member

@anilkeshav27 anilkeshav27 left a comment

Choose a reason for hiding this comment

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

lgtm to me , we could run only the go integration test with /it-go ,

@loewenstein
Copy link

@anilkeshav27 the Go integration tests seem to be fine and a review from you and/or @CCFenner is all I can see that's missing.

@loewenstein
Copy link

Thanks @anilkeshav27 and @CCFenner. Are you going to do the merge?

cc @c0d1ngm0nk3y @pbusko

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@CCFenner
Copy link
Member

/it-go

@loewenstein loewenstein merged commit caee8db into master Sep 18, 2023
@loewenstein loewenstein deleted the custom-kubernetes-user branch September 18, 2023 11:05
daskuznetsova pushed a commit to daskuznetsova/jenkins-library that referenced this pull request Oct 13, 2023
…ons (SAP#4557)

* Allow running as different user on Kubernetes

Co-authored-by: Ralf Pannemans <[email protected]>
Co-authored-by: Johannes Dillmann <[email protected]>
Co-authored-by: Pavel Busko <[email protected]>

* infer securityContext from dockerOptions

Co-authored-by: Ralf Pannemans <[email protected]>
Co-authored-by: Pavel Busko <[email protected]>

* verify --user flag value

---------

Co-authored-by: Johannes Dillmann <[email protected]>
Co-authored-by: Ralf Pannemans <[email protected]>
Co-authored-by: Anil Keshav <[email protected]>
andrew-kireev pushed a commit that referenced this pull request Oct 17, 2023
…ons (#4557)

* Allow running as different user on Kubernetes

Co-authored-by: Ralf Pannemans <[email protected]>
Co-authored-by: Johannes Dillmann <[email protected]>
Co-authored-by: Pavel Busko <[email protected]>

* infer securityContext from dockerOptions

Co-authored-by: Ralf Pannemans <[email protected]>
Co-authored-by: Pavel Busko <[email protected]>

* verify --user flag value

---------

Co-authored-by: Johannes Dillmann <[email protected]>
Co-authored-by: Ralf Pannemans <[email protected]>
Co-authored-by: Anil Keshav <[email protected]>
maxatsap pushed a commit to maxatsap/jenkins-library that referenced this pull request Jul 23, 2024
…ons (SAP#4557)

* Allow running as different user on Kubernetes

Co-authored-by: Ralf Pannemans <[email protected]>
Co-authored-by: Johannes Dillmann <[email protected]>
Co-authored-by: Pavel Busko <[email protected]>

* infer securityContext from dockerOptions

Co-authored-by: Ralf Pannemans <[email protected]>
Co-authored-by: Pavel Busko <[email protected]>

* verify --user flag value

---------

Co-authored-by: Johannes Dillmann <[email protected]>
Co-authored-by: Ralf Pannemans <[email protected]>
Co-authored-by: Anil Keshav <[email protected]>
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.

6 participants