Add support for configuring route annotations

[lewisdenny]

This patch adds the ability to configure route annotations for example haproxy.router.openshift.io/timeout: 120s for configureable timeout if the backend service is slow.

[stuggi]

Do you have a PR how this is exposed via the CRD to an end user via the service operator?

[lewisdenny]

Do you have a PR how this is exposed via the CRD to an end user via the service operator?

Not yet, I will hopefully submit a PR today for this.

[bshephar]

Looks good. Just one minor change

[bshephar]

Link with reference to how this is used:
openstack-k8s-operators/neutron-operator#168

[jpodivin]

Good change, I can see it being useful. But I would like to see some unit test coverage. It can be minimal but at least something.

[stuggi]

In general I agree that we have to be able to customize the annotations for things like timeout. Since this requires a change to all operators when they update to the new lib-common one, I am just wondering if we might want to have a more generic routeOverride possibility. @olliewalsh and I start to look at tls and we also need to be able to set parameters for this. Our idea was to do it similar to [1] and have an override struct type with a reduced copy of the k8s/ocp type, where in the original crd required parameters are then optional. This would also allow us to add new parameters to override in future without changes to the service operators. Also @abays might wanted to start working on some override feature for other deployment parts in next Q.

[1] https://github.com/rabbitmq/cluster-operator/blob/main/api/v1beta1/rabbitmqcluster_types.go#L172-L180

[stuggi]

In general I agree that we have to be able to customize the annotations for things like timeout. Since this requires a change to all operators when they update to the new lib-common one, I am just wondering if we might want to have a more generic routeOverride possibility. @olliewalsh and I start to look at tls and we also need to be able to set parameters for this. Our idea was to do it similar to [1] and have an override struct type with a reduced copy of the k8s/ocp type, where in the original crd required parameters are then optional. This would also allow us to add new parameters to override in future without changes to the service operators. Also @abays might wanted to start working on some override feature for other deployment parts in next Q.

[1] https://github.com/rabbitmq/cluster-operator/blob/main/api/v1beta1/rabbitmqcluster_types.go#L172-L180

If setting route annotations is something we require soon, I am also ok to land this now and change it later as mentioned above

[bshephar]

Good change, I can see it being useful. But I would like to see some unit test coverage. It can be minimal but at least something.

Doesn't look like we have any other tests for the common modules here - except for job_test. Might involve a fairly significant change to implement tests in this case. I would be happy to see it tests in neutron-operator for now, with a seperate PR to implement more test coverage in lib-common.

[bshephar]

In general I agree that we have to be able to customize the annotations for things like timeout. Since this requires a change to all operators when they update to the new lib-common one, I am just wondering if we might want to have a more generic routeOverride possibility. @olliewalsh and I start to look at tls and we also need to be able to set parameters for this. Our idea was to do it similar to [1] and have an override struct type with a reduced copy of the k8s/ocp type, where in the original crd required parameters are then optional. This would also allow us to add new parameters to override in future without changes to the service operators. Also @abays might wanted to start working on some override feature for other deployment parts in next Q.
[1] https://github.com/rabbitmq/cluster-operator/blob/main/api/v1beta1/rabbitmqcluster_types.go#L172-L180

If setting route annotations is something we require soon, I am also ok to land this now and change it later as mentioned above

We could potentially run into issues when scale testing because of this - iiuc the initial neutron issue that bought about this change. The defaults in tripleo are also higher than the OCP default. So we might run into issues a bit earlier here.

But I mean, we could push it out until something breaks I guess. I don't think we should sacrifice good design in favour of something easier to implement across the project at this early stage though. So we're better off getting the design right now and receiving the dividends for our diligence once we GA, rather than doing something as a stop gap at this early stage. Maybe a topic for the podified call this week?

Be good to see a PR of the alternative to facilitate that discussion.

[stuggi]

In general I agree that we have to be able to customize the annotations for things like timeout. Since this requires a change to all operators when they update to the new lib-common one, I am just wondering if we might want to have a more generic routeOverride possibility. @olliewalsh and I start to look at tls and we also need to be able to set parameters for this. Our idea was to do it similar to [1] and have an override struct type with a reduced copy of the k8s/ocp type, where in the original crd required parameters are then optional. This would also allow us to add new parameters to override in future without changes to the service operators. Also @abays might wanted to start working on some override feature for other deployment parts in next Q.
[1] https://github.com/rabbitmq/cluster-operator/blob/main/api/v1beta1/rabbitmqcluster_types.go#L172-L180

