Updates ParametersRef Godocs

[danehans]

What type of PR is this?
/kind documentation

What this PR does / why we need it:
Updates the gatewayclass.spec.parametersRef godocs.

Which issue(s) this PR fixes:

Fixes #524

Does this PR introduce a user-facing change?:

 "NONE"

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danehans

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [danehans]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

@danehans: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-service-apis-verify	de30a8d	link	/test pull-service-apis-verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[robscott]

Right now this pattern is closely tied to IngressClass and associated parameters. I'd prefer not to make a change here until we can generate some kind of consensus in upstream Kubernetes. I had previously raised the idea of adding a namespace to IngressClass paramsRef but there wasn't much support for it at the time. Although this wouldn't exactly be an API change, it feels like a bit of a slippery slope to allow referencing namespaced resources here.

I personally think the ideal here is for params ref to point to a resource at the same scope. If we were going to support referring to namespace-scoped resources, I think it would be easier to understand an explicit namespace reference here. Without that, a reference intended for a resource in an unwritten implementation-specific namespace could be confusing for anyone looking at the GatewayClass resource.

[hbagdi]

I think it would be easier to understand an explicit namespace reference here.

I think so too. Magical resolutions of namespaces are more like a workaround and we should avoid them given we are very early with this API.

In the past, we also had some discussion around about how much do we want ConfigMap to be used in such cases and we decided against it. d9ed654 is one of the commits I could find.
I do think using ConfigMap in such cases are useful as they avoid overheads of a CRD. Using a CRD has less ROI and more tax given that there will only exist one or at the most few instances.

[danehans]

In the past, we also had some discussion around about how much do we want ConfigMap to be used in such cases and we decided against it. d9ed654 is one of the commits I could find.

@hbagdi that commit xref is related to using ConfigMap as the default for the field and not general use of ns-scoped resources. According to meeting notes, the last time we discussed this topic was 1/9/19 and I don't see that we reached consensus. IMO requiring the resource to be cluster-scoped is too rigid. As you mention, this will require all implementors to support a CRD when a ConfigMap may suffice. Additionally, ns-scoped CRDs are far more common than cluster-scoped resources.

I had previously raised the idea of adding a namespace to IngressClass paramsRef but there wasn't much support for it at the time.

@robscott do you have any documentation that explains why this decision was made?

[robscott]

@danehans I don't really understand how we don't have more written documentation around this, but I know it was initially discussed in the Ingress working group and then at the sig-network meeting on May 28. For some reason I can't find a recording of that meeting on YouTube. Although I brought up the suggestion of namespace scoping, I was not sold on it and did not provide the most compelling argument. I know @thockin preferred that parameters resources were at the same scope as the class they were referred by. I personally don't think ConfigMap references are significantly better/different than annotations and would rather see implementations adapt CRDs wherever possible. I think it's worth revisiting upstream - do you want to add something to the agenda for the next sig-network meeting?

[robscott]

@danehans I added an item to the sig-network agenda for Jan 21 to cover this. It would be great to have input from as many people as possible that would appreciate a namespace-scoped option as part of that discussion.

[robscott]

Adding a hold on this until we get some consensus with sig-network upstream.

/hold

[bowei]

Don't we need the namespace to make the reference make sense?

[robscott]

This was originally intended to be used for resources within the same namespace, similar to the upstream resource of the same name. This PR suggested that "within a known namespace" could be interpreted as a predefined/constant namespace for an implementation. I think we do need to clean up this comment because it is confusing, and separately I think we need to reevaluate if we are open to adding an optional namespace field to Gateway ParamsRef.

[robscott]

Following up on this, @danehans and I are planning on working on a KEP for the 1.21 release cycle that would add an optional namespace reference to IngressClass ParametersRef. Initial feedback on sig-network community call was promising. Assuming that KEP is approved, I think it will make sense to move forward here as well.

[danehans]

Following up on this, @danehans and I are planning on working on a KEP for the 1.21 release cycle that would add an optional namespace reference to IngressClass ParametersRef.

xref: kubernetes/enhancements#2366

Thanks @robscott for leading the KEP!

[hbagdi]

xref #543

[danehans]

Closing in favor of #543