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

Allow KubectlBuildWrapper to work when k8s API server is behind firewall #599

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,15 @@ public class KubectlBuildWrapper extends SimpleBuildWrapper {
private final String serverUrl;
private final String credentialsId;
private final String caCertificate;
private final String httpsProxy;

@DataBoundConstructor
public KubectlBuildWrapper(@Nonnull String serverUrl, @Nonnull String credentialsId,
@Nonnull String caCertificate) {
@Nonnull String caCertificate, String httpsProxy) {
this.serverUrl = serverUrl;
this.credentialsId = credentialsId;
this.caCertificate = caCertificate;
this.httpsProxy = httpsProxy;
}

public String getServerUrl() {
Expand All @@ -89,6 +91,10 @@ public String getCaCertificate() {
return caCertificate;
}

public String getHttpsProxy() {
return httpsProxy;
}

@Override
public void setUp(Context context, Run<?, ?> build, FilePath workspace, Launcher launcher, TaskListener listener, EnvVars initialEnvironment) throws IOException, InterruptedException {

Expand All @@ -99,6 +105,7 @@ public void setUp(Context context, Run<?, ?> build, FilePath workspace, Launcher
context.setDisposer(new CleanupDisposer(tempFiles));

String tlsConfig;
String kubectlBegin = "";
if (caCertificate != null && !caCertificate.isEmpty()) {
FilePath caCrtFile = workspace.createTempFile("cert-auth", "crt");
String ca = caCertificate;
Expand All @@ -113,8 +120,10 @@ public void setUp(Context context, Run<?, ?> build, FilePath workspace, Launcher
tlsConfig = " --insecure-skip-tls-verify=true";
}

int status = launcher.launch()
.cmdAsSingleString("kubectl config --kubeconfig=\"" + configFile.getRemote()
kubectlBegin += "kubectl config --kubeconfig=\"";
Copy link
Member

Choose a reason for hiding this comment

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

Can you inline it so that the diff is minimal?


int status = launcher.launch().envs("HTTPS_PROXY="+this.httpsProxy)
Copy link
Member

Choose a reason for hiding this comment

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

Refactor launcher.launch().envs("HTTPS_PROXY="+this.httpsProxy) to a method to remove duplicates. Also, it should handle null httpsProxy.

.cmdAsSingleString(kubectlBegin + configFile.getRemote()
Copy link
Member

Choose a reason for hiding this comment

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

Use launcher.launch().envs("HTTPS_PROXY="+this.https_proxy).cmdAsSingleString... (possibly refactor it to a method to avoid repeating it)

Copy link
Author

Choose a reason for hiding this comment

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

I'll see if this way works in my environment and make the change.

+ "\" set-cluster k8s --server=" + serverUrl + tlsConfig)
.join();
if (status != 0) throw new IOException("Failed to run kubectl config "+status);
Expand Down Expand Up @@ -173,19 +182,19 @@ public void setUp(Context context, Run<?, ?> build, FilePath workspace, Launcher
throw new AbortException("Unsupported Credentials type " + c.getClass().getName());
}

status = launcher.launch()
.cmdAsSingleString("kubectl config --kubeconfig=\"" + configFile.getRemote() + "\" set-credentials cluster-admin " + login)
status = launcher.launch().envs("HTTPS_PROXY="+this.httpsProxy)
.cmdAsSingleString(kubectlBegin + configFile.getRemote() + "\" set-credentials cluster-admin " + login)
.masks(false, false, false, false, false, false, true)
.join();
if (status != 0) throw new IOException("Failed to run kubectl config "+status);

status = launcher.launch()
.cmdAsSingleString("kubectl config --kubeconfig=\"" + configFile.getRemote() + "\" set-context k8s --cluster=k8s --user=cluster-admin")
status = launcher.launch().envs("HTTPS_PROXY="+this.httpsProxy)
.cmdAsSingleString(kubectlBegin + configFile.getRemote() + "\" set-context k8s --cluster=k8s --user=cluster-admin")
.join();
if (status != 0) throw new IOException("Failed to run kubectl config "+status);

status = launcher.launch()
.cmdAsSingleString("kubectl config --kubeconfig=\"" + configFile.getRemote() + "\" use-context k8s")
status = launcher.launch().envs("HTTPS_PROXY="+this.httpsProxy)
.cmdAsSingleString(kubectlBegin + configFile.getRemote() + "\" use-context k8s")
.join();
if (status != 0) throw new IOException("Failed to run kubectl config "+status);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ static KubernetesClient createClient(KubernetesCloud cloud) throws NoSuchAlgorit
String displayName = cloud.getDisplayName();
final Client c = clients.getIfPresent(displayName);
if (c == null) {
KubernetesClient client = new KubernetesFactoryAdapter(cloud.getServerUrl(), cloud.getNamespace(),
KubernetesClient client = new KubernetesFactoryAdapter(cloud.getServerUrl(), cloud.getHttpsProxy(), cloud.getNamespace(),
cloud.getServerCertificate(), cloud.getCredentialsId(), cloud.isSkipTlsVerify(),
cloud.getConnectTimeout(), cloud.getReadTimeout(), cloud.getMaxRequestsPerHost()).createClient();
clients.put(displayName, new Client(getValidity(cloud), client));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ public class KubernetesCloud extends Cloud {
@Nonnull
private List<PodTemplate> templates = new ArrayList<>();
private String serverUrl;
private String httpsProxy;
@CheckForNull
private String serverCertificate;

Expand Down Expand Up @@ -148,6 +149,7 @@ public KubernetesCloud(@NonNull String name, @NonNull KubernetesCloud source) {
this.defaultsProviderTemplate = source.defaultsProviderTemplate;
this.templates.addAll(source.templates);
this.serverUrl = source.serverUrl;
this.httpsProxy = source.httpsProxy;
this.skipTlsVerify = source.skipTlsVerify;
this.addMasterProxyEnvVars = source.addMasterProxyEnvVars;
this.namespace = source.namespace;
Expand Down Expand Up @@ -248,6 +250,15 @@ public void setServerUrl(@Nonnull String serverUrl) {
this.serverUrl = serverUrl;
}

public String getHttpsProxy() {
return httpsProxy;
}

@DataBoundSetter
public void setHttpsProxy(@Nonnull String httpsProxy) {
this.httpsProxy = httpsProxy;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.httpsProxy = httpsProxy;
this.httpsProxy = Util.fixEmpty(httpsProxy);

}

public String getServerCertificate() {
return serverCertificate;
}
Expand Down Expand Up @@ -731,14 +742,15 @@ public FormValidation doTestConnection(@QueryParameter String name, @QueryParame
@QueryParameter String serverCertificate,
@QueryParameter boolean skipTlsVerify,
@QueryParameter String namespace,
@QueryParameter String httpsProxy,
@QueryParameter int connectionTimeout,
@QueryParameter int readTimeout) throws Exception {
Jenkins.get().checkPermission(Jenkins.ADMINISTER);

if (StringUtils.isBlank(name))
return FormValidation.error("name is required");

try (KubernetesClient client = new KubernetesFactoryAdapter(serverUrl, namespace,
try (KubernetesClient client = new KubernetesFactoryAdapter(serverUrl, httpsProxy, namespace,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
try (KubernetesClient client = new KubernetesFactoryAdapter(serverUrl, httpsProxy, namespace,
try (KubernetesClient client = new KubernetesFactoryAdapter(serverUrl, Util.fixEmpty(httpsProxy), namespace,

Util.fixEmpty(serverCertificate), Util.fixEmpty(credentialsId), skipTlsVerify,
connectionTimeout, readTimeout).createClient()) {
// test listing pods
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,23 +71,24 @@ public class KubernetesFactoryAdapter {
private final int connectTimeout;
private final int readTimeout;
private final int maxRequestsPerHost;
private final String httpsProxy;

public KubernetesFactoryAdapter(String serviceAddress, @CheckForNull String caCertData,
@CheckForNull String credentials, boolean skipTlsVerify) {
this(serviceAddress, null, caCertData, credentials, skipTlsVerify);
this(serviceAddress, null, null, caCertData, credentials, skipTlsVerify);
}

public KubernetesFactoryAdapter(String serviceAddress, String namespace, @CheckForNull String caCertData,
public KubernetesFactoryAdapter(String serviceAddress, @CheckForNull String httpsProxy, String namespace, @CheckForNull String caCertData,
@CheckForNull String credentials, boolean skipTlsVerify) {
this(serviceAddress, namespace, caCertData, credentials, skipTlsVerify, DEFAULT_CONNECT_TIMEOUT, DEFAULT_READ_TIMEOUT);
this(serviceAddress, httpsProxy, namespace, caCertData, credentials, skipTlsVerify, DEFAULT_CONNECT_TIMEOUT, DEFAULT_READ_TIMEOUT);
}

public KubernetesFactoryAdapter(String serviceAddress, String namespace, @CheckForNull String caCertData,
public KubernetesFactoryAdapter(String serviceAddress, @CheckForNull String httpsProxy, String namespace, @CheckForNull String caCertData,
@CheckForNull String credentials, boolean skipTlsVerify, int connectTimeout, int readTimeout) {
this(serviceAddress, namespace, caCertData, credentials, skipTlsVerify, connectTimeout, readTimeout, KubernetesCloud.DEFAULT_MAX_REQUESTS_PER_HOST);
this(serviceAddress, httpsProxy, namespace, caCertData, credentials, skipTlsVerify, connectTimeout, readTimeout, KubernetesCloud.DEFAULT_MAX_REQUESTS_PER_HOST);
}

public KubernetesFactoryAdapter(String serviceAddress, String namespace, @CheckForNull String caCertData,
public KubernetesFactoryAdapter(String serviceAddress, @CheckForNull String httpsProxy, String namespace, @CheckForNull String caCertData,
@CheckForNull String credentials, boolean skipTlsVerify, int connectTimeout, int readTimeout, int maxRequestsPerHost) {
this.serviceAddress = serviceAddress;
this.namespace = namespace;
Expand All @@ -97,6 +98,7 @@ public KubernetesFactoryAdapter(String serviceAddress, String namespace, @CheckF
this.connectTimeout = connectTimeout;
this.readTimeout = readTimeout;
this.maxRequestsPerHost = maxRequestsPerHost;
this.httpsProxy = httpsProxy != null && !httpsProxy.isEmpty() ? httpsProxy : null;
Copy link
Member

Choose a reason for hiding this comment

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

Let KubernetesCloud do the validation

}

private StandardCredentials getCredentials(String credentials) {
Expand Down Expand Up @@ -171,6 +173,11 @@ public KubernetesClient createClient() throws NoSuchAlgorithmException, Unrecove
builder = builder.withRequestTimeout(readTimeout * 1000).withConnectionTimeout(connectTimeout * 1000);
builder.withMaxConcurrentRequestsPerHost(maxRequestsPerHost);

if(httpsProxy != null && !httpsProxy.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(httpsProxy != null && !httpsProxy.isEmpty()) {
if(httpsProxy != null) {

It's already checked above

LOGGER.info("Https Proxy used is " + httpsProxy);
Copy link
Member

Choose a reason for hiding this comment

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

Use level fine or even remove it (this is trivial)

builder.withHttpsProxy(httpsProxy);
}

if (!StringUtils.isBlank(namespace)) {
builder.withNamespace(namespace);
} else if (StringUtils.isBlank(builder.getNamespace())) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
<f:textbox/>
</f:entry>

<f:entry field="httpsProxy" title="${%https_proxy}">
<f:textbox/>
</f:entry>

<f:entry title="${%Certificate of certificate authority (CA)}" field="caCertificate">
<f:textarea/>
</f:entry>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<div>
Use proxy when kubernetes api server is behind the firewall.
</div>
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@
<f:textbox/>
</f:entry>

<f:entry title="${%https_proxy}" field="httpsProxy">
<f:textbox/>
</f:entry>

<f:entry title="${%Kubernetes server certificate key}" field="serverCertificate">
<f:textarea/>
</f:entry>
Expand All @@ -25,7 +29,7 @@
<c:select/>
</f:entry>

<f:validateButton title="${%Test Connection}" progress="${%Testing...}" method="testConnection" with="name,serverUrl,credentialsId,serverCertificate,skipTlsVerify,namespace" />
<f:validateButton title="${%Test Connection}" progress="${%Testing...}" method="testConnection" with="name,serverUrl,credentialsId,serverCertificate,skipTlsVerify,namespace,httpsProxy" />

<f:entry title="${%Jenkins URL}" field="jenkinsUrl">
<f:textbox />
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<div>
Use proxy when kubernetes api server is behind the firewall.
</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package org.csanchez.jenkins.plugins.kubernetes;

import io.fabric8.kubernetes.client.KubernetesClient;
import org.junit.Before;
import org.junit.Test;
import static io.fabric8.kubernetes.client.Config.*;
import static org.junit.Assert.assertEquals;

/**
* Test the creation of client with httpsProxy
*/
public class KubernetesClientProviderTest {
private static final String HTTPS_PROXY = "https://example.com:123";
private static final String DEFAULT_HTTPS_PROXY = "https://example.com:1234";

@Before
public void injectEnvironmentVariables() {
System.setProperty(KUBERNETES_HTTPS_PROXY, DEFAULT_HTTPS_PROXY);
}
@Test
public void testHttpsProxyInjection() throws Exception {
KubernetesFactoryAdapter factory = new KubernetesFactoryAdapter("http://example.com", HTTPS_PROXY, null, null, null, false, 1, 1, 1);
KubernetesClient client = factory.createClient();
assertEquals(HTTPS_PROXY, client.getConfiguration().getHttpsProxy());
}

@Test
public void testDefaultHttpsProxyInjection() throws Exception {
KubernetesFactoryAdapter factory = new KubernetesFactoryAdapter("http://example.com", null, null, null, null, false, 1, 1, 1);
KubernetesClient client = factory.createClient();
assertEquals(DEFAULT_HTTPS_PROXY, client.getConfiguration().getHttpsProxy());
}

@Test
public void testEmptyHttpsProxyInjection() throws Exception {
KubernetesFactoryAdapter factory = new KubernetesFactoryAdapter("http://example.com", "", null, null, null, false, 1, 1, 1);
KubernetesClient client = factory.createClient();
assertEquals(DEFAULT_HTTPS_PROXY, client.getConfiguration().getHttpsProxy());
}
}