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

Bake backend config into ServicePort #285

Merged
merged 1 commit into from
May 24, 2018
Merged

Bake backend config into ServicePort #285

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
11 changes: 1 addition & 10 deletions pkg/controller/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,16 +185,7 @@ func NewLoadBalancerController(
})
}

var endpointIndexer cache.Indexer
if ctx.EndpointInformer != nil {
endpointIndexer = ctx.EndpointInformer.GetIndexer()
}
lbc.Translator = translator.NewTranslator(lbc.CloudClusterManager.ClusterNamer,
ctx.ServiceInformer.GetIndexer(),
ctx.NodeInformer.GetIndexer(),
ctx.PodInformer.GetIndexer(),
endpointIndexer,
negEnabled)
lbc.Translator = translator.NewTranslator(lbc.CloudClusterManager.ClusterNamer, ctx, negEnabled)
lbc.tlsLoader = &tls.TLSCertsFromSecretsLoader{Client: lbc.client}

glog.V(3).Infof("Created new loadbalancer controller")
Expand Down
55 changes: 30 additions & 25 deletions pkg/controller/translator/translator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,43 +34,34 @@ import (
"k8s.io/client-go/tools/cache"

"k8s.io/ingress-gce/pkg/annotations"
backendconfigv1beta1 "k8s.io/ingress-gce/pkg/apis/backendconfig/v1beta1"
"k8s.io/ingress-gce/pkg/backendconfig"
"k8s.io/ingress-gce/pkg/context"
"k8s.io/ingress-gce/pkg/controller/errors"
"k8s.io/ingress-gce/pkg/loadbalancers"
"k8s.io/ingress-gce/pkg/utils"
)

