Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GEP-1911] Update BackendRef with GEP details #2444

Merged
merged 11 commits into from
Oct 5, 2023

Conversation

dprotaso
Copy link
Contributor

/kind gep
/kind documentation

What this PR does / why we need it:

  • This takes GEP-1911 and updates the godoc for BackendRef
  • Mention users can signal backend protocol using ServicePort.protocol

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/gep PRs related to Gateway Enhancement Proposal(GEP) kind/documentation Categorizes issue or PR as related to documentation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 28, 2023
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 28, 2023
@dprotaso
Copy link
Contributor Author

cc @andrew-su

/assign @robscott @shaneutt @youngnick

@dprotaso
Copy link
Contributor Author

What's the equivalent of <gateway:experimental> ? I didn't want to add it to the godoc otherwise it would have marked BackendRef experimental which it's not

@robscott
Copy link
Member

@dprotaso fortunately we actually added something for that recently:

// <gateway:experimental:description>
// * Service (Mesh conformance profile, experimental, ClusterIP Services only)
// </gateway:experimental:description>

@dprotaso
Copy link
Contributor Author

hmm.. it didn't update HTTPRoute in the experimental channel 🤔

@dprotaso
Copy link
Contributor Author

Random guess is the generator mutates the CRD - so we should be generating experimental types first (no mutations) then standard one)

@dprotaso
Copy link
Contributor Author

Oddly we call DeepCopy() so 🤔

@dprotaso
Copy link
Contributor Author

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 29, 2023
@dprotaso
Copy link
Contributor Author

Following up - looks like the text is not updated on the HTTPRoute is because of how controller-tools works

Because BackendRef is inlined with HTTPBackendRef - the godoc present at the top level HTTPBackendRef is not present and neither is the godoc on BackendRef

For example this block doesn't appear in the CRD docs for HTTPRoute

// BackendRef is a reference to a backend to forward matched requests to.
//
// A BackendRef can be invalid for the following reasons. In all cases, the
// implementation MUST ensure the `ResolvedRefs` Condition on the Route
// is set to `status: False`, with a Reason and Message that indicate
// what is the cause of the error.
//
// A BackendRef is invalid if:
//
// * It refers to an unknown or unsupported kind of resource. In this
// case, the Reason must be set to `InvalidKind` and Message of the
// Condition must explain which kind of resource is unknown or unsupported.
//
// * It refers to a resource that does not exist. In this case, the Reason must
// be set to `BackendNotFound` and the Message of the Condition must explain
// which resource does not exist.
//
// * It refers a resource in another namespace when the reference has not been
// explicitly allowed by a ReferenceGrant (or equivalent concept). In this
// case, the Reason must be set to `RefNotPermitted` and the Message of the
// Condition must explain which cross-namespace reference is not allowed.
//
// * It refers to a Kubernetes Service that has an incompatible appProtocol
// for the given Route type
//
// Support: Core for Kubernetes Service
//
// Support: Implementation-specific for any other resource
//
// Support for weight: Core
//
// Support for Kubernetes Service appProtocol: Extended
//
// +optional
BackendRef `json:",inline"`

Instead what shows up in the CRD description is just the HTTPBackendRef godoc

// HTTPBackendRef defines how a HTTPRoute should forward an HTTP request.
type HTTPBackendRef struct {

@robscott @shaneutt @youngnick not sure what we want here - do you have opinions?

If we are strict about wanting this documentation to appear on the crd descriptions we'll have to duplicate the godoc and add it to the top of HTTPBackendRef

@dprotaso
Copy link
Contributor Author

If we're otherwise good with the docs as they are on the types then feel free to drop the /hold

Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dprotaso! I actually ended up with more questions this time around as I dug into it a bit more, hopefully these make sense.

apis/v1beta1/shared_types.go Outdated Show resolved Hide resolved
pkg/generator/main.go Show resolved Hide resolved
apis/v1beta1/shared_types.go Show resolved Hide resolved
apis/v1beta1/shared_types.go Outdated Show resolved Hide resolved
apis/v1beta1/shared_types.go Outdated Show resolved Hide resolved
apis/v1beta1/shared_types.go Outdated Show resolved Hide resolved
apis/v1beta1/shared_types.go Outdated Show resolved Hide resolved
geps/gep-1911.md Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 3, 2023
@youngnick
Copy link
Contributor

LGTM so I'll hit the approve button, @robscott up to you after that.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, youngnick

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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2023
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dprotaso! Just a few more nits but otherwise LGTM.

apis/v1alpha2/grpcroute_types.go Outdated Show resolved Hide resolved
apis/v1alpha2/grpcroute_types.go Outdated Show resolved Hide resolved
apis/v1alpha2/grpcroute_types.go Outdated Show resolved Hide resolved
apis/v1alpha2/grpcroute_types.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 5, 2023
@robscott robscott added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 5, 2023
@robscott
Copy link
Member

robscott commented Oct 5, 2023

Thanks @dprotaso!

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 5, 2023
@robscott
Copy link
Member

robscott commented Oct 5, 2023

One non-blocking thing I missed, do you mind adding a release note to the release note block of this PR @dprotaso? (Can happen post-merge, just helps us not miss anything when we're generating the changelog).

@k8s-ci-robot k8s-ci-robot merged commit 9deb057 into kubernetes-sigs:main Oct 5, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. kind/gep PRs related to Gateway Enhancement Proposal(GEP) lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants