Draft: feat/configurable ports #5540
Conversation
Signed-off-by: Argannor <arga@argannor.com>
…ed in runtime configs Signed-off-by: Argannor <arga@argannor.com>
Signed-off-by: Argannor <arga@argannor.com>
Signed-off-by: Argannor <arga@argannor.com>
| value: "{{ .Values.webhooks.port }}" | ||
| {{- end}} | ||
| {{- if and .Values.webhooks.enabled .Values.webhooks.port }} | ||
| - name: "WEBHOOK_PORT" |
There was a problem hiding this comment.
this looks repeat of above ?
| value: ":{{ .Values.metrics.port }}" | ||
| {{- end}} | ||
| {{- if .Values.readiness.port }} | ||
| - name: "HealthProbeBindAddress" |
There was a problem hiding this comment.
HEALTH_PROBE_BIND_ADDRESS to match the env defined in command struct below ?
| @@ -20,6 +20,6 @@ spec: | |||
| release: {{ .Release.Name }} | |||
| ports: | |||
| - protocol: TCP | |||
| port: 9443 | |||
| targetPort: 9443 | |||
| port: {{ .Values.webhooks.port | default "9443" }} | |||
There was a problem hiding this comment.
the webhook Service port here itself, can remain the same 9443, without the need for it to be configurable right ?
Only the crossplane pod's webhook port, that is Service's targetPort below, needs configurability..
Changing the Service port (which IMO unnecessary) would require changes in other places like
in the crossplane initializer which it uses to setup WebhookConfiguration resources..There was a problem hiding this comment.
You're right @ravilr, I didn't think about this before, but the service port can remain the same. I will refactor this again, but I need your input on this, as the override makes it a bit awkward:
- In the runtime config the user can provide a service port with a port diverging from this
- The runtime config just embeds the upstream corev1.ServiceSpec so we can't really prevent this from happening
Would you prefer
- the
runtime_defaults.go#serviceFromRuntimeConfigmethod to override it if present, - a new default override
ServiceWithPort(serviceName string, port int)to be added to theallOverridesmethod
I'm unsure which one is actually less surprising when reading the code. I think I'm leaning towards the second one
There was a problem hiding this comment.
-
In case of crossplane core cli, the newly being added
--webhook-portarg appears to be unambiguous already, capturing the webhook server listener port in the crossplane pod's core container. So, i think the crossplane chart can continue hard-coding the webhookServicespec port to 9443 as before. -
In case of pkg DeploymentRuntimeConfig, 2nd option from your proposal sounds good to me, since we are already expecting the DRC's serviceTemplate.spec override to be matching on the port name
webhook.
| @@ -396,7 +414,16 @@ func ServiceWithSelectors(selectors map[string]string) ServiceOverride { | |||
| // ServiceWithAdditionalPorts adds additional ports to a Service. | |||
| func ServiceWithAdditionalPorts(ports []corev1.ServicePort) ServiceOverride { | |||
| return func(s *corev1.Service) { | |||
| s.Spec.Ports = append(s.Spec.Ports, ports...) | |||
| names := make(map[string]bool) | |||
| for _, p := range s.Spec.Ports { | |||
There was a problem hiding this comment.
Same here, for the runtime providers/functions, the DeploymentRuntimeConfig.spec.serviceTemplate.spec would need to keep the Service port same as 9443 ? with only customizing the targetPort/pod's port.
Service.spec.
ports:
- port: 9443 ## this can remain the same regardless of hostNetwork used for Provider's pod. this is Service network
protocol: TCP
targetPort: ${customizedHostNetworkPort} # this is pod network.
name: webhook
Changing the Service port (which IMO unnecessary) in the Provider's DRC.spec.ServiceTemplate.spec would require changes in other places like
in the CRDs conversion webhook spec, also..
|
Thank you for the review, I'm currently on sick leave and will get back to you as soon as I am able to. |
Description of your changes
When running crossplane (and providers/functions) in the host network the need to configure ports arises.
This MR has two parts:
DeploymentRuntimeConfigto override the default ports (metrics,webhook,grpc) in bothDeploymentandService.Additional merge requests would be necessary to
Putting this on draft to gather your feedback. Please let me know what you think about it and how to proceed with the follow ups.
Contributes to #5520
Crossplane Ports
The following configuration options (flags / environment variables) where added, with their defaults set to the current values:
--webhook-port=9443--metrics-bind-address=:8080--health-probe-bind-address=:8081Additionally these options were added to the values of the helm charts defaulting to unset:
webhooks.portmetrics.portreadiness.portNote that for the configuration options I chose to stick as close as possible to the underlying API for increased flexibility (thus the bind-address choice), while the helm chart is more oriented towards a consistent user experience (thus ports for all 3 options). Let me know what you think about it!
Provider, Functions, Service Ports
The core idea is to allow the user to specify the ports in the DeploymentRuntimeConfig (DRC). For this to work I had to add the
Specsection toServiceTemplate, meaning this PR introduces an API change which should be backwards compatible. Here's a brief summary of the changes:DeploymentWithOptionalPodScrapeAnnotationsuses the port number of a givenmetricsport if set in the DRC deployment template.DeploymentRuntimeWithAdditionalPortsonly adds ports not contained in the DRC deployment template (based on comparing their names).ServiceWithAdditionalPortsonly adds ports not contained in the DRC service template (based on comparing their names).Nameattribute of webhook ports in services towebhook(to facilitate the name comparison).Unit tests were added.
Housekeeping
I have:
make reviewableto ensure this PR is ready for review.Added or updated e2e tests.Linked a PR or a docs tracking issue to document this change.Addedbackport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.