-
Notifications
You must be signed in to change notification settings - Fork 54
Conversation
cc @technosophos -- I know you've done some work towards this in #560 |
5a6700f
to
2377cf0
Compare
2377cf0
to
2b94043
Compare
2d1ee4a
to
729dd18
Compare
This is behind a build flag. Usage: go test -tags=integration ./...
4cfc7ee
to
206a2be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave this a spin trying to install the riff cnab bundle. Except for the one issue mentioned below, the install was successful! 👍
Labels: labelMap, | ||
} | ||
// Mount SA token if a non-zero value for ServiceAccountName has been specified | ||
mountServiceAccountToken := k.ServiceAccountName != "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mountServiceAccountToken := k.ServiceAccountName != "" | |
mountServiceAccountToken := k.ServiceAccountName == "" |
As per the docs credential for the default service account is mounted at /var/run/secrets/kubernetes.io/serviceaccount/token
. When not using a service account, the default service account should be created. I got the exception Could not get kubernetes configuration: failed to read token file \"/var/run/secrets/kubernetes.io/serviceaccount/token\": open /var/run/secrets/kubernetes.io/serviceaccount/token: no such file or directory
when I gave this a spin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was an intentional decision, as we don't know that the kubernetes driver is being used to run invocation images that deploy to the same cluster, or even to kubernetes. I would like to avoid giving permissions to invocation images that haven't been explicitly declared as required by the bundle.
That said, duffle could afford to be more opinionated and set a default service account here:
https://github.com/deislabs/duffle/blob/206a2bec45ddddb9dcf558581f27249a80db835f/pkg/driver/lookup.go#L14-L15
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbawaska also did you try mounting kubeconfig as a credential as requested by the riff bundle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't know that the kubernetes driver is being used to run invocation images that deploy to the same cluster, or even to kubernetes.
To me, the use case for this driver is to run the invocationImage in cluster where the bundle is to be installed. I think we should let the default docker driver handle the deploying to non-kubernetes environments.
I would like to avoid giving permissions to invocation images that haven't been explicitly declared as required by the bundle.
I think the missing piece here is a lack of contract between the driver and the bundle for the parameters needed by the driver. The SERVICE_ACCOUNT and KUBE_NAMESPACE env variables here are env variables for duffle itself, not the invocation image. The bundle can only specify parameters for invocationImages, not duffle.
I like this quote from Allan Kay:
Simple things should be simple, complex things should be possible.
In this context it would mean: provide defaults, but allow the bundle author to override the defaults. more concretely, I think we should:
- provide
default
as the default value for SERVICE_ACCOUNT. The user could use a different service account if desired by setting the env variable. - provide default namespace from kubeconfig for KUBE_NAMESPACE, again, the user could specify a different namespace if desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second though, we could iterate on these issues to make the experience better, and work on some contract between driver and duffle for parameters needed. Let's merge this!
// Run executes the operation inside of the invocation image. | ||
func (k *KubernetesDriver) Run(op *driver.Operation) error { | ||
if k.Namespace == "" { | ||
return fmt.Errorf("KUBE_NAMESPACE is required") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be mandatory, it should be ok to run the installer pod in the default namespace. One less thing for the user to worry about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar idea as #765 (comment). Credentials mounted in invocation images are readable by anyone with secret list
permissions in the target namespace. In a multi-tenant cluster using the default
namespace would be really inappropriate IMO, so from the driver's perspective I think it's best to require a value to be set explicitly.
Installer implementations like duffle may still set a default namespace as this is a public field.
func (k *KubernetesDriver) Config() map[string]string { | ||
return map[string]string{ | ||
"KUBE_NAMESPACE": "Kubernetes namespace in which to run the invocation image", | ||
"SERVICE_ACCOUNT": "Kubernetes service account to be mounted by the invocation image (if empty, no service account token will be mounted)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"SERVICE_ACCOUNT": "Kubernetes service account to be mounted by the invocation image (if empty, no service account token will be mounted)", | |
"SERVICE_ACCOUNT": "Kubernetes service account to be mounted by the invocation image (if empty, default service account token will be mounted)", |
please see below for the rationale.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for @sbawaska's points.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbawaska is happy to merge this PR now. We can add more features later.
|
||
conf, err := clientcmd.BuildConfigFromFlags(settings["MASTER_URL"], kubeconfig) | ||
if err != nil { | ||
panic(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KEEP CALM AND DON'T PANIC
😺
Wouldn't it be better to modify the driver.Configurable
interface so SetConfig
can return an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually did exactly this and then realized the interface is defined in cnab-go. I'm definitely in favor of this, just has to be a separate pr.
See #765 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this can be addressed in a follow-on PR when we can fix the interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure I'm fine with a follow-up 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am with @glyn on this -- there are a few things that should be sorted out in follow-up PRs, but what you have here is an excellent place to start. We should get this merged.
/brig run |
1 similar comment
/brig run |
PR cnabio#765 forgot to commit changes to vendor to sync it to the changes to Gopkg.lock. This syncs the vendor directory with those changes.
Closes #763
This PR includes new integration tests that are behind a build flag. To run them, you'll need to be authenticated to a kubernetes cluster with permissions in the
default
namespace. Then rungo test
with the integration tag: