fix: Adapt the Secrets plugin API to use kubernetes secrets #1166

vinokurig · 2021-07-09T14:35:10Z

Signed-off-by: Igor Vinokur ivinokur@redhat.com

What does this PR do?

Rework the Secrets API from upstream theia to use kubernetes secrets

Screenshot/screencast of this PR

What issues does this PR fix or reference?

fixes eclipse-che/che#19837

How to test this PR?

Start a workspace from devfile:

apiVersion: 1.0.0
metadata:
  name: wksp-xhxvu
components:
  - type: cheEditor
    reference: 'https://raw.githubusercontent.com/chepullreq4/pr-check-files/master/che-theia/pr-1166/che_theia_meta.yaml'
  - type: chePlugin
    reference: 'https://pastebin.com/raw/riqiReMi'

Run Test Get Password command and see a notification with empty (undefined) password.
Run Test Set Password command and see a notification about password change.
Run Test Get Password command and see a notification with the updated password.
Run Test Delete Password command and see a notification about password change.
Run Test Get Password command and see a notification with empty (undefined) password.

P.S. This will work only for newly created namespace, see eclipse-che/che-server#44 (review)

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

Happy Path Channel

HAPPY_PATH_CHANNEL=stable

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

vinokurig · 2021-07-09T14:43:00Z

Depends on eclipse-che/che-server#44

codecov · 2021-07-09T14:52:21Z

Codecov Report

Merging #1166 (e7c5cec) into main (c299f59) will decrease coverage by 0.25%.
The diff coverage is 9.70%.

@@            Coverage Diff             @@
##             main    #1166      +/-   ##
==========================================
- Coverage   32.78%   32.52%   -0.26%     
==========================================
  Files         290      295       +5     
  Lines        9885     9994     +109     
  Branches     1457     1474      +17     
==========================================
+ Hits         3241     3251      +10     
- Misses       6641     6740      +99     
  Partials        3        3

Impacted Files	Coverage Δ
...theia-about/src/browser/about-che-theia-dialog.tsx	`0.00% <0.00%> (ø)`
...credentials/src/browser/che-credentials-service.ts	`0.00% <0.00%> (ø)`
...entials/src/browser/credentials-frontend-module.ts	`0.00% <0.00%> (ø)`
...eia-credentials/src/common/credentials-protocol.ts	`0.00% <0.00%> (ø)`
...eia-credentials/src/node/che-credentials-server.ts	`0.00% <0.00%> (ø)`
...s/src/node/che-theia-credentials-backend-module.ts	`0.00% <0.00%> (ø)`
...rowser/src/browser/che-mini-browser-environment.ts	`0.00% <0.00%> (ø)`
...he-server/src/node/che-server-http-service-impl.ts	`0.00% <0.00%> (ø)`
...-che-server/src/node/che-server-remote-api-impl.ts	`38.88% <0.00%> (ø)`
...browser/contribution/exec-terminal-contribution.ts	`0.00% <0.00%> (ø)`
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 929e119...e7c5cec. Read the comment docs.

dmytro-ndp · 2021-07-12T09:07:34Z

[crw-ci-test --rebuild]

che-bot · 2021-07-12T09:54:48Z

✅ E2E Happy path tests succeed 🎉

See Details

name	link
che-theia	quay.io/crw_pr/che-theia:1166
che-theia-endpoint-runtime-binary	quay.io/crw_pr/che-theia-endpoint-runtime-binary:1166

Test product:

tested with Eclipse Che on K8S (minikube v1.1.1)
E2E test scenario
E2E test pipeline source code

Use comment "[crw-ci-test]" to rerun happy path E2E test.
Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe

vzhukovs

lgtm 👍

benoitf · 2021-07-20T11:42:02Z

extensions/eclipse-che-theia-credentials/src/node/che-credentials-server.ts

+import { CheServerWorkspaceServiceImpl } from '@eclipse-che/theia-remote-impl-che-server/lib/node/che-server-workspace-service-impl';
+
+@injectable()
+export class CheCredentialsServer implements CredentialsServer {


For this classe, we should rely on the mounted files for reading secrets.

About delete and create it should be possible to do it using K8S API

About delete and create it should be possible to do it using K8S API

Could you please elaborate more on this?

azatsarynnyy

If I understand correctly, with this PR, we don't need keytar-based code anymore, e,g, KeytarServiceImpl or KeytarCredentialsProvider.
Can we unbind the unused keytar-related components in Che-Theia to avoid including libsecret into the image?

As recently, we've fixed starting Che-Theia backend by including libsecret in 07aeab7

extensions/eclipse-che-theia-credentials/src/browser/che-credentials-service.ts

extensions/eclipse-che-theia-credentials/src/browser/credentials-frontend-module.ts

extensions/eclipse-che-theia-credentials/src/node/che-theia-credentials-backend-module.ts

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

vinokurig · 2021-08-04T14:43:32Z

@azatsarynnyy

If I understand correctly, with this PR, we don't need keytar-based code anymore, e,g, KeytarServiceImpl or KeytarCredentialsProvider.
Can we unbind the unused keytar-related components in Che-Theia to avoid including libsecret into the image?

As recently, we've fixed starting Che-Theia backend by including libsecret in 07aeab7

When I unbind the KeytarServiceImpl and KeytarCredentialsProvider classes the che-theia build fails with an error:

#21 403.7     'ERROR in ../../node_modules/keytar/build/Release/keytar.node 1:0\n' +
#21 403.7     "Module parse failed: Unexpected character '�' (1:0)\n" +
#21 403.7     'You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders\n' +
#21 403.7     '(Source code omitted for this binary file)\n' +
#21 403.7     ' @ ../../node_modules/keytar/lib/keytar.js 1:13-52\n' +
#21 403.7     ' @ ../../packages/core/lib/node/keytar-server.js 63:13-30\n' +
#21 403.7     ' @ ../../che-theia/extensions/eclipse-che-theia-credentials/lib/browser/credentials-frontend-module.js 16:22-67\n' +
#21 403.7     ' @ ./src-gen/frontend/index.js 100:47-128\n' +
#21 403.7     '\n' +
#21 403.7     'webpack 5.46.0 compiled with 1 error and 75 warnings in 140062 ms\n' +
#21 403.7     '\n' +
#21 403.7     'info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.\n',

Probably we can fix it as a next iteration.

che-bot · 2021-08-04T15:13:15Z

✅ E2E Happy path tests succeed 🎉

See Details

name	link
che-theia	quay.io/crw_pr/che-theia:1166
che-theia-endpoint-runtime-binary	quay.io/crw_pr/che-theia-endpoint-runtime-binary:1166

Test product:

tested with Eclipse Che on K8S (minikube v1.1.1)
E2E test scenario
E2E test pipeline source code

Use comment "[crw-ci-test]" to rerun happy path E2E test.
Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe

vinokurig · 2021-08-17T14:43:46Z

@azatsarynnyy @benoitf Since the server-side part is merged I am going to merge this tomorrow if no more objections.

azatsarynnyy · 2021-08-20T12:33:47Z

@azatsarynnyy @benoitf Since the server-side part is merged I am going to merge this tomorrow if no more objections.

Thanks @vinokurig ! The PR looks good to me 👍

But I'm worried about removing the keytar dependency, since we don't use it in Che-Theia, and we have to provide it with all our Che/CRW images.

...
Probably we can fix it as a next iteration.

It's a good idea. It would be great to create an issue for that and at least investigate it during one of the next sprints.

fix: Adapt the Secrets plugin API to use kubernetes secrets

9f72153

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

vinokurig marked this pull request as ready for review July 15, 2021 09:36

vinokurig requested review from azatsarynnyy, benoitf, RomanNikitenko and vzhukovs as code owners July 15, 2021 09:36

vzhukovs approved these changes Jul 20, 2021

View reviewed changes

benoitf reviewed Jul 20, 2021

View reviewed changes

Merge branch 'main' of github.com:eclipse-che/che-theia into che-19837

1abf5b4

azatsarynnyy reviewed Jul 21, 2021

View reviewed changes

vinokurig marked this pull request as draft July 28, 2021 14:35

vinokurig added 4 commits August 2, 2021 12:43

Merge branch 'main' of github.com:eclipse-che/che-theia into che-19837

c23b707

Merge branch 'main' of github.com:eclipse-che/che-theia into che-19837

e03ad43

Merge branch 'main' of github.com:eclipse-che/che-theia into che-19837

a94f43c

Update to use single secret

e7c5cec

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

vinokurig marked this pull request as ready for review August 4, 2021 14:39

vinokurig merged commit 4432ee3 into main Aug 18, 2021

vinokurig deleted the che-19837 branch August 18, 2021 10:45

che-bot added this to the 7.35 milestone Aug 18, 2021

vinokurig mentioned this pull request Aug 20, 2021

Get rid of the keytar related functionality eclipse-che/che#20340

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Adapt the Secrets plugin API to use kubernetes secrets #1166

fix: Adapt the Secrets plugin API to use kubernetes secrets #1166

vinokurig commented Jul 9, 2021 •

edited

Loading

vinokurig commented Jul 9, 2021

codecov bot commented Jul 9, 2021 •

edited

Loading

dmytro-ndp commented Jul 12, 2021

che-bot commented Jul 12, 2021

vzhukovs left a comment

benoitf Jul 20, 2021

vinokurig Aug 4, 2021

azatsarynnyy left a comment

vinokurig commented Aug 4, 2021

che-bot commented Aug 4, 2021

vinokurig commented Aug 17, 2021

azatsarynnyy commented Aug 20, 2021

fix: Adapt the Secrets plugin API to use kubernetes secrets #1166

fix: Adapt the Secrets plugin API to use kubernetes secrets #1166

Conversation

vinokurig commented Jul 9, 2021 • edited Loading

What does this PR do?

Screenshot/screencast of this PR

What issues does this PR fix or reference?

How to test this PR?

PR Checklist

Reviewers

Happy Path Channel

vinokurig commented Jul 9, 2021

codecov bot commented Jul 9, 2021 • edited Loading

Codecov Report

dmytro-ndp commented Jul 12, 2021

che-bot commented Jul 12, 2021

vzhukovs left a comment

Choose a reason for hiding this comment

benoitf Jul 20, 2021

Choose a reason for hiding this comment

vinokurig Aug 4, 2021

Choose a reason for hiding this comment

azatsarynnyy left a comment

Choose a reason for hiding this comment

vinokurig commented Aug 4, 2021

che-bot commented Aug 4, 2021

vinokurig commented Aug 17, 2021

azatsarynnyy commented Aug 20, 2021

vinokurig commented Jul 9, 2021 •

edited

Loading

codecov bot commented Jul 9, 2021 •

edited

Loading