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

Prevent requiring a "metadata.name" when using include/exclude kind flags #88

Closed
matthewhembree opened this issue Jun 30, 2023 · 10 comments
Assignees
Labels
community-feedback Asking the community what the best solution might be enhancement New feature or request

Comments

@matthewhembree
Copy link

matthewhembree commented Jun 30, 2023

What did you do?

$ helm template my-release . -f values.yaml -f prod.yaml -n production | kubectl slice --exclude-kind Deployment,PodDisruptionBudget,Service -o output-prod

What did you expect to see?

All kinds except what was requested to be excluded. Including a List output as list-.yaml, which is normally output when no --include* or --exclude* flags are present.

Expected:

$ helm template my-release . -f values.yaml -f prod.yaml -n production | kubectl slice -o output-production-1
Wrote output-production-1/poddisruptionbudget-aaaa.yaml -- 241 bytes.
Wrote output-production-1/poddisruptionbudget-bbbb.yaml -- 245 bytes.
Wrote output-production-1/service-cccc.yaml -- 311 bytes.
Wrote output-production-1/service-dddd.yaml -- 278 bytes.
Wrote output-production-1/deployment-eeee.yaml -- 6040 bytes.
Wrote output-production-1/deployment-ffff.yaml -- 5603 bytes.
Wrote output-production-1/deployment-gggg.yaml -- 5587 bytes.
Wrote output-production-1/list-.yaml -- 3972 bytes.
8 files generated.

What did you see instead?

$ helm template my-release . -f values.yaml -f prod.yaml -n production | kubectl slice --exclude-kind Deployment,PodDisruptionBudget,Service -o output-production-2
error: unable to find Kubernetes "metadata.name" field in file number 8

^ File number 8 is the list.

If I'm just doing --exclude-kind, slice shouldn't care about metadata.name

Adding metadata.name does not make a difference.


I'm not personally looking to have lists split recursively like #28.

I really want slice to be more passive with lists. It expects to see metadata.name which runs fine when you don't use any --include* or exclude* flags.

Thanks!

@matthewhembree
Copy link
Author

Well, I guess the workaround is to use metadata.name for List. I had missed a list file, so I added the strikethough to my original post.

I can accept that. Most tools are fine with that present.

There's still the difference in behavior with the include/exclude flags. wontfix is fine. This workaround can be found by others that may have the same issue.

@matthewhembree
Copy link
Author

For those that aren't aware. Lists are nice because you can use YAML anchors/aliases across resources because the List is processed as a single YAML document.

@patrickdappollonio
Copy link
Owner

Hey @matthewhembree!

Would you mind helping me understand your issue? I see you're rendering a Helm chart which you then pass to kubectl-slice.

I assume this Helm chart cannot be sent as-is to Kubernetes, though, right? It's not possible to do this:

helm template . | kubectl apply -f -

In the issue you link, someone asked to have lists parsed. While there are lists in the API, they're internal to Kubernetes and not truly exposed unless it's to render out a more meaningful output than just an array of JSON data.

In fact, kohtala here nails it: the Kubernetes team says to avoid building on top of lists due to this exact issue.

For the second part of it, why is metadata.name required to split even when you're not splitting by name, that's a conscious choice and not a technical limitation: while it's true you might have a resource that doesn't define a name, the request is to always define one. In fact, the Kubernetes docs state that all objects in your cluster have a name (not "might have" or "could have").

Still, I would be happy to support your use case scenario, but we have to do it in a way that a) honours, first and foremost, Kubernetes native behaviour; and b) ensures it doesn't make assumptions of future implementations (because it would mean we would have a snowflake behaviour that people must know and acknowledge which we might have to tear down with a breaking change in the future if our implementation skews away from whatever the Kubernetes org decides).

I'm truly curious though: is your Helm chart not supposed to be installable in a cluster? Say, if you were to do:

helm install -f myvalues.yaml my-app ./app

Would you be able to get a running app in your cluster? If so, would be possible to get from you a minimum reproducible example to work with?

Lastly, we deliberately don't support YAML anchors and the likes mostly because we would have to do two passes: the first one to convert anchors to real values, so then in the second pass, files could be split... Otherwise, you risk splitting a file and an anchor all on different files.

@patrickdappollonio patrickdappollonio self-assigned this Jun 30, 2023
@patrickdappollonio patrickdappollonio added the enhancement New feature or request label Jun 30, 2023
@patrickdappollonio
Copy link
Owner

patrickdappollonio commented Jun 30, 2023

Another note: in this case:

$ helm template my-release . -f values.yaml -f prod.yaml -n production | kubectl slice -o output-production-1
Wrote output-production-1/poddisruptionbudget-aaaa.yaml -- 241 bytes.
Wrote output-production-1/poddisruptionbudget-bbbb.yaml -- 245 bytes.
Wrote output-production-1/service-cccc.yaml -- 311 bytes.
Wrote output-production-1/service-dddd.yaml -- 278 bytes.
Wrote output-production-1/deployment-eeee.yaml -- 6040 bytes.
Wrote output-production-1/deployment-ffff.yaml -- 5603 bytes.
Wrote output-production-1/deployment-gggg.yaml -- 5587 bytes.
Wrote output-production-1/list-.yaml -- 3972 bytes.
8 files generated.

You got a list-.yaml because the name generator enforces it. The default template name is:

{{.kind | lower}}-{{.metadata.name}}.yaml

And this is mentioned in the homepage. Since one of your resources doesn't have a name, you could default it:

helm template my-release . \
    -f values.yaml \
    -f prod.yaml \
    -n production | kubectl slice \
    -o output-production-1 \
    --template '{{.kind | lower}}-{{.metadata.name | default "unnamed" }}.yaml' # note this line

This will correctly process the list of files above, rendering something like:

Wrote output-production-1/poddisruptionbudget-aaaa.yaml -- 241 bytes.
Wrote output-production-1/poddisruptionbudget-bbbb.yaml -- 245 bytes.
Wrote output-production-1/service-cccc.yaml -- 311 bytes.
Wrote output-production-1/service-dddd.yaml -- 278 bytes.
Wrote output-production-1/deployment-eeee.yaml -- 6040 bytes.
Wrote output-production-1/deployment-ffff.yaml -- 5603 bytes.
Wrote output-production-1/deployment-gggg.yaml -- 5587 bytes.
Wrote output-production-1/list-unnamed.yaml -- 3972 bytes.
8 files generated.

Where list-.yaml would now be named list-unnamed.yaml since the default value is now replaced.

@matthewhembree
Copy link
Author

Hey @matthewhembree!

Would you mind helping me understand your issue? I see you're rendering a Helm chart which you then pass to kubectl-slice.

I assume this Helm chart cannot be sent as-is to Kubernetes, though, right? It's not possible to do this:

helm template . | kubectl apply -f -

In the issue you link, someone asked to have lists parsed. While there are lists in the API, they're internal to Kubernetes and not truly exposed unless it's to render out a more meaningful output than just an array of JSON data.

In fact, kohtala here nails it: the Kubernetes team says to avoid building on top of lists due to this exact issue.

Hi @patrickdappollonio!

TL;DR: I guess my concern isn't about Lists, but about how nameless or non-k8s resources are handled when using include/exclude flags.

A list for me is a construct for using YAML anchors and aliases. That's it. Packaging kube resources. I don't care that Kubernetes doesn't know about them. I use lists to avoid repetition on YAML. I have no need to do kubectl get list something. It's just a collection, for me only to pass into helm, kustomize, or kubectl.

Example of DRY across resources and within a resource:

---
apiVersion: v1
kind: List # We're using a List so that we can use YAML anchors/aliases across multiple kube resources. Since they're in a single kubeyaml document/list, this works.
metadata:
  name: name-is-sort-of-not-part-of-the-spec
