openshift / console

OpenShift Cluster Console UI
https://www.openshift.org
Apache License 2.0
392 stars 599 forks source link

Hash or encode controller.devfile.io/creator label when backend validates Web Terminal creator #13879

Open AObuchow opened 1 month ago

AObuchow commented 1 month ago

With https://github.com/openshift/console/commit/e87dc6fc852d23e8fca211cd0d3862b07cefa79e, the OpenShift console expects the controller.devfile.io/creator label on a Web Terminal DevWorkspace to be the users username if they do not have a UID (as is the case for kubeadmin). However, certain characters permitted in usernames are not valid Kubernetes label values.

Thus, changes must be made to the OpenShift Console backend to expect a different value for the controller.devfile.io/creator label. A possible option is to SHA256 hash the username and obtain the hex representation as the expected controller.devfile.io/creator label value. A similar approach was used in the usersettings code.

Another potential option is to use Base32 encoding with an alternate padding character instead of = (which is not a valid character for Kubernetes labels) and change all characters in the encoding to lower case. Unlike Base64, Base32 does not mix lower and upper case characters.

Still, taking a similar approach to the usersettings code of hashing the username would be more consistent and is probably prefferable.

Additional Context

This is a blocker for https://github.com/openshift/console/issues/13696, and a follow-up on an earlier comment of mine..

Whichever hashing or encoding process for the username is used must also be adopted in the DevWorkspace Operator which is responsible for adding the controller.devfile.io/creator label on Web Terminal DevWorkspaces: https://github.com/devfile/devworkspace-operator/issues/1245

AObuchow commented 1 month ago

Note: the usersettings code makes a special case for kube:admin and simply uses the value kubeadmin instead of its hashed-then-hex'ed representation. I'm not sure if there was a particular reason for this (was it due to pre-existing code?) and whether it needs to be considered for this issue. CC: @stlaz