Add --single-namespace install flag for restricted permissions#1721
Add --single-namespace install flag for restricted permissions#1721
Conversation
|
@olix0r I updated the |
cli/cmd/check.go
Outdated
| cmd.PersistentFlags().BoolVar(&options.dataPlaneOnly, "proxy", options.dataPlaneOnly, "Only run data-plane checks, to determine if the data plane is healthy") | ||
| cmd.PersistentFlags().DurationVar(&options.wait, "wait", options.wait, "Retry and wait for some checks to succeed if they don't pass the first time") | ||
| cmd.PersistentFlags().StringVarP(&options.namespace, "namespace", "n", options.namespace, "Namespace to use for --proxy checks (default: all namespaces)") | ||
| cmd.PersistentFlags().BoolVar(&options.singleNamespace, "single-namespace", options.singleNamespace, "Only check the permissions required to operate the control plane in a single namespace") |
There was a problem hiding this comment.
What do you think about mentioning that this flag only applies when --pre is also set? Is that going to change soon?
cli/cmd/install.go
Outdated
|
|
||
| var resourcePrefix string | ||
| if options.singleNamespace { | ||
| resourcePrefix = "linkerd-" |
There was a problem hiding this comment.
This may be part of a larger conversation, but I'd prefer to see as few naming differences between single-namespace and default as possible. A few reasons:
- debugging (asking a user to do
kubectl logs -f deploy/[linkerd-]web - docs / screenshot consistency
- control plane code / testing complexity
| if namespace == "" { | ||
| sharedInformers = informers.NewSharedInformerFactory(k8sClient, 10*time.Minute) | ||
| } else { | ||
| sharedInformers = informers.NewFilteredSharedInformerFactory( |
| cm coreinformers.ConfigMapInformer | ||
| deploy appinformers.DeploymentInformer | ||
| endpoint coreinformers.EndpointsInformer | ||
| ns coreinformers.NamespaceInformer |
| var namespaces []*apiv1.Namespace | ||
| namespaces := make([]*apiv1.Namespace, 0) | ||
|
|
||
| if name == "" && api.namespace != "" { |
There was a problem hiding this comment.
if name != "" && name != api.namespace, should we log and/or return an error?
There was a problem hiding this comment.
If we return an error here, that would break the CLI and web frontend, which omit the namespace when requesting stat/tap data for all namespaces. Instead, in single namespace mode we want to pretend that there's only one namespace in the cluster.
pkg/healthcheck/healthcheck.go
Outdated
| if pod.Status.Phase == v1.PodRunning { | ||
| name := strings.Split(pod.Name, "-")[0] | ||
| // strip the single-namespace "linkerd-" prefix if it exists | ||
| name := strings.TrimPrefix(pod.Name, "linkerd-") |
There was a problem hiding this comment.
Consider making linkerd- a constant and re-using in cli/cmd/install.go?
Related to previous comment, I'd really prefer to do away with this special-casing, as it will increase our support burden.
|
@siggy Thanks for the feedback on this PR. I agree that renaming things as part of a single namespace install will be more of a burden going forward. I spoke to @olix0r about this, and I'm going to back out that change. Instead, we're going to signal that this feature "experimental", likely by changing the flag to indicate that explicitly. But all of that said, I'm also going to wait to update and merge this branch until #1714 is ready, since that change has implications for this one as well. |
Signed-off-by: Kevin Lingerfelt <kl@buoyant.io>
Signed-off-by: Kevin Lingerfelt <kl@buoyant.io>
c9f7bd1 to
961f9ec
Compare
Signed-off-by: Kevin Lingerfelt <kl@buoyant.io>
Signed-off-by: Kevin Lingerfelt <kl@buoyant.io>
|
Ok, have rebased the branch against latest master, backed out the naming changes, and made the |
siggy
left a comment
There was a problem hiding this comment.
Awesome, this looks much simpler! One TIOLI comment, then 🚢 !
| return hc.checkCanCreate("", "rbac.authorization.k8s.io", "v1beta1", "ClusterRole") | ||
| }, | ||
| }) | ||
| if hc.SingleNamespace { |
There was a problem hiding this comment.
TIOLI: consider refactoring this block with something like...
roleType := "ClusterRole"
roleBindingType := "ClusterRoleBinding"
if hc.SingleNamespace {
roleType = "Role"
roleBindingType = "RoleBinding"
}
hc.checkers = append(hc.checkers, &checker{
category: LinkerdPreInstallCategory,
description: fmt.Sprintf("can create %ss", roleType),
fatal: true,
check: func() error {
return hc.checkCanCreate("", "rbac.authorization.k8s.io", "v1beta1", roleType)
},
})
hc.checkers = append(hc.checkers, &checker{
category: LinkerdPreInstallCategory,
description: fmt.Sprintf("can create %ss", roleBindingType),
fatal: true,
check: func() error {
return hc.checkCanCreate("", "rbac.authorization.k8s.io", "v1beta1", roleBindingType)
},
})Signed-off-by: Kevin Lingerfelt <kl@buoyant.io>
adleong
left a comment
There was a problem hiding this comment.
LGTM
Should we also restrict the Prometheus scrape config to only scrape from the controller namespace?
Are there plans in the future for making this more flexible? I'm imagining the ability to specify a list of namespaces that Linkerd will manage. We would then install Roles and RoleBindings into each of those namespaces. This also allows the control plane and data plane to be in separate namespaces without requiring cluster wide access.
|
@adleong This branch includes a modification to the prometheus scrape config so that it will only scrape proxies in the controller namespace when the linkerd2/cli/install/template.go Lines 437 to 443 in 9e57962 Interesting idea about allowing Linkerd to monitor a specified list of namespaces via separate role bindings. That might take a bit of coding effort, since it would require establishing multiple watches per resource, and all of our code assumes a single watch per resource. But yeah, it seems feasible that we could upgrade to that setup. |
|
Arg, I was looking for that prometheus config change but somehow skipped right past it! Thanks, @klingerf! |
#1721 introduced a `--single-namespace` install flag, enabling the control-plane to function within a single namespace. With the introduction of ServiceProfiles, and upcoming identity changes, this single namespace mode of operation is becoming less viable. This change removes the `--single-namespace` install flag, and all underlying support. The control-plane must have cluster-wide access to operate. A few related changes: - Remove `--single-namespace` from `linkerd check`, this motivates combining some check categories, as we can always assume cluster-wide requirements. - Simplify the `k8s.ResourceAuthz` API, as callers no longer need to make a decision based on cluster-wide vs. namespace-wide access. Components either have access, or they error out. - Modify the web dashboard to always assume ServiceProfiles are enabled. Reverts #1721 Part of #2337 Signed-off-by: Andrew Seigner <siggy@buoyant.io>
#1721 introduced a `--single-namespace` install flag, enabling the control-plane to function within a single namespace. With the introduction of ServiceProfiles, and upcoming identity changes, this single namespace mode of operation is becoming less viable. This change removes the `--single-namespace` install flag, and all underlying support. The control-plane must have cluster-wide access to operate. A few related changes: - Remove `--single-namespace` from `linkerd check`, this motivates combining some check categories, as we can always assume cluster-wide requirements. - Simplify the `k8s.ResourceAuthz` API, as callers no longer need to make a decision based on cluster-wide vs. namespace-wide access. Components either have access, or they error out. - Modify the web dashboard to always assume ServiceProfiles are enabled. Reverts #1721 Part of #2337 Signed-off-by: Andrew Seigner <siggy@buoyant.io>
#1721 introduced a `--single-namespace` install flag, enabling the control-plane to function within a single namespace. With the introduction of ServiceProfiles, and upcoming identity changes, this single namespace mode of operation is becoming less viable. This change removes the `--single-namespace` install flag, and all underlying support. The control-plane must have cluster-wide access to operate. A few related changes: - Remove `--single-namespace` from `linkerd check`, this motivates combining some check categories, as we can always assume cluster-wide requirements. - Simplify the `k8s.ResourceAuthz` API, as callers no longer need to make a decision based on cluster-wide vs. namespace-wide access. Components either have access, or they error out. - Modify the web dashboard to always assume ServiceProfiles are enabled. Reverts #1721 Part of #2337 Signed-off-by: Andrew Seigner <siggy@buoyant.io>
This branch updates the
linkerd installandlinkerd checkcommands to accept a--single-namespaceboolean flag, which if provided will modify the linkerd install YAML to use Roles instead of ClusterRoles, and it will adjust the pre-install check accordingly.The bulk of the code changes required for conditionally switching to Roles happens in
controller/k8s/api.go, where we configure the shared informers used by all of our controller components for reading from the Kubernetes API. If the--single-namespaceflag is specified, all watches established in this file are scoped to an individual namespace.Fixes #1705.