items:
  - kind: Service
    apiVersion: v1
    metadata:
      name: &common-name my-repeated-resource-name
    spec:
      type: NodePort
      selector:
        app: *common-name
      ports:
        - name: http
          protocol: TCP
          port: &common-port 80
          targetPort: application
  - kind: Ingress
    apiVersion: networking.k8s.io/v1
    metadata:
      name: *common-name
      annotations:
        kubernetes.io/ingress.class: "ingress-nginx"
        nginx.ingress.kubernetes.io/force-ssl-redirect: "true"
        nginx.ingress.kubernetes.io/proxy-body-size: "100m"
    spec:
      tls:
      - secretName: *common-name
        hosts:
        - &common-hostname my-repeated-hostname.example.com
      rules:

      - host: *common-hostname
        http:
          paths:
          - backend:
              service:
                name: *common-name
                port:
                  number: *common-port
            pathType: ImplementationSpecific

For the second part of it, why is metadata.name required to split even when you're not splitting by name, that's a conscious choice and not a technical limitation: while it's true you might have a resource that doesn't define a name, the request is to always define one. In fact, the Kubernetes docs state that all objects in your cluster have a name (not "might have" or "could have").

That's fine. I suppose. I'll just start using names for lists when I need to use kubectl-slice. There is a bit of community ambiguity. A List isn't a resource in a cluster. It's not in the Kubernetes API. It doesn't have a namespace or UID.

kubectl explain list
error: couldn't find resource for "/v1, Kind=List"

Still, I would be happy to support your use case scenario, but we have to do it in a way that a) honours, first and foremost, Kubernetes native behaviour; and b) ensures it doesn't make assumptions of future implementations (because it would mean we would have a snowflake behaviour that people must know and acknowledge which we might have to tear down with a breaking change in the future if our implementation skews away from whatever the Kubernetes org decides).

I appreciate your asking for clarification. We're probably getting lost in the semantics together.

If kubectl-slice requires metadata.name when using a --include* or --exclude* flag, it should require it when omitting the flags. It's a conscious decision, right?

Maybe the --skip-non-k8s implementation is the conflict. Maybe similar logic is applied to the --include* or --exclude* flags, but not if they're omitted.

My workaround when I want to use either of those flags, is to set a name on the list. Maybe I should be using a name as a general practice. 🤷 It's not a hill I want to die on. 🤣 I was just used to not using a name up until the time that I wanted to exclude certain kinds.

  • kubectl-slice no flags: not strict on metadata.name
  • kubectl-slice --skip-non-k8s: strict
  • kubectl-slice --include*: strict even though --skip-non-k8s isn't used.
  • kubectl-slice --exclude*: strict even though --skip-non-k8s isn't used.

I'll just start naming my lists. I guess I'm being stubborn. I did want to note the discrepancy in the techniques above.

I'm truly curious though: is your Helm chart not supposed to be installable in a cluster? Say, if you were to do:

helm install -f myvalues.yaml my-app ./app

I am just using helm template and kubectl-slice to test what the output is when changing values, kubeyaml, etc. It's part of my iterative helm template development workflow. I'm not interested in using helm install to test if a go-template/helm-value combination works as intended. I just want to see the resources rendered and split; before they arrive at a cluster. Prototyping/development, that's all.

Helm:

  1. helm template | kubectl slice
  2. Look over the output.
  3. PR.
  4. Publish and deploy in a CI/CD pipeline.

Kustomize:

  1. kustomize build . | kubectl slice
  2. Look over the output.
  3. PR.
  4. Publish and deploy in a CI/CD pipeline.

Would you be able to get a running app in your cluster? If so, would be possible to get from you a minimum reproducible example to work with?

Lastly, we deliberately don't support YAML anchors and the likes mostly because we would have to do two passes: the first one to convert anchors to real values, so then in the second pass, files could be split... Otherwise, you risk splitting a file and an anchor all on different files.

I'm not interested in kubectl-slice supporting YAML anchors. It shouldn't see an anchor/alias --ever. Those references are resolved/interpolated well before kubectl-slice comes into play.

One would be correct to say that using anchors in helm is an anti-pattern. True, the go-templates are more flexible. But anchors work great with kustomize.

Thanks for taking the time to collaborate. I'll name my lists. 😄

@matthewhembree
Copy link
Author

And this is mentioned in the homepage. Since one of your resources doesn't have a name, you could default it:

helm template my-release . \
    -f values.yaml \
    -f prod.yaml \
    -n production | kubectl slice \
    -o output-production-1 \
    --template '{{.kind | lower}}-{{.metadata.name | default "unnamed" }}.yaml' # note this line

This is very helpful and a clever implementation.

Thanks!

@matthewhembree
Copy link
Author

matthewhembree commented Jun 30, 2023

^ The command with flag --template '{{.kind | lower}}-{{.metadata.name | default "unnamed" }}.yaml' still breaks when using a flag like --exclude-kind Pod with an unnamed resource/list.

helm template blah . | kubectl slice --exclude-kind Pod -o output-dir --template '{{.kind | lower}}-{{.metadata.name | default "unnamed" }}.yaml'
error: unable to find Kubernetes "metadata.name" field in file number 9

@patrickdappollonio
Copy link
Owner

I see what you mean.

The implementation for exclude-kind and exclude-name have the same background code: they assume they're Kubernetes resources since, well, this app at the end of the day is indeed a plugin for kubectl (but can be used standalone). By being Kubernetes resources they adhere to the rule that you must have a resource to be targetable, that's because we have:

--include pod/foo
--include-kind pod
--include-name foo

Since we don't know what you might want, and kind and name inclusions (or exclusions) come from the same core philosophy that you're excluding Kubernetes resources, then that's where the reason lies as to why when you're excluding by kind, you're still required to provide a name.

All of the --include and --exclude labels get converted to generic <kind>/<name> anyways (as in, passing --include-kind=Pod converts it internally to --include=Pod/*).

I'll update the title of this issue to reflect that maybe what we want to do is not to enforce names if you're doing kind filtering only, but that would mean decoupling the core logic above just for --include which requires you to pass both kind and name and only use different logic for the options --include-kind and --exclude-kind. Out of 6 flags that means only two would have a radically different handling. We can request community feedback, though and see what people might think.

Then the issue might be that someone might come up with the opposite of your proposal: that since this is a Kubernetes plugin, there's an expectation that a name must be provided, because that's an "appropriate" Kubernetes manifest. We can cross that bridge when we get there 😆

I'll keep the issue open so other people might chime in. If there's enough support for this feature, I'll be more than happy to implement it, but as of now, it's a bit of a harder sale with the details considered above (rewriting the core so the -kind flags don't use the standard filtering logic but their own; and the politics around whether a List should be accepted; and finally whether names should be optional).

@patrickdappollonio patrickdappollonio added the community-feedback Asking the community what the best solution might be label Jun 30, 2023
@patrickdappollonio patrickdappollonio changed the title Lists don't have metadata.name. They are not processed passively. Prevent requiring a "metadata.name" when using include/exclude kind flags Jun 30, 2023
@patrickdappollonio
Copy link
Owner

I might have a working solution with a workaround: adding 2 new flags: one to allow empty names, and another to allow empty kinds.

If you allow empty names, yet you decide to filter by name, it'll error our. If you allow empty names but you decide to filter by kind, then that's what you originally wanted so it will allow the request and process it.

Give me a few days and I'll put something up.

@patrickdappollonio
Copy link
Owner

Hey @matthewhembree!

The workaround has been released as part of 1.2.7. Try it out and let me know! I've been using it for a few weeks now to good success (ran into a similar issue than yours).

For the time being, I'm closing this issue, but feel free to reopen and/or post comments with your thoughts!

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
Projects
None yet
Development

No branches or pull requests

2 participants