Add V2 frontend namer #892

skmatti · 2019-10-15T00:41:36Z

This part4 of V2 Namer Migration: #858.
Fixes: #537

This PR introduces a new naming scheme for for the following frontend resources:

Forwarding Rule
Target Proxy
URL Map
SSL Certificates
Ingress managed Static IP

This should resolve frontend resource naming collision issues.

The changes made in v2 naming scheme:

New naming scheme:
k8s{version-id}-{resource-prefix}-{8-char-hash-of-kube-system-uid}-{namespace}-{name}-{8-char-hash}
Namespace and Name are truncated evenly until their combined length is 36 in case of overflows. The 8 character suffix hash is created from full namespace and name.

Example http target proxy: k8s2-tp-ksuid123-namespace-name-wddys49o

Resource names will include an 8 character kube-system uid instead of cluster uid. This makes the new naming policy immune to config map(which tracks cluster uid) changes.

Frontend GC workflow changes:

New GC workflow for v2 naming scheme
- GC will be invoked only on the ingress being synced.
- GC workflow will not leak any GCE resources.

GC workflow

If finalizer is disabled
- GC workflow is untouched i.e, run GC for all ingresses
If finalizer is enabled
- If ingress being synced needs cleanup
  - Frontend GC
    - if V1 naming scheme - run frontend GC for all v1 ingresses.
    - if V2 naming scheme - run frontend GC for ingress being synced.
  - Backend GC - run for all ingresses.
  - Ensure delete finalizers
    - if V1 naming scheme - remove finalizers for all v1 ingresses.
    - if V2 naming scheme - remove finalizer for ingress being synced.
- If ingress being synced does not need cleanup
  - Backend GC - run for all ingresses.

k8s-ci-robot · 2019-10-15T00:41:44Z

Hi @skmatti. 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.

skmatti · 2019-10-15T00:42:04Z

/assign @freehan

skmatti · 2019-10-15T16:47:43Z

/assign @bowei

freehan · 2019-10-18T17:11:47Z

/ok-to-test

freehan

more to come

pkg/utils/common/common.go

pkg/controller/controller.go

pkg/utils/common/finalizer.go

pkg/loadbalancers/interfaces.go

pkg/loadbalancers/l7s.go

pkg/controller/controller.go

pkg/loadbalancers/interfaces.go

pkg/sync/interfaces.go

pkg/utils/utils.go

pkg/utils/namer/frontendnamer.go

rramkumar1 · 2019-11-14T19:24:08Z

pkg/utils/namer/frontendnamer.go

+}
+
+// ForwardingRule returns the name of forwarding rule based on given protocol.
+func (vn *V2IngressFrontendNamer) ForwardingRule(protocol NamerProtocol) string {


Please address this.

pkg/utils/namer/frontendnamer.go

rramkumar1 · 2019-11-14T19:24:55Z

pkg/utils/namer/frontendnamer_test.go

+			"%s2-um-ksuid123-012345678901234567-012345678901234567-4mwbp6m5",
+		},
+	}
+	for _, prefix := range []string{"k8s", "mci"} {


Why is "mci" here?

Testing this with different prefixes, it could have been any random word.

I would change it to "arbitrary-prefix" to make that clear.

cmd/glbc/main.go

rramkumar1 · 2019-11-14T19:30:35Z

pkg/test/utils.go

@@ -80,3 +86,27 @@ func DecodeIngress(data []byte) (*v1beta1.Ingress, error) {

 	return obj.(*v1beta1.Ingress), nil
 }
+
+// Flag is a type representing controller flag.
+type Flag string


What is this used for? I don't see it used anywhere in this PR.

This is used for saving flags when overriding them in unit tests. This potentially other tests from using the value set by a test.
Made this variable private as all of its uses are within the file.

bowei · 2019-11-14T21:05:15Z

pkg/controller/controller.go

+
+	var syncErr error
+	// Perform GC as a deferred function.
+	defer func() {


I very much dislike doing complex actions like this in a defer call as it is very hard to understand the code flow (almost like exception handling in C++/Java).

Can we make this a helper function and call it on the appropriate error paths?

We wanted to have a single function to handle all GC paths. But, I guess this makes it very hard to understand. Modified this back to the original workflow.

bowei · 2019-11-14T21:06:25Z

pkg/controller/controller.go

-		lbc.ctx.Recorder(ing.Namespace).Eventf(ing, apiv1.EventTypeWarning, "Translate", msg.Error())
-		return msg
+		err = fmt.Errorf("error while evaluating the ingress spec: %v", utils.JoinErrs(errs))
+		lbc.ctx.Recorder(ing.Namespace).Eventf(ing, apiv1.EventTypeWarning, "Translate", err.Error())


Is there a reason why we don't always emit an event on all error paths?

Add events for GC errors

bowei · 2019-11-14T21:10:58Z

pkg/controller/controller.go

+// ensureFinalizer ensures that a finalizer is attached.
+func (lbc *LoadBalancerController) ensureFinalizer(ing *v1beta1.Ingress) error {
+	if !flags.F.FinalizerAdd {
+		klog.V(4).Infof("Adding finalizers not enabled")


Do we need to error out if finalizer is not enabled. Moved this check outside so this function does what the name says.

bowei · 2019-11-14T21:26:05Z

Can we make sure that all major actions (deleting, adding finalizer) will result in a log line at V(2)?

bowei · 2019-11-14T21:11:54Z

pkg/controller/controller.go

+		return err
+	}
+	if err := common.EnsureFinalizer(ing, ingClient, finalizerKey); err != nil {
+		klog.Errorf("common.EnsureFinalizer(%q, _, %q) = %v, want nil", ingKey, finalizerKey, err)


leave out ", want nil", it's redundant

Does this use the optimistic concurrency control on write? (see https://github.com/eBay/Kubernetes/blob/master/docs/devel/api-conventions.md#concurrency-control-and-consistency)

Seems like underlying call should be a "PATCH" -- can fix this later, file a bug.

Added a TODO

bowei · 2019-11-14T21:15:16Z

pkg/controller/controller.go

+//    - Does not need cleanup
+//      - Finalizer enabled    :    all backends
+//      - Finalizer disabled   :    v1 frontends and all backends
+func gcPath(ingExists bool, ing *v1beta1.Ingress) (namer.Scheme, bool) {


shouldn't we call this gcNamingScheme?

Can we make the return value less error prone? I think you want to return one of three values, but it's being expressed as a pair ({v1|v2}, bool)

type gcAlgorithm int const ( noCleanupNeeded gcAlgorithm = iota cleanupV1FrontendResources cleanupV2FrontendResources ) func gcAlgorithmForIngress(ingExists ...) gcAlgorithm { ... }

bowei · 2019-11-14T21:23:40Z

pkg/firewalls/controller_test.go

@@ -46,7 +46,7 @@ func newFirewallController() *FirewallController {
 		DefaultBackendSvcPort: test.DefaultBeSvcPort,
 	}

-	ctx := context.NewControllerContext(nil, kubeClient, backendConfigClient, nil, fakeGCE, defaultNamer, ctxConfig)
+	ctx := context.NewControllerContext(nil, kubeClient, backendConfigClient, nil, fakeGCE, defaultNamer, "", ctxConfig)


for raw consts empty string, we usually put the name of the variable to aid the reader:
ctx := context.NewControllerContext(nil, kubeClient, backendConfigClient, nil, fakeGCE, defaultNamer, /*kubeSystemUID*/"", ctxConfig)

Not sure if this needed for tests. gofmt changes this to,
"" /*kubeSystemUID*/
Added this anyway.

bowei · 2019-11-14T21:26:38Z

Looks pretty good, almost there

bowei · 2019-11-15T23:43:38Z

pkg/controller/controller.go

 			return err
 		}
 	}
 	return nil
 }

+// EnsureDeleteV2Finalizer implements Controller.
+func (lbc *LoadBalancerController) EnsureDeleteV2Finalizer(ing *v1beta1.Ingress) error {


This function never returns anything but nil for error.
Either remove the return value or figure out if the error needs to be propagated.

Should return error. Fixed

pkg/controller/controller.go

bowei · 2019-11-15T23:53:15Z

pkg/utils/utils.go

@@ -70,8 +70,21 @@ const (
 	// ToBeDeletedTaint is the taint that the autoscaler adds when a node is scheduled to be deleted
 	// https://github.com/kubernetes/autoscaler/blob/cluster-autoscaler-0.5.2/cluster-autoscaler/utils/deletetaint/delete.go#L33
 	ToBeDeletedTaint = "ToBeDeletedByClusterAutoscaler"
+
+	// NoCleanUpNeeded, CleanupV1FrontendResources and CleanupV2FrontendResources
+	// are used to specify the frontend GC algorithm.


does this type have to be in util package (or exported)

put the constants with the type so the reader doesn't have to search around to find where they are defined.

type FrontendGCAlgorithm int const ( // NoCleanUpNeeded ... ... )

Yes, needs to used in both controller and sync packages.

bowei · 2019-11-16T00:25:52Z

pkg/utils/namer/frontendnamer.go

+	return false
+}
+
+// LbName returns loadbalancer name.


You should document what LbName is used for. Is it the name of a GCP resource or something else?

This is used for generating GCP resource names, added a comment.

pkg/utils/namer/frontendnamer.go

pkg/utils/namer/utils.go

bowei · 2019-11-16T01:12:23Z

pkg/controller/translator/translator_test.go

@@ -65,7 +65,7 @@ func fakeTranslator() *Translator {
 		HealthCheckPath:               "/",
 		DefaultBackendHealthCheckPath: "/healthz",
 	}
-	ctx := context.NewControllerContext(nil, client, backendConfigClient, nil, nil, defaultNamer, ctxConfig)
+	ctx := context.NewControllerContext(nil, client, backendConfigClient, nil, nil, defaultNamer, "" /*kubeSystemUID*/, ctxConfig)


the style is to put the comment in front of the value.
/*kubeSystemUID*/""

bowei · 2019-11-16T01:12:56Z

/lgtm
/approve

k8s-ci-robot · 2019-11-16T01:13:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, skmatti

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~~ [bowei]

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 Oct 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 Oct 15, 2019

k8s-ci-robot assigned freehan Oct 15, 2019

k8s-ci-robot requested review from rramkumar1 and thockin October 15, 2019 00:42

skmatti mentioned this pull request Oct 15, 2019

V2 Frontend Naming Scheme for Ingress-GCE L7 #858

Closed

4 tasks

k8s-ci-robot assigned bowei Oct 15, 2019

skmatti force-pushed the v2-namer branch from 6ea2565 to 91fe43f Compare October 15, 2019 16:54

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 Oct 18, 2019

skmatti force-pushed the v2-namer branch 2 times, most recently from 9aac2ff to 1ae1340 Compare October 21, 2019 20:07

freehan reviewed Oct 21, 2019

View reviewed changes

skmatti force-pushed the v2-namer branch from 1ae1340 to 9b130ae Compare October 25, 2019 18:04

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2019

skmatti force-pushed the v2-namer branch from 9b130ae to 61c90f2 Compare October 25, 2019 18:10

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2019

skmatti force-pushed the v2-namer branch 3 times, most recently from aa060b9 to 74dad25 Compare October 29, 2019 23:59

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 29, 2019

skmatti force-pushed the v2-namer branch from 74dad25 to 1e6d996 Compare October 30, 2019 00:08

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2019

skmatti mentioned this pull request Oct 30, 2019

Add e2e tests for v2 frontend namer #921

Merged

freehan reviewed Nov 4, 2019

View reviewed changes

pkg/controller/controller.go Show resolved Hide resolved

pkg/loadbalancers/interfaces.go Outdated Show resolved Hide resolved

pkg/sync/interfaces.go Outdated Show resolved Hide resolved

skmatti force-pushed the v2-namer branch from 9d41c31 to 25d25ec Compare November 13, 2019 22:34

rramkumar1 reviewed Nov 14, 2019

View reviewed changes

skmatti force-pushed the v2-namer branch 2 times, most recently from efc27ee to a3d1201 Compare November 14, 2019 20:31

bowei reviewed Nov 14, 2019

View reviewed changes

skmatti force-pushed the v2-namer branch 9 times, most recently from b371731 to 86fe9a5 Compare November 15, 2019 04:53

bowei reviewed Nov 16, 2019

View reviewed changes

skmatti force-pushed the v2-namer branch from 86fe9a5 to c84038f Compare November 16, 2019 00:57

Add v2 frontend namer workflow

e7207b0

skmatti force-pushed the v2-namer branch from c84038f to e7207b0 Compare November 16, 2019 01:04

bowei reviewed Nov 16, 2019

View reviewed changes

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

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

k8s-ci-robot merged commit a254862 into kubernetes:master Nov 16, 2019

Cyral mentioned this pull request Jun 18, 2021

HTTP to HTTPS redirection #1075

Closed

Add V2 frontend namer #892

Add V2 frontend namer #892

Conversation

skmatti commented Oct 15, 2019 • edited Loading

k8s-ci-robot commented Oct 15, 2019

skmatti commented Oct 15, 2019

skmatti commented Oct 15, 2019

freehan commented Oct 18, 2019

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

bowei Nov 14, 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

bowei commented Nov 14, 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

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 Nov 14, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowei commented Nov 16, 2019

k8s-ci-robot commented Nov 16, 2019

skmatti commented Oct 15, 2019 •

edited

Loading

bowei Nov 14, 2019 •

edited

Loading