-
Notifications
You must be signed in to change notification settings - Fork 372
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
✨ Add interactive cmd prompt for permission claims #2309
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
552cc9d
to
25c8ff6
Compare
pkg/cliplugins/claims/cmd/cmd.go
Outdated
%[1]s claims get apibinding cert-manager | ||
|
||
# Edit the permission claims' status for all APIBindings in current workspace with an interactive prompt. | ||
%[1]s claims get apibinding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%[1]s claims get apibinding | |
%[1]s claims edit apibinding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reads weird - maybe require --all
or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to just to list all the open claims for all bindings in case a specific binding wasn't given. It follows a similar format to the get
command. If we add --all
flag then we should probably do so for get
too.
} | ||
} | ||
|
||
func readInput(reader *bufio.Reader) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this different from readLine()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could make both the methods into one. The reason for having it different was in future we can extend this to parse an array or process string with a different delimiter. Say,
func readArray(reader *bufio.Reader) []string {
arr := make([]string, 0)
text := readLine(reader)
for _, words := range strings.Split(text, ",") {
arr = append(arr, strings.TrimSpace(words))
}
return arr
}
This is just to make post processing of string separate for future use cases.
return strings.ToLower(strings.Trim(strings.TrimSpace(text), "`'\"")) | ||
} | ||
|
||
func inferText(input string, wr io.Writer) (ClaimAction, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any library to do this for us? I imagine there are other permutations as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried digging into some available options like go-prompt and prompt-ui. These are shell libraries which have more advanced features like auto-complete/suggestions or selections . And for creating simple prompts as we have done, they follow a similar approach as here. And then also looked into some upstream repos, like kubectl (https://github.com/kubernetes/kubectl/blob/master/pkg/cmd/util/editor/editoptions.go#L889) and kubebuilder (https://pkg.go.dev/sigs.k8s.io/kubebuilder/v3/pkg/plugin/util#YesNo), which seemed to follow a similar approach. For (Y)es|(N)o|(S)kip
case, the available possibilities are less (precisely 6 - given that we are converting all inputs into a single case for inferring it).
A question I don't see addressed in the design doc - will there be a way for users to programmatically accept permission claims? I know there's danger there and we might not want to make it too easy, but I'm anticipating someone asking. |
25c8ff6
to
60fd0da
Compare
Agreed, for now my understanding was anyone who can access the workspace where APIBinding was created can change the state of the claim. Or maybe the right wording is, any user who can edit the apibinding can accept/reject respective claim. Are we looking to add another layer of rbac, wherein a user can view the binding, edit the export reference but not permission claim? (more like claims can be modified only be admins?) |
60fd0da
to
1671008
Compare
That's probably sufficient; if someone wants to automate things, they can use Go or wrappers around |
@varshaprasad96: PR needs rebase. 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. |
@varshaprasad96 so sorry for losing sight of this PR! Are you still available to work on it, or would you be interested in trying to find someone to take it over for you? |
@ncdc I can work on this sometime this week and rebase with the latest changes. |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with /lifecycle stale |
I still think we want this / something like this. /remove-lifecycle stale |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with /lifecycle stale |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with /lifecycle rotten |
Summary
This PR introduces an interactive command prompt to edit the status of open claims for apibindings. The user can provide - Yes/No/Skip to accept/reject/skip any action on a permission claim respectively.
Use cases:
What happens after editing?
spec.AcceptablePermssionClaim
gets updated based on the user input. For example:PS: With the updates added to the permission claims spec (like sub-resources, verbs), the prompt's UI will need to change accordingly.
Design doc: https://docs.google.com/document/d/1J31wXY1-2aCyyGFjUlusKoHDzV-zktAmRdIMjMf4OR0/edit
Related issue(s)
Fixes #