Support releases with removed apis

[raffis]

I noticed that we had an older version with a removed api in one namespace and the helm reconcile failed because of it:

      Helm upgrade failed: unable to build kubernetes objects from current release manifest: resource mapping not found for name: "vault" namespace: "" from "": no matches for kind "Ingress" in version "extensions/v1beta1"

It would be a nice feature to attempt to fix that problem automatically using https://github.com/helm/helm-mapkubeapis . I had to fix releases with that version manually which could have been done by the controller.

[kingdonb]

I don't think this is likely to be supported, for a few different reasons.

The first issue is that Helm Controller cannot support plugins because of the nature of how they work; helm shells out to execute plugins, and this is something Helm Controller and Flux at-large decided not to support for security reasons.

The bottom line is that the chart needs to be upgraded, and while some API upgrades can be done safely and automatically by the cluster, this one is very special, because Ingress has moved from one API group (extensions) to another (networking); this is a very narrow case which Kubernetes itself does not support for that reason, and Flux generally defers to Kubernetes to provide this transparent upgrading feature.

I think this is probably out of scope for Flux, but thank you for providing this reference, I hadn't heard of helm-mapkubeapis and did not know there was such a transformer for Helm users to get around this issue.

[raffis]

I am aware of the decision against plugin, I was more thinking about implementing this out of the box. But I do understand if this is considered an edge case 👍🏻

[kingdonb]

Yeah, we all agreed at Flux Bug Scrub today that it would be nice to have this functionality, if it could be built into Helm Controller directly, and it is written as a go package, so it is theoretically possible to incorporate it.

[raffis]

I am happy to contribute it if this is a wanted feature. I'd like to define the API first though.

It should probably be aware of the core kubernetes api deprecations/removals out of the box. Meaning thats a list which would be maintained. I reckon that should be fine as this does not happen all too often. On top of that consumers should be able to define 3rd party api removals, either (or both) directly in a HelmRelease or as a ConfigMap reference (one or more).

There could be a flag to disable or enable it but I left it away for now, I can't imagine any scenario where you want to disable it but pls let me know if you think otherwise.

Something like: (Not certain about the deprecated prefix as it would also target removed apis but its the nicer name).

// HelmRelease
spec:
  deprecatedAPIMappingsFrom: []DeprecatedAPIMappingReference
  deprecatedAPIMappings: []DeprecatedAPIMapping

DeprecatedAPIMappingReference struct {
    // Kind of the values referent, valid values are ('ConfigMap').
    // +kubebuilder:validation:Enum=ConfigMap
    // +required
    Kind string `json:"kind"`

    // Name of the values referent. Should reside in the same namespace as the
    // referring resource.
    // +kubebuilder:validation:MinLength=1
    // +kubebuilder:validation:MaxLength=253
    // +required
    Name string `json:"name"`

    // key is the data key where the mapping can be
    // found at. Defaults to 'map'.
    // When set, must be a valid Data Key, consisting of alphanumeric characters,
    // '-', '_' or '.'.
    // +kubebuilder:validation:MaxLength=253
    // +kubebuilder:validation:Pattern=`^[\-._a-zA-Z0-9]+$`
    // +optional
    key string `json:"valuesKey,omitempty"`

    // Optional marks this DeprecatedAPIMappingReference as optional. When set, a not found error
    // for the values reference is ignored, but any ValuesKey, TargetPath or
    // transient error will still result in a reconciliation failure.
    // +optional
    Optional bool `json:"optional,omitempty"`
}

DeprecatedAPIMapping struct {
    // From is the API looking to be mapped
    DeprecatedAPI string `json:"deprecatedAPI"`

    // To is the API to be mapped to
    NewAPI string `json:"newAPI"`

    // Kubernetes version API is deprecated in
    DeprecatedInVersion string `json:"deprecatedInVersion,omitempty"`

    // Kubernetes version API is removed in
    RemovedInVersion string `json:"removedInVersion,omitempty"`
}

In the helm plugin DeprecatedAPI is a string which just gets looked up and replaced if there is a match, if possible it would be nicer to define this as a properly defined struct, but would need to check first if that makes sense (I guess there was a reason they did it that way)

[raffis]

@kingdonb any opinion about this proposal ☝🏻 ? Should this be implemented?

[kingdonb]

Reading my reply from earlier, I know we were all pretty enthusiastic about the idea when we reviewed this issue before. I don't think we talked about it again. It looks like you have put some thought into it, and I don't want to leave you with a flip response that doesn't do it justice.

I don't think we want to add major API surfaces to Helm Controller if they are not really well defined in upstream Helm itself. On the other hand, if you could add a simple flag-based interface where people who have outdated charts that are in need of upgrade, could choose to enable this. What I don't understand except from your post above, I think this mapkubeapis requires some configuration.

My suggestion is to gate this behind a feature flag, if that makes sense; we're using this approach to release some new Helm Controller features in a controlled way now, and we're finding that there is a lot we can do when we have the freedom to say "this new feature ships disabled by default"

Because if it's a foot-gun, you can at least be assured they'll have to consciously read the docs well enough to locate the carefully documented safety switch, then enable the feature flag, (point it at their own foot... etc.) in order to do any harm.

I don't have strong feelings about this issue reviewing it right now. I think that chart authors should maintain their charts, and I think that chart consumers should choose charts from a solid vendor who maintains the charts well, I don't know how much those things failing can mean that mapkubeapis will be really useful, but someone made it because it was useful to them.

I also recognize that sometimes those jobs are both being done by one person, (as a secondary or tertiary responsibility) and perhaps that person can get a lot more done with the dangerous footgun enabled (but now those other well-known problems that the foot-gun solves fading away in the distance of the rear-view mirror.)