Add support for volume mounts #4673

c0d1ngm0nk3y · 2023-11-14T09:00:06Z

containerMountPath did not work and was partly commented out. For now, it only accepts mountPath with the name volume. If provided, the mountPath will be used as containerMountPath.

Changes

Tests
Documentation

c0d1ngm0nk3y · 2023-11-14T11:45:18Z

@anilkeshav27 As discussed, this is a prerequisite to make detectExecuteScan scan images as well.

anilkeshav27 · 2023-11-20T09:25:17Z

@c0d1ngm0nk3y , i have also forwarded this request to jenkins as a service to make sure that there is no restriction from their side if the pod template changes . i dont know their public github usernames so have forwarded the PR to them to also get a nod from their side

patrick-lerch

Some comments

pkg/config/stepmeta.go

patrick-lerch · 2023-11-20T09:51:14Z

vars/dockerExecute.groovy

+@NonCPS
+def containerMountPathFromVolumeBind(dockerVolumeBind) {
+    if (dockerVolumeBind) {
+        return dockerVolumeBind[0].split(":")[1]


Are you sure all customers will use : as separator?
Docker also provides the option to specify mounts via source and target properties. For those use cases your function is not going to work.
Not sure if this is supported by Piper.

Also why only the first volume get's translated?

Also why only the first volume get's translated?

It is containerMountPath. The user cannot influence WHAT is mounted, only to what path.

Are you sure all customers will use : as separator? Docker also provides the option to specify mounts via source and target properties. For those use cases your function is not going to work. Not sure if this is supported by Piper.

Do you mean this? This ist actually a mount command and not bind. So I do not think it is supported.

As part of the JaaS setup within SAP, we do not permit the use of hostPath volumes. This practice aligns with security best practices, especially in a shared cluster environment. The proposed change would have an impact on our internal customers

@basava12345 , @c0d1ngm0nk3y , the code only talks about the volume mounts in the main app container running in the pod and this functionality is unchanged, if i understand correctly the change can allow any path to be mounted in the side care container ?.

Since the environment is shared does that allow me to bring a path from any other project into my running pod with the side car container or is it a named volume that already exists which i want to mount at a specefic path in my container ?

if its an already existing volume who creates it ?

maybe i missing something here and so want to be sure,

@basava12345 , @c0d1ngm0nk3y , the code only talks about the volume mounts in the main app container running in the pod and this functionality is unchanged,

if i understand correctly the change can allow any path to be mounted in the side care container ?.

Yes, the path in the container can be anything, but that is only the path within the container. WHAT to mount is hardcoded (here: "volume").

Since the environment is shared does that allow me to bring a path from any other project into my running pod with the side car container

I don't se how...

or is it a named volume that already exists which i want to mount at a specefic path in my container ?

Yes. In case of Kubernetes, it is a emptyDir, I do not know what the other orchestrators are doing tbh.

if its an already existing volume who creates it ?

Hopefully, the orchestrator specific code. Or docker when starting it with the correct arguments.

maybe i missing something here and so want to be sure,

I am not 100% sure that we are talking about the same thing, similar to this comment.

I think, I now got your point. On Kubernetes, it is clear that an empty dir is mounted. But we will check what happens for ADO and GHA.

i think it will confusing for users with the current paramter. if someone called dockerExecute with dockerVolumeBind then there is a implicit understanding in code that the target path is taken into account and an emptyDir is mounted into the target path . and this changes the understanding of a traditional volume bind hostPath:targetPath which is not allowed in shared k8s cluster environment but can work outside a shared k8s cluster

we have a clean parameter in dockerExecuteOnKubernetes, couldn't we reuse that as a new param for dockerExecute ? so the behavior remains the same ?

/** * The path to which a volume should be mounted to. This volume will be available at the same * mount path in each container of the provided containerMap. The volume is of type emptyDir * and has the name 'volume'. With the additionalPodProperties parameter one can for example * use this volume in an initContainer. */ 'containerMountPath', /**

To summarise the meeting we had:

We will stick to volumeBind, but make sure that the Name is volume. With that, we would still be flexible to enhance in the future, but for the user there won't be surprises what ist actually mounted. We should document this restriction though.

c0d1ngm0nk3y · 2023-11-22T07:21:31Z

pkg/config/stepmeta.go

+	WorkingDir      string        `json:"workingDir"`
+	Conditions      []Condition   `json:"conditions,omitempty"`
+	Options         []Option      `json:"options,omitempty"`
+	VolumeMounts    []VolumeMount `json:"volumeMounts,omitempty"`


Maybe we should not go for "VolumeMounts []VolumeMounts", but for "MountPath string". That is what we do for kubernetes anyway (ignoring the name and take the first one). So the docker arguments would be in synch. The user could configure the mountPath of an empty and shared volume. WDYT?

Well, maybe it DOES make sense to keep the API generic to be able to easily extend the capabilities later.

c0d1ngm0nk3y · 2023-11-22T07:22:40Z

pkg/config/stepmeta.go

@@ -385,7 +385,7 @@ func (container *Container) commonConfiguration(keyPrefix string, config *map[st
 	}
 	putStringIfNotEmpty(*config, keyPrefix+"Workspace", container.WorkingDir)
 	putSliceIfNotEmpty(*config, keyPrefix+"Options", OptionsAsStringSlice(container.Options))
-	//putSliceIfNotEmpty(*config, keyPrefix+"VolumeBind", volumeMountsAsStringSlice(container.VolumeMounts))
+	putSliceIfNotEmpty(*config, keyPrefix+"VolumeBind", volumeMountsAsStringSlice(container.VolumeMounts))


Can't we put it into the dockerOptions? Ecspecially if we limit it to one empty volume.

anilkeshav27

lgtm

anilkeshav27 · 2023-11-29T14:55:25Z

/it-go

anilkeshav27 · 2023-11-29T15:10:26Z

will merge in the morning , to be sure if anything breaks, we can rework if needed

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

pbusko · 2023-11-30T09:46:22Z

/it-go

loewenstein · 2023-11-30T13:10:13Z

@anilkeshav27 Anything missing?

sonarqubecloud · 2023-12-01T10:00:30Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

anilkeshav27 · 2023-12-01T10:03:05Z

/it-go

anilkeshav27 · 2023-12-01T11:50:49Z

/it-go

vtintillier · 2023-12-04T08:06:37Z

This change broke our pipeline, can you please have a look at #4705 ? Thanks.

* Add support for volume mounts * Adatpt unit test to include VolumeMounts Co-authored-by: Ralf Pannemans <[email protected]> * Only accept volumeMounts with the name volume --------- Co-authored-by: Johannes Dillmann <[email protected]> Co-authored-by: Philipp Stehle <[email protected]> Co-authored-by: Anil Keshav <[email protected]>

c0d1ngm0nk3y requested a review from a team as a code owner November 14, 2023 09:00

c0d1ngm0nk3y mentioned this pull request Nov 15, 2023

feat(detectExecuteScan): Also scan images that are in the CPE #4678

Merged

2 tasks

patrick-lerch reviewed Nov 20, 2023

View reviewed changes

c0d1ngm0nk3y commented Nov 22, 2023

View reviewed changes

c0d1ngm0nk3y force-pushed the k8s-volumes branch from 8175b2d to cd31262 Compare November 28, 2023 15:07

anilkeshav27 approved these changes Nov 29, 2023

View reviewed changes

modulo11 and others added 3 commits November 30, 2023 10:46

Add support for volume mounts

9bc725b

Adatpt unit test to include VolumeMounts

f66aec8

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

Only accept volumeMounts with the name volume

d0f9214

pbusko force-pushed the k8s-volumes branch from 5a01706 to d0f9214 Compare November 30, 2023 09:46

Merge branch 'master' into k8s-volumes

1998543

anilkeshav27 merged commit 6efb21b into master Dec 1, 2023
11 checks passed

anilkeshav27 deleted the k8s-volumes branch December 1, 2023 12:33

vtintillier mentioned this pull request Dec 4, 2023

dockerExecute with dockerVolumeBind is now broken #4705

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for volume mounts #4673

Add support for volume mounts #4673

c0d1ngm0nk3y commented Nov 14, 2023 •

edited

Loading

c0d1ngm0nk3y commented Nov 14, 2023

anilkeshav27 commented Nov 20, 2023

patrick-lerch left a comment

patrick-lerch Nov 20, 2023 •

edited

Loading

patrick-lerch Nov 20, 2023

c0d1ngm0nk3y Nov 20, 2023

c0d1ngm0nk3y Nov 20, 2023

basava12345 Nov 20, 2023

anilkeshav27 Nov 21, 2023 •

edited

Loading

c0d1ngm0nk3y Nov 21, 2023

c0d1ngm0nk3y Nov 21, 2023

anilkeshav27 Nov 28, 2023

c0d1ngm0nk3y Nov 28, 2023

c0d1ngm0nk3y Nov 22, 2023

c0d1ngm0nk3y Nov 22, 2023

c0d1ngm0nk3y Nov 22, 2023

anilkeshav27 left a comment

anilkeshav27 commented Nov 29, 2023

anilkeshav27 commented Nov 29, 2023

pbusko commented Nov 30, 2023

loewenstein commented Nov 30, 2023

sonarqubecloud bot commented Dec 1, 2023

anilkeshav27 commented Dec 1, 2023

anilkeshav27 commented Dec 1, 2023

vtintillier commented Dec 4, 2023

Add support for volume mounts #4673

Add support for volume mounts #4673

Conversation

c0d1ngm0nk3y commented Nov 14, 2023 • edited Loading

Changes

c0d1ngm0nk3y commented Nov 14, 2023

anilkeshav27 commented Nov 20, 2023

patrick-lerch left a comment

Choose a reason for hiding this comment

patrick-lerch Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anilkeshav27 Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anilkeshav27 left a comment

Choose a reason for hiding this comment

anilkeshav27 commented Nov 29, 2023

anilkeshav27 commented Nov 29, 2023

pbusko commented Nov 30, 2023

loewenstein commented Nov 30, 2023

sonarqubecloud bot commented Dec 1, 2023

anilkeshav27 commented Dec 1, 2023

anilkeshav27 commented Dec 1, 2023

vtintillier commented Dec 4, 2023

c0d1ngm0nk3y commented Nov 14, 2023 •

edited

Loading

patrick-lerch Nov 20, 2023 •

edited

Loading

anilkeshav27 Nov 21, 2023 •

edited

Loading