// NewTranslator returns a new Translator.
func NewTranslator(
namer *utils.Namer,
svcLister cache.Indexer,
nodeLister cache.Indexer,
podLister cache.Indexer,
endpointLister cache.Indexer,
negEnabled bool) *Translator {
func NewTranslator(namer *utils.Namer, ctx *context.ControllerContext, negEnabled bool) *Translator {
return &Translator{
namer,
svcLister,
nodeLister,
podLister,
endpointLister,
negEnabled,
namer: namer,
ctx: ctx,
negEnabled: negEnabled,
}
}

// Translator helps with kubernetes -> gce api conversion.
type Translator struct {
namer *utils.Namer
svcLister cache.Indexer
nodeLister cache.Indexer
podLister cache.Indexer
endpointLister cache.Indexer
negEnabled bool
namer *utils.Namer
ctx *context.ControllerContext
negEnabled bool
Copy link
Contributor

Choose a reason for hiding this comment

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

You can do this in a separate PR, but we can also put a flag that indicates whether NEG is enabled in the ContollerContext.

}

// getServicePort looks in the svc store for a matching service:port,
// and returns the nodeport.
func (t *Translator) getServicePort(id utils.ServicePortID) (*utils.ServicePort, error) {
obj, exists, err := t.svcLister.Get(
obj, exists, err := t.ctx.ServiceInformer.GetIndexer().Get(
&api_v1.Service{
ObjectMeta: meta_v1.ObjectMeta{
Name: id.Service.Name,
Expand Down Expand Up @@ -121,12 +112,26 @@ PortLoop:
return nil, errors.ErrSvcNotNodePort{Service: id.Service}
}

var backendConfig *backendconfigv1beta1.BackendConfig
if t.ctx.BackendConfigInformer != nil {
backendConfigInStore, err := backendconfig.GetBackendConfigForServicePort(t.ctx.BackendConfigInformer.GetIndexer(), svc, port)
if err != nil {
return nil, err
}
if backendConfigInStore != nil {
// Object in cache could be changed in-flight. Deepcopy to
// reduce race conditions.
backendConfig = backendConfigInStore.DeepCopy()
}
}

return &utils.ServicePort{
ID: id,
NodePort: int64(port.NodePort),
Protocol: proto,
SvcTargetPort: port.TargetPort.String(),
NEGEnabled: t.negEnabled && negEnabled,
BackendConfig: backendConfig,
}, nil
}

Expand Down Expand Up @@ -194,7 +199,7 @@ func getZone(n *api_v1.Node) string {

// GetZoneForNode returns the zone for a given node by looking up its zone label.
func (t *Translator) GetZoneForNode(name string) (string, error) {
nodes, err := listers.NewNodeLister(t.nodeLister).ListWithPredicate(utils.NodeIsReady)
nodes, err := listers.NewNodeLister(t.ctx.NodeInformer.GetIndexer()).ListWithPredicate(utils.NodeIsReady)
if err != nil {
return "", err
}
Expand All @@ -211,7 +216,7 @@ func (t *Translator) GetZoneForNode(name string) (string, error) {
// ListZones returns a list of zones this Kubernetes cluster spans.
func (t *Translator) ListZones() ([]string, error) {
zones := sets.String{}
readyNodes, err := listers.NewNodeLister(t.nodeLister).ListWithPredicate(utils.NodeIsReady)
readyNodes, err := listers.NewNodeLister(t.ctx.NodeInformer.GetIndexer()).ListWithPredicate(utils.NodeIsReady)
if err != nil {
return zones.List(), err
}
Expand All @@ -228,7 +233,7 @@ func (t *Translator) getHTTPProbe(svc api_v1.Service, targetPort intstr.IntOrStr

// Lookup any container with a matching targetPort from the set of pods
// with a matching label selector.
pl, err := listPodsBySelector(t.podLister, labels.SelectorFromSet(labels.Set(l)))
pl, err := listPodsBySelector(t.ctx.PodInformer.GetIndexer(), labels.SelectorFromSet(labels.Set(l)))
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -280,7 +285,7 @@ func (t *Translator) GatherEndpointPorts(svcPorts []utils.ServicePort) []string
// For NEG backend, need to open firewall to all endpoint target ports
// TODO(mixia): refactor firewall syncing into a separate go routine with different trigger.
// With NEG, endpoint changes may cause firewall ports to be different if user specifies inconsistent backends.
endpointPorts := listEndpointTargetPorts(t.endpointLister, sp.ID.Service.Namespace, sp.ID.Service.Name, sp.SvcTargetPort)
endpointPorts := listEndpointTargetPorts(t.ctx.EndpointInformer.GetIndexer(), sp.ID.Service.Namespace, sp.ID.Service.Name, sp.SvcTargetPort)
for _, ep := range endpointPorts {
portMap[int64(ep)] = true
}
Expand All @@ -304,7 +309,7 @@ func isSimpleHTTPProbe(probe *api_v1.Probe) bool {

// GetProbe returns a probe that's used for the given nodeport
func (t *Translator) GetProbe(port utils.ServicePort) (*api_v1.Probe, error) {
sl := t.svcLister.List()
sl := t.ctx.ServiceInformer.GetIndexer().List()

// Find the label and target port of the one service with the given nodePort
var service api_v1.Service
Expand Down
34 changes: 16 additions & 18 deletions pkg/controller/translator/translator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,40 +52,37 @@ func fakeTranslator(negEnabled bool) *Translator {
ctx := context.NewControllerContext(client, backendConfigClient, apiv1.NamespaceAll, 1*time.Second, negEnabled)
gce := &Translator{
namer: namer,
svcLister: ctx.ServiceInformer.GetIndexer(),
nodeLister: ctx.NodeInformer.GetIndexer(),
podLister: ctx.PodInformer.GetIndexer(),
ctx: ctx,
negEnabled: negEnabled,
}
if ctx.EndpointInformer != nil {
gce.endpointLister = ctx.EndpointInformer.GetIndexer()
}
return gce
}

func TestTranslateIngress(t *testing.T) {
translator := fakeTranslator(false)

svcLister := translator.ctx.ServiceInformer.GetIndexer()

// default backend
svc := test.NewService(types.NamespacedName{Name: "default-http-backend", Namespace: "kube-system"}, apiv1.ServiceSpec{
Type: apiv1.ServiceTypeNodePort,
Ports: []apiv1.ServicePort{{Name: "http", Port: 80}},
})
translator.svcLister.Add(svc)
svcLister.Add(svc)

// first-service
svc = test.NewService(types.NamespacedName{Name: "first-service", Namespace: "default"}, apiv1.ServiceSpec{
Type: apiv1.ServiceTypeNodePort,
Ports: []apiv1.ServicePort{{Port: 80}},
})
translator.svcLister.Add(svc)
svcLister.Add(svc)

// other-service
svc = test.NewService(types.NamespacedName{Name: "second-service", Namespace: "default"}, apiv1.ServiceSpec{
Type: apiv1.ServiceTypeNodePort,
Ports: []apiv1.ServicePort{{Port: 80}},
})
translator.svcLister.Add(svc)
svcLister.Add(svc)

cases := map[string]struct {
ing *extensions.Ingress
Expand Down Expand Up @@ -172,10 +169,10 @@ func TestGetProbe(t *testing.T) {
{NodePort: 3002, Protocol: annotations.ProtocolHTTPS}: "/foo",
}
for _, svc := range makeServices(nodePortToHealthCheck, apiv1.NamespaceDefault) {
translator.svcLister.Add(svc)
translator.ctx.ServiceInformer.GetIndexer().Add(svc)
}
for _, pod := range makePods(nodePortToHealthCheck, apiv1.NamespaceDefault) {
translator.podLister.Add(pod)
translator.ctx.PodInformer.GetIndexer().Add(pod)
}

for p, exp := range nodePortToHealthCheck {
Expand All @@ -194,12 +191,12 @@ func TestGetProbeNamedPort(t *testing.T) {
{NodePort: 3001, Protocol: annotations.ProtocolHTTP}: "/healthz",
}
for _, svc := range makeServices(nodePortToHealthCheck, apiv1.NamespaceDefault) {
translator.svcLister.Add(svc)
translator.ctx.ServiceInformer.GetIndexer().Add(svc)
}
for _, pod := range makePods(nodePortToHealthCheck, apiv1.NamespaceDefault) {
pod.Spec.Containers[0].Ports[0].Name = "test"
pod.Spec.Containers[0].ReadinessProbe.Handler.HTTPGet.Port = intstr.IntOrString{Type: intstr.String, StrVal: "test"}
translator.podLister.Add(pod)
translator.ctx.PodInformer.GetIndexer().Add(pod)
}
for p, exp := range nodePortToHealthCheck {
got, err := translator.GetProbe(p)
Expand Down Expand Up @@ -244,17 +241,17 @@ func TestGetProbeCrossNamespace(t *testing.T) {
},
},
}
translator.podLister.Add(firstPod)
translator.ctx.PodInformer.GetIndexer().Add(firstPod)
nodePortToHealthCheck := map[utils.ServicePort]string{
{NodePort: 3001, Protocol: annotations.ProtocolHTTP}: "/healthz",
}
for _, svc := range makeServices(nodePortToHealthCheck, apiv1.NamespaceDefault) {
translator.svcLister.Add(svc)
translator.ctx.ServiceInformer.GetIndexer().Add(svc)
}
for _, pod := range makePods(nodePortToHealthCheck, apiv1.NamespaceDefault) {
pod.Spec.Containers[0].Ports[0].Name = "test"
pod.Spec.Containers[0].ReadinessProbe.Handler.HTTPGet.Port = intstr.IntOrString{Type: intstr.String, StrVal: "test"}
translator.podLister.Add(pod)
translator.ctx.PodInformer.GetIndexer().Add(pod)
}

for p, exp := range nodePortToHealthCheck {
Expand Down Expand Up @@ -358,8 +355,9 @@ func TestGatherEndpointPorts(t *testing.T) {
},
}

translator.endpointLister.Add(newDefaultEndpoint(ep1))
translator.endpointLister.Add(newDefaultEndpoint(ep2))
endpointLister := translator.ctx.EndpointInformer.GetIndexer()
endpointLister.Add(newDefaultEndpoint(ep1))
endpointLister.Add(newDefaultEndpoint(ep2))

expected := []string{"80", "8080", "8081"}
got := translator.GatherEndpointPorts(svcPorts)
Expand Down
2 changes: 2 additions & 0 deletions pkg/utils/serviceport.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/ingress-gce/pkg/annotations"
backendconfigv1beta1 "k8s.io/ingress-gce/pkg/apis/backendconfig/v1beta1"
)

// ServicePortID contains the Service and Port fields.
Expand All @@ -39,6 +40,7 @@ type ServicePort struct {
Protocol annotations.AppProtocol
SvcTargetPort string
NEGEnabled bool
BackendConfig *backendconfigv1beta1.BackendConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's necessary to make this a pointer. We never will need to modify it.

Copy link
Member Author

Choose a reason for hiding this comment

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

One idea to make this a pointer is that we can distinguish if a ServicePort has any backendConfig associated with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, we can leave it for now, but we should evaluate whether we really need it. I'd like to avoid having a pointer unless we actually have to modify the object.

Copy link
Member Author

Choose a reason for hiding this comment

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

That sounds good, thanks for the quick review!

}

// Description returns a string describing the ServicePort.
Expand Down