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.

Sign up for GitHub

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

Allow user-defined ports on Configuration #2642

Merged
merged 1 commit into from
Dec 18, 2018
Merged

Allow user-defined ports on Configuration #2642

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 6 additions & 4 deletions cmd/queue/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ var (
servingRevision string
servingRevisionKey string
servingAutoscaler string
servingAutoscalerPort string
servingAutoscalerPort int
userTargetPort int
containerConcurrency int
revisionTimeoutSeconds int
statChan = make(chan *autoscaler.Stat, statReportingQueueLength)
Expand All @@ -89,9 +90,10 @@ func initEnv() {
servingNamespace = util.GetRequiredEnvOrFatal("SERVING_NAMESPACE", logger)
servingRevision = util.GetRequiredEnvOrFatal("SERVING_REVISION", logger)
servingAutoscaler = util.GetRequiredEnvOrFatal("SERVING_AUTOSCALER", logger)
servingAutoscalerPort = util.GetRequiredEnvOrFatal("SERVING_AUTOSCALER_PORT", logger)
servingAutoscalerPort = util.MustParseIntEnvOrFatal("SERVING_AUTOSCALER_PORT", logger)
containerConcurrency = util.MustParseIntEnvOrFatal("CONTAINER_CONCURRENCY", logger)
revisionTimeoutSeconds = util.MustParseIntEnvOrFatal("REVISION_TIMEOUT_SECONDS", logger)
userTargetPort = util.MustParseIntEnvOrFatal("USER_PORT", logger)

// TODO(mattmoor): Move this key to be in terms of the KPA.
servingRevisionKey = autoscaler.NewKpaKey(servingNamespace, servingRevision)
Expand Down Expand Up @@ -261,7 +263,7 @@ func main() {
zap.String(logkey.Key, servingRevisionKey),
zap.String(logkey.Pod, podName))

target, err := url.Parse("http://localhost:8080")
target, err := url.Parse(fmt.Sprintf("http://localhost:%d", userTargetPort))
if err != nil {
logger.Fatal("Failed to parse localhost url", zap.Error(err))
}
Expand Down Expand Up @@ -299,7 +301,7 @@ func main() {
}()

// Open a websocket connection to the autoscaler
autoscalerEndpoint := fmt.Sprintf("ws://%s.%s:%s", servingAutoscaler, system.Namespace, servingAutoscalerPort)
autoscalerEndpoint := fmt.Sprintf("ws://%s.%s:%d", servingAutoscaler, system.Namespace, servingAutoscalerPort)
logger.Infof("Connecting to autoscaler at %s", autoscalerEndpoint)
statSink = websocket.NewDurableSendingConnection(autoscalerEndpoint)
go statReporter()
Expand Down
10 changes: 10 additions & 0 deletions pkg/apis/serving/v1alpha1/revision_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,16 @@ const (
RevisionContainerConcurrencyMax RevisionContainerConcurrencyType = 1000
)

const (
// UserPortName is the name that will be used for the Port on the
// Deployment and Pod created by a Revision. This name will be set regardless of if
// a user specifies a port or the default value is chosen.
UserPortName = "user-port"
// DefaultUserPort is the default port value the QueueProxy will
// use for connecting to the user container.
DefaultUserPort = 8080
)

// RevisionSpec holds the desired state of the Revision (from the client).
type RevisionSpec struct {
// TODO: Generation does not work correctly with CRD. They are scrubbed
Expand Down
68 changes: 65 additions & 3 deletions pkg/apis/serving/v1alpha1/revision_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,6 @@ func validateContainer(container corev1.Container) *apis.FieldError {
if container.Name != "" {
ignoredFields = append(ignoredFields, "name")
}
if len(container.Ports) > 0 {
ignoredFields = append(ignoredFields, "ports")
}
if len(container.VolumeMounts) > 0 {
ignoredFields = append(ignoredFields, "volumeMounts")
}
Expand All @@ -144,6 +141,9 @@ func validateContainer(container corev1.Container) *apis.FieldError {
// Complain about all ignored fields so that user can remove them all at once.
errs = errs.Also(apis.ErrDisallowedFields(ignoredFields...))
}
if err := validateContainerPorts(container.Ports); err != nil {
errs = errs.Also(err.ViaField("ports"))
}
// Validate our probes
if err := validateProbe(container.ReadinessProbe).ViaField("readinessProbe"); err != nil {
errs = errs.Also(err)
Expand All @@ -162,6 +162,68 @@ func validateContainer(container corev1.Container) *apis.FieldError {
return errs
}

func validateContainerPorts(ports []corev1.ContainerPort) *apis.FieldError {
if len(ports) == 0 {
return nil
}

var errs *apis.FieldError

// user can set container port which names "user-port" to define application's port.
// Queue-proxy will use it to send requests to application
// if user didn't set any port, it will set default port user-port=8080.
if len(ports) > 1 {
errs = errs.Also(&apis.FieldError{
Message: "More than one container port is set",
Paths: []string{apis.CurrentField},
Details: "Only a single port is allowed",
})
}

userPort := ports[0]
// Only allow empty (defaulting to "TCP") or explicit TCP for protocol
if userPort.Protocol != "" && userPort.Protocol != corev1.ProtocolTCP {
errs = errs.Also(apis.ErrInvalidValue(string(userPort.Protocol), "Protocol"))
}

// Don't allow HostIP or HostPort to be set
var disallowedFields []string
if userPort.HostIP != "" {
disallowedFields = append(disallowedFields, "HostIP")

}
if userPort.HostPort != 0 {
disallowedFields = append(disallowedFields, "HostPort")
}
if len(disallowedFields) != 0 {
errs = errs.Also(apis.ErrDisallowedFields(disallowedFields...))
}

if userPort.ContainerPort < 1 || userPort.ContainerPort > 65535 {
errs = errs.Also(apis.ErrOutOfBoundsValue(strconv.Itoa(int(userPort.ContainerPort)), "1", "65535", "ContainerPort"))
}

// The port is named "user-port" on the deployment, but a user cannot set an arbitrary name on the port
// in Configuration. The name field is reserved for content-negotiation. Currently 'h2c' and 'http1' are
// allowed.
// https://github.com/knative/serving/blob/master/docs/runtime-contract.md#inbound-network-connectivity
validPortNames := map[string]bool{
"h2c": true,
"http1": true,
"": true,
}

if !validPortNames[userPort.Name] {
errs = errs.Also(&apis.FieldError{
Message: fmt.Sprintf("Port name %v is not allowed", ports[0].Name),
Paths: []string{apis.CurrentField},
Details: "Name must be empty, or one of: 'h2c', 'http1'",
})
}

return errs
}

func validateBuildRef(buildRef *corev1.ObjectReference) *apis.FieldError {
if buildRef == nil {
return nil
Expand Down
131 changes: 123 additions & 8 deletions pkg/apis/serving/v1alpha1/revision_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,15 +77,134 @@ func TestContainerValidation(t *testing.T) {
},
want: nil,
}, {
name: "has ports",
name: "has no container ports set",
c: corev1.Container{
Image: "foo",
Ports: []corev1.ContainerPort{},
},
want: nil,
}, {
name: "has valid user port http1",
c: corev1.Container{
Image: "foo",
Ports: []corev1.ContainerPort{{
Name: "http",
Name: "http1",
ContainerPort: 8081,
}},
},
want: nil,
}, {
name: "has valid user port h2c",
c: corev1.Container{
Image: "foo",
Ports: []corev1.ContainerPort{{
Name: "h2c",
ContainerPort: 8081,
}},
},
want: nil,
}, {
name: "has more than one ports with valid names",
c: corev1.Container{
Image: "foo",
Ports: []corev1.ContainerPort{{
Name: "h2c",
ContainerPort: 8080,
}, {
Name: "http1",
ContainerPort: 8181,
}},
},
want: apis.ErrDisallowedFields("ports"),
want: &apis.FieldError{
Message: "More than one container port is set",
Paths: []string{"ports"},
Details: "Only a single port is allowed",
},
}, {
name: "has container port value too large",
c: corev1.Container{
Image: "foo",
Ports: []corev1.ContainerPort{{
ContainerPort: 65536,
}},
},
want: apis.ErrOutOfBoundsValue("65536", "1", "65535", "ports.ContainerPort"),
}, {
name: "has an empty port set",
c: corev1.Container{
Image: "foo",
Ports: []corev1.ContainerPort{{}},
},
want: apis.ErrOutOfBoundsValue("0", "1", "65535", "ports.ContainerPort"),
}, {
name: "has more than one unnamed port",
c: corev1.Container{
Image: "foo",
Ports: []corev1.ContainerPort{{
ContainerPort: 8080,
}, {
ContainerPort: 8181,
Copy link
Member

Choose a reason for hiding this comment

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

What happens if this second ContainerPort is empty? I think it's still an error to have:

Ports: []corev1.ContainerPort{{}, {}}

Copy link
Author

Choose a reason for hiding this comment

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

I have improved validation and cleaned up the tests including a test to cover empty ports in the array.

I currently have it as an error, but I could also be convinced that a ContainerPorts object with a missing ContainerPort value results in the default 8080.

Copy link
Member

Choose a reason for hiding this comment

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

I think a ContainerPorts object with a single missing value results in the default, but two empty ContainerPorts elements suggests selecting two ports, which should still be an error.

}},
},
want: &apis.FieldError{
Message: "More than one container port is set",
Paths: []string{"ports"},
Details: "Only a single port is allowed",
},
}, {
name: "has tcp protocol",
c: corev1.Container{
Image: "foo",
Ports: []corev1.ContainerPort{{
Protocol: corev1.ProtocolTCP,
ContainerPort: 8080,
}},
},
want: nil,
}, {
name: "has invalid protocol",
c: corev1.Container{
Image: "foo",
Ports: []corev1.ContainerPort{{
Protocol: "tdp",
ContainerPort: 8080,
}},
},
want: apis.ErrInvalidValue("tdp", "ports.Protocol"),
}, {
name: "has host port",
c: corev1.Container{
Image: "foo",
Ports: []corev1.ContainerPort{{
HostPort: 80,
ContainerPort: 8080,
}},
},
want: apis.ErrDisallowedFields("ports.HostPort"),
}, {
name: "has host ip",
c: corev1.Container{
Image: "foo",
Ports: []corev1.ContainerPort{{
HostIP: "127.0.0.1",
ContainerPort: 8080,
}},
},
want: apis.ErrDisallowedFields("ports.HostIP"),
}, {
name: "has invalid port name",
c: corev1.Container{
Image: "foo",
Ports: []corev1.ContainerPort{{
Name: "foobar",
ContainerPort: 8080,
Copy link
Member

Choose a reason for hiding this comment

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

Add a test that it's fine to omit ContainerPort altogether?

We should also raise an error if HostIP, HostPort or Protocol is set. (Protocol could be allowed to be TCP.) Feel free to open a separate issue for that fix.

Copy link
Author

Choose a reason for hiding this comment

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

Since I was already making changes I added errors for those fields.

}},
},
want: &apis.FieldError{
Message: fmt.Sprintf("Port name %v is not allowed", "foobar"),
Paths: []string{"ports"},
Details: "Name must be empty, or one of: 'h2c', 'http1'",
},
}, {
name: "has volumeMounts",
c: corev1.Container{
Expand Down Expand Up @@ -152,17 +271,13 @@ func TestContainerValidation(t *testing.T) {
name: "has numerous problems",
c: corev1.Container{
Name: "foo",
Ports: []corev1.ContainerPort{{
Name: "http",
ContainerPort: 8080,
}},
VolumeMounts: []corev1.VolumeMount{{
MountPath: "mount/path",
Name: "name",
}},
Lifecycle: &corev1.Lifecycle{},
},
want: apis.ErrDisallowedFields("name", "ports", "volumeMounts", "lifecycle").Also(
want: apis.ErrDisallowedFields("name", "volumeMounts", "lifecycle").Also(
&apis.FieldError{
Message: "Failed to parse image reference",
Paths: []string{"image"},
Expand Down
2 changes: 0 additions & 2 deletions pkg/reconciler/v1alpha1/revision/resources/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@ const (
// TODO(mattmoor): Make this private once we remove revision_test.go
IstioOutboundIPRangeAnnotation = "traffic.sidecar.istio.io/includeOutboundIPRanges"

userPortName = "user-port"
userPort = 8080
userPortEnvName = "PORT"

autoscalerPort = 8080
Expand Down
50 changes: 34 additions & 16 deletions pkg/reconciler/v1alpha1/revision/resources/deploy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,17 +48,6 @@ var (
MountPath: "/var/log",
}

userPorts = []corev1.ContainerPort{{
Name: userPortName,
ContainerPort: int32(userPort),
}}

// Expose containerPort as env PORT.
userEnv = corev1.EnvVar{
Name: userPortEnvName,
Value: strconv.Itoa(userPort),
}

userResources = corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: userContainerCPU,
Expand All @@ -79,7 +68,7 @@ var (
}
)

func rewriteUserProbe(p *corev1.Probe) {
func rewriteUserProbe(p *corev1.Probe, userPort int) {
if p == nil {
return
}
Expand Down Expand Up @@ -123,19 +112,24 @@ func makePodSpec(rev *v1alpha1.Revision, loggingConfig *logging.Config, observab
// If client provides for some resources, override default values
applyDefaultResources(userResources, &userContainer.Resources)

userContainer.Ports = userPorts
userContainer.VolumeMounts = append(userContainer.VolumeMounts, varLogVolumeMount)
userContainer.Lifecycle = userLifecycle
userContainer.Env = append(userContainer.Env, userEnv)
userPort := getUserPort(rev)
userPortInt := int(userPort)
userPortStr := strconv.Itoa(userPortInt)
// Replacement is safe as only up to a single port is allowed on the Revision
userContainer.Ports = buildContainerPorts(userPort)
userContainer.Env = append(userContainer.Env, buildUserPortEnv(userPortStr))
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to remove any previous env var entries for PORT here?

Copy link
Member

Choose a reason for hiding this comment

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

And/or prefer the PORT env var if the containerPort was not already set?

Copy link
Author

Choose a reason for hiding this comment

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

This currently mimics existing behavior, and I think I am okay with the behavior. Both a user specified PORT and the auto-populated port will be available from the API server when querying the pod or deployment, but the PORT we set should take effect inside the container due to environment variable ordering.

I can't think of a case where you want the environment variable in the container to not be the port where the queue-proxy is sending traffic.

Copy link
Member

Choose a reason for hiding this comment

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

My question was what happens if I have the following in my spec:

spec:
  ...
    containerPort:
      name: http1
    env:
    - name: PORT
      value: 8888

I'm worried that the Deployment pod will look like:

spec:
  ...
    containerPort:
      name: http1
      port: 8080
    env:
    - name: PORT
      value: 8888
    - name: PORT
      value: 8080

Copy link
Author

Choose a reason for hiding this comment

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

In the above example you cannot have a name without a containerPort which should prevent some confusion. However, the bottom part of the example is still possible to have duplicate ports if the user specified an environment variable named PORT.

Whatever we do I would like to have consistent behavior with all environment variables set by Knative ( https://github.com/knative/serving/blob/master/docs/runtime-contract.md#process ).

Lets chat about the behavior we want, and I will open up a new issue to change if needed.

userContainer.Env = append(userContainer.Env, getKnativeEnvVar(rev)...)

// Prefer imageDigest from revision if available
if rev.Status.ImageDigest != "" {
userContainer.Image = rev.Status.ImageDigest
}

// If the client provides probes, we should fill in the port for them.
rewriteUserProbe(userContainer.ReadinessProbe)
rewriteUserProbe(userContainer.LivenessProbe)
rewriteUserProbe(userContainer.ReadinessProbe, userPortInt)
rewriteUserProbe(userContainer.LivenessProbe, userPortInt)

revisionTimeout := int64(rev.Spec.TimeoutSeconds.Duration.Seconds())

Expand All @@ -158,6 +152,30 @@ func makePodSpec(rev *v1alpha1.Revision, loggingConfig *logging.Config, observab
return podSpec
}

func getUserPort(rev *v1alpha1.Revision) int32 {
if len(rev.Spec.Container.Ports) == 1 {
return rev.Spec.Container.Ports[0].ContainerPort
}

Copy link
Member

Choose a reason for hiding this comment

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

Add a TODO to extract EXPOSE information from the container?

Copy link
Author

Choose a reason for hiding this comment

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

Added

//TODO(#2258): Use container EXPOSE metadata from image before falling back to default value

return v1alpha1.DefaultUserPort
}

func buildContainerPorts(userPort int32) []corev1.ContainerPort {
return []corev1.ContainerPort{{
Name: v1alpha1.UserPortName,
ContainerPort: userPort,
}}
}

func buildUserPortEnv(userPort string) corev1.EnvVar {
return corev1.EnvVar{
Name: userPortEnvName,
Value: userPort,
}
}

func MakeDeployment(rev *v1alpha1.Revision,
loggingConfig *logging.Config, networkConfig *config.Network, observabilityConfig *config.Observability,
autoscalerConfig *autoscaler.Config, controllerConfig *config.Controller) *appsv1.Deployment {
Expand Down
Loading