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

Control API Architecture #63

Merged
merged 2 commits into from
Nov 9, 2021
Merged

Control API Architecture #63

merged 2 commits into from
Nov 9, 2021

Conversation

tobru
Copy link
Member

@tobru tobru commented Nov 3, 2021

Checklist

  • Try to isolate changes into separate PRs (to build a better changelog).
  • Categorize the PR by setting a good title and adding one of the labels:
    change, decision, requirement/quality, requirement/functional, dependency
    as they show up in the changelog

@tobru tobru mentioned this pull request Nov 3, 2021
2 tasks
@tobru tobru requested review from ccremer and corvus-ch November 3, 2021 10:02
@ccremer ccremer mentioned this pull request Nov 5, 2021
2 tasks
@tobru tobru requested review from ccremer and corvus-ch November 5, 2021 09:02
Copy link
Contributor

@corvus-ch corvus-ch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sync between Keycloak and an APPUiO Cloud Zone is based on the user name. This architecture now uses the users UUID. While this solves the issue of namespace name uniquenss (no need to prefix), it introduces a cognitive disconect for the users (one system uses username the other uses UUID). I would prefer to stick with username and accept the need for prefexing.

@ccremer
Copy link
Contributor

ccremer commented Nov 5, 2021

The sync between Keycloak and an APPUiO Cloud Zone is based on the user name. This architecture now uses the users UUID. While this solves the issue of namespace name uniquenss (no need to prefix), it introduces a cognitive disconect for the users (one system uses username the other uses UUID). I would prefer to stick with username and accept the need for prefexing.

If we use the username in a metadata.name for namespaces, it will impose a limit to what usernames are possible. We would need to verify that usernames are matching a-z and no certain special characters (https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-label-names). We couldn't use email addresses as usernames for example since they contain @ (depending on a brokered IdP this can be a case). However, in https://kb.vshn.ch/appuio-cloud/references/quality-requirements/usability/user-arbitrary-name.html we defined that users can choose an arbitrary name.

As ugly as it is, we can't just take the username verbatim and prefix it.

@tobru tobru requested a review from corvus-ch November 5, 2021 13:50
@tobru tobru requested a review from corvus-ch November 5, 2021 15:11
@corvus-ch corvus-ch self-requested a review November 8, 2021 09:08
Copy link
Contributor

@corvus-ch corvus-ch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I object against having two conflicting documentations in paralell. While it seems more complicated, I understand and agree with user being a global resource with all those roles and rolebindings (does this work as expected/was this tested?).

@tobru
Copy link
Member Author

tobru commented Nov 8, 2021

I object against having two conflicting documentations in paralell.

As written in the internal chat, the idea is to delete the page which doesn't reflect our proposal. It's only here to have both options during discussions. Before it gets merged, a cleanup will be done.

While it seems more complicated, I understand and agree with user being a global resource with all those roles and rolebindings (does this work as expected/was this tested?).

Testing currently ongoing, but by-the-docs it should ™️ work.

@tobru tobru requested a review from corvus-ch November 9, 2021 08:15
@tobru tobru requested a review from ccremer November 9, 2021 14:51
@tobru tobru merged commit 89e0ca4 into master Nov 9, 2021
@tobru tobru deleted the control-api-arch branch November 9, 2021 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants