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

Conversation

Wilkhu90
Copy link

  • Adding changes to accommodate to situation when k8s API server is behind firewall.
  • This will run kubectl commands using proxy when the value is set and ignore it otherwise.

@jglick jglick changed the title Adding changes to accommodate to situation when k8s API server is behind firewall Allow KubectlBuildWrapper to work when k8s API server is behind firewall Sep 19, 2019
@Wilkhu90
Copy link
Author

Wilkhu90 commented Sep 20, 2019

@jglick @Vlatombe Can you guys review this PR please?? Thanks

@Vlatombe Vlatombe added the enhancement Improvements label Sep 25, 2019
Copy link
Member

@Vlatombe Vlatombe left a comment

Choose a reason for hiding this comment

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

Needs a minimal test ensuring that the https proxy is injected in the kubernetes client configuration. Not sure how the build wrapper change can be tested.

int status = launcher.launch()
.cmdAsSingleString("kubectl config --kubeconfig=\"" + configFile.getRemote()
.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.

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

.cmdAsSingleString("kubectl config --kubeconfig=\"" + configFile.getRemote()
kubectlBegin += "kubectl config --kubeconfig=\"";

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.

@@ -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()) {
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)

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


@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);

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants