Allow preset filtering by domains and emails #7239
Conversation
Signed-off-by: Happy2C0de <46957159+Happy2C0de@users.noreply.github.com>
kubermatic-bot
added
release-note
|
Hi @Happy2C0de. Thanks for your PR. I'm waiting for a kubermatic member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
kubermatic-bot
added
needs-ok-to-test
Happy2C0de
changed the title
|
Question: |
|
/assign @kgroschoff @moelsayed |
…Emails Signed-off-by: Happy2C0de <46957159+Happy2C0de@users.noreply.github.com>
|
/ok-to-test |
kubermatic-bot
added
ok-to-test
|
/retest |
|
/test pre-kubermatic-etcd-launcher-e2e |
|
@zreigz I think the failures are due to some environment problems. They aren't related to this PR. Can you have a look on your side? Thanks |
|
/retest |
probably our CI |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@zreigz LGTM, is there something hindering you from merging this PR? |
|
/lgtm |
|
LGTM label has been added. Git tree hash: 33304b133e630e82bcc64f16cfbf53a22bc9cdcf
|
pkg/provider/kubernetes/preset.go
Outdated
| // otherwise, it has to match the whole email | ||
| if len(domain) == 1 { | ||
| // domain provided | ||
| if len(userDomain) == 2 && strings.EqualFold(userDomain[1], domain[0]) { |
There was a problem hiding this comment.
Did you know that email can contain more then 1 @ symbol?
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes, I knew but you don't allow it anyway.
See: https://github.com/kubermatic/kubermatic/blob/master/pkg/provider/kubernetes/preset.go#L201
here, the current code is doing the exact same thing, except that it only allows domains. I don't see a reason why the current code is ok, but the new one gets blocked by this argument.
i.e.:
userDomain := strings.Split(userInfo.Email, "@")
if len(userDomain) == 2 && strings.EqualFold(userDomain[1], requiredEmailDomain) {
if a user has email foo@bar@acme.com. The current code is looking for bar!? The new code would allow you to set
requiredEmails: ["foo@bar@acme.com"] and everybody would be happy.
|
In general it's MUCH better to create a preliminary issue where we could have an opportunity to discuss the need and execution of a new feature. |
|
@kron4eg Important things to discuss:
Nobody, gave me an answer to none of these issues even though the first two issues, are kind of relevant for your main points. OIDC integration and multi-tenancy. |
Signed-off-by: Happy2C0de <46957159+Happy2C0de@users.noreply.github.com>
|
LGTM label has been added. Git tree hash: b3238dcf72d59dc654542dd9fa660b69a47dce56
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Happy2C0de, kron4eg, zreigz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubermatic-bot
added
the
approved
|
/retest pre-kubermatic-opa-e2e |
|
@Happy2C0de: The
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest Review the full test history Silence the bot with an Also, here is a cat. |
|
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retest Review the full test history Silence the bot with an Also, here is a cat. |
|
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Signed-off-by: Happy2C0de 46957159+Happy2C0de@users.noreply.github.com
What this PR does / why we need it:
It improves the preset filtering but keeps things simple and backwards compatible.
I introduced RequiredEmails as an option on the preset type. It gives users the possibility to limit certain presets to a list of domains and even on specific emails. Both in parallel. This includes the RequiredEmailDomain functionality but the variable is kept in place for backward compatibility.
The RequiredEmailDomain is appended to RequiredEmails such that both work in parallel and that the backward compatibility is assured. Added tests will take care of that.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
I added several tests in order to verify the functionality.
Documentation:
Does this PR introduce a user-facing change?: