[MTB] added block other tenant resources benchmark #1318
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
| @@ -14,5 +14,6 @@ type RunOptions struct { | |||
| Label string | |||
| KClient *kubernetes.Clientset | |||
| TClient *kubernetes.Clientset | |||
| OClient *kubernetes.Clientset | |||
There was a problem hiding this comment.
Can we be a little bit more descriptive while naming different clients?
There was a problem hiding this comment.
Yes, agreed. What would be good names here? Something like:
- ClusterAdminClient
- Tenant1Client
- Tenant2Client
| @@ -130,6 +140,9 @@ func validateFlags(cmd *cobra.Command) error { | |||
| return fmt.Errorf("tenant namespace must be set via --namespace or -n") | |||
| } | |||
|
|
|||
| benchmarkRunOptions.OtherNamespace, _ = cmd.Flags().GetString("other-namespace") | |||
| benchmarkRunOptions.OtherTenant, _ = cmd.Flags().GetString("other-tenant-admin") | |||
There was a problem hiding this comment.
Maybe we can add the arguments in a list itself like -n test test2 --as bob-k8s-access bob2-k8s-access?
There was a problem hiding this comment.
That's a great idea! @phoenixking25 what do you think?
There was a problem hiding this comment.
| if len(resource.Verbs) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| if resource.Namespaced { | ||
| continue | ||
| } |
There was a problem hiding this comment.
| if len(resource.Verbs) == 0 { | |
| continue | |
| } | |
| if resource.Namespaced { | |
| continue | |
| } | |
| if len(resource.Verbs) == 0 || resource.Namespaced { | |
| continue | |
| } |
There was a problem hiding this comment.
Please refactor as suggested.
| for _, resource := range primaryNamespaceResources { | ||
| for _, verb := range verbs { | ||
| access, msg, err := utils.RunAccessCheck(options.OClient, options.TenantNamespace, resource, verb) | ||
| if err != nil { | ||
| options.Logger.Debug(err.Error()) | ||
| return err | ||
| } | ||
| if access { | ||
| return fmt.Errorf(msg) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for _, resource := range primaryNamespaceResources { | ||
| for _, verb := range verbs { | ||
| access, msg, err := utils.RunAccessCheck(options.TClient, options.OtherNamespace, resource, verb) | ||
| if err != nil { | ||
| options.Logger.Debug(err.Error()) | ||
| return err | ||
| } | ||
| if access { | ||
| return fmt.Errorf(msg) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I think these two for loops can be clubbed into a single for loop.
There was a problem hiding this comment.
Yeah this can be made a separate function, right now this particular piece of duplicated at three places
There was a problem hiding this comment.
Please refactor as suggested.
k8s-ci-robot
added
the
needs-rebase
…o block-other-tenant-resources
…o block-other-tenant-resources
k8s-ci-robot
removed
the
needs-rebase
k8s-ci-robot
added
the
approved
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Divya063, JimBugwadia, phoenixking25 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 |
/assign @JimBugwadia