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

Jump to bottom

JENKINS-46253 Add support for subPath to all volume types #1031

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

JENKINS-46253 Add support for subPath to all volume types #1031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
76 changes: 31 additions & 45 deletions src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,24 +24,16 @@

package org.csanchez.jenkins.plugins.kubernetes;

import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.function.Function;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.TcpSlaveAgentListener;
import hudson.Util;
import io.fabric8.kubernetes.api.model.PodSpecFluent;
import hudson.slaves.SlaveComputer;
import io.fabric8.kubernetes.api.model.*;
import io.fabric8.kubernetes.api.model.PodFluent.MetadataNested;
import io.fabric8.kubernetes.api.model.PodFluent.SpecNested;
import io.fabric8.kubernetes.client.utils.Serialization;
import io.jenkins.lib.versionnumber.JavaSpecificationVersion;
import jenkins.model.Jenkins;
import org.apache.commons.lang.StringUtils;
import org.csanchez.jenkins.plugins.kubernetes.model.TemplateEnvVar;
import org.csanchez.jenkins.plugins.kubernetes.pipeline.PodTemplateStepExecution;
Expand All @@ -50,37 +42,19 @@
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

import hudson.TcpSlaveAgentListener;
import hudson.slaves.SlaveComputer;
import io.fabric8.kubernetes.api.model.Container;
import io.fabric8.kubernetes.api.model.ContainerBuilder;
import io.fabric8.kubernetes.api.model.ContainerPort;
import io.fabric8.kubernetes.api.model.EnvVar;
import io.fabric8.kubernetes.api.model.ExecAction;
import io.fabric8.kubernetes.api.model.LocalObjectReference;
import io.fabric8.kubernetes.api.model.Pod;
import io.fabric8.kubernetes.api.model.PodBuilder;
import io.fabric8.kubernetes.api.model.PodFluent.MetadataNested;
import io.fabric8.kubernetes.api.model.PodFluent.SpecNested;
import io.fabric8.kubernetes.api.model.Probe;
import io.fabric8.kubernetes.api.model.ProbeBuilder;
import io.fabric8.kubernetes.api.model.Quantity;
import io.fabric8.kubernetes.api.model.ResourceRequirements;
import io.fabric8.kubernetes.api.model.ResourceRequirementsBuilder;
import io.fabric8.kubernetes.api.model.Volume;
import io.fabric8.kubernetes.api.model.VolumeMount;
import io.fabric8.kubernetes.api.model.VolumeMountBuilder;
import io.fabric8.kubernetes.client.utils.Serialization;
import io.jenkins.lib.versionnumber.JavaSpecificationVersion;
import jenkins.model.Jenkins;

import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import java.nio.file.Paths;
import java.util.*;
Copy link
Member

Choose a reason for hiding this comment

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

Don't use wildcard imports in production code. Please revert.

import java.util.function.Function;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

import static org.csanchez.jenkins.plugins.kubernetes.KubernetesCloud.JNLP_NAME;
import static org.csanchez.jenkins.plugins.kubernetes.PodTemplateUtils.combine;
import static org.csanchez.jenkins.plugins.kubernetes.PodTemplateUtils.isNullOrEmpty;
import static org.csanchez.jenkins.plugins.kubernetes.PodTemplateUtils.substituteEnv;
import static org.csanchez.jenkins.plugins.kubernetes.PodTemplateUtils.*;
Copy link
Member

Choose a reason for hiding this comment

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

Ditto above.


