ParentReference.Namespace has unnecessary pointer to string.

[briantkennedy]

https://github.com/kubernetes-sigs/gateway-api/blob/master/apis/v1beta1/shared_types.go#L48

"Namespace is the namespace of the referent. When unspecified (or empty string), this refers to the local namespace of the Route."

Given that the zero value in Go for a string type is empty string, there's no need for ParentReference.Namespace to be of type *string, rather, string would have the same interface with a simpler implementation.

[shaneutt]

This is commonly done for reasons of encoding to JSON/YAML. Note the // +optional comment above and the json:"namespace,omitempty" declaration to the right: making this a pointer causes YAML encoding to omit the field entirely from the final encoding, so the value of this is to avoid having fields with empty strings in YAML, e.g.:

kind: Gateway
Namespace: ""
Name: example

becomes

kind: Gateway
Name: example

Besides any aesthetic preferences, this means that our webhook validation can avoid having to accept "" as a valid namespace name. This has become a common pattern across many Kubernetes repositories so I believe you'll find this prevalent. Let us know if that explanation is helpful and if it's OK to close this issue, or if you have more questions on this topic.

[briantkennedy]

@robscott does this mean conformance tests should expect an error if a user provides namespace: "" for this field? The docstring for the API explictly says When unspecified (or empty string), this refers to the local namespace of the Route.. Does the docstring need to be changed, or should we change the variable type?

[robscott]

The namespace validation included in the CRDs should prevent empty string from being a valid input. I think the options are effectively nil or a 1+ character string. We should probably update the docstring to more clearly state that. With all of that said, CRD validation can be bypassed, so handling the empty string case as identical to nil is likely a good idea.

[briantkennedy]

So if we want to handle nil and "" as equivalent, why not have a string with "omitempty" behavior instead of the *string and reduce the number of cases that alias the same thing?

[briantkennedy]

Actually, taking a step back from my previous comment, I think our conversation is going something like this:

@briantkennedy : Our docs say nil / "" do the same thing, let's turn two cases into one. @shaneutt : No, "" shouldn't be allowed as a namespace value, let's not do that. @robscott : We have to handle "", it should do the same thing for nil and empty string.

Is that about right? To me, it sounds like @shaneutt and @robscott / the API docstring are in disagreement. Do we even have a consensus on whether namespace: "" should be allowed for this field?

[robscott]

So if we want to handle nil and "" as equivalent, why not have a string with "omitempty" behavior instead of the *string and reduce the number of cases that alias the same thing?

I'd agree that would work and potentially be cleaner for implementations, but it unfortunately makes our CRD validation logic/type definitions messier. Essentially right now we've got a shared set of types, like Namespace that have validation centrally defined. If we were to allow an empty string as a valid namespace, we'd need separate types for "OptionalNamespace" and "RequiredNamespace" where the required namespace had a min length of 1. We'd also need to repeat that pattern for all the other types defined in shared_types.go, and I think it would generally just add more confusion. Alternatively we could require the minlength attribute be added in every place the type was used and required, but that seems like it would be very easy to miss in the future and lead to impossible to fix bugs in the API.

[robscott]

@shaneutt : No, "" shouldn't be allowed as a namespace value, let's not do that. @robscott : We have to handle "", it should do the same thing for nil and empty string.

To clarify, I'm suggesting that "" should be prevented by CRD validation and is unlikely to ever reach your controller, so largely agree with Shane. The only caveat is that technically CRD validation can be bypassed, so following our implementation guidelines, it would be best to at least handle/not crash in that case.

[briantkennedy]

So, to back up a bit, what's the behavior we want for this field when given empty string? It sounds like you're both saying the API should treat empty string as an error since the user isn't supposed to express intent for a namespace with name empty string, is that correct?

[shaneutt]

So, to back up a bit, what's the behavior we want for this field when given empty string? It sounds like you're both saying the API should treat empty string as an error since the user isn't supposed to express intent for a namespace with name empty string, is that correct?

Right, this resonates with me.

[youngnick]

Yes, I think that it's correct to say that the value shouldn't be an empty string, that is an error. That error won't usually be encountered unless someone bypasses CRD validation, but it needs to be handled without crashing.

Given all that, we get back to the original question: Should this be a pointer? In this case, it doesn't need to be, because the difference between the empty string and a nil pointer is not meaningful. However, changing it now is a big breaking change, so we either need to change it before the final v0.5.0 release, or accept that this is how it is forever.

I'm inclined towards the latter, just in case it turns out we need to be able to distinguish between "unset" and "set to empty string".

[shaneutt]

I'm inclined towards the latter, just in case it turns out we need to be able to distinguish between "unset" and "set to empty string".

I'm also inclined towards the latter. For interactions via JSON/YAML it's invisible and so I don't really see a problem there. For implementors and users using Go clients I don't see this as causing any severe technical challenges, but if someone else feels differently I would like to hear from them.

[briantkennedy]

So, the docstring for this field currently specifies that empty string is a valid value (eg namespace: "").

Namespace is the namespace of the referent. When unspecified (or empty string), this refers to the local namespace of the Route.

Should it be updated to read as follows?

Namespace is the namespace of the referent. When unspecified, this refers to the local namespace of the Route. Refer to the Namespace type for valid values.

[shaneutt]

Yep that tracks.

[robscott]

/good-first-issue

[k8s-ci-robot]

@robscott: This request has been marked as suitable for new contributors.

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-good-first-issue command.

In response to this:

/good-first-issue

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.

[asim-reza]

/assign