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

add helper for determining app protocol #904

Merged
merged 6 commits into from
Jan 10, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions pkg/apis/networking/ports.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ const (
ServicePortNameHTTPS = "https"
)

var (
// AppProtocolH2C is the name of the external port of the service for HTTP/2
AppProtocolH2C = "kubernetes.io/h2c"
Copy link
Contributor

@skonto skonto Jan 5, 2024

Choose a reason for hiding this comment

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

How about kubernetes.io/ws? I read that Contour has it on by default but that would be implicit so I suspect we still need to label services for consistency no?

Copy link
Contributor

@skonto skonto Jan 5, 2024

Choose a reason for hiding this comment

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

Could we somehow make clear that this is gateway api related (via a comment maybe)?

Copy link
Contributor Author

@KauzClay KauzClay Jan 5, 2024

Choose a reason for hiding this comment

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

How about kubernetes.io/ws? Is there a plan for this?

Ah, good point. I think I may have misinterpretted Dave's sentence on WS here: knative/serving#14569

He says:

For websocket - this in theory should just be on by default (contour toggled it on for Gateway API). There might be other implementations that have this off but let's revisit that when we find one that doesn't

Which when I first read it, I bet I read that last line and thought "oh, I can just skip it and leave it", but on second pass, that doesn't seem right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could we somehow make clear that this is gateway api related?

Sure, like perhaps calling it GatewayAPIAppProtocolH2C or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, you know, reading Dave's GEP, maybe we don't need to do anything for WS, since upgrades happen by default?

@dprotaso what were you imagining here?

also, how does one indicate that a Knative Service should be a websocket?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, like perhaps calling it GatewayAPIAppProtocolH2C or something?

These constants are technically Kubernetes constants - I don't think we should prefix them with the Gateway API

We can link the KEP so it's clear

https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/3726-standard-application-protocols#new-standard-protocols

Honestly, websocket is a weird one cause Contour should have probably not disabled it. For now we can skip it since I'd imagine most implementations support the upgrade flow via HTTP

So we really need to set the h2c protocol when the user specifies it in their port name (the way it's used for grpc services)

eg.
https://github.com/knative/docs/blob/dab74765e0f0c60789fe97208aa0b0a1e23eb9cd/code-samples/serving/grpc-ping-go/service.yaml#L12C13-L12C22

Copy link
Member

Choose a reason for hiding this comment

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

If we need websocket support we would have to upgrade the runtime contract to define a constants users would use

https://github.com/knative/specs/blob/main/specs/serving/runtime-contract.md#protocols-and-ports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so, to summarize:

  • keep the variable name, but link the KEP in the comment
  • don't move forward with WS right now

is that correct?

As far as this comment:

So we really need to set the h2c protocol when the user specifies it in their port name (the way it's used for grpc services)

My other PR (knative/serving#14757) is where that would happen, right?

Copy link
Member

Choose a reason for hiding this comment

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

My other PR (knative/serving#14757) is where that would happen, right?

Yeah

)

// ServicePortName returns the port for the app level protocol.
func ServicePortName(proto ProtocolType) string {
if proto == ProtocolH2C {
Expand All @@ -55,3 +60,11 @@ func ServicePort(proto ProtocolType) int {
}
return ServiceHTTPPort
}

// AppProtocol returns the value for app level protocol based on the ProtocolType
func AppProtocol(proto ProtocolType) *string {
if proto == ProtocolH2C {
return &AppProtocolH2C
}
return nil
}
23 changes: 23 additions & 0 deletions pkg/apis/networking/ports_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,26 @@ func TestServicePort(t *testing.T) {
})
}
}

func TestAppProtocol(t *testing.T) {
cases := []struct {
name string
proto ProtocolType
expect *string
}{{
name: "pass h2c protocol to get Serving and Activator K8s services for HTTP/2 endpoints",
proto: ProtocolH2C,
expect: &AppProtocolH2C,
}, {
name: "other protocols result in nil",
proto: ProtocolHTTP1,
expect: nil,
}}
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
if got, want := AppProtocol(c.proto), c.expect; !(got == want) {
KauzClay marked this conversation as resolved.
Show resolved Hide resolved
t.Errorf("got = %d, want: %d", got, want)
}
})
}
}
Loading