Speed up admission hook by eliminating deep copy of Ingresses in CheckIngress

[cgorbit]

On big k8s clusters with a lot of Ingresses web admission hook may be too slow.
For example in my real developing k8s cluster with have nginx ingress controller which contains 120 server sections (hosts) and 5271 location sections (endpoints) and have 19MB size as nginx text config.

On such big config amission hook for each ingress update becomes too slow, it lasts about 7 seconds now on setup described above (k8s inside aws ec2).

Sometimes when somebody install helm charts, which contains many ingresses admission hooks must be evaluated many times in a row and we got error from master about admission hook time out (of 30s).

This PR decrease timings for setup described above by 3s.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Performance improvement

Which issue/s this PR fixes

fixes #7297

How Has This Been Tested?

e2e test: https://gist.github.com/cgorbit/431086b0e78f1a8b75cd497235ae8d51
Also tested in runtime of developer's k8s cluster at work.

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I've read the CONTRIBUTION guide
• I have added tests to cover my changes.
• All new and existing tests passed.

[k8s-ci-robot]

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

[tao12345666333]

/ok-to-test

thanks!

[tao12345666333]

/lgtm

[cgorbit]

/assign @rikatz

[rikatz]

Hey @cgorbit and @tao12345666333 reflecting a bit here.

So, trying to get some context, I've seen in past that part of k8s codes as well moved to the approach of deep copying a structure before modifying it.

I'm not sure if this is due to some safeness (multi threads + pointers of the same object, for example) but I'll need to reflect better about this.

I want some thoughts on the security implications of removing the deepcopy in favor of accessing this pointer directly :)

[tao12345666333]

Let's hold it first, and then cancel the hold once we reach a consensus.

/hold

[cgorbit]

@rikatz

1. All Location objects are not from NGINXController.store
2. They are all created each time CheckIngress is called.

• first default locations:

  ingress-nginx/internal/ingress/controller/controller.go

  Lines 1120 to 1127 in 16072ac

  	loc := &ingress.Location{
  	Path: rootLocation,
  	PathType: &pathTypePrefix,
  	IsDefBackend: true,
  	Backend: un,
  	Ingress: ing,
  	Service: &apiv1.Service{},
  	}

• then rest locations:

  ingress-nginx/internal/ingress/controller/controller.go

  Lines 630 to 638 in 16072ac

  	loc := &ingress.Location{
  	Path: nginxPath,
  	PathType: path.PathType,
  	Backend: ups.Name,
  	IsDefBackend: false,
  	Service: ups.Service,
  	Port: ups.Port,
  	Ingress: ing,
  	}

3. Yes, they all share the same Ingress and Service objects, which are from NGINXController.store
4. Original locations don't deep copy Ingress or Service objects from store.

So, I don't see why we can't create more locations in the same way.

[rikatz]

@cgorbit I took some time to proper read the code, here's my concern:

While in loop https://github.com/kubernetes/ingress-nginx/blob/master/internal/ingress/controller/location.go#L47 all locations verifications are "skipped" after some condition is met (loop continues to next item), exactly here something different happens:

https://github.com/kubernetes/ingress-nginx/blob/master/internal/ingress/controller/location.go#L76-L90

So, because this is a pointer, you might end adding the same pointer twice, and when you try to change only the pathType information you can end turning this information into something invalid.

In the end, what is going to happen is, you have a location that you set as pathPrefix and it's skipped here to the next set of instructions,

then this same location is added here and duplicated as pathTypeExact here

When you add your patch, instead of being duplicated, you just turn that pathTypePrefix into a pathTypeExact as you are changing the pointer :)

Maybe some tests about the behavior of a mixed Exact/Prefix location would be good to confirm that this wont break anything :)

[cgorbit]

@rikatz

So, because this is a pointer, you might end adding the same pointer twice, and when you try to change only the pathType information you can end turning this information into something invalid.
When you add your patch, instead of being duplicated, you just turn that pathTypePrefix into a pathTypeExact as you are changing the pointer :)

I don't change the pointer. I don't add the same pointer twice to newLocations.

First I do shallow copy of location into el here. el is not a pointer.

[rikatz]

@cgorbit tested here, thanks for the patience :)
/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgorbit, rikatz, tao12345666333

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 [rikatz]

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

[rikatz]

/hold cancel