Make optional API fields consistently pointers (or not pointers)

[invidian]

/kind feature

Describe the solution you'd like
As discussed in #2271 (comment), some API fields marked as optional are value fields, not pointers, others are pointers. Perhaps this should be unified at some point.

[richardcase]

Thanks @invidian

There is some inconsistency in how we represent optional fields....to pointer or not to pointer, that is the question. We have had some issues with the code gen tools in the past not liking slices of pointers to structs as well.

We should some up with clear guidance and do any refactoring as required.

/triage accepted
/priority backlog
/area api

[richardcase]

/help

[k8s-ci-robot]

@richardcase:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Ankitasw]

Just thinking out loud to help understand issue, below are the Pros and cons of using pointers:

Pros:

• If you need to modify something in the field we should use pointers. Although, this can be avoided if we use getters and setters for each field if we are against using pointers.
• Validations which exists as of today in webhooks are based on nil checks which can be done effectively in case of pointers, unlike primitive types where we have to check differently for each primitive type.
• When using json tags(as widely used in optional fields), it is useful to have pointer fields as we use omitempty to allow nil values while marshalling/unmarshalling.
• If optional fields are struct type, we should go with pointers because of reason above.

Cons:

• Performance is degraded as dereferencing to pointer is costlier as compared to primitive data types.
• panic: runtime error, invalid memory address or nil pointer dereference issue is very common because of using pointers.

As long as we are not constrained about performance, goroutines(as there can be race conditions with pointers) and error handling, we can opt for using pointers in optional fields as otherwise it would become little tedious to use primitive types in some places.
But since we have faced issues previously with slices of pointers to struct, we should not touch those optional fields, and in this case we can make an exception and document it in developers guide.
So to conclude, we cannot be fully consistent with the usage of pointers in optional fields as there are upside and downside of both, but IMO using pointers is still better.

[sedefsavas]

Quoting from https://cluster-api.sigs.k8s.io/contributing#optional-vs-required

Fields SHOULD be pointers if there is a good reason for it, for example:

- the nil and the zero values (by Go standards) have semantic differences.
Note: This doesn’t apply to map or slice types as they are assignable to nil.

- the field is of a struct type, contains only fields with omitempty and you want to prevent that it shows up as an empty object after marshalling (e.g. kubectl get)

As @richardcase mentioned, we are avoiding to use pointer slices, but that does not have a down side as we can still have nil checks.

[Ankitasw]

As @richardcase mentioned, we are avoiding to use pointer slices, but that does not have a down side as we can still have nil checks.

What about the primitive types field like string, int32 etc. In case we don't use pointer, suppose for int32 field how would w know if it is 0 or is not set in JSON response ?

[sedefsavas]

What about the primitive types field like string, int32 etc. In case we don't use pointer, suppose for int32 field how would w know if it is 0 or is not set in JSON response ?

For the rest, pointers are suggested in Kubernetes API conventions doc:

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required

[Ankitasw]

@sedefsavas Then I totally agree with what you suggested here.
Unlike what is suggested in the issue, we cannot be purely consistent about using only pointers or non-pointers. We will have some exceptions, what we can do is we can be consistent about primitive types being used as pointers and other fields as non pointers(put this in developer guide, may be) and change the description of the issue accordingly.

[invidian]

Just a note, when addressing the feedback in #2271, I've noticed that after making field a pointer type in cmd/clusterawsadm/api/bootstrap/v1beta1/types.go, autogenerated code for filling the default values for fields no longer works as expected. I've tried adding some functions like NewS3Buckets() or SetDefaults_S3Buckets(), but it didn't get picked up as I'd expect.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[invidian]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[invidian]

/remove-lifecycle stale

[richardcase]

Should we do this as part of the v1beta2 version bump (#2355)?

@sedefsavas @Ankitasw @Skarlso - wdyt?

[Ankitasw]

Can we do it following the v1beta2 changes? It wont break anything right, since anyways we are not doing any release with v1beta2 as of now? Please correct me if I am wrong.

[Skarlso]

Yeah, let's do this after. That PR is huge already anyways... And I have another issue that's waiting for an updated API to take care off. :D

[Ankitasw]

This is still missed 😢 I am not sure if we would be able to make it in v2.0.0?

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[invidian]

/remove-lifecycle stale

[k8s-triage-robot]

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted