First addon operator integration: CoreDNS

[justinsb]

Hidden behind a feature-flag, but when the UseAddonOperators flag is set,
we now use the cluster-addons CoreDNS operator instead of our built-in
manifests.

[justinsb]

Builds on #8119

[johngmyers]

Suggested change

	Copyright 2019 The Kubernetes Authors.
	Copyright 2020 The Kubernetes Authors.

[justinsb]

Sadly not ;-)

[johngmyers]

The default service account? Those are some really powerful rights being granted here and below. The effort of separating roles is wasted by not separating out service accounts.

[justinsb]

Agreed - I'll take it up upstream along with the idea of precreating RBAC

[johngmyers]

Why not 443?

[justinsb]

This was done upstream - moved to 443 with seemingly no consequence so I'll take it up there along with other changes!

[johngmyers]

I am alarmed by the sheer amount of insecurity and risk that this approach appears to be leading to. Code running with almost unbounded privileges and not a whole lot of segregation. And the point appears to be to download and run code directly from external sources, which is diametrically opposed to what my employer wants.

[justinsb]

So I think we've advanced part of the security posture here. I've hacked up the coredns operator a bit, but if we agree then I think we can upstream those changes.

The idea is that we can (and should) pre-create the RBAC objects that the addon is going to use: the ServiceAccount, the ClusterRole, the ClusterRoleBinding. Because ClusterRoles have to be inherited by the operator if they're going to be created, this doesn't actually cost us anything in terms of updates - updates that need new permissions would always require an operator update; that operator update now includes the new permissions.

This makes the operator now into an application-aware health-checker and configurer for the addon itself (primarily the deployment), which is really the goal.

I personally think this makes the addon operator story good from the point of view of "is every operator running with cluster-admin permissions" - the permissions look pretty reasonable to me... but please take a look @johngmyers !

Now I think there are two things that are not solved, but I think they might be better solved more generally (i.e. I think we should solve them, I'm not sure we should solve them for cluster-addons as they might apply everywhere):

• How do we restrict usage of the ServiceAccount? i.e. if I can create a pod in kube-system, then can I use any ServiceAccount I find there, and act as it. I suspect I'm just missing something here. In general, it's an argument for more namespaces, we might have problems with DNS specifically because I imagine lots of people expect it to be called kube-dns in kube-system.

• How do we prevent "non approved" images from running. Here I think kubernetes has a strong answer in ImagePolicyWebhook, but I don't know to what extent this is actually used.

It would be intriguing to actually combine the two, I think: I probably want much stricter controls on things running in kube-system than I do in some myapp application namespace.

I would propose that we proceed with the RBAC pre-creation, with this behind a feature-flag. I think if we treat the ability to modify CoreDNS objects as equivalent to being able to create Pods in kube-system then I don't think we're changing the security stance (?). CoreDNS objects are only mutable by cluster-admin, by default.

To try this out:

export KOPS_FEATURE_FLAGS="ClusterAddons,UseOperators"

kops create cluster addons.k8s.local --zones us-east-2a --channel file://channels/stable

kops edit cluster addons.k8s.local and add to spec:

  kubeDNS:
    provider: CoreDNS

kops update cluster addons.k8s.local --yes
kops export kubecfg addons.k8s.local --admin
kops validate cluster addons.k8s.local --wait=5m

[johngmyers]

The direction seems promising.

I've seen Slack messages from two sites that have moved a bunch of stuff from kube-system to their own namespaces. Kubernetes has fortunately made kube-system a lot less special than it used to be.

The need for the kube-dns Service in kube-system does make CoreDNS particularly troublesome. To site it in a different namespace would require the operator to manually sync the Endpoints from the other namespace (but it would only need rights to Service and Endpoints in kube-system).

Running an admission controller to limit the image registries is pretty easy. Would the image references in these manifests go through remapping in the assets phase or is this an area where lack of templating would cause a problem?

[johngmyers]

Sure, it's only list, but the apiGroups and resources are remarkably broad.

[justinsb]

Concur... I'll track it down upstream

[johngmyers]

Is patch/update needed on coredns or is it sufficient to only have it on coredns/status? Does the operator really need to be able to write its own instructions?

[justinsb]

Good question - that would be a nice restriction

[justinsb]

(I will take this up in the addon operator after this merges)

[justinsb]

The need for the kube-dns Service in kube-system does make CoreDNS particularly troublesome. To site it in a different namespace would require the operator to manually sync the Endpoints from the other namespace (but it would only need rights to Service and Endpoints in kube-system).

I agree, and maybe we could overcome that (including as you described). I definitely think it's worth trying to really lock down the permissions as much as possible here, both because it'll be a bigger project to move it out of kube-system (not that I'm ruling it out!) and because we're likely to copy these permissions to other operators.

Running an admission controller to limit the image registries is pretty easy. Would the image references in these manifests go through remapping in the assets phase or is this an area where lack of templating would cause a problem?

So we can make sure that the operator manifests go through remapping (if they're not doing so already) but for the manifests the operators deploy, by default I'd imagine they are not going to be sourced from something kops-controlled. We could make them kops-controlled (indeed, that might not be a bad thing from a reliability and security point of view). I'm proposing here that we would mirror upstream manifests, not that we would ourselves take on the definitions of those manifests. I'm not sure if we would do that mirroring in the kops repo (or some other similar place) or whether we would have the CLI mirror on the fly. I'm leaning towards the mirror-on-the-fly approach.

The other option for image mirroring is that we use the image mirroring functionality that is in containerd. Last I checked this would still go to the primary location on a mirror "cache miss", which was not really what I wanted.

[justinsb]

cc @johngmyers I've updated this so that the operator has much fewer permissions.... using a combination of name filtering and pre-creationg of RBAC. (Sadly RBAC name filtering doesn't work on create).

There's also #10381 which proposes a mode where we offer client-side expansion of these operators (i.e. there's no operator running on the server, but we're still able to pull manifests out of the kops binary)

[k8s-ci-robot]

@justinsb: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kops-verify-cloudformation	addaeac	link	/test pull-kops-verify-cloudformation
pull-kops-verify-hashes	d3e8d0a	link	/test pull-kops-verify-hashes

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[justinsb]

So I'd like to suggest that we merge this and continue to improve it behind the feature-flag. It's particularly difficult to make changes here because we're working across multiple repos. As it stands, everything is behind the UseAddonOperators feature flag

Before removing it from the feature-flag, I/we will:

• Split out the RBAC roles for the operator so that the operator has vastly fewer permissions
• For other operators, try to run in dedicated namespaces (coredns is tricky because of the kube-dns service)
• Support an optional no-operator mode, using the client-side operator evaluation technique in WIP: Demonstrate client-side mode for CoreDNS operator #10381.
• Look into whether it's possible to produce a sandbox that doesn't require docker in the WIP: Demonstrate client-side mode for CoreDNS operator #10381 approach. Likely Linux only, but in particular would address the "run in a container" problem. (Maybe not a blocker for removing the feature-flag?)

What do you think @johngmyers ? (I think others are on board...)

[justinsb]

In office hours on Friday, @johngmyers kindly agreed that we should move forward.

That said, I think i'm going to target this to the 1.22 branch. I don't think that's as far away as it sounds, with 1.20 likely going out in the next few days (#11021 being the last known blocker)

If anyone disagrees though, that gives people a bit more time!

/milestone v1.22

[hakman]

😄

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [hakman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment