Add KRM func suppport for PackageVariants

[SimonTheLeg]

This PR adds the KRM func support discussed in https://github.com/johnbelamaric/kpt/blob/pvs-design/docs/design-docs/08-package-variant.md for PackageVariants.

Fixes: nephio-project/nephio#117

Outdated Review Notes

Couple of notes for the reviewer(s) from my side:

1. With this change, the Function struct type of kptfile.v1 is being used inside PackageVariantSpec. Because PackageVariantSpec is a Kubernetes Object, I think we also need to have DeepCopy on all of its subfields. As a result I believe we need DeepCopy on the Function struct type. For now I have just enabled it on the package level. PTAL if that is fine. Another option is to just have it on the Func type and its fields. But then we need to be super careful whenever we use a new struct to not forget the Kubebuilder annotation.
2. I did notice interesting behaviour when writing the tests for this. The spacing of the Kptfile inside the PackageRevisionResources differs whether you have Mutators in the Package revision or not. The reason for this is that I am using kyaml for the Marshal. Kyaml is required to keep the order of the keys in the yaml. But since this can be different from creating a PackageRevisionResource directly from a yaml string, the spacing might be slightly different. For code reference see add none with existing testcase.
   In practice I don't think it makes a difference. The only good solution I was able to come up with would be to have custom Marshaller/Unmarshallers on the PackageRevisionResources that uses then the kyaml as a preprocessor. But I think this is not necessary. Please let me know what you think.
3. More of a question, than a comment. What is the reason why we run the PackageVariant mutators before the ones specified in the original kptfile? We should probably add the answer to this also to the initial proposal.

Those are my thoughts so far, let me know if there's anything else :)

[SimonTheLeg]

I have to admit, I am running out of creative ideas how to solve the DeepCopy issue (which was the reason why the first CI run failed). Essentially the problem is that kyaml's ResourceMeta/TypeMeta/ObjectMeta do not have DeepCopy funcs implemented on them. But since it is external and we directly embed it, we don't really have control over it. So I thought maybe it would be enough to switch from yaml.ResourceMeta to metav1.TypeMeta and metav1.ObjectMeta as these are DeepCopy compatible. You can see a draft for this in the commit "Switch ktpfile from yaml metav1 Typemeta and Objectmeta". However then the issue is that kyaml handles fields that are missing the yaml struct tag differently than sigs.k8s.io/yaml does 🙈 Basically if kyaml cannot find the yaml tag on a struct, it simply takes the name of the struct field and lowercases it (see here). This becomes a problem for the APIVersion field as it becomes "apiversion" instead of the required "apiVersion". But because we embed metav1.TypeMeta and metav1.ObjectMeta, we cannot control the struct tags for their subfields.

At this point, there are only two ideas I have left:
a) we could take all the fields in metav1.TypeMeta and metav1.ObjectMeta and put them directly into the KptFile and set the yaml struct tag directly on them. Would look a bit ugly and be a bit cumbersome to maintain, but it would work.
b) we could make an upstream contribution to kyaml to have DeepCopy on their ResourceMeta (and/or TypeMeta, ObjectMeta). I think this would be the most elegant solution also going forward (kpt has a lot of types that embed ResourceMeta), but of course then we need to wait for that to merge.

Maybe I am missing something obvious, please let me know if you can think of a more elegant solution to fix this. I am all for it :) If not I would proceed with option propose to proceed with option b).

[natasha41575]

we could make an upstream contribution to kyaml to have DeepCopy on their ResourceMeta (and/or TypeMeta, ObjectMeta). I think this would be the most elegant solution also going forward (kpt has a lot of types that embed ResourceMeta), but of course then we need to wait for that to merge.

You can feel free to make this change in kyaml if you'd like. I agree that this is seemingly the most elegant solution. I can review and approve that PR when you make it. Assuming that it is a small and noninvasive change to kyaml, it should be fine.

[mortent]

So I think the easiest solution here is to just use kyaml to parse and update the Kptfile. It does provide some more control over how each field is added to yaml, it makes sure the formatting is consistent (since other kpt functionality that updates the Kptfile uses kyaml), and it makes sure any comments in the Kptfile is preserved.

It is not quite as convenient as reading the content into a go struct. We can look into whether that is possible, but we could try to do that separately from this change.

[johnbelamaric]

Thanks Simon, this is a great start. The main thing that needs to be fixed is to make this idempotent. The first thing that should be done is to remove all the functions added by this PV in past reconciliations. Then you can add them back in. Details are here:

https://github.com/GoogleContainerTools/kpt/blob/main/docs/design-docs/08-package-variant.md#krm-function-pipeline

[natasha41575]

@SimonTheLeg I want to add a warning that sometimes upgrading the kyaml dependency in kpt can be a chore (and probably should go in a separate PR), so while you are waiting for that, I think @mortent's suggestion in #3925 (comment) is a good way to work around it for now.

[johnbelamaric]

I think this will be WAY easier using KubeObject. See the comments.

[johnbelamaric]

Note: I found a bit of a bug where errors in "applyMutation" are not really propagated properly. Or they are, but they are overwritten be subsequent incorrect reconciliations (because findAndUpdate was written before applyMutations, for just doing packageupdates, and I didn't correctly update it when I implemented applyMutations).

I will be fixing it in #3939. I mention it here in case you come across it. You can see it when doing manual/e2e tests. If you focus on your unit tests and get those working, you should be OK.

[SimonTheLeg]

alright, everything should be in order now and I have rebased the PR to make it easier to review.
If there is need to view the old revision history in all its 14 commit glory, I have backed it up here https://github.com/SimonTheLeg/kpt/commits/krm-funcs-for-packagevariant-backup2

[johnbelamaric]

Awesome, looking pretty good.

[johnbelamaric]

Awesome, looking pretty good.

[SimonTheLeg]

okay now everything should be resolved and ready for review

[mortent]

Is this to remove the leading line break? If so, it is probably easier to just use strings.TrimSpace: https://pkg.go.dev/strings#TrimSpace

[SimonTheLeg]

Yes normally strings.TrimSpace works for this. Unfortunately it does not work in our case, as the second line is indented with spaces. This will result in trimming out the spaces of that line:

func Test(t *testing.T) {
	s := `
  second-line indented:
    more indented:
`
	fmt.Println(strings.TrimSpace(s))
	fmt.Println("------")
	fmt.Println(s[1:])
}

// will produce
second-line indented:
    more indented:
------
  second-line indented:
    more indented:

But I am also going to look into the idea of your other comment to make use of kyaml, which maybe makes this obsolete all together.

[mortent]

Not a blocker for this PR. It is just a bit difficult to read with the indentation.

[mortent]

Would it be easier to construct the combined yaml manifests here with the yaml libraries (like kyaml) instead of string manipulation? The indentation here is a bit tricky to follow.

[mortent]

Not a blocker for this PR. It is just a bit difficult to read with the indentation.