Skip to content

Workaround obsolete API versions#7627

Closed
pho-enix wants to merge 1 commit intohelm:masterfrom
pho-enix:k8sAPIVersionMitigation2
Closed

Workaround obsolete API versions#7627
pho-enix wants to merge 1 commit intohelm:masterfrom
pho-enix:k8sAPIVersionMitigation2

Conversation

@pho-enix
Copy link
Copy Markdown
Contributor

This is a workaround for changes in API versions like
extensions/v1beta1 => apps/v1

Ignore group and version.

Signed-off-by: Sebastian Poehn sebastian.poehn@gmail.com

This is a workaround for changes in API versions like
extensions/v1beta1 => apps/v1

Ignore group and version.

Signed-off-by: Sebastian Poehn <sebastian.poehn@gmail.com>
@helm-bot helm-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 19, 2020
@pho-enix
Copy link
Copy Markdown
Contributor Author

#6850
#7575

@pho-enix
Copy link
Copy Markdown
Contributor Author

Drawback of this solution is that it will not catch resource conflicts of pre-existing resources with the same Kind but different GroupVersion. This can only happen for CRDs. I would expect that this case is rather rare. Impact might be that applying the change could fail.

@bacongobbler
Copy link
Copy Markdown
Member

Can you explain the difference between this PR, #7575, and the one @jlegrone proposed in #7625? It's getting really unclear what is the goal with three PRs trying to tackle the same problem, and what the goals of each PR accomplishes. There's not a whole lot of information here to describe what's the intention behind these PRs.

@pho-enix
Copy link
Copy Markdown
Contributor Author

Sorry for raising confusion. I wanted to provide different implementation options so we can decide for the best one.

#7627 Ignores groupVersion when comparing existing vs. planned resources. Thus a change of the API version will no longer block an upgrade. However Kind on it's own is not unique. Example:

kind: Deployment
apiVersion: apps/v1
metadata:
  name: new
kind: Deployment
apiVersion: naughty.inc/v1
metadata:
  name: existing

In the Kubernetes maintained resource types there is no such conflicts. But CRDs could do weird things. Thus this implementation would not detect a pre-existing resource that does not belong to the Helm Release, for the case of clashing Kind + Name + Namespace.

I would favour this solution as it is simple and the described corner-case sounds rather exotic to me.

#7575 Same as #7627 but it partially handles the described corner-case. It will detect the clash, if the pre-existing or the created resource is a CRD.

func objectKey(r *resource.Info) string {
gvk := r.Object.GetObjectKind().GroupVersionKind()
return fmt.Sprintf("%s/%s/%s/%s", gvk.GroupVersion().String(), gvk.Kind, r.Namespace, r.Name)
return fmt.Sprintf("%s/%s/%s", gvk.Kind, r.Namespace, r.Name)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered this as well. Unfortunately ignoring group and version would allow collisions with custom resources from different API groups. Perhaps this is something we might be able to tolerate and call out in docs, but I'm not sure.

@bacongobbler @hickeyma any thoughts?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we could exclude gvk.GroupVersion().String() for native k8s resource types but not for custom resources, then I think this might be fine.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a change in Helm's existing behaviour, right? I don't think we can remove this behaviour if we made the assumption that we would not allow collisions in previous versions of Helm 3.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also do not have any assurance from Upstream Kubernetes that they won't at some point decide to have multiple kinds with the same names that have different group/version.

@bacongobbler
Copy link
Copy Markdown
Member

closed in favour of #7649.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants