villebro commented on code in PR #394:
URL:
https://github.com/apache/superset-kubernetes-operator/pull/394#discussion_r4146961616
##########
charts/superset-operator/values.yaml:
##########
@@ -119,6 +119,11 @@ affinity: {}
# See
[corev1.TopologySpreadConstraint](https://pkg.go.dev/k8s.io/api/core/v1#TopologySpreadConstraint).
topologySpreadConstraints: []
+# -- DNS policy for the manager pod.
+dnsPolicy: ""
+# -- DNS config for the manager pod.
Review Comment:
Please add the type link. I believe this is the correct annotation:
```suggestion
# -- DNS config for the manager pod.
# See
[corev1.PodDNSConfig](https://pkg.go.dev/k8s.io/api/core/v1#PodDNSConfig).
```
##########
charts/superset-operator/tests/optout_and_helpers_test.yaml:
##########
@@ -161,6 +161,51 @@ tests:
path: spec.replicas
value: 0
+ # --- dnsPolicy / dnsConfig ---
+ - it: omits dnsPolicy and dnsConfig by default
+ template: templates/deployment.yaml
+ asserts:
+ - notExists:
+ path: spec.template.spec.dnsPolicy
+ - notExists:
+ path: spec.template.spec.dnsConfig
+
+ - it: fails when dnsPolicy is None without nameservers
+ set:
+ dnsPolicy: None
+ dnsConfig:
+ options:
+ - name: ndots
+ value: "2"
+ template: templates/deployment.yaml
+ asserts:
+ - failedTemplate:
+ errorMessage: "dnsPolicy=None requires at least one
dnsConfig.nameservers entry"
Review Comment:
This can be removed as API server already gates this
##########
charts/superset-operator/values.schema.json:
##########
@@ -140,6 +140,32 @@
"description": "Topology spread constraints for the manager pod. See
https://pkg.go.dev/k8s.io/api/core/v1#TopologySpreadConstraint";,
"type": "array"
},
+ "dnsPolicy": {
+ "description": "DNS policy for the manager pod. Empty uses the
Kubernetes default (ClusterFirst).",
Review Comment:
Please remove the default value reference to avoid drift risk
##########
charts/superset-operator/values.yaml:
##########
@@ -119,6 +119,11 @@ affinity: {}
# See
[corev1.TopologySpreadConstraint](https://pkg.go.dev/k8s.io/api/core/v1#TopologySpreadConstraint).
topologySpreadConstraints: []
+# -- DNS policy for the manager pod.
+dnsPolicy: ""
Review Comment:
nit (non-blocking): We could maybe use `~` here. I know we've used `""`
elsewhere, but it could be a good idea to standardize on `~` throughout,
although that also needs a schema change to accept `null`, so that's a bigger
change. Maybe as a follow-up if you have spare cycles?
##########
charts/superset-operator/templates/deployment.yaml:
##########
@@ -21,6 +21,14 @@ and status, so the combination is never valid.
{{- if and (gt (int .Values.replicas) 1) (not .Values.leaderElection.enabled)
}}
{{- fail "replicas > 1 requires leaderElection.enabled=true" }}
{{- end }}
+{{- /*
+dnsPolicy None drops the cluster resolver entirely, so the pod has no
nameserver
+unless dnsConfig supplies one. The API server rejects that pod spec, which
would
+otherwise surface only as a stuck ReplicaSet after install.
+*/}}
+{{- if and (eq .Values.dnsPolicy "None") (not (dig "nameservers" nil
(.Values.dnsConfig | default dict))) }}
+{{- fail "dnsPolicy=None requires at least one dnsConfig.nameservers entry" }}
+{{- end }}
Review Comment:
Let's not echo API server policies in our checks. Deployment admission
already validates the pod template, so `helm install` fails with the API error
directly, not via a stuck `ReplicaSet`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
