-
Notifications
You must be signed in to change notification settings - Fork 70
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
fix: Add a predefined secret to store credentials #68
Conversation
❌ E2E Happy path tests failed ❗ See Details
Test product:
Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this approach is better than the previous, but would prefer the predefined secret to have an abstract name, e.g. workspace-credentials-secret
. This sort of name maps more naturally onto what is provisioned in the DevWorkspace Operator, since it would look strange for Web Terminals to come with access to che-credentials-secret
.
k8sClient, | ||
SECRETS_ROLE_NAME, | ||
singletonList("secrets"), | ||
singletonList("che-credentials-secret"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we choose a Che-agnostic name -- e.g. workspace-credentials-secret
? If we want to do a similar thing in DWO, it's a lot easier to justify if it's not labelled with a specific platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for workspace-credentials-secret
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+ please consider moving that secret name somewhere to constants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
k8sClient, | ||
SECRETS_ROLE_NAME, | ||
singletonList("secrets"), | ||
singletonList("che-credentials-secret"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+ please consider moving that secret name somewhere to constants
❌ E2E Happy path tests failed ❗ See Details
Test product:
Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe |
@vinokurig can you merge latest "main" please? |
❌ E2E Happy path tests failed ❗ See Details
Test product:
Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe |
❌ E2E Happy path tests failed ❗ See Details
Test product:
Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe |
❌ E2E Happy path tests failed ❗ See Details
Test product:
Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe |
Signed-off-by: Igor Vinokur <ivinokur@redhat.com>
✅ E2E Happy path tests succeed 🎉 See Details
Test product:
Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe |
@skabashnyuk @sparkoo @mshaposhnik I am going to merge this PR tomorrow if no objections? |
...lipse/che/workspace/infrastructure/kubernetes/namespace/AbstractWorkspaceServiceAccount.java
Outdated
Show resolved
Hide resolved
String name, List<String> resources, List<String> apiGroups, List<String> verbs) { | ||
String name, | ||
List<String> resources, | ||
@Nullable List<String> resourceNames, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pass empty list, instread of nullable
...lipse/che/workspace/infrastructure/kubernetes/namespace/AbstractWorkspaceServiceAccount.java
Outdated
Show resolved
Hide resolved
...lipse/che/workspace/infrastructure/kubernetes/namespace/AbstractWorkspaceServiceAccount.java
Outdated
Show resolved
Hide resolved
...pse/che/workspace/infrastructure/kubernetes/namespace/KubernetesWorkspaceServiceAccount.java
Outdated
Show resolved
Hide resolved
✅ E2E Happy path tests succeed 🎉 See Details
Test product:
Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe |
.../java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespace.java
Outdated
Show resolved
Hide resolved
...lipse/che/workspace/infrastructure/kubernetes/namespace/AbstractWorkspaceServiceAccount.java
Outdated
Show resolved
Hide resolved
❌ E2E Happy path tests failed ❗ See Details
Test product:
Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe |
❌ E2E Happy path tests failed ❗ See Details
Test product:
Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe |
canCreateNamespace(identity), | ||
labelNamespaces ? namespaceLabels : emptyMap(), | ||
annotateNamespaces ? namespaceAnnotationsEvaluated : emptyMap()); | ||
if (newNamespace) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, this will be true only in case that we've created the namespace in previous namespace.prepare
call. So what if namespace already exists? Don't we need the secret then? The namespace could be prepared by admins or created by older Che version. What if user removed/modified the secret ? Don't we need to reconcile ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, reworked to check if the secret exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code looks good to me, thanks.
❌ E2E Happy path tests failed ❗ See Details
Test product:
Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe |
❌ E2E Happy path tests failed ❗ See Details
Test product:
Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe |
[crw-ci-test] |
❌ E2E Happy path tests failed ❗ See Details
Test product:
Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe |
[crw-ci-test --rebuild] |
❌ E2E Happy path tests failed ❗ See Details
Test product:
Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe |
[crw-ci-test --rebuild] |
✅ E2E Happy path tests succeed 🎉 See Details
Test product:
Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe |
Signed-off-by: Igor Vinokur ivinokur@redhat.com
What does this PR do?
depends on eclipse-che/che-operator#971
Screenshot/screencast of this PR
What issues does this PR fix or reference?
eclipse-che/che#19837
How to test this PR?
curl -X POST <kubernetes API url>/api/v1/namespaces/<namespace name>/secrets--header "Content-Type: application/json-patch+json" -d '[{ "op": "add", "path": "/data", "value": { "key": "" } }]'
PR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or reference
andHow to test this PR
completedReviewers
Reviewers, please comment how you tested the PR when approving it.