feat(kubernetes): improve messaging in the delete (manifest) stage

[dbyron-sf]

specifically when there's an attempt to delete an unknown kind

Also improve the "delete operation completed successfully for" message for coordinates
that have no name (which is the case when pipelines specify kinds and not manifestName).

before this PR:

with this PR:

[mattgogerly]

Awesome!

[german-muzquiz]

Does this work for dynamically registered CRDs (those that are not specified in the kubernetes account configuration)? I think it should work based on previous PRs to fix delete and patch manifest stages for CRDs, but I'd like to double check.

There's a risk of breaking backwards compatibility for an obscure use case: native kubernetes kinds are hardcoded in this file, and we're not keeping it up to date with changes in kubernetes API versions, which is ok, because the way clouddriver works is that if it finds a kind that is not a CRD and is not in that list, it assigns the NONE kind. Then the default implementation of delete uses the name of the manifest for the kubectl call, which becomes something like kubectl delete corekind, which works because it doesn't need the group name to be specified, because it's a core kind.

In the end, this if condition breaks the case of deleting core kinds which are not in the hardcoded list in clouddriver. For example, try deleting a ReplicationController or ResourceQuota.

[dbyron-sf]

Does this work for dynamically registered CRDs (those that are not specified in the kubernetes account configuration)? I think it should work based on previous PRs to fix delete and patch manifest stages for CRDs, but I'd like to double check.

Yes it does. This test still passes with this PR.

There's a risk of breaking backwards compatibility for an obscure use case: native kubernetes kinds are hardcoded in this file, and we're not keeping it up to date with changes in kubernetes API versions, which is ok, because the way clouddriver works is that if it finds a kind that is not a CRD and is not in that list, it assigns the NONE kind. Then the default implementation of delete uses the name of the manifest for the kubectl call, which becomes something like kubectl delete corekind, which works because it doesn't need the group name to be specified, because it's a core kind.

In the end, this if condition breaks the case of deleting core kinds which are not in the hardcoded list in clouddriver. For example, try deleting a ReplicationController or ResourceQuota.

Hmmm...any thoughts on a good way forward that doesn't break this case?

[german-muzquiz]

I can think on these options:

1. Add missing core kubernetes kinds to the hardcoded list (so anything coming from kubectl api-resources in a newer version cluster). One problem is that this is an ongoing effort, if kubernetes adds another core kind in the future and clouddriver is not updated, spinnaker will not be able to delete those kinds.
2. Catch the KubectlException here when running deployer.delete and parse the error message to know if it is because of a truly missing kind in the cluster. I don't know if the error message is clear enough to be parsed.
3. Add a new method like isValidKind to KubectlJobExecutor that runs a kubectl api-resources, and call it before a delete. If the kind is valid, register it dynamically like CRDs, so subsequent deletes have the right KubernetesKind instance:

if kind == NONE
    if kubectlJobExcutor.isValidKind
        // register kind, same logic as dynamic CRDs
    else
        throw unknown kind exception

[dbyron-sf]

Thanks for these suggestions. Of these, I think the third one has the best chance of success. If we were using a java client library instead of invoking kubectl, perhaps option 2 would be more feasible, but I think it'll be tough to get right as it is. And I agree that the maintenance burden of option 1 is too big.

I started pondering how to implement this. Probably makes sense to discuss in the sig, but using #5334 as a model, it feels like adding code to KubernetesCredentials to periodically call a new KubectlJobExecutor method that runs kubectl api-resources makes sense. Not sure what subclass of KubernetesHandler will make sense...maybe one that already exists, or maybe a new one. At least by name, KubernetesCustomResourceHandler doesn't feel right, but it may do the right thing.

From there, adding a new member and mutator method to GlobalResourcePropertyRegistry and adjusting the get method to use it...and then I'm not sure whether KubernetesKindRegistry needs to know about these new kinds or not.

[german-muzquiz]

Sounds right, the only thing is that probably we can get away without another memoizer periodically calling kubectl api-resources, if we call it on demand when trying to process a kind NONE, or better yet, when creating the instance of KubernetesKind that assigns NONE when there's no known kind in the hardcoded list or in the CRD dynamic list.

[dbyron-sf]

One more link that I think is helpful is to KubectlJobExecutor::kubectlLookupInfo, which KubectlJobExecutor::delete uses to build the kubectl command.

@german-muzquiz I'm questioning this:

Then the default implementation of delete uses the name of the manifest for the kubectl call, which becomes something like kubectl delete corekind, which works because it doesn't need the group name to be specified, because it's a core kind.
because of this code in kubectlLookupInfo:

    if (!Strings.isNullOrEmpty(name)) {
      command.add(kind + "/" + name);
    } else {
      command.add(kind.toString());
    }

Doesn't this mean the kind is important / shows up in the resulting command in addition to the name? Instead of kubectl delete corekind we'd end up with kubectl delete none/corekind? Or maybe I'm missing something. Adding a test is the likely way forward, but still wanted to raise the question here.

[german-muzquiz]

The kind needs to be included in the command, so if the manifest name is not empty the command in the current implementation is kubectl delete corekind/name rather than kubectl delete none/corekind.
This is because the KubernetesKind is a dynamically created kind with the right name but with group NONE, so kind.toString() is corekind.

[dbyron-sf]

Aaah, ok. I seem to have lost the place where that dynamically created kind comes from.

[dbyron-sf]

We discussed this in the kubernetes sig. See the minutes from 2022-03-08. Because deployer.delete gets the kind from the credentials (as opposed to the c object or c.getKind(), it's not clear how deleting core kinds not mentioned in KubernetesKindProperties::getGlobalKindProperties could ever work...but we generally think it does. Time to add some tests....

[mattgogerly]

I've actually just run into a very similar sounding issue. If a custom Kind is not explicitly registered in the credentials definition, and there are no instances of that resource in the account, then the KubernetesUnregisteredCustomResourceHandler handler is used and returns a none kind, and the Delete (Manifest) stage fails.

If there are one or more the resources in the account, the KubernetesCustomResourceHandler is used and this returns the correct kind. This behaviour seems to have been introduced by #5334 and I'm really not sure how best to tackle it.

On the one hand it makes sense that the Delete stage fails if the thing you're trying to delete doesn't exist. On the other hand - does it matter? Perhaps we should introduce an option for the --ignore-not-found kubectl flag in the stage?

Changes required

[dbyron-sf]

@Mergifyio update

[mergify]

update

✅ Branch has been successfully updated