Private registries support by davidfestal · Pull Request #9962 · eclipse-che/che

davidfestal · 2018-06-07T20:33:58Z

What does this PR do?

This PR adds support of private docker registries when running on the Kubernetes and OpenShift infrastructure.

What issues does this PR fix or reference?

redhat-developer/rh-che#680

Related to issue redhat-developer/rh-che#680 Signed-off-by: David Festal <dfestal@redhat.com>

Signed-off-by: David Festal <dfestal@redhat.com>

codenvy-ci · 2018-06-07T20:34:01Z

Can one of the admins verify this patch?

davidfestal · 2018-06-07T20:37:25Z

ci-test

codenvy-ci · 2018-06-08T04:51:51Z

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

Adapt to changes made in upstream PR eclipse-che/che#9962 Signed-off-by: David Festal <dfestal@redhat.com>

garagatyi

Overall looks good. I would love to see code been improved from the support side. I added inlined comments about that.

garagatyi · 2018-06-08T06:40:57Z

...rg/eclipse/che/workspace/infrastructure/kubernetes/provision/ImagePullSecretProvisioner.java

+import org.eclipse.che.workspace.infrastructure.kubernetes.environment.KubernetesEnvironment;
+import org.eclipse.che.workspace.infrastructure.kubernetes.namespace.KubernetesNamespaceFactory;
+
+/** @author David Festal */


Please, add javadocs.

garagatyi · 2018-06-08T06:46:20Z

...rg/eclipse/che/workspace/infrastructure/kubernetes/provision/ImagePullSecretProvisioner.java

+      throws InfrastructureException {
+
+    AuthConfigs authConfigs = userSpecificDockerRegistryCredentialsProvider.getCredentials();
+    if (authConfigs.getConfigs().isEmpty()) {


You may get an NPE here. Please, add a check.

garagatyi · 2018-06-08T06:52:27Z

...rg/eclipse/che/workspace/infrastructure/kubernetes/provision/ImagePullSecretProvisioner.java

+    Base64.Encoder encoder = Base64.getEncoder();
+
+    String config;
+    try (StringWriter strWriter = new StringWriter();


This class contains multiple levels of abstraction - usage of provider, manipulating strings, usage of k8s objects. This makes it too complex. Please, move conversion of AuthConfigs to Secret into a separate class.

Maybe private method could be enough

Fixed with a private method

garagatyi · 2018-06-08T06:54:54Z

...rg/eclipse/che/workspace/infrastructure/kubernetes/provision/ImagePullSecretProvisioner.java

+              (name, authConfig) -> {
+                try {
+                  if (!name.startsWith("https://")) {
+                    name = "https://" + name;


if the name starts from http:// we add "https://" in front of it. Should we really do that?

garagatyi · 2018-06-08T07:04:23Z

...rg/eclipse/che/workspace/infrastructure/kubernetes/provision/ImagePullSecretProvisioner.java

+                  jsonWriter.value(authConfig.getPassword());
+                  jsonWriter.name("email");
+                  jsonWriter.value("email@email");
+                  String auth =


Can you point me to a doc that describes how this object should look like - I don't understand by looking at the code. This also indicates that this code is not clear and might require additional code simplifications or comments to ease support of this code.

I added an example in the Javadoc of the extracted private method

garagatyi · 2018-06-08T07:05:39Z

...rg/eclipse/che/workspace/infrastructure/kubernetes/provision/ImagePullSecretProvisioner.java

+    k8sEnv.getPods().values().forEach(this::provision);
+  }
+
+  public void provision(Pod pod) {


It looks like this method should be private

garagatyi · 2018-06-08T07:08:41Z

...clipse/che/workspace/infrastructure/kubernetes/provision/ImagePullSecretProvisionerTest.java

+                .getBytes());
+  }
+
+  @SuppressWarnings("unchecked")


If you use suppressing of warnings, please, add a comment to the doc that explains why it is safe to suppress warnings.

BTW it's not good practice to use SuppressWarnings for the whole method.

I removed the warnings completely, so no more SuppressWarnings :-)

garagatyi · 2018-06-08T07:10:17Z

...clipse/che/workspace/infrastructure/kubernetes/provision/ImagePullSecretProvisionerTest.java

+    verifyZeroInteractions(podSpec);
+  }
+
+  private static class TestAuthConfig implements AuthConfig {


Please, move auxiliary methods down. It is easier to read test when test cases are higher and all other methods are lower in the class.

skabashnyuk · 2018-06-08T07:51:58Z

infrastructures/kubernetes/pom.xml

        </dependency>
+        <dependency>
+            <groupId>org.eclipse.che.infrastructure.docker</groupId>
+            <artifactId>docker-client</artifactId>


I think this dependency can potentially bring some issues in future. I suggest to move docker auth related code in separate maven module and replace this dependency with it.
@sleshchenko @garagatyi @gazarenkov wdyt?

@skabashnyuk the same applies to the docker-environment dependency that is just below right?

I would say that not the same. It is supposed that docker-environment may be reused by another infrastructure.

Is it a blocker for this PR or could this new module refactoring be done in a next PR ?

Yes. Could you do that please in this PR. I really appreciate your efforts. I guess it will take less time then we spend writing this comments and protect us with potential binding/classloading/configuration issues.

I extracted the docker auth-related classes to a new docker-auth maven module.
@skabashnyuk does it correspond to what you were expecting ?

sleshchenko · 2018-06-08T06:41:41Z

...rg/eclipse/che/workspace/infrastructure/kubernetes/provision/ImagePullSecretProvisioner.java

+  static final String SECRET_NAME = "workspace-private-registries";
+
+  private final UserSpecificDockerRegistryCredentialsProvider
+      userSpecificDockerRegistryCredentialsProvider;


quite a long variable name =)

is it a problem ?

tiny one. I don't like such long names 😄

I reduced its length :-D

sleshchenko · 2018-06-08T07:40:16Z

...clipse/che/workspace/infrastructure/kubernetes/provision/ImagePullSecretProvisionerTest.java

+                .getBytes());
+  }
+
+  @SuppressWarnings("unchecked")


BTW it's not good practice to use SuppressWarnings for the whole method.

sleshchenko · 2018-06-08T07:41:31Z

...rg/eclipse/che/workspace/infrastructure/kubernetes/provision/ImagePullSecretProvisioner.java

+      return;
+    }
+
+    Gson gson = new Gson();


Please explain why do you need to create it here instead of a single line where it is used?

Fixed in other code changes.

sleshchenko · 2018-06-08T07:42:07Z

...rg/eclipse/che/workspace/infrastructure/kubernetes/provision/ImagePullSecretProvisioner.java

+
+    Base64.Encoder encoder = Base64.getEncoder();
+
+    String config;


Why do you need a declaration of this variable here?

Not needed. Fixed.

sleshchenko · 2018-06-08T07:51:23Z

...rg/eclipse/che/workspace/infrastructure/kubernetes/provision/ImagePullSecretProvisioner.java

+    Base64.Encoder encoder = Base64.getEncoder();
+
+    String config;
+    try (StringWriter strWriter = new StringWriter();


Maybe private method could be enough

sleshchenko · 2018-06-08T07:52:20Z

...rg/eclipse/che/workspace/infrastructure/kubernetes/provision/ImagePullSecretProvisioner.java

+                  jsonWriter.value(encoder.encodeToString(auth.getBytes()));
+                  jsonWriter.endObject();
+                } catch (IOException e) {
+                  throw new RuntimeException(e);


Please don't throw RuntimeException. InfrastructureException only is expected to be thrown by this method.

Replacing the forEach with a for loop removed the need for a RuntimeException

Signed-off-by: David Festal <dfestal@redhat.com>

davidfestal · 2018-06-08T12:27:42Z

@garagatyi @sleshchenko I mainly did the changes you requested. Is it OK for you OK now ?

@skabashnyuk The only remaining comment that I didn't fix is this one

@l0rd wdyt ? could be postpone this to a next PR ?

garagatyi · 2018-06-08T14:07:41Z

...rg/eclipse/che/workspace/infrastructure/kubernetes/provision/ImagePullSecretProvisioner.java

+          jsonWriter.value(encoder.encodeToString(auth.getBytes()));
+          jsonWriter.endObject();
+        } catch (IOException e) {
+          throw new InfrastructureException(e);


I think that IOException message here won't be user-friendly. Can you wrap it with an introduction appropriate for a user?

garagatyi · 2018-06-08T14:07:46Z

...rg/eclipse/che/workspace/infrastructure/kubernetes/provision/ImagePullSecretProvisioner.java

+      jsonWriter.flush();
+      return strWriter.toString();
+    } catch (IOException e) {
+      throw new InfrastructureException(e);


I think that IOException message here won't be user-friendly. Can you wrap it with an introduction appropriate for a user?

I added a message to the wrapping exception.

Signed-off-by: David Festal <dfestal@redhat.com>

@skabashnyuk

as requested by @skabashnyuk [here](#9962 (review)) Signed-off-by: David Festal <dfestal@redhat.com>

Signed-off-by: David Festal <dfestal@redhat.com>

davidfestal · 2018-06-08T17:55:38Z

ci-test

codenvy-ci · 2018-06-08T22:53:33Z

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

sleshchenko

LGTM. Please take a look my inlined comments especially about DockerComposeEnvironmentConverter.

sleshchenko · 2018-06-11T05:43:29Z

infrastructures/docker/docker-auth/pom.xml

+    <packaging>jar</packaging>
+    <name>Infrastructure :: Docker :: Docker Auth</name>
+    <properties>
+        <findbugs.failonerror>false</findbugs.failonerror>


Can it be removed?

It should be! Please, do not use this property unless you have strong arguments why we should use it!

This came from a copy/paste of the docker-client pom.xml file.
I just removed it.

sleshchenko · 2018-06-11T05:48:25Z

...rkspace/infrastructure/kubernetes/environment/convert/DockerComposeEnvironmentConverter.java

+ * @author Anton Korneta
+ */
+@Singleton
+public class DockerComposeEnvironmentConverter {


It looks exactly the same as DockerImageEnvironmentConverter but with another name. Please check these changes, I guess it should be reverted.

garagatyi · 2018-06-11T06:15:36Z

...rkspace/infrastructure/kubernetes/environment/convert/DockerComposeEnvironmentConverter.java

+ * @author Anton Korneta
+ */
+@Singleton
+public class DockerComposeEnvironmentConverter {


This class seems to be unrelated to your PR. Can you elaborate why we need it?

yes, it was added by mistake. I just removed it.

Signed-off-by: David Festal <dfestal@redhat.com>

davidfestal · 2018-06-11T08:27:19Z

ci-test

garagatyi

LGTM

codenvy-ci · 2018-06-11T10:49:27Z

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

Adapt to changes made in upstream PR eclipse-che/che#9962 Signed-off-by: David Festal <dfestal@redhat.com>

* Support private docker registry in Che on Kubernetes / Openshift : related to issue redhat-developer/rh-che#680 * Extract the `AuthConfig` and related classes to a new module Signed-off-by: David Festal <dfestal@redhat.com>

* Support private docker registries in OSIO : adapt to changes made in upstream PR eclipse-che/che#9962 * Now upgrade to the `6.6.1` bugfix release the contains required upstream changes Signed-off-by: David Festal <dfestal@redhat.com>

* Support private docker registry in Che on Kubernetes / Openshift : related to issue redhat-developer/rh-che#680 * Extract the `AuthConfig` and related classes to a new module Signed-off-by: David Festal <dfestal@redhat.com>

davidfestal added 4 commits June 7, 2018 16:50

Support private docker registry in Che on Kubernetes / Openshift

353da6e

Related to issue redhat-developer/rh-che#680 Signed-off-by: David Festal <dfestal@redhat.com>

remove unnecessary new line

4138588

Signed-off-by: David Festal <dfestal@redhat.com>

various enhancements

bf2476d

Signed-off-by: David Festal <dfestal@redhat.com>

Add a test

332bf7b

Signed-off-by: David Festal <dfestal@redhat.com>

davidfestal requested a review from garagatyi as a code owner June 7, 2018 20:33

davidfestal requested review from ibuziuk and l0rd June 7, 2018 20:35

davidfestal self-assigned this Jun 7, 2018

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. labels Jun 7, 2018

garagatyi requested a review from sleshchenko June 8, 2018 06:37

davidfestal added a commit to redhat-developer/rh-che that referenced this pull request Jun 8, 2018

Support private docker registries in OSIO

a6b1b66

Adapt to changes made in upstream PR eclipse-che/che#9962 Signed-off-by: David Festal <dfestal@redhat.com>

davidfestal mentioned this pull request Jun 8, 2018

Support private docker registries in OSIO redhat-developer/rh-che#705

Merged

garagatyi reviewed Jun 8, 2018

View reviewed changes

skabashnyuk reviewed Jun 8, 2018

View reviewed changes

sleshchenko reviewed Jun 8, 2018

View reviewed changes

Various requested changes

b4c6561

Signed-off-by: David Festal <dfestal@redhat.com>

l0rd approved these changes Jun 8, 2018

View reviewed changes

garagatyi approved these changes Jun 8, 2018

View reviewed changes

davidfestal added 3 commits June 8, 2018 16:35

Use a shorter name for @sleshchenko :-)

4cfc5c9

Signed-off-by: David Festal <dfestal@redhat.com>

Extract the AuthConfig and related classes to a new module

561fa50

as requested by @skabashnyuk [here](#9962 (review)) Signed-off-by: David Festal <dfestal@redhat.com>

Add better message to rethrown exception

f0a3bd3

Signed-off-by: David Festal <dfestal@redhat.com>

davidfestal requested review from riuvshin and vparfonov as code owners June 8, 2018 17:53

skabashnyuk approved these changes Jun 8, 2018

View reviewed changes

ibuziuk approved these changes Jun 10, 2018

View reviewed changes

sleshchenko reviewed Jun 11, 2018

View reviewed changes

garagatyi suggested changes Jun 11, 2018

View reviewed changes

davidfestal added 2 commits June 11, 2018 10:20

Remove a file that was added by mistake

23fd174

Signed-off-by: David Festal <dfestal@redhat.com>

Don't disable findbugs failOnError feature.

67b87d2

Signed-off-by: David Festal <dfestal@redhat.com>

sleshchenko approved these changes Jun 11, 2018

View reviewed changes

garagatyi approved these changes Jun 11, 2018

View reviewed changes

davidfestal merged commit 7c3d432 into master Jun 11, 2018

benoitf added this to the 6.7.0 milestone Jun 11, 2018

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Jun 11, 2018

davidfestal added a commit to redhat-developer/rh-che that referenced this pull request Jun 12, 2018

Support private docker registries in OSIO

ac6dfdb

Adapt to changes made in upstream PR eclipse-che/che#9962 Signed-off-by: David Festal <dfestal@redhat.com>

benoitf deleted the private-registries-support branch July 11, 2018 07:52

sleshchenko mentioned this pull request Aug 13, 2018

How to configure private repository for workspaces in K8S #10744

Closed

Conversation

davidfestal commented Jun 7, 2018

What does this PR do?

What issues does this PR fix or reference?

Uh oh!

codenvy-ci commented Jun 7, 2018

Uh oh!

davidfestal commented Jun 7, 2018

Uh oh!

codenvy-ci commented Jun 8, 2018

Uh oh!

garagatyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skabashnyuk Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skabashnyuk Jun 8, 2018 •

edited

Loading