Rearrange deployment files into kustomizations

[nicknovitski]

What this PR does / why we need it:

kustomize is pretty good, and it's been integrated into kubectl. This makes it possible for users to create a test deployment as simply as running:

kubectl create ns ingress-nginx
kubectl apply -k github.com/kubernetes/ingress-nginx/deploy/base

It also makes it possible for people to have version-controlled files tracking and combining upstream updates and their own additions and changes, for example:

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
namespace: infrastructure
bases:
- github.com/kubernetes/ingress-nginx/deploy/base?ref=nginx-0.25.0
resources:
- pod-disruption-budget.yaml
patchesStrategicMerge:
- deployment-anti-affinity.yaml
- deployment-increase-replicas.yaml
- deployment-reduce-downtime.yaml
- deployment-resources.yaml
configMapGenerator:
- name: nginx-configuration
  behavior: merge
  literals:
  - use-http2="false"
  - ssl-protocols="TLSv1 TLSv1.1 TLSv1.2"
  - worker-shutdown-timeout="13s"
  # etc

kubectl apply -k also does deployment rollouts on config-map changes.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #3994

[aledbf]

@nicknovitski thank you for working on this!

[nicknovitski]

Ah, I didn't see test/e2e-image/manifests, I'll handle that now.

[nicknovitski]

Oh, interesting.

I noticed and figured out a "fix" for a problem in the Makefile:

 e2e-test:
        if  [ "$(KUBECTL_CONTEXT)" != "minikube" ] && \
-               [ "$(KUBECTL_CONTEXT)" =~ .*kind* ] && \
+               ! echo $(KUBECTL_CONTEXT) | grep kind && \

The e2e tests run this target in a sh shell, which has no =~ operator, so it would never detect if the context name wasn't in the whitelist, just complain about not finding a conditional and then move on to the remaining commands. But the e2e tests use a context name that isn't in the whitelist. I've added it. I hope that was the intended behavior.

[nicknovitski]

In general, I'm changing the e2e tests to re-use the bases I've created in deploy/. This has the benefit of actually testing the same configurations the documentation tells people to use.

It also prompted me to consider how to install multiple ingress controllers in the same cluster, which I hadn't thought about before. It won't be too crazy: I'll separate the ClusterRole into its own base, much like the rbac.yaml has been separate in the past, and change the instructions to tell people to use it.

For example, commands to deploy a brand new default ingress on would be something like this:

kubectl create namespace ingress-nginx
kubectl apply -k github.com/kubernetes/ingress-nginx/deploy/cluster-wide # contains only the ClusterRole
kubectl apply -k github.com/kubernetes/ingress-nginx/deploy/base

Or with a kustomization.yaml:

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
bases:
- github.com/kubernetes/ingress-nginx/deploy/cluster-wide
- github.com/kubernetes/ingress-nginx/deploy/base
resources:
- namespace.yaml # a namespace named `ingress-nginx`

But these could be separable. One user might create the cluster role, another a namespace, and the third an ingress controller in a particular namespace. That last part would look something like this:

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
namespace: staging # or whatever
bases:
- github.com/kubernetes/ingress-nginx/deploy/base
patchesStrategicMerge:
- deployment-namespace-patch.yaml # as in test/e2e-image/overlay, adds "--watch-namespace=$(POD_NAMESPACE)" to container args

[nicknovitski]

The test of the --default-backend-service argument continues to fail, and I don't know why.

This is the complete list of changes that the branch makes to the configuration applied for a given e2e test, including diff output.

A different cluster role is created for each test, with a unique name:
<   name: nginx-ingress-clusterrole
>   name: nginx-ingress-clusterrole-${NAMESPACE}
Each test's role binding has the same name (as it's being created in a namespace anyway), taken from the deployment manifest 
<   name: nginx-ingress-role-${NAMESPACE}
>   name: nginx-ingress-role-nisa-binding
Each test's cluster role binding takes a name from the deployment manifest  
<   name: nginx-ingress-clusterrole-${NAMESPACE}
>   name: nginx-ingress-clusterrole-nisa-binding-${NAMESPACE}
The cluster role binding's roleref correctly points to the new unique cluster role namexxxxx
<   name: nginx-ingress-clusterrole
>   name: nginx-ingress-clusterrole-${NAMESPACE}
The ingress-nginx service gets the same shared labels as everything else
>   labels:
>     app.kubernetes.io/name: ingress-nginx
>     app.kubernetes.io/part-of: ingress-nginx
The deployment uses the general release api group and version:
< apiVersion: extensions/v1beta1
> apiVersion: apps/v1
The deployment also uses the POD_NAMESPACE environment variable:
<         - --watch-namespace=${NAMESPACE}
>         - --watch-namespace=$(POD_NAMESPACE)

