From 5aadd6ecae3cf7d3bce829da2ce3a526abbb61a9 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Sun, 1 Mar 2015 13:34:57 -0800 Subject: [PATCH] Change the v1b3 default for service ContainerPort In v1b1 and v1b2 we choose the "first defined port" if you do not specify a ContainerPort. I am proposing that v1b3 just assume the ContainerPort is the same as the service port unless explicitly provided. This leaves named ports for now, but that is under discussion on its own. This is strictly compatible, though to implement this we have to leave the internal objects with the looser behavior until v1b[12] die. This also adds a link dependency so that when we DO kill v1b[12] the endpoints controller will blow up, prompting a fix. --- pkg/api/testing/fuzzer.go | 21 +++++++++++++++++++++ pkg/api/types.go | 2 ++ pkg/api/v1beta1/register.go | 6 ++++++ pkg/api/v1beta2/register.go | 6 ++++++ pkg/api/v1beta3/defaults.go | 6 ++++++ pkg/api/v1beta3/defaults_test.go | 17 +++++++++++++++++ pkg/api/v1beta3/types.go | 2 +- pkg/service/endpoints_controller.go | 6 ++++++ 8 files changed, 65 insertions(+), 1 deletion(-) diff --git a/pkg/api/testing/fuzzer.go b/pkg/api/testing/fuzzer.go index 2ed41c0cbbf..6c9e6ae3d61 100644 --- a/pkg/api/testing/fuzzer.go +++ b/pkg/api/testing/fuzzer.go @@ -245,6 +245,27 @@ func FuzzerFor(t *testing.T, version string, src rand.Source) *fuzz.Fuzzer { c.Fuzz(&http.Port) c.Fuzz(&http.Host) }, + func(ss *api.ServiceSpec, c fuzz.Continue) { + // TODO: I wish I could say "fuzz myself but without the custom fuzz-func" + c.Fuzz(&ss.Port) + c.Fuzz(&ss.Protocol) + c.Fuzz(&ss.Selector) + c.Fuzz(&ss.PortalIP) + c.Fuzz(&ss.CreateExternalLoadBalancer) + c.Fuzz(&ss.PublicIPs) + c.Fuzz(&ss.SessionAffinity) + // TODO: would be great if types could voluntarily fuzz themselves. + kinds := []util.IntstrKind{util.IntstrInt, util.IntstrString} + ss.ContainerPort.Kind = kinds[c.Rand.Intn(len(kinds))] + switch ss.ContainerPort.Kind { + case util.IntstrInt: + ss.ContainerPort.IntVal = 1 + c.Rand.Intn(65535) // non-zero + ss.ContainerPort.StrVal = "" // needed because we reuse objects without zeroing them + case util.IntstrString: + ss.ContainerPort.StrVal = "x" + c.RandString() // non-empty + ss.ContainerPort.IntVal = 0 // needed because we reuse objects without zeroing them + } + }, ) return f } diff --git a/pkg/api/types.go b/pkg/api/types.go index 9634ec5ed06..bd1eb008cbb 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -724,6 +724,8 @@ type ServiceSpec struct { // ContainerPort is the name or number of the port on the container to direct traffic to. // This is useful if the containers the service points to have multiple open ports. // Optional: If unspecified, the first port on the container will be used. + // As of v1beta3 this field will become required in the internal API, + // and the versioned APIs must provide a default value. ContainerPort util.IntOrString `json:"containerPort,omitempty"` // Required: Supports "ClientIP" and "None". Used to maintain session affinity. diff --git a/pkg/api/v1beta1/register.go b/pkg/api/v1beta1/register.go index f61f806f08a..d98c038230c 100644 --- a/pkg/api/v1beta1/register.go +++ b/pkg/api/v1beta1/register.go @@ -24,6 +24,12 @@ import ( // Codec encodes internal objects to the v1beta1 scheme var Codec = runtime.CodecFor(api.Scheme, "v1beta1") +// Dependency does nothing but give a hook for other packages to force a +// compile-time error when this API version is eventually removed. This is +// useful, for example, to clean up things that are implicitly tied to +// semantics of older APIs. +const Dependency = true + func init() { api.Scheme.AddKnownTypes("v1beta1", &Pod{}, diff --git a/pkg/api/v1beta2/register.go b/pkg/api/v1beta2/register.go index 990aa7b2039..39612227928 100644 --- a/pkg/api/v1beta2/register.go +++ b/pkg/api/v1beta2/register.go @@ -24,6 +24,12 @@ import ( // Codec encodes internal objects to the v1beta2 scheme var Codec = runtime.CodecFor(api.Scheme, "v1beta2") +// Dependency does nothing but give a hook for other packages to force a +// compile-time error when this API version is eventually removed. This is +// useful, for example, to clean up things that are implicitly tied to +// semantics of older APIs. +const Dependency = true + func init() { api.Scheme.AddKnownTypes("v1beta2", &Pod{}, diff --git a/pkg/api/v1beta3/defaults.go b/pkg/api/v1beta3/defaults.go index d90a36f4fb0..543ee228b4a 100644 --- a/pkg/api/v1beta3/defaults.go +++ b/pkg/api/v1beta3/defaults.go @@ -90,5 +90,11 @@ func init() { obj.Path = "/" } }, + func(obj *ServiceSpec) { + if obj.ContainerPort.Kind == util.IntstrInt && obj.ContainerPort.IntVal == 0 || + obj.ContainerPort.Kind == util.IntstrString && obj.ContainerPort.StrVal == "" { + obj.ContainerPort = util.NewIntOrStringFromInt(obj.Port) + } + }, ) } diff --git a/pkg/api/v1beta3/defaults_test.go b/pkg/api/v1beta3/defaults_test.go index be2e3d5aab0..4ddce887145 100644 --- a/pkg/api/v1beta3/defaults_test.go +++ b/pkg/api/v1beta3/defaults_test.go @@ -22,6 +22,7 @@ import ( current "github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta3" "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" + "github.com/GoogleCloudPlatform/kubernetes/pkg/util" ) func roundTrip(t *testing.T, obj runtime.Object) runtime.Object { @@ -112,3 +113,19 @@ func TestSetDefaulEndpointsProtocol(t *testing.T) { t.Errorf("Expected protocol %s, got %s", current.ProtocolTCP, out.Protocol) } } + +func TestSetDefaulServiceDestinationPort(t *testing.T) { + in := ¤t.Service{Spec: current.ServiceSpec{Port: 1234}} + obj := roundTrip(t, runtime.Object(in)) + out := obj.(*current.Service) + if out.Spec.ContainerPort.Kind != util.IntstrInt || out.Spec.ContainerPort.IntVal != 1234 { + t.Errorf("Expected ContainerPort to be defaulted, got %s", out.Spec.ContainerPort) + } + + in = ¤t.Service{Spec: current.ServiceSpec{Port: 1234, ContainerPort: util.NewIntOrStringFromInt(5678)}} + obj = roundTrip(t, runtime.Object(in)) + out = obj.(*current.Service) + if out.Spec.ContainerPort.Kind != util.IntstrInt || out.Spec.ContainerPort.IntVal != 5678 { + t.Errorf("Expected ContainerPort to be unchanged, got %s", out.Spec.ContainerPort) + } +} diff --git a/pkg/api/v1beta3/types.go b/pkg/api/v1beta3/types.go index ced413ee971..30787a3adb0 100644 --- a/pkg/api/v1beta3/types.go +++ b/pkg/api/v1beta3/types.go @@ -747,7 +747,7 @@ type ServiceSpec struct { // ContainerPort is the name or number of the port on the container to direct traffic to. // This is useful if the containers the service points to have multiple open ports. - // Optional: If unspecified, the first port on the container will be used. + // Optional: If unspecified, the service port is used (an identity map). ContainerPort util.IntOrString `json:"containerPort,omitempty"` // Optional: Supports "ClientIP" and "None". Used to maintain session affinity. diff --git a/pkg/service/endpoints_controller.go b/pkg/service/endpoints_controller.go index c776b53ec7f..a5e24076b34 100644 --- a/pkg/service/endpoints_controller.go +++ b/pkg/service/endpoints_controller.go @@ -21,6 +21,8 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta1" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta2" "github.com/GoogleCloudPlatform/kubernetes/pkg/client" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/GoogleCloudPlatform/kubernetes/pkg/util" @@ -65,6 +67,10 @@ func (e *EndpointController) SyncServiceEndpoints() error { endpoints := []api.Endpoint{} for _, pod := range pods.Items { + // TODO: Once v1beta1 and v1beta2 are EOL'ed, this can + // assume that service.Spec.ContainerPort is populated. + _ = v1beta1.Dependency + _ = v1beta2.Dependency port, err := findPort(&pod, service.Spec.ContainerPort) if err != nil { glog.Errorf("Failed to find port for service %s/%s: %v", service.Namespace, service.Name, err)