If setting route annotations is something we require soon, I am also ok to land this now and change it later as mentioned above

We could potentially run into issues when scale testing because of this. iiuc the initial neutron issue that bought about this change. The defaults in tripleo are also higher than the OCP default. So we might run into issues a bit earlier here.

But I mean, we could push it out until something breaks I guess. I don't think we should sacrifice good design in favour or something easier to implement across the project at this early stage though. So we're better off getting the design right now and receiving the dividends for our diligence once we GA, rather than doing something as a stop gap at this early stage. Maybe a topic for the podified call this week?

Be good to see a PR of the alternative to facilitate that discussion.

I'll work on a PR during this week for it, then we can compare/discuss the alternative approach.

[stuggi]

Good change, I can see it being useful. But I would like to see some unit test coverage. It can be minimal but at least something.

Doesn't look like we have any other tests for the common modules here - except for job_test. Might involve a fairly significant change to implement tests in this case. I would be happy to see it tests in neutron-operator for now, with a seperate PR to implement more test coverage in lib-common.

Adding more test coverage to lib-common is a topic for this Q

[gibizer]

In general I agree that we have to be able to customize the annotations for things like timeout. Since this requires a change to all operators when they update to the new lib-common one, I am just wondering if we might want to have a more generic routeOverride possibility.

With each new function param we break all the users of the function as golang has no default value for function parameter concept. I think the generic way out could be to apply the Builder pattern. One example of a builder at work is the way how the operator-sdk builds controllers:

        ctrl.NewControllerManagedBy(mgr).
		For(&novav1.NovaAPI{}).
		Owns(&v1.StatefulSet{}).
		Owns(&corev1.Service{}).
		Owns(&corev1.ConfigMap{}).
		Owns(&routev1.Route{}).
		Owns(&keystonev1.KeystoneEndpoint{}).
		Watches(&source.Kind{Type: &corev1.Secret{}},
			handler.EnqueueRequestsFromMapFunc(r.GetSecretMapperFor(&novav1.NovaAPIList{}))).
		Complete(r)
}

NewControllerManagedBy creates a builder object, them the later calls add components to the builders (these would be function parameters on the constructor of the controller without this pattern) and finally the last Complete call does the actual controller construction based on the "parameters" added to the builder.

I'm not saying we must to apply the Builder pattern here. I just raised this here for a possible generic pattern we might want to switch to when a lib-common function gets new parameters. A possible alternative is the ParameterObject pattern.

[stuggi]

In general I agree that we have to be able to customize the annotations for things like timeout. Since this requires a change to all operators when they update to the new lib-common one, I am just wondering if we might want to have a more generic routeOverride possibility.

With each new function param we break all the users of the function as golang has no default value for function parameter concept. I think the generic way out could be to apply the Builder pattern. One example of a builder at work is the way how the operator-sdk builds controllers:

        ctrl.NewControllerManagedBy(mgr).
		For(&novav1.NovaAPI{}).
		Owns(&v1.StatefulSet{}).
		Owns(&corev1.Service{}).
		Owns(&corev1.ConfigMap{}).
		Owns(&routev1.Route{}).
		Owns(&keystonev1.KeystoneEndpoint{}).
		Watches(&source.Kind{Type: &corev1.Secret{}},
			handler.EnqueueRequestsFromMapFunc(r.GetSecretMapperFor(&novav1.NovaAPIList{}))).
		Complete(r)
}

NewControllerManagedBy creates a builder object, them the later calls add components to the builders (these would be function parameters on the constructor of the controller without this pattern) and finally the last Complete call does the actual controller construction based on the "parameters" added to the builder.

I'm not saying we must to apply the Builder pattern here. I just raised this here for a possible generic pattern we might want to switch to when a lib-common function gets new parameters. A possible alternative is the ParameterObject pattern.

right, we should prevent breaking users. We already have the parameter object here, so the simple way is to just add the new parameter as a ptr via the endpoint Data struct type. https://github.com/openstack-k8s-operators/lib-common/blob/main/modules/common/endpoint/endpoint.go#L48

[gibizer]

[...snip...]

right, we should prevent breaking users. We already have the parameter object here, so the simple way is to just add the new parameter as a ptr via the endpoint Data struct type. https://github.com/openstack-k8s-operators/lib-common/blob/main/modules/common/endpoint/endpoint.go#L48

Ohh if we can extend an already used parameter object then such extension is the least painful thing to do now. And as I said above parameter object is a good alternative to builder so I'm OK with this.

