Add annotation for exposing NEGs #284

agau4779 · 2018-05-23T18:32:03Z

This PR adds a new annotation for exposing standalone NEGs on Services without requiring an Ingress.

Additionally:

Refactors NEG naming to use ServicePort Ports instead of TargetPorts, because it is possible to have multiple ServicePorts point to the same TargetPort. Note that ServicePort Ports are always int32, while TargetPorts may be an int or a string.
Pass ServicePort into via a map mapping Port:TargetPort within /pkg/neg (PortNameMap type)
feature flag around exposed NEGs

To do in later PRs:

merge the expose NEG and ingress NEG annotations

agau4779 · 2018-05-23T21:55:37Z

/assign freehan

nicksardo · 2018-05-24T21:30:25Z

/ok-to-test

freehan · 2018-05-24T21:34:59Z

pkg/neg/controller.go

+	glog.V(2).Infof("Applying annotation to service: %s", annotation)
+
+	service.Annotations[negVisibilityAnnotationKey] = annotation
+	c.serviceLister.Update(service)


do not omit the error

freehan · 2018-05-24T21:36:21Z

pkg/neg/controller.go

+		res.NetworkEndpointGroups[port] = c.namer.NEG(namespace, name, port)
+	}
+
+	return fmt.Sprintf("%+v", res), nil


Use a struct to represent the annotation schema. And then use json.Marshal to convert it into json.

freehan · 2018-05-24T21:37:30Z

pkg/annotations/service.go

 	// To enable this feature, the value of the annotation must be "true".
 	// This annotation should be specified on services that are backing ingresses.
 	// WARNING: The feature will NOT be effective in the following circumstances:
 	// 1. NEG feature is not enabled in feature gate.
 	// 2. Service is not referenced in any ingress.
 	// 3. Adding this annotation on ingress.
-	NetworkEndpointGroupAlphaAnnotation = "alpha.cloud.google.com/load-balancer-neg"


let us keep the boolean trigger for now.

Add a separate annotation for expose neg

freehan

need to remove visibility annotation or make it empty when trigger annotations are removed.

freehan · 2018-05-25T22:35:11Z

pkg/annotations/service.go

+// ExposeNegAnnotation is the format of the annotation associated with the
+// cloud.google.com/use-neg key.
+type ExposeNegAnnotation struct {
+	SvcPorts map[string]NegAttributes


You can add json tags: https://eager.io/blog/go-and-json/

freehan · 2018-05-25T22:35:55Z

pkg/annotations/service.go

+	// NEGAnnotation is the annotation key to specify standalone NEGs associated
+	// with the service. This should be a valid JSON string, for example:
+	// {"SvcPorts":{"80":{"Enabled":true}}}
+	NEGAnnotation = "cloud.google.com/use-neg"


expose-load-balancer-neg

freehan · 2018-05-25T22:36:58Z

pkg/annotations/service.go

-func (svc Service) NEGEnabled() bool {
+// NEGIngress returns true if the annotation is to be applied on
+// Ingress-referenced ports
+func (svc Service) NEGIngress() bool {
 	v, ok := svc.v[NetworkEndpointGroupAlphaAnnotation]


change to something like:

NEGEnabledForIngress

freehan · 2018-05-25T22:37:10Z

pkg/annotations/service.go

 	v, ok := svc.v[NetworkEndpointGroupAlphaAnnotation]
 	return ok && v == "true"
 }

+// NEGEnabled is true if the service uses NEGs.
+func (svc Service) NEGEnabled() bool {


freehan · 2018-05-25T22:37:48Z

pkg/annotations/service.go

+
+	var portMap ExposeNegAnnotation
+	if err := json.Unmarshal([]byte(v), &portMap); err != nil {
+		return nil, fmt.Errorf("NEG annotation %s is not well-formed", v)


Add a TODO or something to point to the documentation.

freehan · 2018-05-25T22:39:07Z

pkg/neg/controller.go

@@ -265,6 +294,30 @@ func (c *Controller) synced() bool {
 		c.ingressSynced()
 }

+type negSvcState struct {


Same here: use json tag

Use json tag here.

negServiceState

freehan · 2018-05-25T22:40:08Z

pkg/annotations/service.go

@@ -40,6 +41,11 @@ const (
 	// 3. Adding this annotation on ingress.
 	NetworkEndpointGroupAlphaAnnotation = "alpha.cloud.google.com/load-balancer-neg"


cloud.google.com/use-load-balancer-neg

this was the original annotation for NEG + Ingress - is it ok to change it?

yes. it is okay

we should change to beta?

note that if we put beta, it will stay forever right now (new deprecation rules)

bowei · 2018-05-25T22:40:35Z

/assign

freehan · 2018-05-29T19:16:27Z

pkg/annotations/service.go

+
+	// NEGAnnotation is the annotation key to specify standalone NEGs associated
+	// with the service. This should be a valid JSON string, for example:
+	// {"service_ports":{"80":{"enabled":true}}}


Let us do this:

{"service_ports":{"80":{}}}

freehan · 2018-05-29T19:17:11Z

pkg/annotations/service.go

@@ -57,6 +63,18 @@ const (
 	ProtocolHTTP2 AppProtocol = "HTTP2"
 )

+// ExposeNegAnnotation is the format of the annotation associated with the
+// NEGAnnotation key.
+type ExposeNegAnnotation struct {


Make it empty for now. Leave a comment to say there will be future extensions in this struct

freehan · 2018-05-30T23:26:52Z

pkg/neg/controller.go

+		if err == nil {
+			svcPorts = svcPorts.Union(negSvcPorts)
+		} else {
+			glog.Warning("Failed to parse %v annotation on Service, err: %v. Ignoring the annotation.", annotations.NEGAnnotation, err)


use event recorder to record and error event to the service object

Also check if the service port actually exists. If not, record an event.

Add a TODO to say that will move the validation logic to validation webhook.

freehan · 2018-05-30T23:26:57Z

pkg/neg/controller.go

@@ -265,6 +294,30 @@ func (c *Controller) synced() bool {
 		c.ingressSynced()
 }

+type negSvcState struct {


Use json tag here.

freehan · 2018-05-30T23:36:03Z

pkg/annotations/service.go

+// ExposeNegAnnotation is the format of the annotation associated with the
+// NEGAnnotation key. ServicePorts present in this map will be NEG-enabled.
+type ExposeNegAnnotation struct {
+	SvcPorts map[string]NegAttributes `json:"service_ports,omitempty"`


Can we get rid of the service_ports key?

"cloud.google.com/expose-load-balancer-neg": "{"80":{}, "8080": {}}"

bowei · 2018-05-30T23:22:01Z

pkg/annotations/service.go

+	NetworkEndpointGroupAlphaAnnotation = "cloud.google.com/use-load-balancer-neg"
+
+	// NEGAnnotation is the annotation key to specify standalone NEGs associated
+	// with the service. This should be a valid JSON string, for example:


can you put where the actual schema struct is defined?

bowei · 2018-06-01T17:46:07Z

pkg/annotations/service.go

-func (svc Service) NEGEnabled() bool {
+// NEGEnabledForIngress returns true if the annotation is to be applied on
+// Ingress-referenced ports
+func (svc Service) NEGEnabledForIngress() bool {


is using this as a value type on purpose? (svc *Service)

bowei · 2018-06-01T17:46:44Z

pkg/annotations/service.go

+// NEGExposed is true if the service exposes NEGs
+func (svc Service) NEGExposed() bool {
+	v, ok := svc.v[NEGAnnotation]
+	return ok && len(v) > 0


wouldn't len(v) be == 0 if it didn't exist?

bowei · 2018-06-01T17:47:11Z

pkg/annotations/service.go

+		return nil, fmt.Errorf("No NEG ServicePorts specified")
+	}
+
+	// TODO: add link to documentation


bowei · 2018-06-01T17:47:43Z

pkg/annotations/service.go

+	// TODO: add link to documentation
+	var portMap ExposeNegAnnotation
+	if err := json.Unmarshal([]byte(v), &portMap); err != nil {
+		return nil, fmt.Errorf("NEG annotation %s is not well-formed", v)


...well-formed: %v", v, err

might as well report the error so people know how to fix

bowei · 2018-06-01T17:50:35Z

pkg/neg/controller.go

+	if annotations.FromService(service).NEGExposed() {
+		negSvcPorts, err := annotations.FromService(service).NEGServicePorts()
+		if err == nil {
+			knownPorts := sets.NewString()


This feels like it should be a helper function.

bowei · 2018-06-01T17:51:10Z

pkg/neg/controller.go

+		negSvcPorts, err := annotations.FromService(service).NEGServicePorts()
+		if err == nil {
+			knownPorts := sets.NewString()
+			for _, sp := range service.Spec.Ports {


This block that collects the service ports should be a helper function and unit tested

bowei · 2018-06-01T17:51:37Z

pkg/neg/controller.go

+
+	if len(svcPorts) > 0 {
+		annotation, err := c.negVisibilityAnnotation(namespace, name, svcPorts.List())
+		service.Annotations[negVisibilityAnnotationKey] = annotation


I would like this to be part of the annotations pkg to keep annotation related code together

bowei · 2018-06-01T17:51:59Z

pkg/neg/controller.go

@@ -265,6 +294,30 @@ func (c *Controller) synced() bool {
 		c.ingressSynced()
 }

+type negSvcState struct {


negServiceState

bowei · 2018-06-01T17:52:35Z

pkg/neg/controller.go

+// associated with the given ports.
+// NetworkEndpointGroups is a mapping between ServicePort and NEG name
+// Zones is a list of zones where the NEGs exist.
+func (c *Controller) negVisibilityAnnotation(namespace, name string, ports []string) (string, error) {


move the logic to annotations

…tPort mapping.

freehan

rename the pkg/neg/annotation.go to pkg/neg/utils.go

freehan · 2018-06-19T22:09:41Z

pkg/flags/flags.go

-	NEG:   true,
+	Http2:      true,
+	NEG:        true,
+	NEGExposed: true,


default to false?

freehan · 2018-06-19T22:10:58Z

pkg/annotations/service.go

+
+	// NEGStatusKey is the annotation key whose value is the status of the NEGs
+	// on the Service, and is applied by the NEG Controller.
+	NEGStatusKey = "cloud.google.com/neg"


cloud.google.com/neg-status

freehan · 2018-06-19T22:11:14Z

pkg/annotations/service.go

+	// with the service. This should be a valid JSON string, as defined in
+	// ExposeNegAnnotation.
+	// example: {"80":{},"443":{}}
+	ExposeNEGAnnotationKey = "cloud.google.com/expose-load-balancer-neg"


cloud.google.com/neg

freehan · 2018-06-19T22:12:07Z

pkg/annotations/service.go

+
+// NegAttributes houses the attributes of the NEGs that are associated with the
+// service. Future extensions to the Expose NEGs annotation should be added here.
+type NegAttributes struct{}


Name string

freehan · 2018-06-19T22:14:28Z

pkg/neg/annotations.go

+
+// NegServiceState contains name and zone of the Network Endpoint Group
+// resources associated with this service
+type NegServiceState struct {


rename to NegStatus

freehan · 2018-06-19T22:14:39Z

pkg/neg/annotations.go

+// associated with the given ports.
+// NetworkEndpointGroups is a mapping between ServicePort and NEG name
+// Zones is a list of zones where the NEGs exist.
+func GenNegServiceState(zones []string, portToNegs PortNameMap) NegServiceState {


GetNegStatus

freehan · 2018-06-19T22:23:45Z

pkg/neg/controller_test.go

+	testCases := []struct {
+		desc            string
+		previousPortMap PortNameMap
+		portMap         PortNameMap


add a boolean speicify expect service update or not

let me find an example for you.

freehan · 2018-06-20T00:05:15Z

/lgtm

freehan · 2018-06-20T00:09:34Z

I0620 00:05:56.000] Checking go vet: FAIL
I0620 00:05:56.000] # k8s.io/ingress-gce/pkg/annotations
I0620 00:05:56.000] pkg/annotations/service.go:81: struct field tag `json:name,omitempty` not compatible with reflect.StructTag.Get: bad syntax for struct tag value
I0620 00:05:56.001] 
I0620 00:05:56.415] build/rules.mk:188: recipe for target 'test' failed
W0620 00:05:56.421] make: *** [test] Error 1

freehan · 2018-06-20T17:37:07Z

/lgtm

Automatic merge from submit-queue (batch tested with PRs 65338, 64535). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. [GCE] e2e test for expose neg on gce ingress **What this PR does / why we need it**: - Adds e2e test for the expose NEG annotation (which allows for standalone NEGs) **Special notes for your reviewer**: Note, kubernetes/ingress-gce#350 must be merged first before this is merged. `[Unreleased]` tag is on this PR because it depends on code from kubernetes/ingress-gce#350 and kubernetes/ingress-gce#284 being in an Ingress release. Will update this test and test-infra once this is released in the next Ingress. **Release note**: ```release-note NONE ```

agau4779 force-pushed the expose-negs branch from 9a07676 to 82b268e Compare May 23, 2018 20:26

agau4779 changed the title ~~[WIP] add annotation for expose NEGs~~ Add annotation for exposing NEGs May 23, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 23, 2018

k8s-ci-robot assigned freehan May 23, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 24, 2018

freehan reviewed May 24, 2018

View reviewed changes

agau4779 force-pushed the expose-negs branch 2 times, most recently from 4a83279 to 80b03d8 Compare May 25, 2018 18:50

freehan reviewed May 25, 2018

View reviewed changes

k8s-ci-robot assigned bowei May 25, 2018

agau4779 force-pushed the expose-negs branch 2 times, most recently from 70f9758 to 6a2cd8d Compare May 29, 2018 16:24

freehan reviewed May 29, 2018

View reviewed changes

agau4779 force-pushed the expose-negs branch 2 times, most recently from f57641d to 9dfff4c Compare May 29, 2018 20:49

agau4779 mentioned this pull request May 30, 2018

[GCE] e2e test for expose neg on gce ingress kubernetes/kubernetes#64535

Merged

freehan reviewed May 30, 2018

View reviewed changes

agau4779 force-pushed the expose-negs branch 2 times, most recently from 27998a4 to a4aa68a Compare May 31, 2018 22:51

bowei reviewed Jun 1, 2018

View reviewed changes

agau4779 force-pushed the expose-negs branch 2 times, most recently from 73252dc to 85f09ef Compare June 1, 2018 22:24

agau4779 added 6 commits June 19, 2018 10:31

name NEGs and NEG backendservices with ServicePort.Port

306ccdf

enable standalone NEG annotation. represent ServicePort by Port:Targe…

d5babfd

…tPort mapping.

tests

cc7fe20

SvcPort->Port, SvcTargetPort->TargetPort. Address PR comments.

3d36e23

additional tests

769948d

add feature flag around exposed negs

0b7923f

agau4779 force-pushed the expose-negs branch from dfec099 to 531a794 Compare June 19, 2018 19:32

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 19, 2018

agau4779 mentioned this pull request Jun 19, 2018

[GCE] update GCE Neg naming scheme in e2e test kubernetes/kubernetes#65235

Closed

move expose NEG logic into NEG package

c89571a

agau4779 force-pushed the expose-negs branch from 531a794 to c89571a Compare June 19, 2018 21:02

freehan reviewed Jun 19, 2018

View reviewed changes

rename variables

85b5462

agau4779 force-pushed the expose-negs branch 3 times, most recently from 0a7c879 to 4bb2b9c Compare June 20, 2018 00:03

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 20, 2018

agau4779 added 2 commits June 20, 2018 09:21

add todos

b511933

annotations -> utils.go

94e385f

agau4779 force-pushed the expose-negs branch from 4bb2b9c to 94e385f Compare June 20, 2018 16:22

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 20, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 20, 2018

freehan merged commit 3722e74 into kubernetes:master Jun 20, 2018

agau4779 mentioned this pull request Jun 20, 2018

merge Ingress NEG annotation and Expose NEG annotation #350

Merged

agau4779 deleted the expose-negs branch June 20, 2018 17:51

		@@ -40,6 +41,11 @@ const (
		// 3. Adding this annotation on ingress.
		NetworkEndpointGroupAlphaAnnotation = "alpha.cloud.google.com/load-balancer-neg"

Add annotation for exposing NEGs #284

Add annotation for exposing NEGs #284

Conversation

agau4779 commented May 23, 2018 • edited Loading

agau4779 commented May 23, 2018

nicksardo commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freehan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowei commented May 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freehan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freehan commented Jun 20, 2018

freehan commented Jun 20, 2018

freehan commented Jun 20, 2018

agau4779 commented May 23, 2018 •

edited

Loading