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.

Sign up for GitHub

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

Add support for volume mounts #4673

Merged
merged 4 commits into from
Dec 1, 2023
+56 −35
Merged

Add support for volume mounts #4673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9bc725b
Add support for volume mounts
modulo11 Nov 10, 2023
f66aec8
Adatpt unit test to include VolumeMounts
phil9909 Nov 20, 2023
d0f9214
Only accept volumeMounts with the name volume
c0d1ngm0nk3y Nov 28, 2023
1998543
Merge branch 'master' into k8s-volumes
anilkeshav27 Dec 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 29 additions & 24 deletions pkg/config/stepmeta.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import (
"github.com/pkg/errors"
)

const SupportedVolumeName = "volume"

// StepData defines the metadata for a step, like step descriptions, parameters, ...
type StepData struct {
Metadata StepMetadata `json:"metadata"`
Expand Down Expand Up @@ -105,25 +107,25 @@ type StepOutputs struct {
// Container defines an execution container
type Container struct {
//ToDo: check dockerOptions, dockerVolumeBind, containerPortMappings, sidecarOptions, sidecarVolumeBind
Command []string `json:"command"`
EnvVars []EnvVar `json:"env"`
Image string `json:"image"`
ImagePullPolicy string `json:"imagePullPolicy"`
Name string `json:"name"`
ReadyCommand string `json:"readyCommand"`
Shell string `json:"shell"`
WorkingDir string `json:"workingDir"`
Conditions []Condition `json:"conditions,omitempty"`
Options []Option `json:"options,omitempty"`
//VolumeMounts []VolumeMount `json:"volumeMounts,omitempty"`
Command []string `json:"command"`
EnvVars []EnvVar `json:"env"`
Image string `json:"image"`
ImagePullPolicy string `json:"imagePullPolicy"`
Name string `json:"name"`
ReadyCommand string `json:"readyCommand"`
Shell string `json:"shell"`
WorkingDir string `json:"workingDir"`
Conditions []Condition `json:"conditions,omitempty"`
Options []Option `json:"options,omitempty"`
VolumeMounts []VolumeMount `json:"volumeMounts,omitempty"`
Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

}

// ToDo: Add the missing Volumes part to enable the volume mount completely
// VolumeMount defines a mount path
// type VolumeMount struct {
// MountPath string `json:"mountPath"`
// Name string `json:"name"`
//}
type VolumeMount struct {
Name string `json:"name"`
MountPath string `json:"mountPath"`
}

// Option defines an docker option
type Option struct {
Expand Down Expand Up @@ -385,7 +387,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))
Copy link
Member Author

Choose a reason for hiding this comment

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

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


}

Expand Down Expand Up @@ -518,11 +520,14 @@ func ResolveMetadata(gitHubTokens map[string]string, metaDataResolver func() map
return metadata, nil
}

//ToDo: Enable this when the Volumes part is also implemented
//func volumeMountsAsStringSlice(volumeMounts []VolumeMount) []string {
// e := []string{}
// for _, v := range volumeMounts {
// e = append(e, fmt.Sprintf("%v:%v", v.Name, v.MountPath))
// }
// return e
//}
func volumeMountsAsStringSlice(volumeMounts []VolumeMount) []string {
c0d1ngm0nk3y marked this conversation as resolved.
Show resolved Hide resolved
e := []string{}
for _, v := range volumeMounts {
if v.Name != SupportedVolumeName {
log.Entry().Warningf("Unsupported volume name: %q, only %q is supported", v.Name, SupportedVolumeName)
continue
}
e = append(e, fmt.Sprintf("%v:%v", v.Name, v.MountPath))
}
return e
}
20 changes: 10 additions & 10 deletions pkg/config/stepmeta_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,10 +397,10 @@ func TestGetContextDefaults(t *testing.T) {
{Name: "opt1", Value: "optValue1"},
{Name: "opt2", Value: "optValue2"},
},
//VolumeMounts: []VolumeMount{
// {MountPath: "mp1", Name: "mn1"},
// {MountPath: "mp2", Name: "mn2"},
//},
VolumeMounts: []VolumeMount{
{MountPath: "mp1", Name: "volume"},
{MountPath: "mp2", Name: "mn2"},
},
},
},
Sidecars: []Container{
Expand All @@ -419,10 +419,10 @@ func TestGetContextDefaults(t *testing.T) {
{Name: "opt3", Value: "optValue3"},
{Name: "opt4", Value: "optValue4"},
},
//VolumeMounts: []VolumeMount{
// {MountPath: "mp3", Name: "mn3"},
// {MountPath: "mp4", Name: "mn4"},
//},
VolumeMounts: []VolumeMount{
{MountPath: "mp3", Name: "mn3"},
{MountPath: "mp4", Name: "volume"},
},
},
},
},
Expand Down Expand Up @@ -451,7 +451,7 @@ func TestGetContextDefaults(t *testing.T) {
assert.Equal(t, true, d.Defaults[0].Steps["testStep"]["dockerPullImage"], "dockerPullImage default not available")
assert.Equal(t, "/test/dir", d.Defaults[0].Steps["testStep"]["dockerWorkspace"], "dockerWorkspace default not available")
assert.Equal(t, []interface{}{"opt1 optValue1", "opt2 optValue2"}, d.Defaults[0].Steps["testStep"]["dockerOptions"], "dockerOptions default not available")
//assert.Equal(t, []interface{}{"mn1:mp1", "mn2:mp2"}, d.Defaults[0].Steps["testStep"]["dockerVolumeBind"], "dockerVolumeBind default not available")
assert.Equal(t, []interface{}{"volume:mp1"}, d.Defaults[0].Steps["testStep"]["dockerVolumeBind"], "dockerVolumeBind default not available")

assert.Equal(t, "/sidecar/command", d.Defaults[0].Steps["testStep"]["sidecarCommand"], "sidecarCommand default not available")
assert.Equal(t, map[string]interface{}{"env3": "val3", "env4": "val4"}, d.Defaults[0].Steps["testStep"]["sidecarEnvVars"], "sidecarEnvVars default not available")
Expand All @@ -461,7 +461,7 @@ func TestGetContextDefaults(t *testing.T) {
assert.Equal(t, "/sidecar/command", d.Defaults[0].Steps["testStep"]["sidecarReadyCommand"], "sidecarReadyCommand default not available")
assert.Equal(t, "/sidecar/dir", d.Defaults[0].Steps["testStep"]["sidecarWorkspace"], "sidecarWorkspace default not available")
assert.Equal(t, []interface{}{"opt3 optValue3", "opt4 optValue4"}, d.Defaults[0].Steps["testStep"]["sidecarOptions"], "sidecarOptions default not available")
//assert.Equal(t, []interface{}{"mn3:mp3", "mn4:mp4"}, d.Defaults[0].Steps["testStep"]["sidecarVolumeBind"], "sidecarVolumeBind default not available")
assert.Equal(t, []interface{}{"volume:mp4"}, d.Defaults[0].Steps["testStep"]["sidecarVolumeBind"], "sidecarVolumeBind default not available")
})

t.Run("Container conditions", func(t *testing.T) {
Expand Down
13 changes: 13 additions & 0 deletions vars/dockerExecute.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ void call(Map parameters = [:], body) {
}

def securityContext = securityContextFromOptions(config.dockerOptions)
def containerMountPath = containerMountPathFromVolumeBind(config.dockerVolumeBind)
if (env.POD_NAME && isContainerDefined(config)) {
container(getContainerDefined(config)) {
withEnv(dockerEnvVars) {
Expand All @@ -208,6 +209,7 @@ void call(Map parameters = [:], body) {
stashContent: config.stashContent,
stashNoDefaultExcludes: config.stashNoDefaultExcludes,
securityContext: securityContext,
containerMountPath: containerMountPath,
]

if (config.sidecarImage) {
Expand Down Expand Up @@ -379,6 +381,17 @@ def securityContextFromOptions(dockerOptions) {
return securityContext
}

/*
* Picks the first volumeBind option and translates it into containerMountPath, currently only one fix volume is supported
*/
@NonCPS
def containerMountPathFromVolumeBind(dockerVolumeBind) {
if (dockerVolumeBind) {
return dockerVolumeBind[0].split(":")[1]
Copy link

@patrick-lerch patrick-lerch Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

Also why only the first volume get's translated?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also why only the first volume get's translated?

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Member

@anilkeshav27 anilkeshav27 Nov 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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,

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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',
 /**

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

}
return ""
}

boolean isContainerDefined(config) {
Map containerMap = ContainerMap.instance.getMap()

Expand Down
5 changes: 4 additions & 1 deletion vars/dockerExecuteOnKubernetes.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,9 +582,12 @@ private List getContainerList(config) {
command : []
]
def resources = getResources(sideCarContainerName, config)
if(resources) {
if (resources) {
containerSpec.resources = resources
}
if (config.containerMountPath) {
containerSpec.volumeMounts = [[name: "volume", mountPath: config.containerMountPath]]
}
result.push(containerSpec)
}
return result
Expand Down