Allow customize route via route.Override #293

stuggi · 2023-07-04T15:52:17Z

Adds an OverridSpec to the Route which allows to customize metadata.Annotations, metadata.Labels and spec of a route. The override values get merged into the object definition created by the operator. This allows e.g. to add custom labels, configure the route via annotations as in [1], or set TLS parameters.

[1] https://docs.openshift.com/container-platform/4.13/networking/routes/route-configuration.html#nw-route-specific-annotations_route-configuration

Jira: OSP-21715
Jira: OSP-26299

stuggi · 2023-07-04T15:56:01Z

need to add more tests

stuggi · 2023-07-04T15:58:16Z

example on service operator usage in openstack-k8s-operators/keystone-operator#269

modules/common/route/types.go

bshephar

I like the approach, it makes the interface within lib-common much more flexible. It does however put some of the complexity burden back on to the user. As they need to understand how to set these options in their yaml files. But, ultimately the added extensibility and maintainability of this is nice.

I'm not sure if we need to literally copy the entire RouteSpec though. Could we maybe import and just set Spec to RouteSpec?

bshephar · 2023-07-04T21:30:30Z

modules/common/route/types.go

+	Spec *Spec `json:"spec,omitempty" protobuf:"bytes,2,opt,name=spec"`
+}
+
+// EmbeddedLabelsAnnotations is an embedded subset of the fields included in k8s.io/apimachinery/pkg/apis/meta/v1.ObjectMeta.


Without reading this comment, it wasn't immediately obvious to me what the "Embedded" nature of this field would be. Would it possibly make more sense to call it MetadataLabelsAnnotations to better represent the fact that it's a v1.ObjectMeta subset?

sure, could rename it

modules/common/route/types.go

stuggi · 2023-07-05T05:46:34Z

I like the approach, it makes the interface within lib-common much more flexible. It does however put some of the complexity burden back on to the user. As they need to understand how to set these options in their yaml files. But, ultimately the added extensibility and maintainability of this is nice.

I'm not sure if we need to literally copy the entire RouteSpec though. Could we maybe import and just set Spec to RouteSpec?

to be able to override a subset of the spec, we have to make sure that all parameters in the override spec are optional and not required as in the original spec definition. thats why we can not just use it.

bshephar · 2023-07-05T06:09:03Z

I like the approach, it makes the interface within lib-common much more flexible. It does however put some of the complexity burden back on to the user. As they need to understand how to set these options in their yaml files. But, ultimately the added extensibility and maintainability of this is nice.
I'm not sure if we need to literally copy the entire RouteSpec though. Could we maybe import and just set Spec to RouteSpec?

to be able to override a subset of the spec, we have to make sure that all parameters in the override spec are optional and not required as in the original spec definition. thats why we can not just use it.

Right yeah, sorry I see Gibi mentioned the same thing and you responded to his comment. Thanks for the clarification.

modules/common/route/route.go

modules/common/route/types.go

gibizer

Looks good to me. Extra thanks for the documentation and the test coverage.

modules/common/go.mod

modules/common/route/route.go

stuggi · 2023-07-13T10:16:33Z

/hold

stuggi · 2023-07-13T14:29:40Z

modules/common/route/types.go

+// kind is allowed. Use 'weight' field to emphasize one over others.
+// Copy of RouteTargetReference in https://github.com/openshift/api/blob/master/route/v1/types.go,
+// parameters set to be optional, have omitempty, and no default.
+type TargetReference struct {


I also had to create a copy of RouteTargetReference setting omitempty and removing defaults. Otherwise the defaulting would have overwritten values set by the operator

abays

/lgtm

Allows to customize the service route vie the `spec.Override.Route`. This allows e.g. to add custom labels, configure the route via annotations as in [1], or set TLS parameters. ~~~ apiVersion: keystone.openstack.org/v1beta1 kind: KeystoneAPI metadata: name: keystone namespace: openstack spec: ... override: routeOverride: metadata: annotations: haproxy.router.openshift.io/timeout: "60" labels: mylabel: boo ~~~ [1] https://docs.openshift.com/container-platform/4.13/networking/routes/route-configuration.html#nw-route-specific-annotations_route-configuration Depends-On: openstack-k8s-operators/lib-common#293 Jira: OSP-21715 Jira: OSP-26299

stuggi · 2023-07-14T07:43:40Z

/hold not landing before dev-preview

Allows register https endpoints when certificate is provided via the endpoint override. Depends-On: openstack-k8s-operators#293 Jira: OSP-26299

Allows to customize the service route vie the `spec.Override.Route`. This allows e.g. to add custom labels, configure the route via annotations as in [1], or set TLS parameters. ~~~ apiVersion: keystone.openstack.org/v1beta1 kind: KeystoneAPI metadata: name: keystone namespace: openstack spec: ... override: routeOverride: metadata: annotations: haproxy.router.openshift.io/timeout: "60" labels: mylabel: boo ~~~ [1] https://docs.openshift.com/container-platform/4.13/networking/routes/route-configuration.html#nw-route-specific-annotations_route-configuration Depends-On: openstack-k8s-operators/lib-common#293 Jira: OSP-21715 Jira: OSP-26299

Allows register https endpoints when certificate is provided via the endpoint override. Depends-On: openstack-k8s-operators#293 Jira: OSP-26299

Allows to customize the service route vie the `spec.Override.Route`. This allows e.g. to add custom labels, configure the route via annotations as in [1], or set TLS parameters. ~~~ apiVersion: keystone.openstack.org/v1beta1 kind: KeystoneAPI metadata: name: keystone namespace: openstack spec: ... override: routeOverride: metadata: annotations: haproxy.router.openshift.io/timeout: "60" labels: mylabel: boo ~~~ [1] https://docs.openshift.com/container-platform/4.13/networking/routes/route-configuration.html#nw-route-specific-annotations_route-configuration Depends-On: openstack-k8s-operators/lib-common#293 Jira: OSP-21715 Jira: OSP-26299

stuggi · 2023-07-26T09:07:12Z

rebased

stuggi · 2023-07-26T12:33:11Z

/hold

Adds an OverridSpec to the Route which allows to customize metadata.Annotations, metadata.Labels and spec of a route. The override values get merged into the object definition created by the operator. This allows e.g. to add custom labels, configure the route via annotations as in [1], or set TLS parameters. [1] https://docs.openshift.com/container-platform/4.13/networking/routes/route-configuration.html#nw-route-specific-annotations_route-configuration Jira: OSP-21715 Jira: OSP-26299

stuggi · 2023-07-26T16:41:18Z

/unhold

stuggi · 2023-07-26T16:42:47Z

@gibizer @abays @bshephar are we good to merge this now that lib-common is tagged for 0.1.0?

gibizer · 2023-07-27T14:17:44Z

@gibizer @abays @bshephar are we good to merge this now that lib-common is tagged for 0.1.0?

I'm OK to land this now. My comments were answered and we tagged lib-common. This PR does not create a backward incompatible changes for our service operators so if we need to fix something else in lib-common for the dev preview we can do that on main still after this PR.

abays

/lgtm

stuggi mentioned this pull request Jul 4, 2023

Add support for configuring route annotations #288

Closed

stuggi requested review from olliewalsh, gibizer and bshephar July 4, 2023 15:57

gibizer reviewed Jul 4, 2023

View reviewed changes

modules/common/route/types.go Outdated Show resolved Hide resolved

bshephar reviewed Jul 4, 2023

View reviewed changes

stuggi commented Jul 5, 2023

View reviewed changes

modules/common/route/types.go Outdated Show resolved Hide resolved

gibizer reviewed Jul 5, 2023

View reviewed changes

modules/common/route/route.go Outdated Show resolved Hide resolved

modules/common/route/route.go Show resolved Hide resolved

modules/common/route/types.go Outdated Show resolved Hide resolved

stuggi force-pushed the route_override branch 3 times, most recently from 44c2e9d to 7450e90 Compare July 11, 2023 14:06

stuggi requested review from abays and dprince July 12, 2023 14:54

stuggi force-pushed the route_override branch 4 times, most recently from 595e29d to 928f97b Compare July 13, 2023 09:13

gibizer reviewed Jul 13, 2023

View reviewed changes

modules/common/go.mod Show resolved Hide resolved

modules/common/route/route.go Show resolved Hide resolved

modules/common/route/route.go Show resolved Hide resolved

stuggi force-pushed the route_override branch from 928f97b to ad75654 Compare July 13, 2023 14:26

stuggi commented Jul 13, 2023

View reviewed changes

abays approved these changes Jul 13, 2023

View reviewed changes

stuggi added a commit to stuggi/lib-common that referenced this pull request Jul 14, 2023

[TLS] Support https/http endpoints

a279a9b

Allows register https endpoints when certificate is provided via the endpoint override. Depends-On: openstack-k8s-operators#293 Jira: OSP-26299

stuggi added a commit to stuggi/lib-common that referenced this pull request Jul 14, 2023

[TLS] Support https/http endpoints

6562efc

Allows register https endpoints when certificate is provided via the endpoint override. Depends-On: openstack-k8s-operators#293 Jira: OSP-26299

stuggi added a commit to stuggi/lib-common that referenced this pull request Jul 14, 2023

[TLS] Support https/http endpoints

d6d7de6

Allows register https endpoints when certificate is provided via the endpoint override. Depends-On: openstack-k8s-operators#293 Jira: OSP-26299

bshephar mentioned this pull request Jul 17, 2023

[TLS] Support https/http endpoints #303

Closed

lewisdenny mentioned this pull request Jul 25, 2023

Add support for configuring route annotations openstack-k8s-operators/neutron-operator#168

Closed

stuggi force-pushed the route_override branch from ad75654 to c2a3252 Compare July 26, 2023 09:06

stuggi force-pushed the route_override branch from c2a3252 to 2b3293a Compare July 26, 2023 14:58

stuggi force-pushed the route_override branch from 2b3293a to a5afd03 Compare July 26, 2023 15:05

stuggi mentioned this pull request Jul 26, 2023

Allow customize service via service.Override and handle endpoint protocol #313

Closed

abays approved these changes Jul 28, 2023

View reviewed changes

abays merged commit 36c6d81 into openstack-k8s-operators:main Jul 28, 2023

Allow customize route via route.Override #293

Allow customize route via route.Override #293

Uh oh!

Conversation

stuggi commented Jul 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuggi commented Jul 4, 2023

Uh oh!

stuggi commented Jul 4, 2023

Uh oh!

Uh oh!

bshephar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bshephar Jul 4, 2023

Choose a reason for hiding this comment

Uh oh!

stuggi Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stuggi commented Jul 5, 2023

Uh oh!

bshephar commented Jul 5, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gibizer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stuggi commented Jul 13, 2023

Uh oh!

stuggi Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

abays left a comment

Choose a reason for hiding this comment

Uh oh!

stuggi commented Jul 14, 2023

Uh oh!

stuggi commented Jul 26, 2023

Uh oh!

stuggi commented Jul 26, 2023

Uh oh!

stuggi commented Jul 26, 2023

Uh oh!

stuggi commented Jul 26, 2023

Uh oh!

gibizer commented Jul 27, 2023

Uh oh!

abays left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stuggi commented Jul 4, 2023 •

edited

Loading

bshephar left a comment •

edited

Loading