Updates TransformXXX Functions in k8s pkg

[danehans]

Updates TransformXXX functions to not return an error if the provided and returned objects are the same type. Instead, the provided object is returned without any transformations.

Fixes: #26240

[danehans]

@odinuge since you commented in #25604, PTAL.

[odinuge]

Thanks! The code looks good to me!

Outside the scope of this PR; I think at some point it would be more useful to extraxct the godoc for these functions to a common description, so we avoid having it duplicated that much; given its so easy to miss one and get inconsistent comments - given they ~all implement the same thing, just for different types.

[giorio94]

Thanks! /lgtm

[giorio94]

Outside the scope of this PR; I think at some point it would be more useful to extraxct the godoc for these functions to a common description, so we avoid having it duplicated that much; given its so easy to miss one and get inconsistent comments - given they ~all implement the same thing, just for different types.

One step further might be to leverage generics to extract all the common logic, having only a single function as parameter which takes care of the actual conversion from the source to the destination type. This would reduce the overall code duplication and ensure consistency across all implementations. Related: #26367

[danehans]

@giorio94 thanks for the heads-up regarding #26367. Based on my comment, we may be able to close this PR and have #26367 fix #26240.

[giorio94]

@giorio94 thanks for the heads-up regarding #26367. Based on my comment, we may be able to close this PR and have #26367 fix #26240.

The two PRs are currently addressing two separate aspects (this one tackles the transformation functions which do an actual type transformation, while #26367 the ones which don't mutate the object type).

[danehans]

@giorio94 thanks for the clarification. I noticed the TransformToGeneric() func in #26367 and thought it was using generics for all transformation funcs. Back to your #26244 (comment), should I update this PR to use generics? Since #26240 is expected to be a release blocker, IMO it's better for this PR to introduce minimal changes and tackle the transition to generics in a separate PR that is merged after the release. Thoughts?

[giorio94]

Back to your #26244 (comment), should I update this PR to use generics? Since #26240 is expected to be a release blocker, IMO it's better for this PR to introduce minimal changes and tackle the transition to generics in a separate PR that is merged after the release. Thoughts?

Yeah, I agree that it is better to drive this fix to completion and tackle more invasive refactorings in a followup.

[joestringer]

I re-ran K8sUpstreamNetConformance tests. From the sheer number of tests that failed, I don't quite understand whether some of the test failures could be related to the PR or the workflow is just a bit broken sometimes. :(

[tommyp1ckles]

@danehans These changes look very reasonable to me, however I am still a bit confused as to how this situation might arise?

In the only usage of this transform function in the operator, the k8s client service interface returns a *v1.Service type.

[tommyp1ckles]

/test

[danehans]

@tommyp1ckles thanks for the review. I agree that TransformToK8sService() should only receive a k8s Service. However, this PR makes TransformToK8sService() and other transform functions backward compatible prior to the changes introduced by #25604. I also believe this PR follows the updated guidance for using TransformFunc. I can revert the TransformToK8sService() changes, especially since it's not being used as a TransformFunc, but I think it's safer to keep it as-is at this point of the release cycle and reassess after the release. Thoughts?

[danehans]

@joestringer thanks for rerunning the K8sUpstreamNetConformance test. It's now passing but now the ConformanceEKS test is failing due to:

Error reading Cilium logs: error getting cilium-agent logs for kube-system/cilium-zxm7g: Get "https://192.168.116.242:10250/containerLogs/kube-system/cilium-zxm7g/cilium-agent?sinceTime=2023-06-23T06%3A13%3A12Z&timestamps=true": dial tcp 192.168.116.242:10250: i/o timeout
[=] Test [all-entities-deny]

I don't believe the ConformanceEKS test failures are related to this PR. Are you able to rerun this test?

[tommyp1ckles]

@danehans Thanks for the clarification, lgtm

[joestringer]

/test

[joestringer]

I tried retriggering tests to kick off the gateway-conformance jobs, but I think actually the PR needs to be rebased against the latest main branch. @danehans could you rebase and retrigger the tests?

[joestringer]

/test