Skip to content

Commit

Permalink
fix(trait): harmonize service port name
Browse files Browse the repository at this point in the history
As Knative accepts a limited set of names, we can harmonize the default values to use the same accepted names.

Close #5409
  • Loading branch information
squakez committed Apr 24, 2024
1 parent 57149cf commit 9739592
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 11 deletions.
13 changes: 8 additions & 5 deletions pkg/trait/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,12 @@ import (
)

const (
defaultContainerName = "integration"
defaultContainerPort = 8080
defaultContainerPortName = "http"
defaultContainerName = "integration"
defaultContainerPort = 8080
// Knative does not want name=http, it only supports http1 (HTTP/1) and h2c (HTTP/2)
// https://github.com/knative/specs/blob/main/specs/serving/runtime-contract.md#protocols-and-ports
// We can harmonize and keep using the same port name also for any other deployment strategy
defaultContainerPortName = "h2c"
defaultServicePort = 80
containerTraitID = "container"
)
Expand Down Expand Up @@ -266,13 +269,13 @@ func (t *containerTrait) configureService(e *Environment, container *corev1.Cont
if name == "" {
name = defaultContainerPortName
}

containerPort := corev1.ContainerPort{
Name: name,
ContainerPort: int32(t.Port),
Protocol: corev1.ProtocolTCP,
}
if !isKnative {
// Knative does not want name=http
containerPort.Name = name
// The service is managed by Knative, so, we only take care of this when it's managed by us
service := e.Resources.GetServiceForIntegration(e.Integration)
if service != nil {
Expand Down
4 changes: 2 additions & 2 deletions pkg/trait/container_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,7 @@ func TestDeploymentContainerPorts(t *testing.T) {
container := environment.GetIntegrationContainer()
assert.Len(t, container.Ports, 1)
assert.Equal(t, int32(8081), container.Ports[0].ContainerPort)
assert.Equal(t, "http", container.Ports[0].Name)
assert.Equal(t, "h2c", container.Ports[0].Name)
svc := environment.Resources.GetServiceForIntegration(environment.Integration)
assert.Len(t, svc.Spec.Ports, 1)
assert.Equal(t, int32(8081), svc.Spec.Ports[0].Port)
Expand Down Expand Up @@ -653,5 +653,5 @@ func TestKnativeServiceContainerPorts(t *testing.T) {
container := environment.GetIntegrationContainer()
assert.Len(t, container.Ports, 1)
assert.Equal(t, int32(8081), container.Ports[0].ContainerPort)
assert.Equal(t, "", container.Ports[0].Name)
assert.Equal(t, "h2c", container.Ports[0].Name)
}
6 changes: 3 additions & 3 deletions pkg/trait/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,13 +129,13 @@ func TestServiceWithDefaults(t *testing.T) {

assert.Len(t, s.Spec.Ports, 1)
assert.Equal(t, int32(80), s.Spec.Ports[0].Port)
assert.Equal(t, "http", s.Spec.Ports[0].Name)
assert.Equal(t, "http", s.Spec.Ports[0].TargetPort.String())
assert.Equal(t, "h2c", s.Spec.Ports[0].Name)
assert.Equal(t, "h2c", s.Spec.Ports[0].TargetPort.String())

assert.Len(t, d.Spec.Template.Spec.Containers, 1)
assert.Len(t, d.Spec.Template.Spec.Containers[0].Ports, 1)
assert.Equal(t, int32(8080), d.Spec.Template.Spec.Containers[0].Ports[0].ContainerPort)
assert.Equal(t, "http", d.Spec.Template.Spec.Containers[0].Ports[0].Name)
assert.Equal(t, "h2c", d.Spec.Template.Spec.Containers[0].Ports[0].Name)

assert.Empty(t, s.Spec.Type) // empty means ClusterIP
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/trait/trait_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func TestOpenShiftTraitsWithWebAndConfig(t *testing.T) {
assert.NotNil(t, env.GetTrait("service"))
assert.NotNil(t, env.GetTrait("route"))
assert.NotNil(t, res.GetService(func(svc *corev1.Service) bool {
return svc.Name == TestDeploymentName && svc.Spec.Ports[0].TargetPort.StrVal == "http"
return svc.Name == TestDeploymentName && svc.Spec.Ports[0].TargetPort.StrVal == "h2c"
}))
}

Expand Down

0 comments on commit 9739592

Please sign in to comment.