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

Allow scope to be passed as array #150

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SiebelsTim
Copy link
Contributor

Scopes are currently passed as a scope string, separating scopes by spaces.
Clients can grow to many scopes, resulting in a very long string.

This change allows us to specify scopes using the property scopeArray. That way, we can separate scopes by newlines.
Additionally, this allows us to comment a single scope temporarily or add a comment for a specific scope, e.g. as a reason why that client has this scope granted.

Checklist

  • I have read the contributing guidelines and signed the CLA.
  • I have referenced an issue containing the design document if my change introduces a new feature.
  • I have read the security policy.
  • I confirm that this pull request does not address a security vulnerability.
    If this pull request addresses a security vulnerability,
    I confirm that I got approval (please contact [email protected]) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added the necessary documentation within the code base (if appropriate). -- Is kubectl explain enough?

Scopes are currently passed as a scope string, separating scopes by
spaces.
Clients can grow to many scopes, resulting in a very long string.

This change allows us to specify scopes using the property scopeArray.
That way, we can separate scopes by newlines.
Additionally, this allows us to comment a single scope temporarily or
add a comment for a specific scope, e.g. as a reason why that client has
this scope granted.
@Demonsthere
Copy link
Collaborator

Hi there!
I am kinda conflicted here 😕. On one hand, it is a pretty simple change, and we can have both scopes and scropeArray in the CR, on the other hand a proper solution imho would be to add a mutationwebhook, which would transform the CR into the new format, wdyt?

@SiebelsTim
Copy link
Contributor Author

Hi!
I think the greatest downside is having both fields present in the CR. That is prone to bugs.

I don't know if we can solve this though. I don't have much experience with mutation webhooks, but from what I see, we would need to have both fields present in the CRD to be able to transmit the payload to the API server. Only if that is given, the webhook is called; otherwise it is denied by the API server directly.

Considering that, introducing mutating webhooks doesn't seem worth the effort to me. The only upside I see is that the resulting resource is clean: It would only contain one of the fields, not both. Am I missing something here?

@Demonsthere
Copy link
Collaborator

After some thought, i think the api is in a early enough version, that we can leave both and mark the scopes fields as deprecated, removing it after some time :)

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.

2 participants