Skip to content

fix: NetworkPolicies of Control-Planes #1701

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

Closed
wants to merge 2 commits into from
Closed

fix: NetworkPolicies of Control-Planes #1701

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7fe1d3b
fix: NetworkPolicies of Control-Planes
leoncamel Apr 17, 2024
b7eb25e
fix: revert some typos
leoncamel Apr 19, 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
20 changes: 12 additions & 8 deletions chart/templates/networkpolicy.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ spec:
- ports:
- port: 443
- port: 8443
to:
- to:
Copy link
Member

@FabianKramm FabianKramm Apr 18, 2024

Choose a reason for hiding this comment

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

This changes logic from an and to an or, why is this required? This basically allows communication with all pods on port 443 and 8443 as well as all ports on the control-plane

Copy link
Author

@leoncamel leoncamel Apr 19, 2024

Choose a reason for hiding this comment

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

Same as previous one. I misunderstand the grammar of NetworkPolicyEgressRule. Just revert back

- podSelector:
matchLabels:
release: {{ .Release.Name }}
Expand Down Expand Up @@ -78,21 +78,25 @@ spec:
matchLabels:
release: {{ .Release.Name }}
egress:
# Allows outgoing connections to all pods with
# port 443, 8443 or 6443. This is needed for host Kubernetes
# access
# Allows outgoing connections to all necessary pods with
# port 443, 8443 or 6443 or system dns. This is needed for
# host Kubernetes access.
- ports:
- port: 443
- port: 8443
- port: 6443
# Allows outgoing connections to all vcluster workloads
# or kube system dns server
- port: 53
protocol: UDP
- port: 53
protocol: TCP
Copy link
Member

@FabianKramm FabianKramm Apr 18, 2024

Choose a reason for hiding this comment

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

Since we already have the rule below that selects DNS in the kube-system namespace, why is this still required?

Copy link
Author

@leoncamel leoncamel Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The previous NetworkPolicy vc-work-{{ .Release.Name }} will work on workload node based on its podSelector:

  podSelector:
    matchLabels:
      vcluster.loft.sh/managed-by: {{ .Release.Name }}

And, the control-plane of a vcluster still need to communicate host's kube-dns. That is why we want add this rule. And it did crash without this rule.

Meanwhile, the original version of first podSelector: {}, is too loose. We only need vcluster's control-plane to communicate necessary nodes, like:

        - podSelector:
            matchLabels:
              release: {{ .Release.Name }}

- to:
- podSelector: {}
- podSelector:
matchLabels:
release: {{ .Release.Name }}
- namespaceSelector:
matchLabels:
kubernetes.io/metadata.name: 'kube-system'
podSelector:
- podSelector:
Copy link
Member

@FabianKramm FabianKramm Apr 18, 2024

Choose a reason for hiding this comment

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

Is this necessary? Since vCluster should only communicate with pods in the namespace kube-system and the labels k8s-app=kube-dns, this would change the logic to an OR and then basically allow communication with all pods in the kube-system namespace.

Copy link
Author

@leoncamel leoncamel Apr 19, 2024

Choose a reason for hiding this comment

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

OK. I misunderstand the grammar of NetworkPolicyEgressRule. Just revert back

matchLabels:
k8s-app: kube-dns
policyTypes:
Expand Down