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

k8s-infra: server routing strategies & basic TLS #8822

Closed
wants to merge 18 commits into from
Closed

k8s-infra: server routing strategies & basic TLS #8822

wants to merge 18 commits into from

Conversation

guydc
Copy link
Contributor

@guydc guydc commented Feb 17, 2018

Signed-off-by: Guy Daich guy.daich@sap.com

What does this PR do?

Introduce an External Server Exposer Strategy, responsible for exposing service ports associated with external servers, making them accessible from outside the cluster.

Move server exposure to shared k8s infra level:

  • Extract Openshift External Server Exposer to dedicated class.
  • Consolidate Openshift Server Exposer and Server converter with Kubernetes Infra.

Provide three options for exposing external (and secondary) servers in k8s infra:

  • multi-host: unique hostname for each component, like Che Openshift infrastructure.
  • single-host: single hostname for all components. Can be used in conjunction with TLS.
  • default-host: default ingress hostname. Can be used for local development without dynamic DNS (based on ingress IP).

Add basic TLS support:

  • Integrate with cert-manager in single-host configuration, to issue a LetsEncrypt certificate. Use cluster-wide issuers and ingress-shim annotations.
    Add Ingress TLS provisioning.
  • Move TLS_ENABLED configuration to Kubernetes Infra Level.

Update Docs:

  • Add example value files for more convenient helm deployment of multi-user, default-host and TLS configurations.
  • Update readme deployment instructions.

Test PR

Follow instructions to set up minikube, helm, tiller, cert-manager.

Follow specific instructions for single/multi-user, default-host installation on minikube.

Routing strategies tested locally on minikube & minishift.

What issues does this PR fix or reference?

This PR fixes #8694.
This PR is part of kubernetes infrastructure epic #5908.

Release Notes

Docs PR

Currently, documented only in the Che Kubernetes Helm deployment instructions.

Signed-off-by: Guy Daich <guy.daich@sap.com>
@codenvy-ci
Copy link

Can one of the admins verify this patch?

2 similar comments
@codenvy-ci
Copy link

Can one of the admins verify this patch?

@codenvy-ci
Copy link

Can one of the admins verify this patch?

@skabashnyuk
Copy link
Contributor

ci-build

@codenvy-ci
Copy link

Build # 4475 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/4475/ to view the results.

Signed-off-by: Guy Daich <guy.daich@sap.com>
@guydc
Copy link
Contributor Author

guydc commented Feb 18, 2018

ci-build

1 similar comment
@skabashnyuk
Copy link
Contributor

ci-build

@codenvy-ci
Copy link

Build # 4476 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/4476/ to view the results.

