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

Add support for processing Kubernetes list from "kubectl" #28

Open
kohtala opened this issue Jan 25, 2022 · 15 comments
Open

Add support for processing Kubernetes list from "kubectl" #28

kohtala opened this issue Jan 25, 2022 · 15 comments
Assignees
Labels
community-feedback Asking the community what the best solution might be enhancement New feature or request help wanted Extra attention is needed

Comments

@kohtala
Copy link

kohtala commented Jan 25, 2022

What did you do?

kubectl get -A all -o yaml > k8s.yml
go get github.com/patrickdappollonio/kubectl-slice
~/go/bin/kubectl-slice -f k8s.yml -o .

It installed v1.1.0.

What did you expect to see?

The list items be split to individual files.

What did you see instead?

One list-.yml file equal to the k8s.yml.

@patrickdappollonio
Copy link
Owner

Hey @kohtala appreciate the report!

This is an interesting one. Mainly because the output is not a "multi YAML" but instead a Kubernetes resource of type List. We could split it too but I think we might have to introduce either a check-before-processing or a flag to make it work.

If you have any preference on that, happy to hear it! It's not a hard or difficult feature to implement at all!

@patrickdappollonio patrickdappollonio self-assigned this Jan 25, 2022
@patrickdappollonio patrickdappollonio added community-feedback Asking the community what the best solution might be enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Jan 25, 2022
@kohtala
Copy link
Author

kohtala commented Jan 25, 2022

Thanks. The List is not any kind of resource. I searched for documentation and found some in the API Concepts

Keep in mind that the Kubernetes API does not have a kind named List.

kind: List is a client-side, internal implementation detail for processing collections that might be of different kinds of object. Avoid depending on kind: List in automation or other code.

So I think it should be safe to perform this processing by default. I believe there also is never a name property. It is best if the README.md were clear it handles by default both kinds of lists.

A flag to disable the feature might become necessary, if someone finds they need to split lists out of a multi YAML. But I believe this to be rare. But I've been wrong before.

@patrickdappollonio
Copy link
Owner

Yes, sorry, I used the "Kubernetes object" very loosely. It is a definition/spec in the kubectl app so it's easier to word it that way, although inaccurate.

My main concern is also outlined in what you shared, especifically:

Avoid depending on kind: List in automation or other code.

What this means is that the implementation might change. It could help us with the direction though, perhaps the way to go is having something like:

kubectl-slice -f k8s.yaml --as-list -o .

Where the --as-list would process it.

My concern to not auto-add this feature is because we are parsing multi-yamls, so something like this:

file1: true
---
file2: true
---
file3: false

And the List kind does not match this input, so it generates some sort of a side consequence (as in, now you have to educate and say "btw! remember that tool that splits multi-yamls? well you can also feed in a List and it'll work too!).

Open to hear your thoughts around it! You mention at the end to have the feature opt-out (as in, process List like if it was any other multi-yaml) while I suggest it as opt-in with --as-list. Curious to know your use case!

@kohtala
Copy link
Author

kohtala commented Jan 26, 2022

I understood the reference

Avoid depending on kind: List in automation or other code.

to refer to object validation and such automation. For example helm should not be used to generate kind Lists because kubectl may not be involved to validate and apply the List to cluster so there is no-one to understand List.

Looking at the issues from where the documentation came (kubernetes/website#25944), I believe kubectl just happened to use List instead of a multi YAML for kubectl get to produce one file the kubectl apply or delete can take in. Unfortunately I found no clear documentation on the file format in the kubectl reference.

Since this program is already called kubectl-slice and it works invoked with kubectl slice, it would seem natural it supports the kubectl List without additional options just like the kubectl apply and delete do.

@patrickdappollonio
Copy link
Owner

patrickdappollonio commented Jan 27, 2022

Hey @kohtala,

The only reason this program starts with kubectl- is because it's a requirement to be used/installed as a plugin from krew. Otherwise, like many of my other apps, it could be called just slice 😄

(Side note, the original app was even called split, and when we worked on #1 is when the app got renamed from split to kubectl-slice, because split already exists in the Krew index)

You mention "it would seem natural it supports the kubectl List without additional options" but if we look at the original intention of the software, which is to parse multi-YAML files (files separated by --- concatenated into a single file). A list isn't one of them, it's just an object wrapper at that point.

Adding this would mean the current people using this software would now magically receive the ability to process List which they might not be expecting, and it opens a bigger window of details that I'm not sure we'll get there. For example, consider I have the following YAML:

---
first: true
---
apiVersion: v1
kind: List
metadata:
  resourceVersion: ""
  selfLink: ""
items:
- apiVersion: v1
  kind: Service
  metadata:
    name: kubernetes
    namespace: default
- apiVersion: v1
  kind: Service
  metadata:
    name: kube-dns
    namespace: kube-system

... Do we process the second file as a list and we get in return, say, 3 files instead of 2 just because there's a list inside?

This is why I think we might need more feedback on this. As before, if you had a concrete use case scenario, please let me know, it would definitely help shape the direction of kubectl-slice.

@patrickdappollonio
Copy link
Owner

After watching the video from the SIG APIMachinery linked from kubernetes/website#25944 it's even more evident this seems a client-side only, specific to kubectl and not valid in other resources -- there are talks about Helm, automation software like ArgoCD and Flux, and others -- so I would still be inclined to support it, just under a flag, something like:

kubectl get pods -o yaml | kubectl slice --from-list

Where the --from-list will disable multi-YAML parsing and, instead, enable kind: List processing. That way, if they ever decide not to move forward with this type of list, we can quickly remove the code and no side consequences would happen to the app moving forward.

Still, would love to know what everyone else thinks.

@cbmdfc
Copy link

cbmdfc commented Jan 27, 2022

been keeping an eye on this discussion, and I think this should be a feature flag rather than built-in behavior... I wouldn't want the CLI to magically change how it parses resources

either that or release a new major version and announce it as kind of a breaking change but not really?

your last comment @patrickdappollonio makes sense, kubectl-slice --from-list makes lots of sense/ I would want this feature to be in because I use lists too, but the feature flag doesn't seem that bothersome

@douglasmakey
Copy link
Contributor

I agree that this should be a feature flag rather than the default behaviour. For me, keeping the CLI behaviour works with multi-yaml is more intuitive and familiar.

@kohtala
Copy link
Author

kohtala commented Jan 28, 2022

I am a stranger to this project and definitely do not feel like I would need to tell you what to do. There must be lots of uses I do not know about. As someone that just found this utility, the naming got me thinking it had more to do with kubectl than you seem to think it has. I do not know if other users would find the option as much strange.

I tried to see if kubectl passes some information to the plugin it is being invoked through it. I did not find it, but if the plugin has some way of knowing whether it was invoked through kubectl, I think that could be used to change the behavior to match kubectl apply etc.

If there is a change to behaviour, a major version is a good point.

@patrickdappollonio
Copy link
Owner

I think based on the previous responses, it's better to leave it off as a feature flag.

I tried to see if kubectl passes some information to the plugin it is being invoked through it. I did not find it, but if the plugin has some way of knowing whether it was invoked through kubectl, I think that could be used to change the behavior to match kubectl apply etc.

I think this wouldn't work. If your easiest way to install the app is through Krew, then you really don't need kubectl to run it, yet you must use it because of krew. At that point, if we detect if the plugin was invoked through Kubectl, chances are the answer will almost be "always" 😛

If there is a change to behaviour, a major version is a good point.

I would still be afraid of implementing it without a flag. It seems to me the flag is a better option overall and matches the other people's thoughts.

I am a stranger to this project and definitely do not feel like I would need to tell you what to do.

I appreciate the thought! Really do 😄 the reason why we're asking you for your feedback is not to tell me what to do, but instead, explain your requirement and your use case. Yes, at the end of the day it might be my decision, but I would rather leave it to the collective what would work best for everyone.

You happen to run into a behaviour which is part of the process and even a misunderstanding for Kubernetes too, I wouldn't blame you for consider it standard behaviour.

I would love to go forward, I've needed this feature myself as well, and as such, I would love to implement it, but based on everyone else's feedback, I would put it under a feature flag.

@kohtala
Copy link
Author

kohtala commented Feb 3, 2022

My use case was really in the original post. I just get bunch of objects from a Kubernetes cluster, but need to have them in separate files. This time I was running trivy config on them to see if it reports configuration issues for a cluster I had running and it does not scan into a List. I could make an issue for them as well, but thought this could be a nice addition to this tool.

A feature flag is fine. If there is a change of mind later, an opposite flag can be added when the major version is changed.

What I ment with the telling you what to do was that I can't go into the solution in detail like if you should use a flag or not. I find some people make up their mind and insist on it. I do not. I try to offer the background that I have, which is not much. You have my blessing to go forward on which ever way you decide to do it based on your wider and deeper background.

Thank you.

@patrickdappollonio patrickdappollonio removed the good first issue Good for newcomers label Feb 18, 2022
@patrickdappollonio patrickdappollonio changed the title Does not recurse into kind: List Add support for processing Kubernetes list from "kubectl" Feb 20, 2023
@patrickdappollonio
Copy link
Owner

Hello everyone (and @kohtala)!

Sorry for the long pause. I had a recommendation from a coworker who proposed that the list-to-multi-yaml could be a subcommand rather than part of the expected behaviour of this program.

For example, consider the following:

kubectl get pods -o yaml | kubectl slice list-to-yaml -f - -o all.yaml

This would read the list and parse it as a multi-YAML file. Then you could pipe its output to normal kubectl slice as usual:

kubectl slice -f all.yaml

Or everything done in a single shot with UNIX pipes:

kubectl get pods -o yaml | kubectl slice list-to-yaml -f - --stdout | kubectl slice -f - --stdout

This way:

  1. It's an opt-in and not a mandatory behaviour.
  2. People who really need the feature can just get familiar with the subcommand without getting familiar with the rest of the tool
  3. If Kubernetes ever decides to get rid of this behaviour (since it's expected not to be built on top of this) then we just remove the subcommand.

Let me know what you all think!

@kohtala
Copy link
Author

kohtala commented Sep 6, 2023

Since Kubernetes API does not have a kind named List, I think a single document YAML (as opposed to multi document YAML) with this kubectl specific kind does not need this complexity.

But if it is how you feel you like it done, please do so. This does have the advantage, that there is a generic list-to-yaml tool should one be useful for any other tasks.

@patrickdappollonio
Copy link
Owner

True, but again, adding it to the main software part (the slicing) without a commitment from the Kubernetes org to have it be consistent seems wrong.

Making it into a separate subcommand means you can just strip off that part of the command instead and/or produce fixes for just that part, without touching the main part of the software.

@cbmdfc
Copy link

cbmdfc commented Sep 7, 2023

100% agree with you @patrickdappollonio subcommand is good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-feedback Asking the community what the best solution might be enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants