Adding a new required param breaks existing scripts. Is this really required?

I dont see any value of adding this annotation on cluster objects. Running kubectl get clusters <cluster-name> -o yaml will not give me a reusable yaml file that I can use with another federation.

IIUC the original issue was because we store some federation objects in host cluster and since the host cluster can be hosting multiple federation objects it makes sense to add these annotation there.
Cluster objects are already stored in federation control plane, so skip the annotation for clusters?

Hmm. Do you mean that you can't use an old version of kubefed with a newer federation? Or vice versa? Can you give an example of what would fail that we need to continue supporting?

On a side note, do we have a grid for which versions of kubefed, the FCP, the host cluster Kubernetes version and the joined cluster versions we need to support, in new federation and upgrade cases? I know that we want to maintain backwards compatibility, but I'm not sure I really understand what sort of compatibility we're trying to maintain.

Adding the annotation does give you the ability to get all objects related to a particular federation, rather than having to get all the annotated objects and then the cluster objects separately.

That all said, I'm happy to remove the cluster annotations. I just want to make sure I understand the benefit.

On a side note, do we have a grid for which versions of kubefed, the FCP, the host cluster Kubernetes version and the joined cluster versions we need to support, in new federation and upgrade cases? I know that we want to maintain backwards compatibility, but I'm not sure I really understand what sort of compatibility we're trying to maintain.

We have neither concretely defined nor documented that right now. I guess that will be one of the important work items in taking federation to GA.

But having said that, I think @nikhiljindal's comment holds. I did not think about that before.

Adding the annotation does give you the ability to get all objects related to a particular federation, rather than having to get all the annotated objects and then the cluster objects separately.

You need to do that anyway. You need to get all the annotated objects from the host cluster and then separately get the cluster objects from the federation API server irrespective of the annotations. Even if your objects are annotated, I believe you still need to get each type of object separately and filter for objects that contain the annotation you care about. Our API machinery does not allow getting objects with a given annotation across different API types.

@nikhiljindal, having said all that, I don't think adding the annotation here is that dangerous. This required param is an internal detail and @perotinus is making it mandatory to establish the contract between the provider and the client. The client here is the kubefed join implementation. Users who run the kubefed join command don't have to care about this. So it is not going to break them. It is just that the implementation of kubefed join should provide these annotation values which it does.

To clarify, the code here is kubectl and not kubefed. We have defined policies for kubectl version skew support that we cant break.

Ah, I see. kubectl vs kubefed makes a big difference.

@nikhiljindal @madhusudancs Are we agreed on removing the annotation from the cluster objects, and so reverting the changes to the ClusterGenerator?

Ah, I see. kubectl vs kubefed makes a big difference.

FWIW, we do need to be strict about not breaking kubefed as well (now that its beta)

@nikhiljindal @madhusudancs Are we agreed on removing the annotation from the cluster objects, and so reverting the changes to the ClusterGenerator?

Yes I dont see much advantage of adding the annotation but to be clear, reverting the change in ClusterGenerator and adding annotation to cluster object when its created via kubefed are different independent points. kubefed should not be using ClusterGenerator. Its providing the yaml I believe.

kubefed should not be using ClusterGenerator? That seems like a separate issue. Is it being tracked somewhere?

I'll revert all of the code that adds the annotation to the cluster objects later today unless someone objects. We can always add it back later.

OK, code removed.

Just to close this discussion, I am summarizing our offline discussion here:

To clarify, the code here is kubectl and not kubefed. We have defined policies for kubectl version skew support that we cant break.

ClusterGenerator is not kubectl code, it is kubefed code in the sense that kubefed is the only client. It sits in kubectl package to reuse some of the machinery there.

FWIW, we do need to be strict about not breaking kubefed as well (now that its beta)

Agreed. But this doesn't break kubefed too. It is just changing the internal detail.

kubefed should not be using ClusterGenerator. Its providing the yaml I believe.

kubefed is using ClusterGenerator. In fact, it is the only consumer of that interface. It uses the kubectl generator machinery to simplify a number of other things (read: reinvent the wheel).

But we decided to remove the annotation from cluster object because it is unnecessary code.