Skip to content

revert(operator): remove operator-managed NetworkPolicy - #560

Open
miledxz wants to merge 1 commit into
mainfrom
revert/network-policy
Open

revert(operator): remove operator-managed NetworkPolicy#560
miledxz wants to merge 1 commit into
mainfrom
revert/network-policy

Conversation

@miledxz

@miledxz miledxz commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Removes the operator-managed NetworkPolicy feature that was introduced in #473 and hardened in #514.

The generated policy restricted:

  • client port 6379 to the same namespace only
  • admin port 9999 to the operator and peer Dragonfly pods only

This was disruptive for the common setup where databases and workloads live in separate namespaces (#546) and blocked external Prometheus / VictoriaMetrics scrapers from reaching /metrics (#495).

This PR fully reverts the feature to the pre-#473 state. Users who need network restrictions can apply their own NetworkPolicy, which the operator will no longer reconcile away.

Breaking change

Deployments relying on the operator to create a NetworkPolicy (via the default or networkPolicyEnabled: true) will no longer get one. The networkPolicyEnabled field is removed from the CRD; any manifests setting it must drop the field.

Fixes #546
Fixes #495

@miledxz
miledxz requested a review from Abhra303 July 9, 2026 05:27
@moredure moredure mentioned this pull request Jul 9, 2026
3 tasks
@ashotland

Copy link
Copy Markdown
Contributor

Should we add some tests to avoid re-introducing that ?

@miledxz

miledxz commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Should we add some tests to avoid re-introducing that ?

yes, we can do it for sure

{{- toYaml .Values.rbacProxy.resources | nindent 12 }}
{{- end }}
- name: manager
command:

@moredure moredure Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

version of chart should be updated as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think yes but not sure cc @Abhra303

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

right now we release both with same version at the same time.

@moredure

moredure commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

@miledxz is it worth splitting into changes to the app and changes to the helm chart?

@miledxz

miledxz commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

@miledxz is it worth splitting into changes to the app and changes to the helm chart?

I think we can do that

@Abhra303

Abhra303 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

@miledxz is it worth splitting into changes to the app and changes to the helm chart?

yes, it is worth it to split.

@alx8

alx8 commented Aug 6, 2026

Copy link
Copy Markdown

Production data point supporting this revert — v1.6.1, entire ArgoCD installation down

Adding a concrete incident in case it helps prioritise merging, since this has been approved for a while.

On 2026-08-05 we upgraded dragonfly-operator to v1.6.1 on a production cluster and it took our whole ArgoCD installation down.

The part that makes this sharper than #546: in v1.6.1 the CRD ships networkPolicyEnabled with default: true. Users who never opted in, and whose manifests don't mention the field, get the NetworkPolicy created for them on upgrade. There is no opt-out you can pre-set beforehand if you don't know the field exists — and it isn't visible in a values diff, because the CRD is templated rather than living in crds/.

Generated policy on our cluster:

spec:
  podSelector:
    matchLabels:
      app: dragonfly-ops
      app.kubernetes.io/name: dragonfly
      app.kubernetes.io/part-of: dragonfly
  policyTypes: [Ingress]
  ingress:
  - from:
    - podSelector: {}          # same namespace only
    ports:
    - {port: 6379, protocol: TCP}

Impact. ArgoCD uses Dragonfly as its Redis cache from another namespace. All 29 Applications went Sync: Unknown and auto-sync refused on every one of them — status is Unknown because the resource tree can't be written to the cache. ArgoCD therefore could not deploy its own fix; recovery required a manual kubectl patch on the Dragonfly CRs. A separate app also lost its session store and began failing readiness probes.

Two things worth noting for anyone diagnosing this:

  1. NetworkPolicy drops are silent, so clients report i/o timeout, not connection refused. That distinction is the fastest way to tell a policy block from a dead listener.
  2. Dragonfly itself stays perfectly healthy throughout — pod Running, CR Ready, Service endpoints present. Nothing about the Dragonfly instance points at the cause.

One request on the removal itself. Since this PR drops networkPolicyEnabled from the CRD, please call out in the release notes that manifests setting it must remove the field. That's the same advertising gap #546 asked for, just in the other direction — anyone who worked around the v1.6.x default by setting networkPolicyEnabled: false will be carrying that field when the revert lands.

Separately, while this remains unmerged: the CRD field's description currently reads "The NetworkPolicy restricts admin port access to operator and peer pods only." That describes only the 9999 rule, not the 6379 one that actually causes the breakage. If this PR stalls further, correcting that description would be a cheap standalone improvement.

@Abhra303

Copy link
Copy Markdown
Contributor

@miledxz can you fix the conflict and merge it please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NetworkPolicy blocks metrics scraping from external pods Cluster access restriction in v1.6.0 should be advertised better

5 participants