Add recipe support for Kubernetes secrets / OpenShift routes #12486

amisevsk · 2019-01-22T05:44:57Z

What does this PR do?

Add support for Secrets and Routes in OpenShift/Kubernetes recipes.

Routes will only work in OpenShift recipes, as they are not a Kubernetes object.
There is some minimal user dashboard validation (metadata + fields on secrets only) since all validation is done through the Kubernetes side (and so it doesn't make sense to mention routes there). Since this is more involved and will likely be harder to read, it should be done in a separate PR / issue.
- Additionally, more validation for secrets could be beneficial, as e.g. data values must be base64 encoded.
Routes are renamed by the existing mechanism in Che. This should not affect behavior.
Secrets are left unmodified. Renaming can be added later if we find it is necessary (I don't really see a use case, personally).

This PR is broken up into 3 commits:

The first commit adds support for routes on the infra side
The second commit adds support for secrets on the infra side.
The third commit adds the necessary framework for UD validation.

A sample recipe for testing:

kind: List
apiVersion: v1
items:
  - 
    apiVersion: v1
    kind: Pod
    metadata:
      name: ws
    spec:
      containers:
        - 
          image: 'eclipse/che-dev:nightly'
          name: dev
          resources:
            limits:
              memory: 512Mi
          volumeMounts:
          - name: test
            mountPath: "/test"
            readOnly: true
      volumes:
      - name: test
        secret:
          secretName: test-secret
  -
    apiVersion: v1
    kind: Secret
    metadata:
      name: test-secret
    data:
      test1: dGVzdDE=
    stringData:
      test2: test2
  -
    apiVersion: v1
    kind: Route
    metadata:
      name: test-route
    spec:
      host: test.192.168.42.162.nip.io
      port:
        targetPort: http
      to:
        kind: Service
        name: che-host

What issues does this PR fix or reference?

#11508, #11530, #12522

Release Notes

Add support for secrets and routes in Kubernetes/OpenShift recipes

Docs PR

TODO

garagatyi

Looks good! Your commits have leftovers of conflict resolution error. Please, use squash when you merge.

.../eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactory.java

...clipse/che/workspace/infrastructure/kubernetes/environment/KubernetesEnvironmentFactory.java

.../eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactory.java

garagatyi · 2019-01-22T08:26:19Z

ci-test

garagatyi · 2019-01-22T08:27:01Z

@ashumilova There is the 3rd commit in this PR that contains changes in UD. Please, review that part.

ibuziuk · 2019-01-22T14:56:44Z

@amisevsk could you please provide a recipe for testing?

amisevsk · 2019-01-22T22:28:18Z

@ibuziuk It's hidden in the original PR message :)

kind: List
apiVersion: v1
items:
  - 
    apiVersion: v1
    kind: Pod
    metadata:
      name: ws
    spec:
      containers:
        - 
          image: 'eclipse/che-dev:nightly'
          name: dev
          resources:
            limits:
              memory: 512Mi
          volumeMounts:
          - name: test
            mountPath: "/test"
            readOnly: true
      volumes:
      - name: test
        secret:
          secretName: test-secret
  -
    apiVersion: v1
    kind: Secret
    metadata:
      name: test-secret
    data:
      test1: dGVzdDE=
    stringData:
      test2: test2
  -
    apiVersion: v1
    kind: Route
    metadata:
      name: test-route
    spec:
      host: test.192.168.42.162.nip.io
      port:
        targetPort: http
      to:
        kind: Service
        name: che-host

amisevsk · 2019-01-23T01:28:30Z

@garagatyi I thought I had caught all the merge conflicts, don't know where that one came from. I'll remove it in rebase before merging.

garagatyi · 2019-01-23T09:13:04Z

@amisevsk there are no leftovers in the final diff. But since you prepared PR with the commit-by-commit approach (thanks for that) I found it in one of the commits and found that it was fixed in the following one

ibuziuk · 2019-01-24T16:41:03Z

ci-test

ibuziuk

@amisevsk great job! tested on minishift and routes / secrets are created / deleted with workspace just fine

ibuziuk · 2019-01-24T16:53:32Z

@ashumilova could you please take a look at UD part of the PR, before we proceed with merging ?

che-bot · 2019-01-24T20:28:25Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12486
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ibuziuk · 2019-01-24T20:39:17Z

@vkuznyetsov @rhopp could QA provide their approval based on the test report. There are 6 test failures, but I do not think those are related to the PR

amisevsk · 2019-01-24T22:25:31Z

Rebased onto current master and removed artifacts from a merge conflict in first commit.

sleshchenko

1
During manual testing, I successfully created a stack from Kubernetes recipe with content you provide. But then I failed to create a workspace from such stack with the following error

Can we improve validation on Dashboard in this PR or in another issue?
Please improve the validation message in EnvFactories to contain object kind and name instead of ObjectMeta json.

2 Then I created a stack using OpenShift recipe you provided. I edited config of created workspace and changed Route kind to Route1, as result I get 500 exception from workspace API

The same question about improving UX. It would be nice to improve dashboard, API to handle such cases

3
A workspace created from your provided recipe is started successfully. But what I see here there is no any integrity validation, recipe contains Route that references service that is not present in recipe. Do you think is there use case for such workspace? Or maybe we should add integrity validation for Routes, like we have for PVC (Pods can not reference PVCs which are not present in the recipe). See https://github.com/eclipse/che/blob/master/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/environment/KubernetesEnvironmentPodsValidator.java#L59

4
The same question about recipe integrity validation but about secrets. Should we allow using Secret sources volumes that are not defined in the recipe?
It's just a question since I don't know what the right approach and just wanted to raise discussion.

...ipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactoryTest.java

amisevsk · 2019-01-26T01:01:24Z

@sleshchenko Regarding the big issues in your review: the short of it is that Route validation is a bigger task than this PR (in my opinion). All validation goes through kubernetes-machine-recipe-parser.ts, and Routes are not Kubernetes objects. As a result:

While we should block creation of Kube stacks with Routes, enabling the check (which is implemented here) would block creation of OpenShift stacks as well
I agree that this should be improved -- I'll work on it. Should be relatively easy to implement on the infra side. Note that this will happen for any errors of this sort, not Routes specifically (thrown here-- WDYT about moving it to a new issue?
Will look into this before Monday.
I think there's a use case for secrets outside the recipe, because otherwise we require secrets used in workspace to be stored in a trivially decodable format in recipes. There's definitely a case in my mind where a user would want to create a secret more securely and still use it in a workspace.

amisevsk · 2019-01-26T01:13:21Z

Created an issue for point 2. above: #12522

To add: validation on the dashboard side is currently not possible, and requires the more significant refactor to allow different validation paths for OpenShift vs. Kubernetes.

sleshchenko · 2019-01-28T08:50:39Z

I think there's a use case for secrets outside the recipe, because otherwise we require secrets used in workspace to be stored in a trivially decodable format in recipes. There's definitely a case in my mind where a user would want to create a secret more securely and still use it in a workspace.

Makes sense for me, thanks for the explanation.

amisevsk · 2019-01-28T21:15:43Z

@sleshchenko I've added a commit to fix some of the validation issues:

Avoid the internal server error when a recipe cannot be loaded (e.g. with kind: Route1) (Point 2 above)
Show message like Found unknown object type in recipe -- name: 'test-route', kind: 'Route' when unsupported object is in recipe (Point 1 above)
Throw validation exception if route refers to service that is not in recipe.

Anything I missed?

sleshchenko

Great job 👍

Please take a look my minor inline comments

sleshchenko · 2019-01-29T09:12:56Z

...clipse/che/workspace/infrastructure/kubernetes/environment/KubernetesEnvironmentFactory.java

+      list =
+          clientFactory.create().lists().load(new ByteArrayInputStream(content.getBytes())).get();
+    } catch (KubernetesClientException e) {
+      String message = e.getCause() == null ? e.getMessage() : e.getCause().getLocalizedMessage();


e.getMessage() but e.getCause().getLocalizedMessage() <- Did you make it on the purpose?

No, was an accident trying to find a decent way to avoid the newlines and messy cause. Fixed.

sleshchenko · 2019-01-29T09:13:00Z

...clipse/che/workspace/infrastructure/kubernetes/environment/KubernetesEnvironmentFactory.java

+      if (message.contains("\n")) {
+        // Clean up message if it comes from JsonMappingException. Format is e.g.
+        // `No resource type found for:v1#Route1\n at [...]`
+        message = message.split("\\n")[0];


you use the only first element, so you can limit elements

sleshchenko · 2019-01-29T09:14:35Z

.../eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactory.java

+      if (message.contains("\n")) {
+        // Clean up message if it comes from JsonMappingException. Format is e.g.
+        // `No resource type found for:v1#Route1\n at [...]`
+        message = message.split("\\n")[0];


The same as in KubernetesEnvironmentFactory

sleshchenko · 2019-01-29T09:15:27Z

...clipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentValidator.java

@@ -0,0 +1,47 @@
+package org.eclipse.che.workspace.infrastructure.openshift.environment;


please execute mvn license:format to add missing license

Thanks, missed that.

sleshchenko · 2019-01-29T09:19:36Z

...clipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentValidator.java

+    validateRoutesMatchServices(env);
+  }
+
+  private void validateRoutesMatchServices(OpenShiftEnvironment env) throws ValidationException {


A check may be a bit improved by checking Route target port.

Please consider adding unit tests for added test case

Added tests for this validation step.

I agree that the check could be improved by the docs on routes are not very good, and it seems that port options can get complicated. At a certain point it's going to be caveat emptor on validation.

amisevsk · 2019-01-31T03:08:31Z

@ashumilova If you have a moment, could you take a look at commit 81ea06f ? I'd like to merge this some time this week.

Add infrastructure side support for specifying routes in OpenShift/Kubernetes recipes. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Add infrastructure side support for secrets in Kubernetes / OpenShift recipes. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Add mild validation (metadata, kind) for recipes containing secrets. Note that routes are currently unvalidated since they are not permissible in Kubernetes recipes (as they are OpenShift objects) and all validation is done through the kubernetes parser / validator. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

ibuziuk · 2019-01-31T13:45:05Z

ci-test

che-bot · 2019-01-31T17:11:20Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12486
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

amisevsk · 2019-01-31T21:34:33Z

PR Quality Review is a false positive, the unused field is necessary for injection.

- Avoid error internal server error when yaml in recipe is invalid (e.g. specifies "Route1") - Add validation for Routes in recipes (must refer to Service also in recipe) - Add test cases for OpenShiftEnvironmentFactory handling of Routes - Minor cleanup and simplification. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

ibuziuk · 2019-01-31T21:40:16Z

@vkuznyetsov could you please clarify, who from QA side should provide approval for this PR ?

amisevsk · 2019-01-31T21:41:05Z

Rebased and squashed fixup commits.

artaleks9 · 2019-02-01T11:22:50Z

Selenium tests execution on Eclipse Che Multiuser on OCP (https://ci.codenvycorp.com/job/che-pullrequests-test-ocp/1496/) doesn't show any regression against this Pull Request.

amisevsk added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Jan 22, 2019

amisevsk requested a review from sleshchenko January 22, 2019 05:44

amisevsk requested review from ashumilova, benoitf and garagatyi as code owners January 22, 2019 05:44

che-bot added the kind/enhancement A feature request - must adhere to the feature request template. label Jan 22, 2019

garagatyi approved these changes Jan 22, 2019

View reviewed changes

ibuziuk self-requested a review January 22, 2019 13:52

ibuziuk requested review from vkuznyetsov and rhopp January 24, 2019 16:45

ibuziuk approved these changes Jan 24, 2019

View reviewed changes

amisevsk force-pushed the recipe-secret-route branch from 4ea51c3 to e61bd55 Compare January 24, 2019 22:25

sleshchenko reviewed Jan 25, 2019

View reviewed changes

...ipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactoryTest.java Show resolved Hide resolved

This was referenced Jan 28, 2019

Get logs from pvc helper pods when job fails #12514

Merged

Dashboard cannot validate OpenShift-specific stacks #12535

Closed

sleshchenko approved these changes Jan 29, 2019

View reviewed changes

slemeur mentioned this pull request Jan 29, 2019

Che 7 Beta #12170

Closed

69 tasks

amisevsk added 3 commits January 31, 2019 00:22

Add support for routes in k8s/OpenShift recipes

783ee26

Add infrastructure side support for specifying routes in OpenShift/Kubernetes recipes. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Add support for secrets in k8s/OpenShift recipes

88a37fd

Add infrastructure side support for secrets in Kubernetes / OpenShift recipes. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

ashumilova approved these changes Jan 31, 2019

View reviewed changes

amisevsk force-pushed the recipe-secret-route branch from c223255 to 55edf19 Compare January 31, 2019 21:40

amisevsk merged commit 2f4c7c3 into eclipse-che:master Feb 1, 2019

This was referenced Feb 1, 2019

Support Secret object in Kubernetes/OpenShift recipe #11508

Closed

Che throws server exception when OpenShift/k8s recipe cannot be loaded. #12522

Closed

che-bot removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Feb 1, 2019

che-bot added this to the 6.18.0 milestone Feb 1, 2019

This was referenced Feb 1, 2019

Support Route object in OpenShift recipe #11530

Closed

Workspace.Next finalization: support K8S Secret redhat-developer/rh-che#1159

Closed

Workspace.Next finalization: support OpenShift Routes redhat-developer/rh-che#1158

Closed

		@@ -0,0 +1,47 @@
		package org.eclipse.che.workspace.infrastructure.openshift.environment;

Add recipe support for Kubernetes secrets / OpenShift routes #12486

Add recipe support for Kubernetes secrets / OpenShift routes #12486

Conversation

amisevsk commented Jan 22, 2019 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

garagatyi left a comment

Choose a reason for hiding this comment

garagatyi commented Jan 22, 2019

garagatyi commented Jan 22, 2019

ibuziuk commented Jan 22, 2019

amisevsk commented Jan 22, 2019

amisevsk commented Jan 23, 2019

garagatyi commented Jan 23, 2019

ibuziuk commented Jan 24, 2019

ibuziuk left a comment

Choose a reason for hiding this comment

ibuziuk commented Jan 24, 2019

che-bot commented Jan 24, 2019

ibuziuk commented Jan 24, 2019

amisevsk commented Jan 24, 2019

sleshchenko left a comment • edited Loading

Choose a reason for hiding this comment

amisevsk commented Jan 26, 2019

amisevsk commented Jan 26, 2019

sleshchenko commented Jan 28, 2019

amisevsk commented Jan 28, 2019

sleshchenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amisevsk commented Jan 31, 2019

ibuziuk commented Jan 31, 2019

che-bot commented Jan 31, 2019

amisevsk commented Jan 31, 2019

ibuziuk commented Jan 31, 2019

amisevsk commented Jan 31, 2019

artaleks9 commented Feb 1, 2019

amisevsk commented Jan 22, 2019 •

edited

Loading

sleshchenko left a comment •

edited

Loading