Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat/configurable ports #5540

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

feat/configurable ports #5540

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
00b79f5
feat: add configuration options for all ports
Argannor Apr 2, 2024
ded0a47
feat: allow ports for providers, functions and services to be specifi…
Argannor Apr 3, 2024
089f7a9
chore: formatting
Argannor Apr 3, 2024
3636a4f
refactor: use constants
Argannor Apr 3, 2024
8a7db1f
feat: disallow customization of webhook service port
Argannor Jun 2, 2024
9e66345
fix: remove duplication
Argannor Jun 2, 2024
fb52530
fix: set HEALTH_PROBE_BIND_ADDRESS in helm chart
Argannor Jun 2, 2024
73d8f65
chore: document ServiceWithPort override
Argannor Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions apis/pkg/v1beta1/deployment_runtime_config_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package v1beta1

import (
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

Expand Down Expand Up @@ -59,6 +60,10 @@ type ServiceTemplate struct {
// Metadata contains the configurable metadata fields for the Service.
// +optional
Metadata *ObjectMeta `json:"metadata,omitempty"`

// Spec contains the configurable spec fields for the Service object.
// +optional
Spec *corev1.ServiceSpec `json:"spec,omitempty"`
}

// ServiceAccountTemplate is the template for the ServiceAccount object.
Expand Down
6 changes: 6 additions & 0 deletions apis/pkg/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions cluster/charts/crossplane/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ and their default values.
| `imagePullSecrets` | The imagePullSecret names to add to the Crossplane ServiceAccount. | `{}` |
| `leaderElection` | Enable [leader election](https://docs.crossplane.io/latest/concepts/pods/#leader-election) for the Crossplane pod. | `true` |
| `metrics.enabled` | Enable Prometheus path, port and scrape annotations and expose port 8080 for both the Crossplane and RBAC Manager pods. | `false` |
| `metrics.port` | The port the metrics server listens on. | `""` |
| `nodeSelector` | Add `nodeSelectors` to the Crossplane pod deployment. | `{}` |
| `packageCache.configMap` | The name of a ConfigMap to use as the package cache. Disables the default package cache `emptyDir` Volume. | `""` |
| `packageCache.medium` | Set to `Memory` to hold the package cache in a RAM backed file system. Useful for Crossplane development. | `""` |
Expand All @@ -101,6 +102,7 @@ and their default values.
| `rbacManager.replicas` | The number of RBAC Manager pod `replicas` to deploy. | `1` |
| `rbacManager.skipAggregatedClusterRoles` | Don't install aggregated Crossplane ClusterRoles. | `false` |
| `rbacManager.tolerations` | Add `tolerations` to the RBAC Manager pod deployment. | `[]` |
| `readiness.port` | The port the readyz server listens on. | `""` |
| `registryCaBundleConfig.key` | The ConfigMap key containing a custom CA bundle to enable fetching packages from registries with unknown or untrusted certificates. | `""` |
| `registryCaBundleConfig.name` | The ConfigMap name containing a custom CA bundle to enable fetching packages from registries with unknown or untrusted certificates. | `""` |
| `replicas` | The number of Crossplane pod `replicas` to deploy. | `1` |
Expand All @@ -124,6 +126,7 @@ and their default values.
| `serviceAccount.customAnnotations` | Add custom `annotations` to the Crossplane ServiceAccount. | `{}` |
| `tolerations` | Add `tolerations` to the Crossplane pod deployment. | `[]` |
| `webhooks.enabled` | Enable webhooks for Crossplane and installed Provider packages. | `true` |
| `webhooks.port` | The port the webhook server listens on. | `""` |

### Command Line

Expand Down
22 changes: 19 additions & 3 deletions cluster/charts/crossplane/templates/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,14 +130,14 @@ spec:
port: readyz
ports:
- name: readyz
containerPort: 8081
containerPort: {{ .Values.readiness.port | default "8081" }}
{{- if .Values.metrics.enabled }}
- name: metrics
containerPort: 8080
containerPort: {{ .Values.metrics.port | default "8080" }}
{{- end }}
{{- if .Values.webhooks.enabled }}
- name: webhooks
containerPort: 9443
containerPort: {{ .Values.webhooks.port | default "9443" }}
{{- end }}
{{- with .Values.securityContextCrossplane }}
securityContext:
Expand Down Expand Up @@ -174,6 +174,22 @@ spec:
- name: "WEBHOOK_ENABLED"
value: "false"
{{- end }}
{{- if and .Values.webhooks.enabled .Values.webhooks.port }}
- name: "WEBHOOK_PORT"
value: "{{ .Values.webhooks.port }}"
{{- end}}
{{- if and .Values.webhooks.enabled .Values.webhooks.port }}
- name: "WEBHOOK_PORT"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks repeat of above ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, removed the duplicate

value: "{{ .Values.webhooks.port }}"
{{- end}}
{{- if and .Values.metrics.enabled .Values.metrics.port }}
- name: "METRICS_BIND_ADDRESS"
value: ":{{ .Values.metrics.port }}"
{{- end}}
{{- if .Values.readiness.port }}
- name: "HealthProbeBindAddress"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HEALTH_PROBE_BIND_ADDRESS to match the env defined in command struct below ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it, thanks

value: ":{{ .Values.readiness.port }}"
{{- end}}
- name: "TLS_SERVER_SECRET_NAME"
value: crossplane-tls-server
- name: "TLS_SERVER_CERTS_DIR"
Expand Down
4 changes: 2 additions & 2 deletions cluster/charts/crossplane/templates/service.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,6 @@ spec:
release: {{ .Release.Name }}
ports:
- protocol: TCP
port: 9443
targetPort: 9443
port: {{ .Values.webhooks.port | default "9443" }}
Copy link
Contributor

@ravilr ravilr Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

crossplane/cluster/charts/crossplane/templates/deployment.yaml

Line 98 in 21af307

- name: "WEBHOOK_SERVICE_PORT"
in the crossplane initializer which it uses to setup WebhookConfiguration resources..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. the runtime_defaults.go#serviceFromRuntimeConfig method to override it if present,
  2. a new default override ServiceWithPort(serviceName string, port int) to be added to the allOverrides method

I'm unsure which one is actually less surprising when reading the code. I think I'm leaning towards the second one

Copy link
Contributor

@ravilr ravilr May 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. In case of crossplane core cli, the newly being added --webhook-port arg 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 webhook Service spec port to 9443 as before.

  2. 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.

cc @phisco @turkenh for the maintainers to weigh in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The chart now hard-codes 9443 as the service port and an override was added to ensure the port is always 9443. Test case was adjusted accordingly

targetPort: {{ .Values.webhooks.port | default "9443" }}
{{- end }}
8 changes: 8 additions & 0 deletions cluster/charts/crossplane/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ service:
webhooks:
# -- Enable webhooks for Crossplane and installed Provider packages.
enabled: true
# -- The port the webhook server listens on.
port: ""

rbacManager:
# -- Deploy the RBAC Manager pod and its required roles.
Expand Down Expand Up @@ -146,6 +148,12 @@ securityContextRBACManager:
metrics:
# -- Enable Prometheus path, port and scrape annotations and expose port 8080 for both the Crossplane and RBAC Manager pods.
enabled: false
# -- The port the metrics server listens on.
port: ""

readiness:
# -- The port the readyz server listens on.
port: ""

# -- Add custom environmental variables to the Crossplane pod deployment.
# Replaces any `.` in a variable name with `_`. For example, `SAMPLE.KEY=value1` becomes `SAMPLE_KEY=value1`.
Expand Down
Loading
Loading