Kubectl plugin for resources that have a particular ownerref #73
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @somtochiama. Thanks for your PR. I'm waiting for a kubernetes-sigs 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. |
k8s-ci-robot
added
the
needs-ok-to-test
k8s-ci-robot
added
the
size/XL
| ns = flag.String("ns", "", "namespace") | ||
| ) | ||
|
|
||
| func main() { |
There was a problem hiding this comment.
We probably want to evolve this to use the shared code from cli-runtime, for example https://github.com/kubernetes/sample-cli-plugin
| "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | ||
| "k8s.io/client-go/discovery" | ||
| "k8s.io/client-go/tools/clientcmd" | ||
| "k8s.io/client-go/util/homedir" |
There was a problem hiding this comment.
I needed to add _ "k8s.io/client-go/plugin/pkg/client/auth" to auth to a GKE cluster
|
|
||
| func run(kubeconfig *string) error { | ||
| if len(os.Args) < 3 { | ||
| fmt.Println("Please complete command: kubectl ownerref [KIND] [NAME]") |
There was a problem hiding this comment.
Nit: you can just return the error here, as you handle it by calling os.Exit(1)
| Version: version, | ||
| Resource: resource.Name, | ||
| } | ||
| PrintResource(clientset, res, os.Args[1], os.Args[2]) |
There was a problem hiding this comment.
Nit: It's usually clearer if you start off by assigning kind := os.Args[1] and name := os.Args[2] early in the method. Effectively parse/process the args first, then use the processed values.
| Resource: resource.Name, | ||
| } | ||
| PrintResource(clientset, res, os.Args[1], os.Args[2]) | ||
| } |
There was a problem hiding this comment.
Another nit; you should probably log a warning if PrintResource returns an error, e.g. with klog.Warningf
| fmt.Println() | ||
| } | ||
|
|
||
| func getGroupVersion(s string) (string, string) { |
There was a problem hiding this comment.
You can also use schema.ParseGroupVersion
| var ownerItems []unstructured.Unstructured | ||
| for _, item := range list.Items { | ||
| for _, ownerRef := range item.GetOwnerReferences() { | ||
| if strings.ToLower(ownerRef.Kind) == kind && ownerRef.Name == name { |
There was a problem hiding this comment.
Can we have a TODO here to resolve the kind. (I think it's fine to do later, but I hit this when I tried doing kubectl ownerref rs foo, instead of kubectl ownerref replicaset foo)
| } | ||
|
|
||
| for _, apiResource := range apiResourceList { | ||
| for _, resource := range apiResource.APIResources { |
There was a problem hiding this comment.
Idea to speed this up in future: we can run these in parallel. We likely need some limit, so it's going to be tricky, but that will speed things up a lot!
|
|
||
| func PrintResource(c dynamic.Interface, resource schema.GroupVersionResource, kind, name string) error{ | ||
| listOpts := metav1.ListOptions{} | ||
| resourceList, err := c.Resource(resource).Namespace(*ns).List(context.Background(), listOpts) |
There was a problem hiding this comment.
I think the rules on ownerrefs are that a cluster-scoped object can own a cluster-scoped object or a namespace-scoped object. A namespace-scoped object can only own a namespace-spaced object in the same namespace.
If I'm remembering those rules correctly, we can probably prune the search space a little bit. It looks like you're passing in the namespace, which is good when resource is namespace-scoped. If namespace is cluster-scoped, we probably shouldn't pass in a namespace.
If the object we're looking for is cluster scoped, we can likely skip namespace-scoped resources. But... I'd imagine that will be relatively rare.
| } | ||
|
|
||
| func PrintResource(c dynamic.Interface, resource schema.GroupVersionResource, kind, name string) error{ | ||
| listOpts := metav1.ListOptions{} |
There was a problem hiding this comment.
Another potential optimization: I think it's possible to just get the ObjectMeta, see e.g. kubernetes/kubernetes#77819
|
This is an awesome first step! I have lots of ideas on how to evolve this / make this faster, but it works as-is. (The biggest gotchas were the authn import, and the fact that you have to use the full kind). So I propose we merge this and evaluate whether it's as useful as we think it's going to be! Then if so, we can optimize it / tackle some of the nits. /ok-to-test |
k8s-ci-robot
added
ok-to-test
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb, SomtochiAma 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 |
k8s-ci-robot
added
the
approved
No description provided.