Existing v1beta1 owner references prevent upgrades to v1

[sebgl]

Forked from #2184 (comment).

We use controller-runtime's setControllerReference() for several resources. The underlying comparison mechanism does not support updating resources from an apiVersion=v1beta1 to apiVersion=v1. setControllerReference detects a mismatch and returns an error.

I created an issue in controller-runtime with more details: kubernetes-sigs/controller-runtime#699

Until this is resolved the right way, we can probably come up with a workaround of our own.

[pebrc]

Until this is resolved the right way, we can probably come up with a workaround of our own.

Like un-setting before re-setting? Or simply constructing the owner reference ourselves without relying on controller-runtime?

[sebgl]

I did some experiments with patching the existing ownerRef with the newer apiVersion when the above error is returned:

// SetControllerReference wraps SetControllerReference to handle owner references apiVersion upgrade.
func SetControllerReference(owner, object metav1.Object, scheme *runtime.Scheme) error {
	object.GetResourceVersion()
	err := controllerutil.SetControllerReference(owner, object, scheme)
	switch err.(type) {
	case nil:
		return nil
	case *controllerutil.AlreadyOwnedError:
		// There's already an owner for that resource.
		// It may actually be the same, but with a different apiVersion.
		return patchExistingOwnerRef(owner, object, scheme)
	default:
		return err
	}
}

// patchExistingOwnerRef is a workaround for https://github.com/kubernetes-sigs/controller-runtime/issues/699.
// It handles the case where `SetControllerReference` returns an error because the same owner is already set, but
// with a different apiVersion. It may happen if we upgraded resources from eg. `v1beta1` to `v1`. In such case,
// we want to replace the existing owner with its newest api version.
// TODO: not necessary once https://github.com/kubernetes-sigs/controller-runtime/issues/699 is fixed.
func patchExistingOwnerRef(
	owner metav1.Object,
	object metav1.Object,
	scheme *runtime.Scheme,
) error {
	ownerAsRuntimeObj, ok := owner.(runtime.Object)
	if !ok {
		return fmt.Errorf("%T is not a runtime.Object, cannot call SetControllerReference", owner)
	}
	ownerGVK, err := apiutil.GVKForObject(ownerAsRuntimeObj, scheme)
	if err != nil {
		return err
	}
	ownerRefs := object.GetOwnerReferences()
	for i, existingRef := range ownerRefs {
		if existingRef.Kind == ownerGVK.Kind && existingRef.Name == owner.GetName() &&
			existingRef.APIVersion != ownerGVK.Version {
			// the owner is already set, but with a different version
			// remove entry from existing owner refs
			ownerRefs = append(ownerRefs[:i], ownerRefs[i+1:]...)
			object.SetOwnerReferences(ownerRefs)
			log.Info(
				"Updating ownerReference due to apiVersion mismatch",
				"namespace", object.GetNamespace(),
				"name", object.GetName(),
				"owner_kind", ownerGVK.Kind,
				"owner_name", owner.GetName(),
				"from_owner_version", existingRef.APIVersion,
				"to_owner_version", ownerGVK.Version,
			)
			break
		}
	}
	// set controller reference again
	return controllerutil.SetControllerReference(owner, object, scheme)
}

However for the actual resource to then be updated we need to make sure our reconciler comparison method for updates does account for owner ref comparisons.

[sebgl]

Another way of doing it, instead of replacing the existing ownerRef with the updated one, could be to just "ignore" the error if an ownerRef exists for eg. v1beta1 while we're dealing with v1 resources.

I'm not sure what's the impact of having ownerReferences around pointing to old versions of the CRDs we don't use anymore (and maybe don't include at all in the CRD spec).

[sebgl]

Created an issue in kube-storage-version-migrator kubernetes-sigs/kube-storage-version-migrator#49 (not sure that's the best place for it, but it seems to somehow be in the scope of that project?).

[sebgl]

FWIW it seems possible to set the apiVersion of an ownerReference to anything. I just updated a secret's ownerReference with apiVersion: elasticsearch.k8s.elastic.co/v1beta1 to apiVersion: elasticsearch.k8s.elastic.co/v180.
The automated deletion of that secret upon owner deletion still worked as expected. I guess the actual version in there may never be used, it could just be an artifact of using the group/version standard syntax.

Relates kubernetes/kubernetes#65200

Slack convo: https://kubernetes.slack.com/archives/C0EG7JC6T/p1575039839348100
It seems OK to have outdated apiVersions in ownerReferences.

[sebgl]

I'll work on a fix in controller-runtime to ignore versions difference in SetControllerReference.

[sebgl]

Upstream PR merged, I'll duplicate the fix in our codebase temporarily until we can rely on a new release of controller-runtime.