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

Invalid default namespace name #15323

Closed
4 tasks done
nwalens opened this issue Nov 26, 2019 · 42 comments
Closed
4 tasks done

Invalid default namespace name #15323

nwalens opened this issue Nov 26, 2019 · 42 comments
Labels
area/che-server area/dashboard kind/epic A long-lived, PM-driven feature request. Must include a checklist of items that must be completed. severity/P1 Has a major impact to usage or development of the system.

Comments

@nwalens
Copy link

nwalens commented Nov 26, 2019

By default it seems that Che creates namespaces based on usernames which in turn means that the username has to comply with the DNS naming standards.

In my case, my username is like foo.bar and when I create a new workspace, Che tries to create an Openshift project named foo.bar-che which is not accepted by Openshift.

@sleshchenko sleshchenko changed the title Invalid namespace names Invalid default namespace name Nov 26, 2019
@sleshchenko sleshchenko transferred this issue from eclipse-che/che-operator Nov 26, 2019
@sleshchenko
Copy link
Member

@nwalens Thanks for reporting it. It's an important issue we need to handle

@sleshchenko sleshchenko added kind/bug Outline of a bug - must adhere to the bug report template. team/platform severity/P1 Has a major impact to usage or development of the system. labels Nov 26, 2019
@skabashnyuk skabashnyuk added this to the Backlog - Platform milestone Nov 26, 2019
@skabashnyuk
Copy link
Contributor

rfc1035/rfc1123 label (DNS_LABEL) : An alphanumeric (a-z, and 0-9) string, with a maximum length of 63 characters, with the '-' character allowed anywhere except the first or last character, suitable for use as a hostname or segment in a domain name

I can suggest such a solution for this use-case.

  1. We do not allow to log in users with a username that is not compatible with https://github.com/eBay/Kubernetes/blob/master/docs/design/identifiers.md#definitions rfc1035/rfc1123 label (DNS_LABEL) in all cases: example of code to check. https://github.com/kubernetes/apimachinery/blob/master/pkg/util/validation/validation.go#L42-L71
  2. Same as 1. but only in case if a username is used as a placeholder for namespaces.

I think variant 1. is safer since we might use username in other places that require the same restrictions.

WDYT @metlos @sleshchenko @slemeur @l0rd @sparkoo ?

@sparkoo
Copy link
Member

sparkoo commented Nov 28, 2019

I'm technically ok with that, but I'm not sure we can allow limitation like that. What if company uses some incompatible format for employees usernames? Do we force them to treat Che deployment in different way to all their other internal services?

@skabashnyuk
Copy link
Contributor

Do we force them to treat Che deployment in different way to all their other internal services?

I would say: If the companies want to use usernames as names of k8s objects, they should carefully think about username restrictions.
Anyway we may use less strict option 2)

@sparkoo
Copy link
Member

sparkoo commented Nov 28, 2019

I'm for option 2. and provide another placeholder for namespaces <username_k8s>, that converts username to DNS friendly + append hash.
Keep simple <username>-che as default with documented limitation and suggest to use <username_k8s> if needed.

@skabashnyuk
Copy link
Contributor

and provide another placeholder for namespaces <username_k8s>, that converts username to DNS friendly + append hash.

It has too many edge cases. I would try to avoid going this way.

@metlos
Copy link
Contributor

metlos commented Dec 3, 2019

One other idea would be to store the value that should be used for <username> expansion inside keycloak as an additional attribute. We could then retrieve this on the che side and use it.

@alexeykazakov, would this approach be workable for your case as well, given what you said in #15300 (comment) ?

@skabashnyuk
Copy link
Contributor

@metlos then it becomes a new feature. Not username placeholder but attribute from OpenID profile. Right?

@alexeykazakov
Copy link

One other idea would be to store the value that should be used for expansion inside keycloak as an additional attribute. We could then retrieve this on the che side and use it.

But how keycloak will get that compliant username? And how you can make sure that it's exactly the namespace which is expected to be used?

As I mentioned in #15300 (comment) you can't really guarantee username -> namespace mapping inside Che.
For example if you try to convert john.smith to john-smith you may fail if there is already a user with john-smith username. So, you will need to convert it to something like john-smith-1. And how would you know what namespace to be use for that user? john-smith-1-che or john-smith-che?

It could work if che creates these namespaces. But if Che is supposed to use the existing namespaces (like in Hosted Toolchain) then the namespace has to come from the cluster/operator admin/controller. I was thinking that the namespace to be used can be stored as an annotation in OpenShift User (or Identity) resource. And it should be set by the admin or, in case of Hosted Toolchain, by Toolchain operator during namespace provisioning.
If there is no such annotation then use something by default (but the default value may fail).

@metlos
Copy link
Contributor

metlos commented Dec 5, 2019

I was thinking that the namespace to be used can be stored as an annotation in OpenShift User (or Identity) resource.

That's exactly what I had in mind. There would be an externally set attribute on the keycloak identity that we would use as an expansion for <username> inside the namespace name.

@alexeykazakov
Copy link

That's exactly what I had in mind. There would be an externally set attribute on the keycloak identity that we would use as an expansion for inside the namespace name.

By Keycloak Identity you mean JWT token generated by Keycloak, right? But how it's supposed to be set in the token?

