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.

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

Ensure NetworkPolicy for seed API server #2339

Merged

Conversation

danielfoehrKn
Copy link
Contributor

@danielfoehrKn danielfoehrKn commented May 15, 2020

What this PR does / why we need it:
The gardenlet deploys Network Policies that should isolate the workload in the Shoot namespace in the seed from the Seed components.

This PR creates two controllers that are responsible for the creation and update of the network policy "allow-to-seed-apiserver".

Which issue(s) this PR fixes:
Fixes #2287

Special notes for your reviewer:
The dependency watchdog prober was relying on the allow-to-seed-apiserver NetworkPolicy which allowed a too broad set of ip ranges including public IPs. Now that the allow-to-seed-apiserver Policy only allows the Seed API server ip, the dependency watchdog prober needs the NetworkPolicy "allow-to-public".

@mvladev

Release note:

The NetworkPolicy 'allow-to-seed-apiserver'  has been improved to only allow the seed apiserver endpoint IPs. A new controller in the Gardenlet creates and updates the aforementioned NetworkPolicy. 
It is now possible to use a Kubernetes Cluster with private api server endpoints as a Seed cluster.

How to categorize this PR?

/area networking
/kind enhancement
/priority critical

Copy link

@mvladev mvladev left a comment

Choose a reason for hiding this comment

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

Few comments:

  • There should be only 1 workqueue in which you'll put the namespaces in which the networkpolicy is effective - shoot and garden NS.
  • The controller should listen for changes on Endpoints, Namespaces and NetworkPolicies.
  • When kubernetes Endpoint is updated then it will add to the namespaceQueue all namespaces in which this networkpolicy is effective - shoot and garden NS. It's better to do n O(1) then O(n).
  • When Namespace is added, it will add its name to the queue.
  • When NetworkPolicy is added which is the one we'll reconcile, it will put the namespace in which it is in the naemspaceQueue.
  • The controller would simply do a get for the kubernetes endpoint (using a lister / cached client), then try to check if the policy is correct / needs updates. it'll only operate on a single namespace (O(1)).

@danielfoehrKn
Copy link
Contributor Author

danielfoehrKn commented May 16, 2020

@mvladev

There should be only 1 workqueue in which you'll put the namespaces in which the networkpolicy is effective - shoot and garden NS.

Sure makes sense. Atm. the endpoint worker updates all NetworkPolicies - by putting it on the namespace queue we could spread the load on multiple workers.

When NetworkPolicy is added which is the one we'll reconcile, it will put the namespace in which it is in the naemspaceQueue.

I would rather have, like it is implemented now, the controller creating the NetworkPolicy instead of the shoot reconciliation creating an empty NetworkPolicy. What would be the advantage of the latter?

The controller should listen for changes on Endpoints, Namespaces and NetworkPolicies.

What is the use case for also watching NetworkPolicies?. As described above I would like to create and update it in the controller instead of applying it via helm chart in the reconcile.

The only use case I see for that is when someone alters the NetworkPolicy so that we reconcile it back to the desired state. Is that a valid one or is the overhead to much because most likely that will not happen?

EDIT: Updated to only work on one the namespace queue.

@mvladev
Copy link

mvladev commented May 17, 2020

What is the use case for also watching NetworkPolicies?. As described above I would like to create and update it in the controller instead of applying it via helm chart in the reconcile.

The only use case I see for that is when someone alters the NetworkPolicy so that we reconcile it back to the desired state. Is that a valid one or is the overhead to much because most likely that will not happen?

Well the idea is that it'll always react on changes. If for example someone deletes this network policy, nothing will re-create it as nothing will add it to the queue. The only option would be to restart the controller or edit the namespace or kubernetes endpoint.

@danielfoehrKn
Copy link
Contributor Author

@mvladev I was asking because for the core Kubernetes controllers do not watch Endpoints resources - so a manual edit of an endpoints resource will not be reconciled back to its desired state.
I can only suspect that a watch / informer for all endpoint resources in a large clusters is too costly (considering the benefits).
But I guess for the NetworkPolicies it might be ok.

I have now added a NetworkPolicy watch here.
I also added a pre-check in the namespace reconcile that prevents updates if the NetworkPolicy has already the desired state.
This prevents duplicate updates as the controller both creates / updates the NetworkPolicy but also watches it -> manual updates to the NetworkPolicy would otherwise trigger a second update.

Copy link
Member

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

Nice PR, looking forward to this feature! :)

@ghost
Copy link

ghost commented May 19, 2020

Command "/ to keep the NetworkPolicy "allow-to-seed-apiserver" in sync." either unknown, wrongly parameterized or not permitted for issue/PR.

@rfranzke
Copy link
Member

/area networking
/area security
/size l
/needs changes

@ghost ghost added area/networking Networking related area/security Security related size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs/changes labels May 19, 2020
@danielfoehrKn danielfoehrKn force-pushed the enhancement/networkpolicy-ctrl branch from 39a9577 to f25d6a4 Compare May 19, 2020 10:12
@rfranzke
Copy link
Member

/needs rebase

@danielfoehrKn
Copy link
Contributor Author

PTAL @mvladev

@rfranzke
Copy link
Member

/assign @mvladev @rfranzke

@ghost ghost assigned mvladev and rfranzke May 28, 2020
Copy link

@mvladev mvladev left a comment

Choose a reason for hiding this comment

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

Just a small nit

@danielfoehrKn
Copy link
Contributor Author

I think the test needs another test run - an unrelated test failed due to timeout.
Can I do that via a command?

Copy link
Member

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

/lgtm

@rfranzke rfranzke merged commit 37bf908 into gardener:master Jun 3, 2020
timuthy added a commit to gardener/gardener-extension-shoot-cert-service that referenced this pull request Jun 10, 2020
```improvement operator
Required network policies for Cert-Management have been aligned with the latest changes from Gardener (gardener/gardener#2339).
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/networking Networking related area/security Security related kind/enhancement Enhancement, improvement, extension size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sync kube-apiserver endpoints to allow-to-seed network policy
7 participants