cluster-api-provider-aws icon indicating copy to clipboard operation
cluster-api-provider-aws copied to clipboard

Published 20 hours ago • verified publisher icon kubernetes-sigs

Make optional API fields consistently pointers (or not pointers)

Open invidian opened this issue 3 years ago • 16 comments
trafficstars

/kind feature

Describe the solution you'd like As discussed in https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/2271#discussion_r811789340, some API fields marked as optional are value fields, not pointers, others are pointers. Perhaps this should be unified at some point.

invidian avatar Feb 22 '22 10:02 invidian

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 avatar Feb 22 '22 10:02 richardcase

/help

richardcase avatar Feb 22 '22 10:02 richardcase

@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.

k8s-ci-robot avatar Feb 22 '22 10:02 k8s-ci-robot

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.

Ankitasw avatar Feb 22 '22 13:02 Ankitasw

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.

sedefsavas avatar Feb 23 '22 02:02 sedefsavas

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 ?

Ankitasw avatar Feb 23 '22 05:02 Ankitasw

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

sedefsavas avatar Feb 23 '22 06:02 sedefsavas

@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.

Ankitasw avatar Feb 23 '22 07:02 Ankitasw

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.

invidian avatar Mar 21 '22 15:03 invidian

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

k8s-triage-robot avatar Jun 19 '22 16:06 k8s-triage-robot

/remove-lifecycle stale

invidian avatar Jun 19 '22 16:06 invidian

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

k8s-triage-robot avatar Sep 17 '22 16:09 k8s-triage-robot

/remove-lifecycle stale

invidian avatar Sep 17 '22 16:09 invidian

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

@sedefsavas @Ankitasw @Skarlso - wdyt?

richardcase avatar Sep 20 '22 08:09 richardcase

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.

Ankitasw avatar Sep 20 '22 09:09 Ankitasw

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

Skarlso avatar Sep 20 '22 09:09 Skarlso

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

Ankitasw avatar Nov 09 '22 16:11 Ankitasw

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

k8s-triage-robot avatar Feb 08 '23 12:02 k8s-triage-robot

/remove-lifecycle stale

invidian avatar Feb 08 '23 13:02 invidian

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

k8s-triage-robot avatar Feb 08 '24 13:02 k8s-triage-robot