Automatically provision personal namespace #95
Conversation
Extend Workspace manager to support automatic provisioning of personal namespaces. The name of the namespace to create will be calculated from the user name, which is expected to be an email address. The user that namespace is created for will get konflux-admin permissions on the namespace. The implementation is idempotent. This feature should be turned on using environment variables. This change also adds an additional test suite. In order to run it at the same time with the existing test suite (the default of go test), allow to use different ports for workspace-manager. In order to use the fakeclient provided by controller-runtime, its version was updated. Signed-off-by: gbenhaim <[email protected]>
gbenhaim
force-pushed
the
provision-for-pr-2
branch
from
43eb930
to
710463e
Compare
|
@filariow this is a temporary solution until it would be possible to create namespace through the UI. It would be removed when the UI adds support for creating namespaces. |
|
|
||
| func normalizeEmail(email string) string { | ||
| ret := strings.Replace(email, "@", "-", -1) | ||
| ret = strings.Replace(ret, "+", "-", -1) |
There was a problem hiding this comment.
Any concern that users with different emails will get the same normalized email? e.g. [email protected] and [email protected] (or users with very long emails that start the same)
There was a problem hiding this comment.
I agree that this may be a concern long term. The alternatives I can think of to avoid collisions will lead to ugly namespace names and/or break idempotency, like appending a kubernetes generated suffix or using IdP's user Id.
What if we allow the user to define the namespace name instead? We'll break idempotency, but we'll also avoid collisions. We should also implement a quota mechanism, that can be very simple for now: we can store the user email (or email hash to be RFC1123 compliant) in an annotation/label. In the create endpoint we can check that the user has not already created a namespace by checking this annotation/label on existing namespaces.
| UserIdAnnotation string = "konflux-ci.dev/requester-user-id" | ||
| ) | ||
|
|
||
| func normalizeEmail(email string) string { |
There was a problem hiding this comment.
I think the function name is misleading. It returns the namespace name rather than just normalizing the email.
|
|
||
| err = nsp.k8sClient.Create(c.Request().Context(), rb) | ||
| if errors.IsAlreadyExists(err) { | ||
| c.Logger().Warn("Role binding for the initial admin already exists.") |
There was a problem hiding this comment.
Suppose that 2 users share the same normalized email, the second user will get no access to the workspace (which is good), but will also not get indication for that. I think this might not be an issue, but maybe we should take a note of that or something.
| ) | ||
|
|
||
| var _ = DescribeTable( | ||
| "Test CheckNSExistHandler", |
There was a problem hiding this comment.
| "Test CheckNSExistHandler", | |
| "Test CreateNSHandler", |
| port := "5000" | ||
| if val, ok := os.LookupEnv("WM_HTTP_PORT"); ok { | ||
| port = val | ||
| } |
There was a problem hiding this comment.
If you like this can be simplified as
| port := "5000" | |
| if val, ok := os.LookupEnv("WM_HTTP_PORT"); ok { | |
| port = val | |
| } | |
| // import "cmp" | |
| // const defaultHttpPort = "5000" | |
| port := cmp.Or(os.Getenv("WM_HTTP_PORT"), defaultHttpPort) |
| var NotSignedUp SignupStatusReason = "NotSignedUp" | ||
| var UnknownSignUpStatus SignupStatusReason = "Unknown" |
There was a problem hiding this comment.
If I'm not mistaken we could use const here
| var NotSignedUp SignupStatusReason = "NotSignedUp" | |
| var UnknownSignUpStatus SignupStatusReason = "Unknown" | |
| const NotSignedUp SignupStatusReason = "NotSignedUp" | |
| const UnknownSignUpStatus SignupStatusReason = "Unknown" |
|
|
||
| func normalizeEmail(email string) string { | ||
| ret := strings.Replace(email, "@", "-", -1) | ||
| ret = strings.Replace(ret, "+", "-", -1) |
There was a problem hiding this comment.
I agree that this may be a concern long term. The alternatives I can think of to avoid collisions will lead to ugly namespace names and/or break idempotency, like appending a kubernetes generated suffix or using IdP's user Id.
What if we allow the user to define the namespace name instead? We'll break idempotency, but we'll also avoid collisions. We should also implement a quota mechanism, that can be very simple for now: we can store the user email (or email hash to be RFC1123 compliant) in an annotation/label. In the create endpoint we can check that the user has not already created a namespace by checking this annotation/label on existing namespaces.
Extend Workspace manager to support automatic provisioning of personal namespaces.
The name of the namespace to create will be calculated from the user name, which is expected to be an email address.
The user that namespace is created for will get konflux-admin permissions on the namespace.
The implementation is idempotent.
This feature should be turned on using environment variables.
This change also adds an additional test suite. In order to run it at the same time with the existing test suite (the default of go test), allow to use different ports for workspace-manager.
In order to use the fakeclient provided by controller-runtime, its version was updated.