Revert to preferring IP addresses for Node listeners by default (#244)

nightkr · sbernauer · web-flow · commit 0b38a1cf2120 · 2024-11-06T16:00:13.000Z
* Revert to preferring IP addresses for Node listeners by default

* Changelog

* Docs

* Use op-rs release. Also fix tests

* docs: Add newline

---------

Co-authored-by: Sebastian Bernauer &lt;sebastian.bernauer@stackable.tech&gt;
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,14 +6,14 @@ All notable changes to this project will be documented in this file.
 
 ### Added
 
-- `Listener.status.addresses` can now be configured to prefer either IP addresses or DNS hostnames ([#233]).
+- `Listener.status.addresses` can now be configured to prefer either IP addresses or DNS hostnames ([#233], [#244]).
 - The operator can now run on Kubernetes clusters using a non-default cluster domain.
   Use the env var `KUBERNETES_CLUSTER_DOMAIN` or the operator Helm chart property `kubernetesClusterDomain` to set a non-default cluster domain ([#237]).
 
 ### Changed
 
 - `Listener.status.addresses` for NodePort listeners now includes replicas that are currently unavailable ([#231]).
-- `Listener.status.addresses` now defaults to DNS hostnames for all service types (previously NodePort and ClusterIP would prefer IP addresses, [#233]).
+- BREAKING: `Listener.status.addresses` now defaults to DNS hostnames for ClusterIP services, rather than IP addresses ([#233], [#244]).
 - Stale Listener subobjects will now be deleted ([#232]).
 - Tagged Listener Services with the SDP labels ([#232]).
 
@@ -30,6 +30,7 @@ All notable changes to this project will be documented in this file.
 [#234]: https://github.com/stackabletech/listener-operator/pull/234
 [#237]: https://github.com/stackabletech/listener-operator/pull/237
 [#238]: https://github.com/stackabletech/listener-operator/pull/238
+[#244]: https://github.com/stackabletech/listener-operator/pull/244
 
 ## [24.7.0] - 2024-07-24
 
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.nix b/Cargo.nix
diff --git a/Cargo.toml b/Cargo.toml
@@ -21,7 +21,7 @@ prost = "0.13"
 prost-types = "0.13"
 serde = "1.0"
 snafu = "0.8"
-stackable-operator = { git = "https://github.com/stackabletech/operator-rs.git", tag = "stackable-operator-0.80.0" }
+stackable-operator = { git = "https://github.com/stackabletech/operator-rs.git", tag = "stackable-operator-0.81.0" }
 strum = { version = "0.26", features = ["derive"] }
 socket2 = { version = "0.5", features = ["all"] }
 tokio = { version = "1.40", features = ["full"] }
diff --git a/crate-hashes.json b/crate-hashes.json
diff --git a/deploy/helm/listener-operator/crds/crds.yaml b/deploy/helm/listener-operator/crds/crds.yaml
@@ -25,14 +25,17 @@ spec:
               description: Defines a policy for how [Listeners](https://docs.stackable.tech/home/nightly/listener-operator/listener) should be exposed. Read the [ListenerClass documentation](https://docs.stackable.tech/home/nightly/listener-operator/listenerclass) for more information.
               properties:
                 preferredAddressType:
-                  default: Hostname
+                  default: HostnameConservative
                   description: |-
-                    Whether addresses should prefer using the IP address (`IP`) or the hostname (`Hostname`).
+                    Whether addresses should prefer using the IP address (`IP`) or the hostname (`Hostname`). Can also be set to `HostnameConservative`, which will use `IP` for `NodePort` service types, but `Hostname` for everything else.
 
-                    The other type will be used if the preferred type is not available. By default `Hostname` is used.
+                    The other type will be used if the preferred type is not available.
+
+                    Defaults to `HostnameConservative`.
                   enum:
                     - Hostname
                     - IP
+                    - HostnameConservative
                   type: string
                 serviceAnnotations:
                   additionalProperties:
diff --git a/docs/modules/listener-operator/pages/listenerclass.adoc b/docs/modules/listener-operator/pages/listenerclass.adoc
@@ -66,7 +66,7 @@ Additionally, many cloud providers charge for load-balanced traffic.
 [#addresstype]
 == Address types
 
-The Stackable Listener Operator supports both IP addresses and DNS hostnames. The preferred address type for a given ListenerClass can be configured using the `ListenerClass.spec.preferredAddressType` field. If no `preferredAddressType` is specified then it defaults to xref:#addresstype-hostname[].
+The Stackable Listener Operator supports both IP addresses and DNS hostnames. The preferred address type for a given ListenerClass can be configured using the `ListenerClass.spec.preferredAddressType` field. If no `preferredAddressType` is specified then it defaults to xref:#addresstype-hostname-conservative[].
 
 NOTE: If the preferred address type is not supported for a given environment then another type will be used.
 
@@ -81,6 +81,12 @@ but does not require any special client configuration (beyond what the xref:#ser
 
 The DNS hostname of a resource. Clients must be able to resolve these addresses in order to connect, which may require special DNS configuration.
 
+[#addresstype-hostname-conservative]
+=== HostnameConservative
+
+A pseudo-addresstype that is equivalent to xref:#addresstype-ip[] for xref:#servicetype-nodeport[] services, and xref:#addresstype-hostname[] for all others.
+This means that we default to hostnames where "safe", but don't assume that nodes are resolvable by external clients.
+
 == Default ListenerClasses
 
 The Stackable Data Platform assumes the existence of a few predefined ListenerClasses, and will use them by default as appropriate:
diff --git a/rust/operator-binary/src/csi_server/node.rs b/rust/operator-binary/src/csi_server/node.rs
@@ -464,7 +464,7 @@ async fn local_listener_addresses_for_pod(
             })?;
 
         Ok(node_primary_addresses(&node)
-            .pick(listener_class.spec.preferred_address_type)
+            .pick(listener_class.spec.resolve_preferred_address_type())
             .map(|(address, address_type)| ListenerIngress {
                 // nodes: Some(vec![node_name.to_string()]),
                 address: address.to_string(),
diff --git a/rust/operator-binary/src/listener_controller.rs b/rust/operator-binary/src/listener_controller.rs
@@ -364,6 +364,7 @@ pub async fn reconcile(
         .add(&ctx.client, svc)
         .await
         .context(ApplyServiceSnafu { svc: svc_ref })?;
+    let preferred_address_type = listener_class.spec.resolve_preferred_address_type();
 
     let nodes: Vec<Node>;
     let kubernetes_service_fqdn: String;
@@ -384,9 +385,7 @@ pub async fn reconcile(
             .await?;
             addresses = nodes
                 .iter()
-                .flat_map(|node| {
-                    node_primary_addresses(node).pick(listener_class.spec.preferred_address_type)
-                })
+                .flat_map(|node| node_primary_addresses(node).pick(preferred_address_type))
                 .collect::<Vec<_>>();
             ports = svc
                 .spec
@@ -408,7 +407,7 @@ pub async fn reconcile(
                         ip: ingress.ip.as_deref(),
                         hostname: ingress.hostname.as_deref(),
                     }
-                    .pick(listener_class.spec.preferred_address_type)
+                    .pick(preferred_address_type)
                 })
                 .collect();
             ports = svc
@@ -422,7 +421,7 @@ pub async fn reconcile(
         }
         ServiceType::ClusterIP => {
             let cluster_domain = &cluster_info.cluster_domain;
-            addresses = match listener_class.spec.preferred_address_type {
+            addresses = match preferred_address_type {
                 AddressType::Ip => svc
                     .spec
                     .iter()
diff --git a/tests/templates/kuttl/smoke-nodeport/05-create-listenerclass.yaml b/tests/templates/kuttl/smoke-nodeport/05-create-listenerclass.yaml
@@ -0,0 +1,5 @@
+---
+apiVersion: kuttl.dev/v1beta1
+kind: TestStep
+commands:
+  - script: envsubst '$NAMESPACE' < 05_listenerclass.yaml | kubectl apply -n $NAMESPACE -f -
diff --git a/tests/templates/kuttl/smoke-nodeport/05_listenerclass.yaml.j2 b/tests/templates/kuttl/smoke-nodeport/05_listenerclass.yaml.j2
@@ -2,7 +2,7 @@
 apiVersion: listeners.stackable.tech/v1alpha1
 kind: ListenerClass
 metadata:
-  name: listener-operator-test-smoke-nodeport
+  name: listener-operator-test-smoke-nodeport-$NAMESPACE
 spec:
   serviceType: NodePort
   preferredAddressType: {{ test_scenario['values']['addressType'] }}
diff --git a/tests/templates/kuttl/smoke-nodeport/10-assert.yaml.j2 b/tests/templates/kuttl/smoke-nodeport/10-assert.yaml.j2
@@ -13,4 +13,10 @@ metadata:
   name: listener-nginx-long-name-approaching-k8s-limits-0
 status:
   ingressAddresses:
+# FIXME: This test seems to assume that Nodes always have a hostname *and* a IP address (which should be the case on
+# e.g. kind)! We probably need to relax this assertion at a later point of time
+{% if test_scenario['values']['addressType'] == 'HostnameConservative' %}
+    - addressType: IP
+{% else %}
     - addressType: {{ test_scenario['values']['addressType'] }}
+{% endif %}
diff --git a/tests/templates/kuttl/smoke-nodeport/10-create-nginx-statefulset.yaml b/tests/templates/kuttl/smoke-nodeport/10-create-nginx-statefulset.yaml
@@ -1,47 +1,5 @@
 ---
-apiVersion: apps/v1
-kind: StatefulSet
-metadata:
-  # Intentionally long name to trigger #110
-  name: nginx-long-name-approaching-k8s-limits
-spec:
-  serviceName: nginx
-  selector:
-    matchLabels:
-      app: nginx
-  replicas: 2
-  template:
-    metadata:
-      labels:
-        app: nginx
-    spec:
-      containers:
-        - name: nginx
-          image: nginxinc/nginx-unprivileged:1.25.2
-          ports:
-            - name: http
-              containerPort: 8080
-          volumeMounts:
-            - name: listener
-              mountPath: /listener
-            - name: metadata
-              mountPath: /usr/share/nginx/html/
-      volumes:
-        - name: metadata
-          downwardAPI:
-            items:
-              - path: pod-name
-                fieldRef:
-                  fieldPath: metadata.name
-  volumeClaimTemplates:
-    - metadata:
-        name: listener
-        annotations:
-          listeners.stackable.tech/listener-class: listener-operator-test-smoke-nodeport
-      spec:
-        accessModes:
-          - ReadWriteMany
-        storageClassName: listeners.stackable.tech
-        resources:
-          requests:
-            storage: 1
+apiVersion: kuttl.dev/v1beta1
+kind: TestStep
+commands:
+  - script: envsubst '$NAMESPACE' < 10_nginx-statefulset.yaml | kubectl apply -n $NAMESPACE -f -
diff --git a/tests/templates/kuttl/smoke-nodeport/10_nginx-statefulset.yaml b/tests/templates/kuttl/smoke-nodeport/10_nginx-statefulset.yaml
@@ -0,0 +1,47 @@
+---
+apiVersion: apps/v1
+kind: StatefulSet
+metadata:
+  # Intentionally long name to trigger #110
+  name: nginx-long-name-approaching-k8s-limits
+spec:
+  serviceName: nginx
+  selector:
+    matchLabels:
+      app: nginx
+  replicas: 2
+  template:
+    metadata:
+      labels:
+        app: nginx
+    spec:
+      containers:
+        - name: nginx
+          image: nginxinc/nginx-unprivileged:1.25.2
+          ports:
+            - name: http
+              containerPort: 8080
+          volumeMounts:
+            - name: listener
+              mountPath: /listener
+            - name: metadata
+              mountPath: /usr/share/nginx/html/
+      volumes:
+        - name: metadata
+          downwardAPI:
+            items:
+              - path: pod-name
+                fieldRef:
+                  fieldPath: metadata.name
+  volumeClaimTemplates:
+    - metadata:
+        name: listener
+        annotations:
+          listeners.stackable.tech/listener-class: listener-operator-test-smoke-nodeport-$NAMESPACE
+      spec:
+        accessModes:
+          - ReadWriteMany
+        storageClassName: listeners.stackable.tech
+        resources:
+          requests:
+            storage: 1
diff --git a/tests/test-definition.yaml b/tests/test-definition.yaml
@@ -7,6 +7,7 @@ dimensions:
     values:
       - IP
       - Hostname
+      - HostnameConservative
 tests:
   - name: smoke-nodeport
     dimensions:
