Add CSM NEG support. #827

cadmuxe · 2019-08-15T22:00:06Z

No description provided.

k8s-ci-robot · 2019-08-15T22:00:08Z

Welcome @cadmuxe!

It looks like this is your first PR to kubernetes/ingress-gce 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-gce has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2019-08-15T22:00:15Z

Hi @cadmuxe. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cadmuxe · 2019-08-15T22:01:17Z

/cc @freehan

cadmuxe · 2019-08-15T22:01:46Z

This is a draft, need add tests.

rramkumar1 · 2019-08-16T13:38:25Z

/ok-to-test

freehan · 2019-08-16T19:39:15Z

cmd/glbc/main.go

@@ -197,7 +205,7 @@ func runControllers(ctx *ingctx.ControllerContext) {
 	fwc := firewalls.NewFirewallController(ctx, flags.F.NodePortRanges.Values())

 	// TODO: Refactor NEG to use cloud mocks so ctx.Cloud can be referenced within NewController.
-	negController := neg.NewController(negtypes.NewAdapter(ctx.Cloud), ctx, lbc.Translator, ctx.ClusterNamer, flags.F.ResyncPeriod, flags.F.NegGCPeriod, neg.NegSyncerType(flags.F.NegSyncerType), flags.F.EnableReadinessReflector)
+	negController := neg.NewController(negtypes.NewAdapter(ctx.Cloud), ctx, lbc.Translator, ctx.ClusterNamer, flags.F.ResyncPeriod, flags.F.NegGCPeriod, neg.NegSyncerType(flags.F.NegSyncerType), flags.F.EnableReadinessReflector, flags.F.EnableCSM)


refactor neg.NewController to take controller context

Discussed offline, will keep this by now.

freehan · 2019-08-16T19:41:42Z

pkg/annotations/destination_rule.go

+import "encoding/json"
+
+// PortSubsetNegMap is the mapping between service port:subset to NEG name
+type PortSubsetNegMap map[string]string


you probably need map[string]map[string]string?

"cloud.google.com/neg-status": `{ "network_endpoint_groups": { "v1": { "9080": "somehash-default-reviews-v1-9080", } "v2": { "9080": "somehash-default-reviews-v2-9080", } }

freehan · 2019-08-16T19:43:45Z

pkg/utils/namer.go

+// NEGWithSubset returns the gce neg name based on the service namespace, name
+// target port and Istio:DestinationRule subset if provided. NEG naming convention:
+//
+//   {prefix}{version}-{clusterid}-{namespace}-{name}-{service port}[-subset]-{hash}


{prefix}{version}-{clusterid}-{namespace}-{name}-{subset}-{service port}-{hash}?

consider spliting the namer NegForServicePort and NegForDestinationRule

Address code review feedback. change the neg status to map[string]map[string]string

freehan · 2019-08-21T23:43:17Z

pkg/neg/controller.go

@@ -544,3 +656,53 @@ func getIngressServicesFromStore(store cache.Store, svc *apiv1.Service) (ings []
 	}
 	return
 }
+
+func castToDestinationRule(drus *unstructured.Unstructured) (*istioV1alpha3.DestinationRule, error) {


add comment

freehan · 2019-08-21T23:43:26Z

pkg/neg/controller.go

+	return servicePortMap
+}
+
+func getDestinationRulesFromStore(store cache.Store, svc *apiv1.Service) (drs map[namespaceNamePair]*istioV1alpha3.DestinationRule) {


add comment

freehan · 2019-08-23T17:36:38Z

pkg/neg/controller.go

 	// syncTracker tracks the latest time that service and endpoint changes are processed
 	syncTracker utils.TimeTracker

 	// reflector handles NEG readiness gate and conditions for pods in NEG.
 	reflector readiness.Reflector
 }

+type namespaceNamePair struct {


use types.NamespacedName, no need to have a new structure

freehan · 2019-08-23T17:40:17Z

pkg/neg/controller.go

+		}
+		targetServiceNamespace := drUnstructed.GetNamespace()
+		drHost := dr.Host
+		if strings.Contains(dr.Host, ".") {


add an example here

add the link https://github.com/istio/api/blob/1.2.4/networking/v1alpha3/destination_rule.pb.go#L186

freehan · 2019-08-23T18:10:35Z

pkg/neg/controller.go

+		klog.Errorf("Failed to convert informer object to DestinationRule")
+		return
+	}
+	targetServiceNamespace := drus.GetNamespace()


Have a util function for parsing hosts in DestinationRule. Return namespace/name of service

shared the util function with getDestinationRulesFromStore

freehan · 2019-08-23T22:54:11Z

pkg/neg/types/types.go

+	Subset string
+
+	// Subset label, should set together with Subset.
+	SubsetLabels string


why not use map[string]string?

okay. Got it. If it is simpler, then just do it this way.

freehan · 2019-08-23T23:01:55Z

pkg/neg/controller.go

+	if c.enableCSM {
+		// Find all destination rules that using this service.
+		destinationRules := getDestinationRulesFromStore(c.destinationRuleLister, service)
+		servicePorts := gatherPortMappingFromService(service)


IIUC, this only creates NEGs per each (destination rule subsets X service ports)

If c.enableCSM, you also need to create NEGs for each service port. Except for kube-system services and kubernetes.default service

freehan · 2019-08-23T23:09:28Z

pkg/neg/controller.go

+		destinationRules := getDestinationRulesFromStore(c.destinationRuleLister, service)
+		servicePorts := gatherPortMappingFromService(service)
+		for destinationRuleNN, destinationRule := range destinationRules {
+			destinationRulePortInfoMap := negtypes.NewPortInfoMapWithDestinationRule(namespace, name, servicePorts, c.namer, true, destinationRule)


Need to validate if the destinationRule has unique subset names. If not, error out.

I confirmed with the Istio team that they want user to create unique subset names but did not enforce it with validation.

freehan · 2019-08-23T23:10:15Z

pkg/neg/controller.go

@@ -422,6 +472,44 @@ func (c *Controller) syncNegStatusAnnotation(namespace, name string, portMap neg
 	return err
 }

+func (c *Controller) syncDestinationRuleNegStatusAnnotation(namespace, destinationRuleName string, portmap negtypes.PortInfoMap) error {


add comment

freehan · 2019-08-23T23:10:26Z

pkg/neg/controller.go

@@ -463,6 +551,30 @@ func (c *Controller) enqueueIngressServices(ing *v1beta1.Ingress) {
 	}
 }

+func (c *Controller) enqueueDestinationRule(obj interface{}) {


add a comment

freehan · 2019-08-28T21:43:57Z

pkg/context/context.go

@@ -101,6 +108,15 @@ func NewControllerContext(
 		healthChecks:            make(map[string]func() error),
 	}

+	if config.EnableCSM && dynamicClient != nil {
+		destrinationGVR := schema.GroupVersionResource{Group: "networking.istio.io", Version: "v1alpha3", Resource: "destinationrules"}


nit: add a WARNING here.
The destinationRule group version is v1alpha3 in group networking.istio.io. Need to update as istio API graduates.

Done, thanks

freehan · 2019-08-28T21:51:28Z

pkg/neg/controller.go

+		destinationRules := getDestinationRulesFromStore(c.destinationRuleLister, service)
+		// Fill all service ports into portinfomap
+		servicePorts := gatherPortMappingFromService(service)
+		for destinationRuleNN, destinationRule := range destinationRules {


nit: s/destinationRuleNN/namespacedName ?

freehan · 2019-08-28T22:48:14Z

pkg/annotations/destination_rule.go

+type PortSubsetNegMap map[string]map[string]string
+
+type DestinationRuleNEGStatus struct {
+	NetworkEndpointGroups PortSubsetNegMap `json:"network_endpoint_groups,omitempty"`


I mean move the comment from above to here

freehan · 2019-08-28T22:59:53Z

pkg/neg/controller.go

+			}
+		}
+		// If no destionationRules for this service, create one NEG for every ports.
+		if len(destinationRules) == 0 {


You need to create it anyway. Remove the if statement here

freehan · 2019-08-28T23:02:04Z

pkg/neg/controller.go

@@ -324,9 +377,15 @@ func (c *Controller) processService(key string) error {
 		if err = c.syncNegStatusAnnotation(namespace, name, portInfoMap); err != nil {
 			return err
 		}
+		// Merge destinationRule related NEG after the Service NEGStatus Sync, we don't want DR related NEG status go into service.
+		if err := portInfoMap.Merge(csmPortInfoMap); err != nil {


consider flag gating this?

probably not needed

…reated. more comments update the gke-self-managed.sh script to support CSM mode.

freehan

/lgtm
/approve

k8s-ci-robot · 2019-08-29T20:40:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cadmuxe, freehan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [freehan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 15, 2019

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 15, 2019

k8s-ci-robot requested review from bowei and MrHohn August 15, 2019 22:00

k8s-ci-robot requested a review from freehan August 15, 2019 22:01

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 16, 2019

freehan self-assigned this Aug 16, 2019

freehan reviewed Aug 16, 2019

View reviewed changes

cadmuxe added 3 commits August 20, 2019 13:37

update vendor

d5ec1f4

csm neg init

f130722

Address code review feedback. change the neg status to map[string]map[string]string

update tests

4639262

freehan reviewed Aug 23, 2019

View reviewed changes

freehan reviewed Aug 28, 2019

View reviewed changes

CSM mode: create NEGs for all service ports when no destinationrule c…

b8cca63

…reated. more comments update the gke-self-managed.sh script to support CSM mode.

freehan reviewed Aug 29, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 29, 2019

k8s-ci-robot merged commit d793ae8 into kubernetes:master Aug 29, 2019

cadmuxe mentioned this pull request Nov 14, 2019

REQUEST: New membership for cadmuxe kubernetes/org#1416

Closed

6 tasks

Add CSM NEG support. #827

Add CSM NEG support. #827

Conversation

cadmuxe commented Aug 15, 2019

k8s-ci-robot commented Aug 15, 2019

k8s-ci-robot commented Aug 15, 2019

cadmuxe commented Aug 15, 2019

cadmuxe commented Aug 15, 2019

rramkumar1 commented Aug 16, 2019

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 Aug 16, 2019 • edited Loading

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

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

k8s-ci-robot commented Aug 29, 2019

freehan Aug 16, 2019 •

edited

Loading