/**
* Helper class to build Pods from PodTemplates
Expand Down Expand Up @@ -173,8 +147,20 @@ public Pod build() {
//We need to normalize the path or we can end up in really hard to debug issues.
final String mountPath = substituteEnv(Paths.get(volume.getMountPath()).normalize().toString().replace("\\", "/"));
if (!volumeMounts.containsKey(mountPath)) {
volumeMounts.put(mountPath, new VolumeMountBuilder() //
.withMountPath(mountPath).withName(volumeName).withReadOnly(false).build());
VolumeMountBuilder volumeMountBuilder = new VolumeMountBuilder() //
.withMountPath(mountPath)
.withName(volumeName)
.withReadOnly(false);

final String volumeSubPath = volume.getSubPath();
if (StringUtils.isNotBlank(volumeSubPath)) {
// We need to normalize the subPath, or we can end up in really hard to debug issues Just in case.
final String subPath = substituteEnv(Paths.get(volumeSubPath).normalize().toString().replace("\\", "/"));
Comment on lines +157 to +158
Copy link
Member

Choose a reason for hiding this comment

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

Is that normalization really needed? What happens if the agent is on Windows?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello Viatombe,
I'm answering this because this piece of code is also in my original PR wich was intended only for ConfigMap volumes.
I think the answer applies on both.
To be honest I do not know what will happen in a windows container.
But what I can see is that that normalization is there since 4 years ago. in commit: 43afca4c3209dc6b1d8b768b157ab53cc12da052 made by Alex Johnson ajohnson@bombora.com
The only modification I did was to surround it with an if to control whether the new parameter is empty or not.
So I guess that piece of code is as secure or insecure as it was before and is out of the intention of this PR to modify/evaluate that.

Copy link
Member

Choose a reason for hiding this comment

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

It's fine, I didn't see that the same code was applied to mountPath.

if (StringUtils.isNotBlank(subPath)) {
volumeMountBuilder = volumeMountBuilder.withSubPath(subPath);
}
}
volumeMounts.put(mountPath, volumeMountBuilder.build());
volumes.put(volumeName, volume.buildVolume(volumeName));
i++;
}
Expand Down
16 changes: 8 additions & 8 deletions src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodVolumes.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
package org.csanchez.jenkins.plugins.kubernetes;

import java.util.List;

import io.fabric8.kubernetes.api.model.VolumeMount;

import java.util.List;

Comment on lines +5 to +6
Copy link
Member

Choose a reason for hiding this comment

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

Please revert

@Deprecated
public class PodVolumes {

Expand All @@ -15,12 +15,12 @@ public static abstract class PodVolume extends org.csanchez.jenkins.plugins.kube
public static class EmptyDirVolume extends org.csanchez.jenkins.plugins.kubernetes.volumes.EmptyDirVolume {

public EmptyDirVolume(String mountPath, Boolean memory) {
super(mountPath, memory);
super(mountPath, memory, null);
}

protected Object readResolve() {
return new org.csanchez.jenkins.plugins.kubernetes.volumes.EmptyDirVolume(this.getMountPath(),
this.getMemory());
this.getMemory(), null);
}
}

Expand All @@ -41,25 +41,25 @@ protected Object readResolve() {
public static class HostPathVolume extends org.csanchez.jenkins.plugins.kubernetes.volumes.HostPathVolume {

public HostPathVolume(String hostPath, String mountPath) {
super(hostPath, mountPath);
super(hostPath, mountPath, null);
}

protected Object readResolve() {
return new org.csanchez.jenkins.plugins.kubernetes.volumes.HostPathVolume(this.getHostPath(),
this.getMountPath());
this.getMountPath(), null);
}
}

@Deprecated
public static class NfsVolume extends org.csanchez.jenkins.plugins.kubernetes.volumes.NfsVolume {

public NfsVolume(String serverAddress, String serverPath, Boolean readOnly, String mountPath) {
super(serverAddress, serverPath, readOnly, mountPath);
super(serverAddress, serverPath, readOnly, mountPath, null);
}

protected Object readResolve() {
return new org.csanchez.jenkins.plugins.kubernetes.volumes.NfsVolume(this.getServerAddress(),
this.getServerPath(), this.getReadOnly(), this.getMountPath());
this.getServerPath(), this.getReadOnly(), this.getMountPath(), null);
}
}

Expand Down
21 changes: 15 additions & 6 deletions src/main/java/org/csanchez/jenkins/plugins/kubernetes/volumes/ConfigMapVolume.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,26 +24,31 @@

package org.csanchez.jenkins.plugins.kubernetes.volumes;

import org.jenkinsci.Symbol;
import org.kohsuke.stapler.DataBoundConstructor;

import hudson.Extension;
import hudson.model.Descriptor;
import io.fabric8.kubernetes.api.model.Volume;
import io.fabric8.kubernetes.api.model.VolumeBuilder;
import org.jenkinsci.Symbol;
import org.kohsuke.stapler.DataBoundConstructor;
Comment on lines +31 to +32
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid this kind of change that is just making reviews and blaming more complex.


public class ConfigMapVolume extends PodVolume {

private String mountPath;
private String subPath;
private String configMapName;
private Boolean optional;

@DataBoundConstructor
public ConfigMapVolume(String mountPath, String configMapName, Boolean optional) {
@DataBoundConstructor
public ConfigMapVolume(String mountPath, String configMapName, Boolean optional, String subPath) {
Comment on lines +41 to +42
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this, you could add a setter for subPath annotated with @DataBoundSetter

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello, viatombe,
I'm going to need a bit more explanation on why this is not the correct way of doing things. or how you preffer me to do it
I do not know why is wrong the go from 3 parameters in the constructor to 4.
I do not know the difference between @DataBoundConstructor and @DataboundSetter.
however @DataBoundSetter was not used anywhere in the previous code.
and I'm just adding a new parameter to the constructor. keeping the old one for compatibility issues.
I do not know where is the issue. please explain.
I'm very happy to learn the right way of doing things.

Copy link
Member

@Vlatombe Vlatombe Oct 4, 2021 •
edited

Choose a reason for hiding this comment

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

You can read the explanation here -- Constructor vs. setters.
In that case ideally only mountPath and configMapName should be left in the constructor since they are mandatory, and optional and subPath should be set using setters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks Vincent,
the link was quite useful and I understand the difference.
I've done the modifications on my original code/PR
All the other issues are not in my original code/PR.
Please consider merging PR while @hameno fixes the issues on his side.

this.mountPath = mountPath;
this.subPath = subPath;
this.configMapName = configMapName;
this.optional = optional;
}

public ConfigMapVolume(String mountPath, String configMapName, Boolean optional) {
this(mountPath, configMapName, optional, (String) null);
}

@Override
public Volume buildVolume(String volumeName) {
Expand All @@ -68,7 +73,11 @@ public String getMountPath() {
public Boolean getOptional() {
return optional;
}


public String getSubPath() {
return subPath;
}

@Extension
@Symbol("configMapVolume")
public static class DescriptorImpl extends Descriptor<PodVolume> {
Expand Down
22 changes: 14 additions & 8 deletions src/main/java/org/csanchez/jenkins/plugins/kubernetes/volumes/EmptyDirVolume.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,15 @@

package org.csanchez.jenkins.plugins.kubernetes.volumes;

import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;

import org.jenkinsci.Symbol;
import org.kohsuke.stapler.DataBoundConstructor;

import hudson.Extension;
import hudson.model.Descriptor;
import io.fabric8.kubernetes.api.model.Volume;
import io.fabric8.kubernetes.api.model.VolumeBuilder;
import org.jenkinsci.Symbol;
import org.kohsuke.stapler.DataBoundConstructor;

import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;

public class EmptyDirVolume extends PodVolume {

Expand All @@ -43,18 +42,25 @@ public class EmptyDirVolume extends PodVolume {
private String mountPath;
@CheckForNull
private Boolean memory;
private String subPath;

@DataBoundConstructor
public EmptyDirVolume(String mountPath, Boolean memory) {
public EmptyDirVolume(String mountPath, Boolean memory, String subPath) {
this.mountPath = mountPath;
this.memory = memory;
this.subPath = subPath;
}

@Override
public String getMountPath() {
return mountPath;
}

@Override
public String getSubPath() {
return subPath;
}

public String getMedium() {
return getMemory() ? MEMORY_MEDIUM : DEFAULT_MEDIUM;
}
Expand Down Expand Up @@ -82,4 +88,4 @@ public String getDisplayName() {
return "Empty Dir Volume";
}
}
}
}
9 changes: 8 additions & 1 deletion src/main/java/org/csanchez/jenkins/plugins/kubernetes/volumes/HostPathVolume.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,14 @@

public class HostPathVolume extends PodVolume {
private String mountPath;
private String subPath;
private String hostPath;

@DataBoundConstructor
public HostPathVolume(String hostPath, String mountPath) {
public HostPathVolume(String hostPath, String mountPath, String subPath) {
this.hostPath = hostPath;
this.mountPath = mountPath;
this.subPath = subPath;
}

public Volume buildVolume(String volumeName) {
Expand All @@ -53,6 +55,11 @@ public String getMountPath() {
return mountPath;
}

@Override
public String getSubPath() {
return subPath;
}

public String getHostPath() {
return hostPath;
}
Expand Down
22 changes: 14 additions & 8 deletions src/main/java/org/csanchez/jenkins/plugins/kubernetes/volumes/NfsVolume.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,30 +24,31 @@

package org.csanchez.jenkins.plugins.kubernetes.volumes;

import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;

import org.jenkinsci.Symbol;
import org.kohsuke.stapler.DataBoundConstructor;

import hudson.Extension;
import hudson.model.Descriptor;
import io.fabric8.kubernetes.api.model.Volume;
import io.fabric8.kubernetes.api.model.VolumeBuilder;
import org.jenkinsci.Symbol;
import org.kohsuke.stapler.DataBoundConstructor;

import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;

public class NfsVolume extends PodVolume {
private String mountPath;
private String subPath;
private String serverAddress;
private String serverPath;
@CheckForNull
private Boolean readOnly;

@DataBoundConstructor
public NfsVolume(String serverAddress, String serverPath, Boolean readOnly, String mountPath) {
public NfsVolume(String serverAddress, String serverPath, Boolean readOnly, String mountPath, String subPath) {
this.serverAddress = serverAddress;
this.serverPath = serverPath;
this.readOnly = readOnly;
this.mountPath = mountPath;
this.subPath = subPath;
}

public Volume buildVolume(String volumeName) {
Expand All @@ -61,6 +62,11 @@ public String getMountPath() {
return mountPath;
}

@Override
public String getSubPath() {
return subPath;
}

public String getServerAddress() {
return serverAddress;
}
Expand All @@ -82,4 +88,4 @@ public String getDisplayName() {
return "NFS Volume";
}
}
}
}