Improved namespace handling for manifest reporting #1423
Conversation
| { | ||
| List<string> outputLines = new List<string> | ||
| { | ||
| "NAME SHORTNAMES APIVERSION NAMESPACED KIND VERBS CATEGORIES", |
source/Calamari.Tests/KubernetesFixtures/ManifestReporterTests.cs
Outdated
Show resolved
Hide resolved
| var activeColumn = new Column(); | ||
|
|
||
| var previousChar = SpaceChar; | ||
| for (var i = 0; i < headerRow.Length; i++) |
There was a problem hiding this comment.
we use the width of the headers as to dictate how we substring each row
| log.Warn("Unable to retrieve resource scoping using kubectl api-resources. Using default resource scopes."); | ||
| return DefaultResourceScopeLookup; |
There was a problem hiding this comment.
Any error here is warned, but we fallback to our defaults
| //we check to see if there is an explicit helm namespace defined first | ||
| //then fallback on the action/target default namespace | ||
| //otherwise fallback on default | ||
| return variables.Get(SpecialVariables.Helm.Namespace) ?? variables.Get(SpecialVariables.Namespace) ?? "default"; |
There was a problem hiding this comment.
Helm steps can have their own default namespace, so fallback on that first, then the target/action namespace, then the cluster default
| string ResolveNamespace(YamlMappingNode rootManifestNode, IVariables variables); | ||
| } | ||
|
|
||
| public class KubernetesManifestNamespaceResolver : IKubernetesManifestNamespaceResolver |
There was a problem hiding this comment.
This is the new class that contains all the logic for resolving a namespace for a given manifest root node.
| @@ -83,63 +92,31 @@ public void GivenInValidManifest_ShouldNotPostServiceMessage(string enabledFeatu | |||
|
|
|||
| [TestCase(nameof(FeatureToggle.KubernetesLiveObjectStatusFeatureToggle))] | |||
| [TestCase(OctopusFeatureToggles.KnownSlugs.KubernetesObjectManifestInspection)] | |||
| public void GivenNamespaceInManifest_ShouldReportManifestNamespace(string enabledFeatureToggle) | |||
There was a problem hiding this comment.
We don't test this kind of logic in the manifest reporter as this is the responsibility of the KubernetesManifestNamespaceResolver
| parsedResult.Should() | ||
| .NotBeEmpty(); |
There was a problem hiding this comment.
Would be worth asserting that the output is as expected - I'd be happy with a really cut down list compared to what you've got to save the typing.
There was a problem hiding this comment.
ah yeah, I meant to do that. I think the original reason it was like this was I was using the test to debug and fix it... Good catch!
Currently, for non-namespaced resource types (such as a
PersistentVolume, orClusterRoleBinding), they are reported with the implicit namespace.This is a less than ideal solution as it gives an incorrect impression of the namespace of the resources, especially for manifest inspection.
This PR updates the manifest reporting to make a kubectl call to
api-resourceswhich returns an output that contains all the know resource types and if they are namespaced are not. Unfortunately it's a shitty format and can't be outputted asjson, so we have a specific parser.If we can't load that info, we then have a preset list of known values as a fallback.
We then use this lookup to work out if we need to remove any namespace information.
We only remove namespace information for resources where we know they are non-namespaced
A large chunk of this PR is cribbed from @liam-mackie's previous work.
EDIT:
As the helm KOS code performs a similar action for resolving the namespace, I have coalesced this logic into a new class, the
KubernetesManifestNamespaceResolver. This code is responsible for the logic around what namespace to resolve based on the manifest, the variables and if we know if it's a namespaced resource or not (via the new cli call above)Further info
There are a number of scenario's that can occur. In the following scenario's, "known" or "unknown" refers to if the cluster knows about the resource type and can tell us if it's namespaced or not.
A namespaced resource has the namespace in the manifest
✔️ The namespace in the manifest is used
A namespaced resouce does not have a namespace in the manifest
✔️ The implicit namespace is used
A unknown namespaced resource has a namespace in the manifest
✔️ The namespace in the manifest is used
A unknown namespaced resource does not have a namespace in the manifest
✔️ The implicit namespace is used
A known non-namespaced resource does not have a namespace in the manifest
✔️ The namespace is
nullA known non-namespaced resource has a namespace in the manifest
✔️ The namespace in the manifest is ignored and the namespace is
nullA unknown non-namespaced resource has a namespace in the manifest
❓ The namespace in the manifest is used. This is incorrect, but we can't know any different.
A unknown non-namespaced resource does not have a namespace in the manifest
❓ The implicit namespace in the manifest is used. This is incorrect, but we can't know any different.