Update to Operator SDK 1.8.0

[lucacome]

TODO:

• update SDK API markers
• figure out why the CSV is not populating from the base

[pleshakov]

Hi @lucacome

Please see my comments and suggestions.
Could you also fix any linting errors?

What's the upgrade plan from the previous version to the new one? Will the enabled auto-upgrade of the operator fail?

[pleshakov]

this wasn't called here previously? but in only once in https://github.com/nginxinc/nginx-ingress-operator/blob/master/pkg/controller/nginxingresscontroller/nginxingresscontroller_controller.go#L185
what's the rationale for calling it on every reconcile?

[lucacome]

I think we should always check that everything that we need is in place, before we were checking only some of the resources. Ideally, we should refactor to use controllerutil.CreateOrUpdate where possible.

[pleshakov]

once the operator creates those common resources, it doesn't monitor them - if those are changed or removed manually, the operator doesn't do anything. Operator assumed that those resources are not removed/changed.

If we want to prevent those possible changes/removals, I think we should have a separate reconcile loop that monitors those resources and restores them if those were changed. Note that the current reconcile loop will not be triggered if those resources were changed.

Because it is not required for the sdk upgrade, I suggest doing in a separate pull request.

[lucacome]

how is this different than calling checkPrerequisites on every loop? Also calling it during setup fails, so not sure where else I could call it.

[pleshakov]

how is this different than calling checkPrerequisites on every loop?

(a)
if the problem we're trying to solve is to prevent those resources from being modified, then calling checkPrerequisites will not work. Let me explain why:

Let's say some of those common resources is modified: an RBAC ClusterRole is removed, for example. If that is the case, the deployed Ingress Controllers will stop working correctly. However, after the removal of that ClusterRole, the Reconcile will not be called, so the Operator will not fix the Ingress Controllers. The Reconcile only will be called when any of the resources of any Ingress Controller are modified:

		For(&k8sv1alpha1.NginxIngressController{}).
		Owns(&appsv1.Deployment{}).
		Owns(&appsv1.DaemonSet{}).
		Owns(&v1.ServiceAccount{}).
		Owns(&v1.Service{}).
		Owns(&v1.ConfigMap{}).
		Owns(&v1.Secret{}).

Also, making the Operator ensure the common resources are always restored is the change of the existing logic. Is it required for SDK upgrade? Shall we minimize the number of changes and focus on upgrading the SDK only? The more changes we make, the more bugs we can potentially bring and it also makes it more difficult to review, because of the increased size/scope.

(b)

Also calling it during setup fails, so not sure where else I could call it.

It looks like you were trying to solve this specific problem. Can we address that failure rather than changing the operator logic?

[lucacome]

I'm not trying to solve any problem. checkPrerequisites was already called in the loop and has a similar logic to createCommonResources, so I don't understand why you're saying that the first one can be in the loop and the second can't.

You want me to move back createCommonResources out of the loop, right? Can you tell me where you think it should go? Calling it from SetupWithManager doesn't work because the resources are not ready yet.

[pleshakov]

I'm not trying to solve any problem. checkPrerequisites was already called in the loop and has a similar logic to createCommonResources, so I don't understand why you're saying that the first one can be in the loop and the second can't.

checkPrerequisites are prerequisites for a single Ingress Controller
createCommonResources are shared prerequisites for all Ingress Controllers and they are much broader. I mentioned here #109 (comment) that it changes the logic and it is not related to the SDK upgrade.

Calling it from SetupWithManager doesn't work because the resources are not ready yet.

what error do you get?

[lucacome]

"error getting ClusterRole: the cache is not started, can not read objects"

[pleshakov]

@lucacome To read the objects from Kubernetes API, the existing code uses mgr.GetAPIReader(). The new code uses mgr.GetClient(), that's why you see the error. Note that mgr.GetAPIReader() doesn't require caches to start.

The existing code:

nginx-ingress-operator/pkg/controller/nginxingresscontroller/nginxingresscontroller_controller.go

Lines 122 to 123 in 2c44794

	clientReader := mgr.GetAPIReader()
	clientWriter := mgr.GetClient()

[pleshakov]

@lucacome
What's the upgrade plan from the previous version to the new one? Do we need any upgrade instructions? Will the enabled auto-upgrade of the operator fail/succeed?

[lucacome]

@f5yacobucci