I can't think of any way these changes would have broken only that particular test. I've run out of things to try, and I don't have much more time to work on it.

I'd very much appreciate help or suggestions from anyone who can make them. ☹️

[aledbf]

@nicknovitski this is more than I expected.
Please

• open a different PR to change the code form appsv1beta1 to appsv1 (remove those changes here)
• I just pushed a change to the e2e image updating the go version (not related to this PR) and a newer kubectl version (Update go to 1.12.5, kubectl to 1.14.1 and kind to 0.2.1 #4064)

If you rebase after these two suggestions this should pass.

Also, apologies for the delay to provide feedback.

[nicknovitski]

Sure. To be honest, I only made the appsv1 changes across the e2e tests to see if it fixed the default-backend-service error, but since they did not, I don't mind removing them.

e: sadly that failed, and brought back the failure of the test of the from-to-www-redirect annotation.

[nicknovitski]

I have fixed those two tests by patching the deployment object to have apiVersion: extensions/v1beta1 again. I'm disturbed that worked.

Currently, on master, the deployment object manifests in the deploy folder use apps/v1, and the one in the test/e2e-image/manifests folder uses extensions/v1beta1. I think that's a problem, but it's not one I'm introducing (or fixing) with this PR. At the very least, the explicit patch will make the difference more visible. I could also add a TODO comment.

[aledbf]

@nicknovitski please address my comment and squash the commits once this passes e2e test

[nicknovitski]

Is the isolation still necessary when each test gets its own cluster role? I thought the test was marked serial because it was modifying a shared resource. If the resource is no longer shared, then it's safe to run in parallel. But even if so, I can leave that change for another pull request. -- Nick

On Tue, May 7, 2019, at 11:34 AM, Manuel Alejandro de Brito Fontes wrote: ***@***.**** commented on this pull request. In test/e2e/settings/pod_security_policy.go <#4055 (comment)>: > @@ -38,7 +39,7 @@ const (

ingressControllerPSP = "ingress-controller-psp" )

…

-var _ = framework.IngressNginxDescribe("[Serial] Pod Security Policies", func() {

Please don't remove [Serial] This is used to run this particular test in isolation to avoid issues with PSP — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4055 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABE6KJ4VKMDPDEXGJN7AWTPUHDSRANCNFSM4HJQP7VQ>.

[aledbf]

Is the isolation still necessary when each test gets its own cluster role?

Yes because the PSP test requires a different clusterrole that only makes sense in that scenario and that's why requires a serial run. I suggest to rollback all the changes in pod_security_policy.go

[aledbf]

/approve

[aledbf]

@ElvinEfendi this is ready for review

[nicknovitski]

Rebased to resolve a conflict I introduced.

[aledbf]

@nicknovitski please rebase

[nicknovitski]

Rebased and added worker-processes: "1" to the e2e-test configuration.

[aledbf]

/ok-to-test

[aledbf]

/retest

[codecov-io]

Codecov Report

Merging #4055 into master will increase coverage by 0.1%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    #4055     +/-   ##
=========================================
+ Coverage   57.65%   57.75%   +0.1%     
=========================================
  Files          87       87             
  Lines        6452     6432     -20     
=========================================
- Hits         3720     3715      -5     
+ Misses       2300     2286     -14     
+ Partials      432      431      -1

Impacted Files	Coverage Δ
internal/ingress/annotations/parser/main.go	81.63% <0%> (-2.01%)	⬇️
internal/ingress/controller/config/config.go	98.54% <0%> (-0.03%)	⬇️
internal/ingress/zz_generated.deepcopy.go	0% <0%> (ø)	⬆️
internal/ingress/status/status.go	73.18% <0%> (ø)	⬆️
internal/ingress/controller/nginx.go	29% <0%> (ø)	⬆️
internal/ingress/controller/controller.go	46.82% <0%> (+0.26%)	⬆️
internal/ingress/controller/store/store.go	62.05% <0%> (+0.29%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff80dca...51ad0bc. Read the comment docs.

[ElvinEfendi]

Was this a necessary change? Why?

[ElvinEfendi]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, ElvinEfendi, nicknovitski

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 [ElvinEfendi,aledbf]

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

[AndrewX192]

FYI the change that just landed breaks the installation guide which calls for

kubectl apply -f https://raw.githubusercontent.com/kubernetes/ingress-nginx/master/deploy/mandatory.yaml

This file no longer exists.

[aledbf]

@AndrewX192 please check the docs again. (we need to run a manual task to update the docs)