I was thinking that a cluster admin (in case of Hosted Toolchain it will be toolchain operator) adds an annotation to OpenShift User (or Identity) resource. And this is attributed is picked up by Che. How Che get access to it is another question. If you don't want/can't get the OpenShift User/Identity resource directly from the cluster for some reason then theoretically you could pass that attribute through Keycloak User/Identity attribute and map it to some JST claim in the token. But I have no idea how you could make Keycloak to add such attribute to its Identity using the OpenShift User.meta.attributes["].

@metlos
Copy link
Contributor

metlos commented Dec 9, 2019

I am not that well versed in what information is passed around in the tokens, but I know that we read the the user object from Keycloak to establish e.g. their email address. Using the same call, I believe we can also obtain the free-form attributes that can be attached to the user in Keycloak. From there it's just a matter of propagating it throughout our call-chains, I believe.

@skabashnyuk skabashnyuk added this to To do in Platform-2020-01-07 via automation Dec 10, 2019
@alexeykazakov
Copy link

alexeykazakov commented Dec 12, 2019

As far as I understand Keycloak gets email directly from the user. During the first login Che Keycloak asks me to provide my email. Then it's mapped by Keycloak to the token claim.
We can't ask users to provide the namespace during the login. To be honest that email/name form from Keycloak looks pretty weird too (hey, I just logged in using my IdP which has all information and now I have to type it again? why?).
Anyway. I don't really see how user (che) namespace name could be passed via Keycloak.

@mingfang
Copy link

Can we replaces illegal characters with -?
e.g. foo.bar to foo-bar-che

@alexeykazakov
Copy link

Can we replaces illegal characters with -?
e.g. foo.bar to foo-bar-che

It’s not that simple. What if there is already foo-bar-che user?

@triceam
Copy link

triceam commented Jun 22, 2020

Running into this too...

When integrated with oAuth, I'm seeing my username default to IAM#amtrice@us.ibm.com (which will always yield an invalid workspace name if left as-is. If you change the username to a valid one, the issue goes away. At the very least, can the initial username be scrubbed so it does not default to a bad value? By changing the default, we at least minimize the user's chances of going down a bad path.

IE:

Here's the default experience:

image

Eventually, you'll end up with this error and no way to work around it:

image

However, if you change the username like so:

image

Everything works fine:

image

@skabashnyuk
Copy link
Contributor

skabashnyuk commented Jul 30, 2020

I've sent some thoughts to the mail list https://www.eclipse.org/lists/che-dev/msg03864.html. Please participate if you are interested

@l0rd
Copy link
Contributor

l0rd commented Nov 25, 2020

I will try to summarize current situation to understand if we can close this issue:

Currently a developer starts his first Che workspace and...

If a namespace with labels and annotations described here exist, in particular with label che.eclipse.org/username: <username>, the workspace objects will be created there.

Otherwise a new namespace will be created. A sanitized <username> will be used in the namespace name. The namespace will be labelled/annotated as documented here.

So, in both cases, even if <username> is "invalid", the workspace will be created successfully.

@skabashnyuk on OpenShift with OAuth enabled, does <username> correspond to OpenShift username or Che username? In other words, if the user changes his username as described here, is Che able to create the workspace in the namespace with annotation che.eclipse.org/username: <openshift-username>?
And why the developer needs admin privileges in the namespace?

@alexeykazakov this looks like the proper way to fix #17265, as soon as you will be able to annotate code namespaces we should switch to this mechanism.

@alexeykazakov
Copy link

alexeykazakov commented Nov 25, 2020

this looks like the proper way to fix #17265, as soon as you will be able to annotate code namespaces we should switch to this mechanism.

I'm not sure I fully understand the docs.. Please correct me if I'm wrong but you are saying that we can:

  • Add CHE_INFRA_KUBERNETES_NAMESPACE_ANNOTATIONS: che.eclipse.org/username=<username> to CheCluster
  • Add che.eclipse.org/username: ${username} annotation to all *-code namespaces

Is it correct?

But what ${username} is supposed to be used here? Is it Che username or OpenShift username? If it's OpenShift then we can add this annotation. But mismatching between openshift and che username might cause other issues anyway. For example Che user delete API requires Che/Keyckloak user ID. To get this ID we need to use Che Keycloak endpoint to map the Che/Keycloak username to the ID. But OpenShift controllers/operators have no idea what Che username is if it doesn't match the OpenShift username.

@sparkoo
Copy link
Member

sparkoo commented Nov 26, 2020

on OpenShift with OAuth enabled, does <username> correspond to OpenShift username or Che username? In other words, if the user changes his username as described here, is Che able to create the workspace in the namespace with annotation che.eclipse.org/username: <openshift-username>?

AFAIK we use Che username, so it won't work with OpenShift one.

And why the developer needs admin privileges in the namespace?

We're using impersonated openshift client to create/delete the workspace objects, so user has to have the permissions to manage objects in the namespace. When we're creating the namespace from Che, User has admin permissions to the namespace as well.

@l0rd
Copy link
Contributor

l0rd commented Nov 26, 2020

@sparkoo ok but the doc link I provided is in the section "Pre-creating namespaces for users". If the namespace is pre-created why does the Che user need admin role? Edit role should be enough.

@sparkoo
Copy link
Member

sparkoo commented Nov 26, 2020

@sparkoo ok but the doc link I provided is in the section "Pre-creating namespaces for users". If the namespace is pre-created why does the Che user need admin role? Edit role should be enough.

@l0rd you may be correct. I'm not sure if I've tested it with edit role. I will try and let you know.

@alexeykazakov
Copy link

I'm not sure if it's changed in the latest Che but edit role was not enough because Che created Roles/RoleBindings in the workspace namespace and the edit role does not grant these permissions.
This is why we have to explicitly grant Sandbox users the permissions to manage RBAC in their -code namespaces.

@l0rd
Copy link
Contributor

l0rd commented Nov 26, 2020

I'm not sure if it's changed in the latest Che but edit role was not enough because Che created Roles/RoleBindings in the workspace namespace and the edit role does not grant these permissions.

Gotcha. Would it be possible to create those rolebindings as part of the namespace provisioning?

@alexeykazakov
Copy link

Gotcha. Would it be possible to create those rolebindings as part of the namespace provisioning?

If they are not "dynamic" and can be added as an OpenShift template to https://github.com/codeready-toolchain/host-operator/blob/master/deploy/templates/nstemplatetiers/basic/ns_code.yaml then yes, we can do that.

@sparkoo
Copy link
Member

sparkoo commented Nov 26, 2020

@l0rd @alexeykazakov I've just tested it and I can confirm that

  • we work with Che usernames only
  • only edit ClusterRole does not work for the reason Alexey mentioned. Workspace fail to start due to lack of permissions when trying to get Roles.

The backport PR of the precreated namespace feature is here #18471 If you're ok with it, I'll merge on Friday. The documentation is already there in 7.20.x and I have no idea why (#18471 (comment)).

@sparkoo
Copy link
Member

sparkoo commented Nov 27, 2020

@alexeykazakov currently it is possible to use only che username to match prepared namespaces https://www.eclipse.org/che/docs/che-7/installation-guide/configuring-namespace-strategies/#_pre_creating_namespaces_for_users.

We can add support for openshift username or openshift userid as well. Do you think that will fix your issue here?

@l0rd
Copy link
Contributor

l0rd commented Nov 30, 2020

We can add support for openshift username or openshift userid as well. Do you think that will fix your issue here?

@alexeykazakov would adding che.eclipse.org/openshift-userid: ${userid} or che.eclipse.org/openshift-username: ${username} annotation to all *-code namespaces work? We could then avoid patching Keycloak registration form.

@alexeykazakov
Copy link

@l0rd The ${username} in che.eclipse.org/openshift-username: ${username} is the OpenShift Username, correct?
So, for alexeykazakov OpenShift user it will be che.eclipse.org/openshift-username: alexeykazakov in alexeykazakov-code namespace. And then even if I chose another username (let's say alexeykazakov-che when logging into the CRW Dashboard then CRW will still use alexeykazakov-code namespace, correct?
If so, then we can add it but:

We can add support for openshift username or openshift userid as well. Do you think that will fix your issue here?

We would need to get that ^^^ working in our CRW instance before we can start using che.eclipse.org/openshift-username annotation without the read-only KC registration form. Otherwise our deactivation will be broken.

Also,.. how far away are we from dropping that redundant (from the UX point of view) Keycloak form completely?

@l0rd
Copy link
Contributor

l0rd commented Dec 1, 2020

@l0rd The ${username} in che.eclipse.org/openshift-username: ${username} is the OpenShift Username, correct?

Correct

So, for alexeykazakov OpenShift user it will be che.eclipse.org/openshift-username: alexeykazakov in alexeykazakov-code namespace. And then even if I chose another username (let's say alexeykazakov-che when logging into the CRW Dashboard then CRW will still use alexeykazakov-code namespace, correct?

Correct

We can add support for openshift username or openshift userid as well. Do you think that will fix your issue here?

We would need to get that ^^^ working in our CRW instance before we can start using che.eclipse.org/openshift-username annotation without the read-only KC registration form. Otherwise our deactivation will be broken.

Sure, we are not there yet. I wanted to understand if we should plan to work on it or not. In the meantime you may start adding the label che.eclipse.org/openshift-username: <openshift-username> to -code namespaces.

Also,.. how far away are we from dropping that redundant (from the UX point of view) Keycloak form completely?

That's a separate topic. We should probably have a call to discuss it. Anyway we do not have a plan for that yet.

@sparkoo
Copy link
Member

sparkoo commented Dec 1, 2020

the username placeholder must be used in annotation, because label value has DNS character limitation https://www.eclipse.org/che/docs/che-7/installation-guide/configuring-namespace-strategies/#_pre_creating_namespaces_for_users

@sparkoo
Copy link
Member

sparkoo commented Dec 1, 2020

@alexeykazakov @l0rd I've created issue for openshift-username #18500

@alexeykazakov
Copy link

@sparkoo what about https://<che-host>/api/user/find?name=<username> endpoint which we use to obtain the che-keycloak user ID? We need that ID to use it in the che user deletion endpoint.
See https://issues.redhat.com/browse/CRT-632?focusedCommentId=15394994&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-15394994

Currently that endpoint requires Che username. We don't have it but it's not a problem when openshift username matches the che username. Will it be covered by #18500 ?

@sparkoo
Copy link
Member

sparkoo commented Dec 2, 2020

Will it be covered by #18500 ?

feels like different issue to me

@alexeykazakov
Copy link

Ok. But this would be a blocker for us. We can't start relying on the username annotation until OpenShift usernames a fully supported in /api/user?find too.

@skabashnyuk
Copy link
Contributor

@alexeykazakov the team has some difficulties to understand the complications you have and the directions you see to solve it.

From what we know in the environment you have OpenShift Name= Keycaloak Name = Che name
because Kycloak UI theme is used that does not allow username change. So you should not be blocked at this moment.

Potentially we want to remove the requirement with Kycloak UI theme. That is why @sparko created
#18500 issue. in this case, you only need information that is stored
in the OpenShift User object like that

apiVersion: v1
items:
- apiVersion: user.openshift.io/v1
  groups: null
  identities:
  - htpasswd_provider:opentlc-mgr
  kind: User
  metadata:
    creationTimestamp: "2020-11-30T07:36:06Z"
    name: opentlc-mgr <---------
    resourceVersion: "153533"
    selfLink: /apis/user.openshift.io/v1/users/opentlc-mgr
    uid: 302108f9-7ae9-447d-8a8f-84a9a92b78a5
kind: List
metadata:
  resourceVersion: ""
  selfLink: ""

If that is not suitable for you can you explain what user's information you have that you can put in namespace labels so later we can identify him on che server side.
Ideally without calling /api/user?find method.

@alexeykazakov
Copy link

From what we know in the environment you have OpenShift Name= Keycaloak Name = Che name because Kycloak UI theme is used that does not allow username change. So you should not be blocked at this moment.

We are still affected by that issue until this read-only theme is available in CRW Addon.

Potentially we want to remove the requirement with Kycloak UI theme. That is why @sparko created #18500 issue. in this case, you only need information that is stored in the OpenShift User object like that

It you drop that read-only theme and users can edit the username again then OpenShift Name= Keycaloak Name = Che name won't be true anymore. Or I'm missing something?

Ideally without calling /api/user?find method.

We need /api/user?find to obtain the Che/Keycloak User ID to be able to delete the Che user. Currently it requires Che username. But on our side we know the OpenShift username only. If Che username == OpenShift User name then it's not a problem. But if you users can change the Che username so it doesn't match the OpenShift username then we can't use /api/user?find any longer which will break our GDPR and deactivation flows.

@skabashnyuk
Copy link
Contributor

I've talked today with @ibuziuk. We believe that you may rely on the fact that OpenShift Name= Keycaloak Name = Che name in your environment so users deactivation flows should be pretty simple for you.

@alexeykazakov
Copy link

I've talked today with @ibuziuk. We believe that you may rely on the fact that OpenShift Name= Keycaloak Name = Che name in your environment so users deactivation flows should be pretty simple for you.

But we can't rely on it if you drop the read-only form and users are able to change the username. I thought this is what you were asking. If we can drop that form and rely on the username annotation instead.
Did I misunderstand you?

@skabashnyuk
Copy link
Contributor

@alexeykazakov for sure we don't want to make your or any of our customer's life more complicated. I've just said that we "Potentially we want to remove the requirement with Kycloak UI theme" and that is only in case if that will not introduce new complications. I believe here we are mixing two problems together. That I'm not sure that it is reasonable to track in this in this concrete issue
That is 1. Pre-created by admin user's k8s namespaces. 2. User's de-provisioning.

BTW.
As I can see Keycloak maintain a connection between real OpenShift User. And since Che user id =(always) Keycloak User id.
The connection between Che user and OpenShift user is always can be determined.
Знімок екрана 2020-12-07 о 10 02 22
Знімок екрана 2020-12-07 о 10 02 13

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/che-server area/dashboard kind/epic A long-lived, PM-driven feature request. Must include a checklist of items that must be completed. severity/P1 Has a major impact to usage or development of the system.
Projects
None yet
Development

No branches or pull requests

10 participants