Only redact Kinds starting "Secret.", avoiding SealedSecret and similar

[puzza007]

Fixes the error below for e.g. SealedSecret.v1alpha1.bitnami.com

Traceback (most recent call last):
  File "/Users/po/src/kubediff/kubediff", line 48, in <module>
    main()
  File "/Users/po/src/kubediff/kubediff", line 42, in main
    failed = check_files(args, printer, config)
  File "/Users/po/src/kubediff/kubedifflib/_diff.py", line 246, in check_files
    differences += check_file(printer, path, config)
  File "/Users/po/src/kubediff/kubedifflib/_diff.py", line 174, in check_file
    printer.diff(path, difference)
  File "/Users/po/src/kubediff/kubedifflib/_diff.py", line 214, in diff
    self._write('%s', difference.to_text(self._current.kind))
  File "/Users/po/src/kubediff/kubedifflib/_diff.py", line 35, in to_text
    message = self.message % ((len(self.args[0]) * '*'), (len(self.args[1]) * '*'))
TypeError: object of type 'NoneType' has no len()

[dimitropoulos]

any reason not to just do

if kind.startswith('Secret.') and len(self.args) == 2:

?

that, after all, seems to be closer to what the code is intending, with the additional benefit of not causing a bug later if secrets are ever versioned at v2.

[puzza007]

Nope! That's definitely better. I've force pushed that change.

[bboreham]

It seems to me the failure arises because args has not been set up the way the code expects, so tweaking the spelling of "secret" is not really getting to the heart of the matter.

Why not check args ? How does it arise that args can be set different ways ? Is that an issue ?

@torz you added this code - do you have a view ?

Can we have tests for the various cases?

[dimitropoulos]

@bboreham my interpretation is that the previous code falsely assumed that there would not be another kubernetes resource that contains the word "secret" as a substring. for example when debugging on this line we see that kind has the value Secret.v1. for a kubernetes Secret resource, and something like SealedSecret.v1. for a kubernetes SealedSecret resource. This code will then wrongly take a codepath that was written to handle Secrets for a SealedSecret. @puzza007 do you agree with that synopsis? I'm running a test now with a SealedSecret to verify.

[dimitropoulos]

I can confirm that kind in this function is SealedSecret.v1alpha1.bitnami.com for a SealedSecret resource. What's curious about that to me is that there's no . afterwards like with a regular Secret. Here's a table just to lay it out (I added some other resources as a reference point):

Resource Kind	API Version	to_text kind
Secret	v1	Secret.v1.
SealedSecret	bitnami.com/v1alpha1	SealedSecret.v1alpha1.bitnami.com
Deployment	extensions/v1beta1	Deployment.v1beta1.extensions
Service	v1	Service.v1.
DaemonSet	apps/v1	DaemonSet.v1.apps
PriorityClass	scheduling.k8s.io/v1beta1	PriorityClass.v1beta1.scheduling.k8s.io
ClusterRole	rbac.authorization.k8s.io/v1	ClusterRole.v1.rbac.authorization.k8s.io
HelmRelease	flux.weave.works/v1beta1	HelmRelease.v1beta1.flux.weave.works
Pod	v1	Pod.v1.

in short, and at the least, this code seems to be .-separating something that already (on occasion, but not most often) has .s in it...

that's why there's a . at the end of a secret resource because there is no namespace (e.g. extensions for a Deployment) to put at the end - so it's blank.

the pattern seems to be:
Resource kind . apiVersion version . apiVersion namespace
with the caveats (perhaps two separate bugs) that it seems to assume that:

1. there will always be an apiVersion namespace (where there isn't for Secrets, for example)
2. (by virtue of the fact that the kind is . separated) there will not be a . in the apiVersion namespace

[puzza007]

Yes, this is what I thought was going on 👍

…

On Wed, 13 Feb 2019, 05:13 Dimitri Mitropoulos, ***@***.***> wrote: @bboreham <https://github.com/bboreham> my interpretation is that the previous code falsely assumed that there would not be another kubernetes resource that contains the word "secret". for example when debugging on this line we see that kind has the value Secret.v1. for a kubernetes Secret resource, and something like SealedSecret.v1. for a kubernetes SealedSecret resource. This code will then wrongly take a codepath that was written to handle Secrets for a SealedSecret. @puzza007 <https://github.com/puzza007> do you agree with that synopsis. I'm running a test now with a SealedSecret to verify. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#78 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAE_rl8tZ87rjubAvDkGJIem1aTSjgFks5vMug9gaJpZM4aW61N> .

[rade]

I believe what you call 'namespace' here, kubernetes calls 'group'. From https://kubernetes.io/docs/reference/generated/kubectl/kubectl-commands#get

TYPE[.VERSION][.GROUP]

Presumably neither type nor group are permitted to contain ., whereas version is, hence there is no ambiguity.

To Bryan's point though... what is args? What ensures that when resource kind is of type Secret (regardless of version and group), and there are two args, then args[0] and args[1] are things on which len can be invoked?

[bboreham]

It's trying to print out %s != %s with the yaml and Kubernetes values as first and second arg, and in this specific crash the yaml value is "null" which reads into Python as None.

[awh]

@puzza007 has #82 fixed your crash? If so, can we retitle this PR as a fix for #81?

[puzza007]

Hi @awh. I've just tried with 12ef0a3 and it works for me 👍