-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() { | ||
|
@@ -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 { | ||
|
||
|
@@ -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; | ||
|
@@ -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=\""; | ||
|
||
int status = launcher.launch().envs("HTTPS_PROXY="+this.httpsProxy) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactor |
||
.cmdAsSingleString(kubectlBegin + configFile.getRemote() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
@@ -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); | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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; | ||||||
|
||||||
|
@@ -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; | ||||||
|
@@ -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; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
|
||||||
public String getServerCertificate() { | ||||||
return serverCertificate; | ||||||
} | ||||||
|
@@ -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, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
Util.fixEmpty(serverCertificate), Util.fixEmpty(credentialsId), skipTlsVerify, | ||||||
connectionTimeout, readTimeout).createClient()) { | ||||||
// test listing pods | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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; | ||||||
|
@@ -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; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let |
||||||
} | ||||||
|
||||||
private StandardCredentials getCredentials(String credentials) { | ||||||
|
@@ -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()) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
It's already checked above |
||||||
LOGGER.info("Https Proxy used is " + httpsProxy); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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())) { | ||||||
|
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,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()); | ||
} | ||
} |
There was a problem hiding this comment.
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?