Ipalloc newKO

[henderiw]

added ipalloc with new parser interface.

[kispaljr]

I propose not to create an interface for each of our K8s resources with 3 methods for each of its fields, and keep the interface and the K8s API go struct in sync later, but use generic methods instead to read/write data into the kpt package.
This means that the implementation of business logic in our kpt functions will use the go structs of those K8s resources that we have to maintain anyway. All validation logic, setter functions, etc. should be added to those structs as opposed to the interfaces.

Here is a proof-of-concept implementation for this proposal:
https://github.com/nokia/nephio-nephio/tree/setspec/krm-functions/setspec-test

Notes on the implementation:

• the repo above contains a kpt function implementation that showcases how both the current SDK and my proposal is used in a kpt function implementation
• in this file: https://github.com/nokia/nephio-nephio/blob/setspec/krm-functions/setspec-test/testdata/test1/_expected.yaml
  you can find the output of the function: the resource named "currentsdk" is manipulated with the current SDK, and the other using my proposal. (You can check if they really generated the results in _expected.yaml by actually running the go test)
• as you can see this PoC implementation proposal is almost equivalent in terms of keeping the comments with the current SDK
• both versions will keep all comments in Release 1 supported scenarios
• the implementation of SetSpec() is a temporary fix, the proper place to implement this is in the underlying kpt function SDK that we use, more precisely here: https://github.com/GoogleContainerTools/kpt-functions-sdk/blob/e8e9cb3c3ae2a19c22f52701d1542cf24e541df0/go/fn/internal/map.go#L138
  at the line that reads: // TODO: Copy comments? :))
  Unfortunately different CLA (+ my company's policy) prevents me to contribute there
• naturally SetSpec() should be generalized and used for Status, as well (edited)

[kispaljr]

I propose to remove all GetXXX() methods from the interface because of the following reasons:
This functionality is provided by the kpt-fn go SDK that we build on (github.com/GoogleContainerTools/kpt-functions-sdk/go/fn), not just for specific types, but in a generic manner to any type (let it be ipamv1alpha1.IPAllocation, or DataNetworkSpec, or any Kubernetes resource or subresource go types.
So, instead of adding these methods into this interface, we can just do this:

var ipAllocStruct ipamv1alpha1.IPAllocation
err = ipAllocObj.As(&ipAllocStruct)

and then just use the fields of ipAllocStruct

[kispaljr]

I propose to remove all SetXXX() and DeleteXXX() methods that sets one of the fields of the Spec, and replace it with a generic function that works for any Kubernetes Resource type that has a spec field. This would update the whole Spec at once, preserving as much comments as possible (all if nothing was changed). The advantage of this would be that we don't have to reimplement this functionality for each field of each type again, in these interfaces like IPAllocation.

Simply craft a new ipamv1alpha1.IPAllocationSpec struct by optionally getting the current spec field values by the method described above (KubeObject.As()), and then updating the fields of the go struct, and then just use the new SetSpec() generic function on the corresponding fn.KubeObject, (or just use the SetSpec() method of this IPAllocation interface)

var ipAllocObj fn.KubebObject  = ...
var ipAllocSpec ipamv1alpha1.IPAllocationSpec
// set the fields of the spec to the desired state
ipAllocSpec.Index = 0
// ...
err := SetSpec(ipAllocObj, &ipAllocSpec)

[kispaljr]

The easiest implementation of SetSpec() and SetStatus() would be to send a Pull Request to the underlying SDK that fixes this TODO and copies the comments recursively :)

func (o *MapVariant) setYAMLNode(key string, node *yaml.Node) {
	children := o.node.Content
	if len(children)%2 != 0 {
		log.Fatalf("unexpected number of children for map %d", len(children))
	}

	for i := 0; i < len(children); i += 2 {
		keyNode := children[i]

		k, ok := asString(keyNode)
		if ok && k == key {
			// TODO: Copy comments?
			oldNode := children[i+1]
			children[i+1] = node
			children[i+1].FootComment = oldNode.FootComment
			children[i+1].HeadComment = oldNode.HeadComment
			children[i+1].LineComment = oldNode.LineComment
			return
		}
	}

	o.node.Content = append(o.node.Content, buildStringNode(key), node)
}

[kispaljr]

I propose to remove all SetXXX() and DeleteXXX() methods that sets one of the fields of the Status, and replace it with a generic function that works for any Kubernetes Resource that has a status field. This would update the whole status at once, preserving as much comments as possible (all if nothing was changed). The advantage of this would be that we don't have to reimplement this functionality for each field of each type again in these interfaces like IPAllocation.

Simply craft a new ipamv1alpha1.IPAllocationStatus struct by optionally getting the current status field values by the method described above (KubeObject.As()), and then updating the fields of the go struct, and then just use the new SetStatus() generic function on the corresponding fn.KubeObject:

var ipAllocObj fn.KubebObject = ...
var ipAllocStatus ipamv1alpha1.IPAllocationStatus
// set the fields of the spec to the desired state
ipAllocStatus.Gateway = "..."

err := SetStatus(ipAllocObj, &ipAllocStatus)

[kispaljr]

The github.com/GoogleContainerTools/kpt-functions-sdk/go/fn SDK that we use parses all YAML files before calling our "main" function (actually called Run()). It wraps the parsed YAML nodes into a hierarchy of KubeObject and SubObject instances. In order to avoid duplicate parsing, parsing from a YAML file (byte slice) should always be replaced by converting from a KubeObject, e.g. in this case by using the NewFromKubeObject() method of this interface.

[kispaljr]

The pull request doesn't build on my machine due to this example.com import.
Should it be replaced with something like:
github.com/nephio-project/nephio/krm-functions/lib/parser ?

[kispaljr]

The latest tag on github.com/nokia/k8s-ipam is v.0.0.3, and that version doesn't contain the apis/alloc/ipam/v1alpha1 path (but the main branch does).
That confuses go mod tidy, since for go the latest module is v0.0.3, and not the main branch.
Could you add a new tag to the k8s-ipam repo to make it easier to import?

[gvbalaji]

Please see my comments on #121 which is base for others. I beleive if we take that path implementation of these individual libraries will be simpler.

[kispaljr]

Just wanted to point out that this PR contains files from #121 and #122 .
This may be fine, I am not sure what our policy is in this scenario.

[johnbelamaric]

Just wanted to point out that this PR contains files from #121 and #122 . This may be fine, I am not sure what our policy is in this scenario.

We should rebase in these cases (rebase is preferable to merge commits).

[johnbelamaric]

So, I am more aligned with Istvan on getting rid of the getters and setters. The idea would be that instead of getters and setters, if you want the convenience of using these in strongly typed Go structs, you unmarshal it (GetGoStruct), then you call set SetFromGoStruct, which sets the values on the underlying KubeObject, rather than just marshalling it.

I think this would work to preserve comments. The usage would be something like:

ifcWrapper := interfacev1alpha1.NewFromYaml(b)
ifc := ifcWrapper.GetGoStruct() // or maybe the more traditional pass in a pointer
ifc.CNIType = "foo"
ifcWrapper.SetFromGoStruct(ifc)

So - we have ONE getter that returns the Go struct, ONE setter that accepts the Go struct. The underlying KubeObject is updated based on the contents of the Go struct.

This is very close to that; all that is missing is the SetFromGoStruct (and to reinstate GetGoStruct).

But I am not going to push, this may just be bike shedding at this point. It's more important to get it merged soon.

[henderiw]

We want to go to the gostruct approach but we need to validate that approach if it works with all lists, maps, etc w/o loosing comments when you insert, etc. So this is indeed our final target. We decided to do this in the interim, so we can start testing the fns and in parallel we will go to the go struct approach.

[henderiw]

This aligns the ip allocation with the new KubeObjectExt

[gvbalaji]

thanks @henderiw .
/approve

[kispaljr]

If I understand correctly, the main reason to have this wrapper at all, is the lack of static type checking in the interface{} type parameter of SetSpec and SetStatus. I agree that this is a valid concern.
However I suggest to use the power of Go generics to fix this, as opposed to implementing it one-by-one for each type (avoiding this is the main reason for having generics).
Here s my proposal to add static type checking: #141

[kispaljr]

I somehow missed @johnbelamaric 's comment, so far, sorry

I think this would work to preserve comments. The usage would be something like:

ifcWrapper := interfacev1alpha1.NewFromYaml(b)
ifc := ifcWrapper.GetGoStruct() // or maybe the more traditional pass in a pointer
ifc.CNIType = "foo"
ifcWrapper.SetFromGoStruct(ifc)

So - we have ONE getter that returns the Go struct, ONE setter that accepts the Go struct. The underlying KubeObject is updated based on the contents of the Go struct.

I very much like the idea of one SetFromGoStruct() as opposed to a separate SetSpec/SetStatus. Here is my PR for that: #142 . It can live together with #141 (types-safe SetSpec & SetStatus), or it can completely replace the separate setters.

[ganchandrasekaran]

/lgtm

[henderiw]

The differences between the approaches are very small.
Here is my breakdown:

KubeObjectExt: expects the developer to know generics and add the right type in the generic field of the go struct. More error prune, but checked in the library.
Explicit typed interface. needs explicit code although could be generated. Easier for people to grasp.
So it depends on the target for these. A deep go developer prefers KubeObjectExt, the less experienced dev prefers Explicit typed interfaces. How I personally look at this is I look at the consumer of this and given the SDK targets less experienced people. As such this is why we created the explicit methods.

Personally I prefer KubeObjectExt, but the barrier for the consumers is higher.
My 2 cents.

Example Here.

Explicit
alloc, err := vlanlibv1alpha1.NewFromKubeObject(vlanalloc)
if err != nil {
return nil, err
}
KubeObjectExt
alloc, err := ko.NewFromKubeObject*vlanv1alpha1.VLANAllocation
if err != nil {
return nil, err
}

[henderiw]

/approve

[nephio-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gvbalaji, henderiw

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [henderiw]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ganchandrasekaran]

/lgtm