Allow customize route via spec.Override.Route #269

stuggi · 2023-07-04T15:53:14Z

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

Jira: OSP-21715
Jira: OSP-26299

stuggi · 2023-07-04T15:53:20Z

/hold

openshift-ci · 2023-07-04T15:53:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stuggi

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~~ [stuggi]

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

gibizer · 2023-07-05T09:11:55Z

api/v1beta1/keystoneapi_types.go

+// KeystoneAPIOverrideSpec to override the generated manifest of several child resources.
+type KeystoneAPIOverrideSpec struct {
+	// Override configuration for the Route created to serve traffic to the service
+	Route *route.OverrideSpec `json:"route,omitempty"`


So lib-common does a strategicpatch.StrategicMergePatch between the original route spec an this override. I think we still need to define for the user what behavior that means.
My guesses / questions:

the list of labels and annotations are merged

the host, subdomain and path fields are overridden in the original spec if defined here

Does the to field which is a struct RouteTargetReference overridden field by field or as a single entity. I.e can I override the to.weight independently without defining the to.name or I need to define both in the override?

AlternateBackends is a list. Does the items in that list merged with the items in the override?

... you probably interpolate my questions about the behavior of the rest of the fields.

So lib-common does a strategicpatch.StrategicMergePatch between the original route spec an this override. I think we still need to define for the user what behavior that means. My guesses / questions:

the list of labels and annotations are merged

correct, new labels/annotations get merged with the ones created by the operator. If the user provides the same annotation/label as the operator creates, the one from the user will overwrite the one from the operator.

the host, subdomain and path fields are overridden in the original spec if defined here
correct

Does the to field which is a struct RouteTargetReference overridden field by field or as a single entity. I.e can I override the to.weight independently without defining the to.name or I need to define both in the override?

you have to override the full struct since those are required values, otherwise we have to create a copy of them as well and make them all optional.

AlternateBackends is a list. Does the items in that list merged with the items in the override?

this depends on the list patchStrategy. https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/#notes-on-the-strategic-merge-patch . so this is different from list to list. if patchStrategy:"merge is set they get merged, otherwise they get replaced.

... you probably interpolate my questions about the behavior of the rest of the fields.

in general

required parameters of sub structs have to be named, otherwise we have to created copies of them and make copies and make all parameters optional.

I tried to keep it simple for this initial approach and just make a copy of the top level and make required parameter optional

for lists as mentioned above it depends on the patchStrategy . an example of those can be found at https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.27/#podspec-v1-core where you see Container has merge as strategy set. when you scroll through the podSpec, e.g. readinessGates does not has merge set.

I'll try to add more tests to lib-common which should cover those scenarios

added more tests in lib-common

@gibizer let me know if the above answers and the enhanced tests in the lib-common pr answers your questions

Thank you for the explanation and the extra test coverage. Can we document somehow the behavior you described above in the doc of this field so that the end user will know what to expect?

yes that makes sense. I'll do that

updated the lib-common PR to describe the behavior

gibizer

This looks OK to me now. I will go and review the lib-common dependency.

gibizer · 2023-07-13T09:24:20Z

api/v1beta1/keystoneapi_types.go

+// KeystoneAPIOverrideSpec to override the generated manifest of several child resources.
+type KeystoneAPIOverrideSpec struct {
+	// Override configuration for the Route created to serve traffic to the service
+	Route *route.OverrideSpec `json:"route,omitempty"`


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

openshift-merge-robot · 2023-07-21T18:28:59Z

PR needs rebase.

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.

rabi · 2023-07-26T02:40:16Z

@stuggi Any plan for rebasing this and the lib-common PR soon? Looking at the frequent 504 gateway timeouts in jobs[1], I suspect we need to increase haproxy.router.openshift.io/timeout from the default 30s(?).

[1] https://logserver.rdoproject.org/10/410/740a298105dfd2f689f5ce2e36d22c369ec2ae33/github-check/openstack-operator-crc-podified-edpm-baremetal/d17206e/job-output.txt

gibizer · 2023-07-27T14:26:00Z

After talking to @stuggi this might not needed in the service operators if we move the route creation to the openstack-operator level instead. That seems to be an idea worth to explore.

openshift-ci · 2023-08-08T06:02:40Z

@stuggi: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/images	`e2b8581`	link	true	`/test images`
ci/prow/precommit-check	`e2b8581`	link	true	`/test precommit-check`
ci/prow/keystone-operator-build-deploy-kuttl	`e2b8581`	link	true	`/test keystone-operator-build-deploy-kuttl`
ci/prow/functional	`e2b8581`	link	true	`/test functional`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

stuggi · 2023-10-02T07:04:10Z

addressed by openstack-k8s-operators/openstack-operator#416

openshift-ci bot requested review from dprince and olliewalsh July 4, 2023 15:53

openshift-ci bot added the do-not-merge/hold label Jul 4, 2023

openshift-ci bot added the approved label Jul 4, 2023

This was referenced Jul 4, 2023

Add support for configuring route annotations openstack-k8s-operators/lib-common#288

Closed

Allow customize route via route.Override openstack-k8s-operators/lib-common#293

Merged

gibizer reviewed Jul 5, 2023

View reviewed changes

openshift-merge-robot added the needs-rebase label Jul 7, 2023

stuggi force-pushed the route_overrides branch from 80aa5f0 to 8d65148 Compare July 11, 2023 07:30

openshift-merge-robot removed the needs-rebase label Jul 11, 2023

stuggi force-pushed the route_overrides branch 2 times, most recently from 5374983 to 8e4d6bf Compare July 11, 2023 14:11

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

stuggi force-pushed the route_overrides branch 3 times, most recently from 447b8c7 to 2dd623f Compare July 13, 2023 09:17

gibizer reviewed Jul 13, 2023

View reviewed changes

stuggi force-pushed the route_overrides branch from 2dd623f to 2ac5697 Compare July 13, 2023 14:31

stuggi force-pushed the route_overrides branch from 2ac5697 to e2b8581 Compare July 14, 2023 06:41

openshift-merge-robot added the needs-rebase label Jul 21, 2023

stuggi closed this Oct 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow customize route via spec.Override.Route #269

Allow customize route via spec.Override.Route #269

stuggi commented Jul 4, 2023 •

edited

Loading

stuggi commented Jul 4, 2023

openshift-ci bot commented Jul 4, 2023

gibizer Jul 5, 2023

stuggi Jul 7, 2023

stuggi Jul 12, 2023

stuggi Jul 12, 2023

gibizer Jul 12, 2023

stuggi Jul 13, 2023

stuggi Jul 13, 2023

gibizer Jul 13, 2023

gibizer left a comment

gibizer Jul 13, 2023

openshift-merge-robot commented Jul 21, 2023

rabi commented Jul 26, 2023

gibizer commented Jul 27, 2023

openshift-ci bot commented Aug 8, 2023

stuggi commented Oct 2, 2023

Allow customize route via spec.Override.Route #269

Allow customize route via spec.Override.Route #269

Conversation

stuggi commented Jul 4, 2023 • edited Loading

stuggi commented Jul 4, 2023

openshift-ci bot commented Jul 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibizer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-merge-robot commented Jul 21, 2023

rabi commented Jul 26, 2023

gibizer commented Jul 27, 2023

openshift-ci bot commented Aug 8, 2023

stuggi commented Oct 2, 2023

stuggi commented Jul 4, 2023 •

edited

Loading