Group is not optional in LocalObjectReference

[tamalsaha]

LocalObjectReference group field is not marked optional. So, it can't be left unspecified or set to empty string.

In BackendTLSPolicy, we try to ref to a Secret and CRD fails validation check as group is ""

Previous conv in Slack: https://kubernetes.slack.com/archives/CR0H13KGA/p1725096986532939

[youngnick]

Thanks for opening this, this is a relatively simple change that just needs +optional added to the Group field.

/good-first-issue

[k8s-ci-robot]

@youngnick: 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:

Thanks for opening this, this is a relatively simple change that just needs +optional added to the Group field.

/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-sigs/prow repository.

[theBeginner86]

Hi @youngnick Please assign me this issue. I'd like to fix this. Thanks

[tamalsaha]

Though it is a simple change, it relaxes the constraints on an api already marked v1. Not sure that is halal with the maintainers? Should this struct be copied for the BackendTLSPolicy and only relaxed there or relaxed everywhere? @youngnick

[candita]

This is strange. I didn't have any issue with this in my PR for testing BackendTLSPolicy. caCertificateRefs should look something like this.

spec:
  targetRefs:
  - group: ""
    kind: Service
    name: "backendtlspolicy-test"
  validation:
    caCertificateRefs:
    - group: ""
      kind: Secret
      name: "backend-tls-checks-certificate"

[tamalsaha]

I guess there is nothing to fix here. I remember getting error from k8s but I can't reproduce this in k8s 1.31 anymore.

required just means something is set (even if that is empty ""). https://stackoverflow.com/a/45577763/244009

Sorry for the false alert.

[arkodg]

there's still a bug here, the doc string says when unspecified or empty , this can imply the field is optional, but rn its required

[tamalsaha]

@arkodg please see here: https://kubernetes.slack.com/archives/CR0H13KGA/p1726101749146929?thread_ts=1725096986.532939&cid=CR0H13KGA

[youngnick]

I've reopened because I agree that this field should be optional. Kind and Name still need to be required though.

This is a good-first-issue still, because it's a small, contained change.

[Thealisyed]

Hi @youngnick,

I hope you’re doing well! Could you please assign this task to me? I’d like to take it if it's still up for grabs :)

[Thealisyed]

/assign

[candita]

Thanks for opening this, this is a relatively simple change that just needs +optional added to the Group field.

/good-first-issue

Hey @youngnick would you be willing to remove this tag and the help-wanted one, if @tamalsaha is working on it, and it's on hold now? https://github.com/kubernetes-sigs/gateway-api/pull/3332#issuecomment-2345227715

[tamalsaha]

I am not working on it. I don't understand what is acceptable to the maintainers. If anyone wants to take over, please feel free to do so.

[youngnick]

As per the last comment on #3332 (https://github.com/kubernetes-sigs/gateway-api/pull/3332#issuecomment-2345227715), @robscott suggested that we instead change the doc string to just say "when empty string" instead of "when unspecified or empty string". On reflection, I probably tend to agree.

Let's drop this onto the community meeting agenda for this week and see if there can be some consensus.

[EyalPazz]

Hi @youngnick , is there still something to do here?

[youngnick]

The required change is to change "when unspecified or empty string" to "when set to the empty string"

[EyalPazz]

can i be assigned to this?

[EyalPazz]

/assign