@@ -36,6 +38,13 @@
@Singleton
public class OpenShiftServersConverter implements ConfigurationProvisioner<OpenShiftEnvironment> {

ExternalServerExposerStrategy openshiftExternalServerExposer;
Copy link
Member

Choose a reason for hiding this comment

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

Please make this field private and generify it with OpenShiftEnvironment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

class removed.

/** @author Guy Daich */
public interface ExternalServerExposerStrategy<T extends KubernetesEnvironment> {

public void exposeExternalServers(
Copy link
Member

@sleshchenko sleshchenko Feb 19, 2018

Choose a reason for hiding this comment

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

public access modifier can be removed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sleshchenko Strategy is used by the ServerConverter, in another package (k8s.server), and also implemented by OS infra, so should be public.

Copy link
Member

Choose a reason for hiding this comment

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

Sure it should be public. IDE shows me warning that it is redundant because public access modifier is default one for interfaces methods.

String machineName, Pod pod, Container container, OpenShiftEnvironment openShiftEnvironment) {
super(Collections.emptyMap(), machineName, pod, container, openShiftEnvironment);
this.openShiftEnvironment = openShiftEnvironment;
ExternalServerExposerStrategy OpenshiftExternalServerExposerStrategy,
Copy link
Member

Choose a reason for hiding this comment

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

Please generify this variable with OpenShiftEnvironment.
Also please rename this variable, it's quite unusual for java to name variable with first upper character.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Container container,
OpenShiftEnvironment kubernetesEnvironment) {
super(
OpenshiftExternalServerExposerStrategy, machineName, pod, container, kubernetesEnvironment);
}

@Override
protected void exposeExternalServers(
Copy link
Member

@sleshchenko sleshchenko Feb 19, 2018

Choose a reason for hiding this comment

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

Since KubernetesServerExposer#exposeExternalServers method implementation is the same as in OpenShiftServerExposer, looks like we can remove this overriding.

@@ -91,95 +87,22 @@
*/
public class OpenShiftServerExposer extends KubernetesServerExposer<OpenShiftEnvironment> {
Copy link
Member

@sleshchenko sleshchenko Feb 19, 2018

Choose a reason for hiding this comment

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

This class looks like useless. The one possible reason to have it - defining docs for exposing external servers on OpenShift.
Maybe after adding java doc into OpenShiftExternalServerExposer this class can be removed. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try consolidating it with k8s infra, by using k8s server converter in OpenShiftEnvironmentProvisioner, and move the docs like you suggest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link

@garagatyi garagatyi left a comment

Choose a reason for hiding this comment

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

Great PR, @guydaichs! I like how you simplified code and caught overall code design of the infrastructures subsystem. Could you take a look at my inlined comments about minor things I spotted in the PR?

@@ -44,3 +44,10 @@ data:
{{- else }}
CHE_INFRA_KUBERNETES_INGRESS_ANNOTATIONS__JSON: '{"nginx.ingress.kubernetes.io/rewrite-target": "/","nginx.ingress.kubernetes.io/ssl-redirect": "false","nginx.ingress.kubernetes.io/proxy-connect-timeout": "3600","nginx.ingress.kubernetes.io/proxy-read-timeout": "3600"}'
{{- end }}
{{- if .Values.isHostBased }}
CHE_INFRA_KUBERNETES_SERVER_STRATEGY: "host"

Choose a reason for hiding this comment

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

Consider renaming var to CHE_INFRA_KUBERNETES_SERVER__STRATEGY. We have vars naming policy:

  • dots in properties separate components (in case of env vars single underscores)
  • underscores in properties separate words in a single component term (in case of env vars double underscores)

E.g.
che.component1.sub_component_2_mb -> CHE_COMPONENT1_SUB__COMPONENT__2__MB
where MB is unit of measurement in case when property is a number

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


IngressRule ingressRule = ingressRuleBuilder.build();

IngressSpec ingressSpec = new IngressSpecBuilder().withRules(ingressRule).build();

Choose a reason for hiding this comment

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

@guydaichs Would you be willing to contribute unit tests for this class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

import org.eclipse.che.api.core.model.workspace.config.ServerConfig;
import org.eclipse.che.workspace.infrastructure.kubernetes.environment.KubernetesEnvironment;

/** @author Guy Daich */

Choose a reason for hiding this comment

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

Could you add javadocs to this class and/or the method?

import org.eclipse.che.api.core.model.workspace.config.ServerConfig;
import org.eclipse.che.workspace.infrastructure.kubernetes.Annotations;

/** @author Guy Daich */

Choose a reason for hiding this comment

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

Could you add javadocs to this class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

Could you please merge two java docs here. I mean

/**
* Doc...
* 
* @author
*/

@Inject
public IngressHostExternalServerExposer(
@Named("infra.kubernetes.ingress.annotations") Map<String, String> ingressAnnotations,
@Named("che.domain") String domain) {

Choose a reason for hiding this comment

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

Can you elaborate if che.domain is a new variable? Does it differ from che.host?

Copy link
Contributor

Choose a reason for hiding this comment

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

In host-based routing, we assume that che-master and all workspaces are accessed through different sub-domains of the same domain. For example:

  • Che-server would be available at this address: master.che.my-k8s.com
  • The workspace agent of a specific workspace would be available at this address: server-4401-serverofflzacl-dev-machine.che.my-k8s.com
  • The user app for the same workspace would be available at this address: server-8080-serverofflzacl-dev-machine.che.my-k8s.com
  • Other workspaces and corresponding agents

In the example above che.host is master.che.my-k8s.com, whereas che.domain is che.my-k8s.com.

We chose this approach for several reasons:

  1. In certain landscapes you might want to serve more than one che-master. This is especially true in continuous integration scenarios, staging environments, etc. In this case che.domain could be che-test.my-k8s.com, che-prod.my-k8s.com and so on.
  2. For TLS, we would want to issue a single wildcard certificate for each Che deployment. E.g. *.che.my-k8s.com.

We could consider not setting the CHE_HOST environment variable upon deployment of che-master, and instead setting it programmatically based on CHE_DOMAIN, but we don't know what effect this would have on the Docker- and OpenShift-based Che infrastructures.

Choose a reason for hiding this comment

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

I see. Thank you for the detailed explanation!
From what I see it is used by k8s infra only. So shouldn't we rename it to something k8s infra specific, e.g. che.infra.kubernetes.che_domain?

Choose a reason for hiding this comment

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

I understand the reason why you decided to add it. But can you change the name to be compliant with the naming of other properties? For example che.infra.kubernetes.ingress.domain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@garagatyi - agreed. I'll fix it.

@@ -114,6 +102,21 @@
* servicePort: [8080|web-app] ---->> Service.spec.ports[0].[port|name]
* </pre>
*
* <pre>
* Host-Based Ingress exposing service's port:

Choose a reason for hiding this comment

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

Maybe it worth to move examples to different strategies and add {@see StrategyInterfaceClass} link to the class? WDYT @sleshchenko @guydaichs ?

Copy link
Member

Choose a reason for hiding this comment

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

@garagatyi I think it would be better to move examples to different strategies.

* @author Sergii Leshchenko
* @author Alexander Garagatyi
*/
public class OpenShiftExternalServerExposer

Choose a reason for hiding this comment

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

Could you add javadocs to this class?

@benoitf benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/enhancement A feature request - must adhere to the feature request template. kind/epic A long-lived, PM-driven feature request. Must include a checklist of items that must be completed. labels Feb 23, 2018
@skabashnyuk
Copy link
Contributor

ci-test

@codenvy-ci
Copy link

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:8822
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@slemeur slemeur removed the kind/epic A long-lived, PM-driven feature request. Must include a checklist of items that must be completed. label Feb 26, 2018
Signed-off-by: Guy Daich <guy.daich@sap.com>
Signed-off-by: Guy Daich <guy.daich@sap.com>
Signed-off-by: Guy Daich <guy.daich@sap.com>
guydc and others added 7 commits March 8, 2018 08:29
Signed-off-by: Guy Daich <guy.daich@sap.com>
Signed-off-by: Eyal Barlev <perspectivus@gmail.com>
Signed-off-by: Guy Daich <guy.daich@sap.com>
Signed-off-by: Guy Daich <guy.daich@sap.com>
* Add sample values files
* Fix merge issues

Signed-off-by: Guy Daich <guy.daich@sap.com>
* rename TLS default value in che.env
* remove openshift server exposer (consolidated with k8s)

Signed-off-by: Guy Daich <guy.daich@sap.com>
@guydc guydc requested a review from skabashnyuk as a code owner March 12, 2018 18:23
@guydc guydc changed the title k8s-infra: add host/path-based ingress strategies k8s-infra: server routing strategies & basic TLS Mar 12, 2018
@guydc
Copy link
Contributor Author

guydc commented Mar 12, 2018

@sleshchenko @garagatyi I reworked this PR, and added some functionality to support TLS. Can you review?

Signed-off-by: Guy Daich <guy.daich@sap.com>
@skabashnyuk skabashnyuk requested a review from l0rd March 13, 2018 08:07

#### Default Host
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't Default Host be at the same level than Single User and Multi User (a section of Deployment Options)? Same for TLS Enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


* Master: `https://che-<che-namespace>.your-domain/`
* Keycloak: `https://che-<che-namespace>.your-domain/auth/`
* Workspaces servers: `https://<che-namespace>.your-domain/<path-to-server>`
Copy link
Contributor

Choose a reason for hiding this comment

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

Workspaces servers should be in the format https://che- too (with the che- prefix) or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

Generally, I like server exposing strategies approach 👍
Please take a look my minor comments how it can be improved.

@@ -433,6 +433,10 @@ CHE_SINGLE_PORT=false
##### Kubernetes Infrastructure #####
##### #####
#

# Create routes with Transport Layer Security (TLS) enabled
Copy link
Member

Choose a reason for hiding this comment

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

Previously Routes here meant Routes OpenShift objects. Do you think it is OK to leave the same comment for Kubernetes infrastructure?

Choose a reason for hiding this comment

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

+1 to fix the comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -447,7 +447,7 @@ che.infra.kubernetes.client.http.connection_pool.keep_alive_min=5
che.infra.openshift.project=

# Create routes with Transport Layer Security (TLS) enabled
che.infra.openshift.tls_enabled=false
che.infra.kubernetes.tls_enabled=false
Copy link
Member

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

*/
public class IngressTlsProvisioner implements ConfigurationProvisioner<KubernetesEnvironment> {

protected final boolean isTlsEnabled;
Copy link
Member

@sleshchenko sleshchenko Mar 15, 2018

Choose a reason for hiding this comment

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

Do you mind making these fields private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


@Inject
public MultiHostIngressExternalServerExposer(
@Named("infra.kubernetes.ingress.annotations") Map<String, String> ingressAnnotations,
Copy link
Member

Choose a reason for hiding this comment

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

Looks like it is named infra.kubernetes.ingress.annotations by mistake.
I think it's good time to rename it to che.infra.kubernetes.ingress.annotations. @garagatyi WDYT?

Choose a reason for hiding this comment

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

It was done on purpose. I didn't use che. prefix to make it clear that it is not injectable from properties and should be provided by a special provider instead. The reason is that we inject it as a property in a different format and that property gets converted into this injection on Che start.
So -1 to rename it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree with @garagatyi

import org.eclipse.che.api.core.model.workspace.config.ServerConfig;
import org.eclipse.che.workspace.infrastructure.kubernetes.Annotations;

/** @author Guy Daich */
Copy link
Member

Choose a reason for hiding this comment

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

Could you please merge two java docs here. I mean

/**
* Doc...
* 
* @author
*/

public MultiHostIngressExternalServerExposer(
@Named("infra.kubernetes.ingress.annotations") Map<String, String> ingressAnnotations,
@Named("che.infra.kubernetes.ingress.domain") String domain) {
if (ingressAnnotations == null) {
Copy link
Member

Choose a reason for hiding this comment

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

Previously this check was placed in one class, now in three classes. Maybe it makes sense to move it in IngressAnnotationsProvider

Choose a reason for hiding this comment

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

We have such provider and it provides this binding. We can either inject provider instead of a map here or not check the map for nullability since it is injected as a not nullable dependency.
I think injecting of Provider instead of binding it to a map would make code clearer.
I had to do this in the first place. @guydaichs @sleshchenko WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved check to provider.

@@ -130,24 +104,19 @@
public static final int SERVER_UNIQUE_PART_SIZE = 8;
public static final String SERVER_PREFIX = "server";

private final Map<String, String> ingressAnnotations;
protected final ExternalServerExposerStrategy kubernetesExternalServerExposerStrategy;
Copy link
Member

Choose a reason for hiding this comment

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

Could you generify this field with <T> to avoid unchecked call here https://github.com/eclipse/che/pull/8822/files#diff-ff394ec17243637d9571b51b4aa23b99R206

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@Inject
public IngressTlsProvisioner(
@Named("che.infra.kubernetes.tls_enabled") boolean isTlsEnabled,
@Named("che.infra.kubernetes.tls_secret") String tlsSecretName) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd say that new configuration properties should be added to che.properties with default values and docs.

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

@Override
public void exposeExternalServers(
Copy link
Member

Choose a reason for hiding this comment

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

Looks like three strategies classes have very similar code with ingress builders. Maybe it would be better (maybe not) to create abstract IngressExternalServerExposerStrategy with a protected method like #createIngress(String name, @Nullable String host, String path, ...)

Choose a reason for hiding this comment

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

Like the idea to reduce the amount of the code and not repeat yourself but it depends on how simple would be the code. @guydaichs up to you to decide whether it is better or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

created AbstractIngressExternalServerExposerStrategy, where shared logic resides.

Copy link

@garagatyi garagatyi left a comment

Choose a reason for hiding this comment

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

Sorry that you have faced such a strict PR review process but there are reasons for that:

  • you are changing components that are in a core of new architecture and our previous core was a mess because we had not enough review. So we are trying to do our best now to keep it clear and maintainable.
  • your PRs quality is good and we treat you as a person who wants to commit clean solution that doesn't need to be sanitized by maintainers after the merge.

We have such reviews usually for PRs on a maintainers level or with significant changes to the core components.

// when working in single-host mode, nginx controller wil reuse the che-master secret
// https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/api/extensions/v1beta1/types.go
if (!isNullOrEmpty(tlsSecretName)) {
ingressTLSBuilder.withSecretName(tlsSecretName);

Choose a reason for hiding this comment

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

Not sure I got it right but it seems that is called twice - both in any case and when the secret name is neither null nor empty. Shouldn't we delete first call of this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactor mistake. fixed.

}

IngressTLS ingressTLS = ingressTLSBuilder.build();
List<IngressTLS> ingressTLSList = new ArrayList<>(Arrays.asList(ingressTLS));

Choose a reason for hiding this comment

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

I think some IDE might show warning because of usage of Arrays.asList instead of Collections.singletonList here. Consider the last variant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -447,7 +447,7 @@ che.infra.kubernetes.client.http.connection_pool.keep_alive_min=5
che.infra.openshift.project=

# Create routes with Transport Layer Security (TLS) enabled
che.infra.openshift.tls_enabled=false
che.infra.kubernetes.tls_enabled=false

Choose a reason for hiding this comment

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

+1

@@ -433,6 +433,10 @@ CHE_SINGLE_PORT=false
##### Kubernetes Infrastructure #####
##### #####
#

# Create routes with Transport Layer Security (TLS) enabled

Choose a reason for hiding this comment

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

+1 to fix the comment

@Inject
public IngressTlsProvisioner(
@Named("che.infra.kubernetes.tls_enabled") boolean isTlsEnabled,
@Named("che.infra.kubernetes.tls_secret") String tlsSecretName) {

Choose a reason for hiding this comment

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

+1


@Inject
public MultiHostIngressExternalServerExposer(
@Named("infra.kubernetes.ingress.annotations") Map<String, String> ingressAnnotations,

Choose a reason for hiding this comment

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

It was done on purpose. I didn't use che. prefix to make it clear that it is not injectable from properties and should be provided by a special provider instead. The reason is that we inject it as a property in a different format and that property gets converted into this injection on Che start.
So -1 to rename it.

public MultiHostIngressExternalServerExposer(
@Named("infra.kubernetes.ingress.annotations") Map<String, String> ingressAnnotations,
@Named("che.infra.kubernetes.ingress.domain") String domain) {
if (ingressAnnotations == null) {

Choose a reason for hiding this comment

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

We have such provider and it provides this binding. We can either inject provider instead of a map here or not check the map for nullability since it is injected as a not nullable dependency.
I think injecting of Provider instead of binding it to a map would make code clearer.
I had to do this in the first place. @guydaichs @sleshchenko WDYT?

}

@Override
public void exposeExternalServers(

Choose a reason for hiding this comment

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

Like the idea to reduce the amount of the code and not repeat yourself but it depends on how simple would be the code. @guydaichs up to you to decide whether it is better or not

Map<String, ServerConfigImpl> ingressServers =
Annotations.newDeserializer(ingress.getMetadata().getAnnotations()).servers();
assertEquals(ingressServers.get("http-server").getProtocol(), "https");
assertEquals(ingressServers.get("ws-server").getProtocol(), "wss");

Choose a reason for hiding this comment

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

Looks like there are 3 things that can be covered in addition:

  • secret name if it is neither null nor empty
  • secret name if it is null
  • secret name if it is empty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

*
* @author Sergii Leshchenko
*/
public class OpenShiftServerExposerTest {

Choose a reason for hiding this comment

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

I don't quite understand why these tests should be removed. @guydaichs can you elaborate on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. OpenshiftServerExposer doesn't exist as a class, but openshift server exposure logic in KubernetesServerExposer should still be tested. I adapted the test class.

@guydc
Copy link
Contributor Author

guydc commented Mar 15, 2018

@l0rd @garagatyi @sleshchenko : Thanks for the thorough review and meaningful comments. I'll make the required changes and let you know.

@garagatyi
Copy link

@guydaichs I'm sorry but I've merged PR that moves deployment files to another folder. This has affected your PR. I think that the easiest way to adapt your PR is to rebase it against master and then move files that would be left from dockerfiles/init/modules/che-kubernetes-helm to deploy/kubernetes/helm/che. I hope you won't get any rebase conflicts.

@garagatyi
Copy link

@guydaichs If you need assistance with the changes caused by my PR I can do required changes in your branch.

@guydc
Copy link
Contributor Author

guydc commented Apr 4, 2018

Opened a fresh PR: #9329.
closing this one.

@guydc guydc closed this Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A feature request - must adhere to the feature request template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an ability to use host-based routing instead of path-based for k8s infra
9 participants