[stuggi]

this would be an alternative approach with a common way to provide overrides to the route. we'd also need passing parameters for TLS to the route and could be handled with this. #293 / openstack-k8s-operators/keystone-operator#269

[gibizer]

this would be an alternative approach with a common way to provide overrides to the route. we'd also need passing parameters for TLS to the route and could be handled with this. #293 / openstack-k8s-operators/keystone-operator#269

OK, this discussion has multiple aspects. I only looked at how to extend the ExposeEndpoint lib-common function to take annotations. And the fact that adding a new param there is breaking all the users. While replacing the params there with a builder pattern or a parameter object, would, at least in the future, allow us to extend the amount of data passed in without impacting the current users if the default value of that that is good for them.

Now I see that there is already a Data struct passed to ExposeEndpoint for each Endpoint to be exposed. And that struct can be the parameter object we extend, instead of adding a new parameter directly to the call. This seem OK to me.

Then I looked at the how the Data is expanded to a full RouteSpec and exposed in the KeystoneAPI. There I'm a bit hesitant as we are exposing a lot of things. I'm not saying it is wrong that we expose a full RouteSpec, I just don't fully grasp how the override route Spec will interact with the original route spec the operator generates.
In the current example I imagine that the annotations passed here are "extra" annotations added to the already existing annotations on the Route. (However the code currently does not do that, it overrides any annotations already exists on the Route). Similarly we might need to define what the override RouteSpec will do top of the original Route spec.

[gibizer]

Below we directly override existing annotations on the route. I'm wondering if we want to do a merge similarly how the lables are added.

		route.Labels = util.MergeStringMaps(route.Labels, r.route.Labels)
		route.Annotations = r.route.Annotations

[stuggi]

this would be an alternative approach with a common way to provide overrides to the route. we'd also need passing parameters for TLS to the route and could be handled with this. #293 / openstack-k8s-operators/keystone-operator#269

OK, this discussion has multiple aspects. I only looked at how to extend the ExposeEndpoint lib-common function to take annotations. And the fact that adding a new param there is breaking all the users. While replacing the params there with a builder pattern or a parameter object, would, at least in the future, allow us to extend the amount of data passed in without impacting the current users if the default value of that that is good for them.

Now I see that there is already a Data struct passed to ExposeEndpoint for each Endpoint to be exposed. And that struct can be the parameter object we extend, instead of adding a new parameter directly to the call. This seem OK to me.

Then I looked at the how the Data is expanded to a full RouteSpec and exposed in the KeystoneAPI. There I'm a bit hesitant as we are exposing a lot of things. I'm not saying it is wrong that we expose a full RouteSpec, I just don't fully grasp how the override route Spec will interact with the original route spec the operator generates. In the current example I imagine that the annotations passed here are "extra" annotations added to the already existing annotations on the Route. (However the code currently does not do that, it overrides any annotations already exists on the Route). Similarly we might need to define what the override RouteSpec will do top of the original Route spec.

I guess that update is related to my other proposal PR? if so lets move it there

[gibizer]

this would be an alternative approach with a common way to provide overrides to the route. we'd also need passing parameters for TLS to the route and could be handled with this. #293 / openstack-k8s-operators/keystone-operator#269

OK, this discussion has multiple aspects. I only looked at how to extend the ExposeEndpoint lib-common function to take annotations. And the fact that adding a new param there is breaking all the users. While replacing the params there with a builder pattern or a parameter object, would, at least in the future, allow us to extend the amount of data passed in without impacting the current users if the default value of that that is good for them.
Now I see that there is already a Data struct passed to ExposeEndpoint for each Endpoint to be exposed. And that struct can be the parameter object we extend, instead of adding a new parameter directly to the call. This seem OK to me.
Then I looked at the how the Data is expanded to a full RouteSpec and exposed in the KeystoneAPI. There I'm a bit hesitant as we are exposing a lot of things. I'm not saying it is wrong that we expose a full RouteSpec, I just don't fully grasp how the override route Spec will interact with the original route spec the operator generates. In the current example I imagine that the annotations passed here are "extra" annotations added to the already existing annotations on the Route. (However the code currently does not do that, it overrides any annotations already exists on the Route). Similarly we might need to define what the override RouteSpec will do top of the original Route spec.

I guess that update is related to my other proposal PR? if so lets move it there

True the last paragraph is more about your PR.

[stuggi]

I think we have agreement to go with generic way in #293 , just waiting for dev-preview cut to get it landed

[lewisdenny]

Closing in favour of #293