fiaas / fiaas-deploy-daemon

fiaas-deploy-daemon is the core component of the FIAAS platform
https://fiaas.github.io/
Apache License 2.0
55 stars 31 forks source link

Generalize secret support in FIAAS #71

Closed xavileon closed 4 years ago

xavileon commented 4 years ago

FIAAS currently supports importing secrets into pods in two different ways:

It would be helpful to generalize how the init container for strongbox works, define a very simple interface for it, so we could swap in any other secret implementation. By doing so, we could support other secret managements tools like AWS Secret Manager or Vault.

mortenlj commented 4 years ago

Actually, there are three ways. The third one is a somewhat generic init container variant that is (was?) used by FINN to implement Vault support.

It might be worth trying to make the Strongbox and Generic init container solutions into one solution that is truly generic, but I'm not sure how easy that will be. There is a reason we didn't use the existing generic init container solution for Strongbox (I think it's because Strongbox requires additional configuration).

xavileon commented 4 years ago

Ah cool! maybe that's all we need then 👍

gregjones commented 4 years ago

I wasn't aware of that generic option either, it looks good, but might need more config options... 🙀 I'm also wondering whether the flow for strongbox might actually work with secret-manager with no changes, as probably all it needs is the IAM role and the group could be re-purposed?

An issue might be for people migrating, that the image is set at the namespace-level, but maybe an image that works with both could somehow be made... 🤔

gregjones commented 4 years ago

So we had some discussions internally about this, and we think the following might be a nice way to support the cases we'd like to, namely:

We extend paas.yml so it can take the following:

extensions:
  secrets:
    type: aws-parameter-store
    parameters: # arbitrary k-v pairs set as env on the init-container
      AWS_REGION: eu-central-7
    annotations: # annotation that will be added to the secret-init-container
      iam.amazonaws.com/role: arn::etc.

The 'type' would be a reference to an image stored in the fiaas-deploy-daemon config, e.g.

cluster_config.yaml: |-
[...]
  secret-init-containers:
    aws-parameter-store: containers.example.com/foo/parameter-store-init-container:latest
    strongbox: containers.example.com/foo/strongbox-store-init-container:latest

The Secrets implementation would then have extra logic to check for extensions.secrets, which if set matches the 'type' to a registered image, and adds it as an init-container passing the parameters and setting the annotations.

We think that a combination of env vars and annotations can support other backends.

What do you think?

oyvindio commented 4 years ago

I think this might be a good pattern for secrets init containers that strictly require some application-specific configuration, like an IAM role ARN in the case of aws-parameter-store or IAM role ARN + groups in the case of strongbox. At the same time I think it would be good if the implementation of this could support using secrets-init-container images that do not require application-specific configuration without any additional configuration in the paas.yml/fiaas.yml file. This would make it possible to transition applications from Kubernetes secrets to (parameter-less) secrets-init-container, the other way around, or between different secrets-init-container implementations without changing each applications configuration. Do you have any thoughts on making that work ?

mortenlj commented 4 years ago

I like the point @oyvindio makes, because I can easily see a situation where you would have different secret mechanisms depending on environment. Especially during a migration from one mechanism to another.

Today the strongbox integration uses the strongbox key to provide the relevant configuration, and I think it might be worth keeping the notion that each type of secret integration has a separate key.

It might make sense to group them under a common parent key (like secrets), but by using a type-specific key, it allows for multiple secret configurations in the same file. On the other hand, if FDD is configured to support multiple mechanisms at the same time, how would they be combined if they are both specified in the app config?

gregjones commented 4 years ago

At the same time I think it would be good if the implementation of this could support using secrets-init-container images that do not require application-specific configuration without any additional configuration in the paas.yml/fiaas.yml file

Maybe a reserved 'type' in the deploy-daemon config could mean it's added by default to an app that doesn't specify any secrets config of its own?

So with deploy-daemon config for a namespace containing:

cluster_config.yaml: |-
[...]
  secret-init-containers:
    aws-parameter-store: containers.example.com/foo/parameter-store-init-container:latest
    default: containers.example.com/foo/default-init-container:latest

Any applications that don't ask for a specific 'type', would get the container tagged 'default'. For a setup where you don't want any default, it would be left out, and no secret-container would be added.

gregjones commented 4 years ago

I like the point @oyvindio makes, because I can easily see a situation where you would have different secret mechanisms depending on environment. Especially during a migration from one mechanism to another.

So the proposal is precisely to enable migration from one to another, by having the alternatives available at the namespace-level, and then apps can choose which one they want to use. Did you mean something different?

Today the strongbox integration uses the strongbox key to provide the relevant configuration, and I think it might be worth keeping the notion that each type of secret integration has a separate key.

It might make sense to group them under a common parent key (like secrets), but by using a type-specific key, it allows for multiple secret configurations in the same file. On the other hand, if FDD is configured to support multiple mechanisms at the same time, how would they be combined if they are both specified in the app config?

You mean that the app config would look like this?

extensions:
  secrets:
    aws-parameter-store:
      parameters: # arbitrary k-v pairs set as env on the init-container
        AWS_REGION: eu-central-7
      annotations: # annotation that will be added to the secret-init-container
        iam.amazonaws.com/role: arn::etc.
    other-provider:
      parameters:
        FOO: BAR

I think as long as the keys for the providers weren't part of the schema, this would still work for us, but I don't think it's something we'd need. Would there be issues with the containers initialising at the same time? I think users would have to understand that the secrets should be distinct, and not rely on any order of execution, right?

mortenlj commented 4 years ago

The original idea behind the extensions key was that below that would be keys that are not part of the core fiaas configuration. It was supposed to be handled by some kind of extension mechanism. For that reason, it was always intended to not be part of the schema, and FDD was not supposed to care.

Of course, since then, we have added keys to the extensions section that FDD does care about, and we haven't made a proper extension mechanism, so I guess any considerations as to what the original idea was is mostly theoretical at this point.

IMHO, keys under extensions should not be considered part of the schema, but should be documented by the component that uses them. For now, the only component that uses keys under extensions is FDD, and so it makes sense to include the documentation for those keys along with the rest of the schema in the FDD documentation, even if they aren't actually part of the schema.

gregjones commented 4 years ago

Ok, so combining the revised options would mean something like this:

paas.yml can now contain multiple configs for secret init-containers:

extensions:
  secrets:
    aws-parameter-store:
      parameters: # arbitrary k-v pairs set as env on the init-container
        AWS_REGION: eu-central-7
      annotations: # annotation that will be added to the secret-init-container
        iam.amazonaws.com/role: arn::etc.
    other-provider:
      parameters:
        FOO: BAR

The keys inside 'secrets' are used to lookup the images from config inside fiaas-deploy-daemon. An empty object would be a value value for an init-container that needs no parameters, i.e.

extensions:
  secrets:
    no-config: {}

The config in fiaas-deploy-daemon would look like this:

cluster_config.yaml: |-
[...]
  secret-init-containers:
    aws-parameter-store: containers.example.com/foo/parameter-store-init-container:latest
    other-provider: containers.example.com/other/provider:latest
    default: containers.example.com/foo/default-init-container:latest

A key of 'default' in this map of images will mean it will be used if 'secrets' in the application's config is empty.

Sound ok?

mortenlj commented 4 years ago

I mostly agree, I only have two concerns:

1) I don't particularly like the no parameters case. On the other hand, I'm not sure how to solve it, so maybe it's fine anyway. 2) If the default is to not use an init-container at all, how is that configured?

oyvindio commented 4 years ago

So the proposal is precisely to enable migration from one to another, by having the alternatives available at the namespace-level, and then apps can choose which one they want to use. Did you mean something different?

I meant to enable migration e.g. from secrets init container to kubernetes secrets, or from one init container image to another without the application configuration (paas.yml/fiaas.yml) having to change. Meaning, if there is only one secrets init container configured in fiaas-deploy-daemon's configuration, and it does not require any parameters, it should be used by default without any application configuration required. If the secrets init container is removed from the fiaas-deploy-daemon configuration and the application is redeployed, it should use Kubernetes secrets only; with no application configuration changes necessary. I think it is important to keep it possible to change things "behind the scenes" like this when we can.

Some examples of default behavior based on @gregjones example above just to try to illustrate my point:

1) Let's assume that we have fiaas-deploy-daemon as configured in

The config in fiaas-deploy-daemon would look like this:

cluster_config.yaml: |-
[...]
  secret-init-containers:
    aws-parameter-store: containers.example.com/foo/parameter-store-init-container:latest
    other-provider: containers.example.com/other/provider:latest
    default: containers.example.com/foo/default-init-container:latest
  • If we have paas.yml with no configuration under extensions.secrets, it would get the image configured under secret-init-containers.default as secrets-init-container. Any Kubernetes secret with name matching the application would not be mounted.

2) In a different setting, let's assume that we have fiaas-deploy-daemon configured with a few available secrets-init-containers, but no secrets-init-container.default, like the following:

cluster_config.yaml: |-
[...]
  secret-init-containers:
    aws-parameter-store: containers.example.com/foo/parameter-store-init-container:latest
    other-provider: containers.example.com/other/provider:latest
  • If we have paas.yml with no configuration under extensions.secrets, it would get no secrets init container, but a Kubernetes secret with name matching the application would be mounted if it exists. (Does this make sense?)
oyvindio commented 4 years ago

An empty object would be a value value for an init-container that needs no parameters, i.e.

extensions:
  secrets:
    no-config: {}

I'm not sure I understand when this application configuration would be used; could you try to explain what you mean?

gregjones commented 4 years ago

I'm not sure I understand when this application configuration would be used; could you try to explain what you mean?

This was meant to show that an application could specify a secrets-container (that's been made available at the f-d-d level) simply by adding the 'key' under secrets, without having to specify any parameters or annotations.

Meaning, if there is only one secrets init container configured in fiaas-deploy-daemon's configuration, and it does not require any parameters, it should be used by default without any application configuration required.

I think this is fine as long as it is a specific key for that default (i.e. "default"), not just "if there is only one", as otherwise it's not possible to have the scenario where we provide an optional secrets-container to users.

If the secrets init container is removed from the fiaas-deploy-daemon configuration and the application is redeployed, it should use Kubernetes secrets only; with no application configuration changes necessary.

I think this matches what I'm proposing.

If we have paas.yml with no configuration under extensions.secrets, it would get no secrets init container, but a Kubernetes secret with name matching the application would be mounted if it exists. (Does this make sense?)

It makes sense. I didn't plan on changing any of the logic about when kubernetes-secrets are used; I think that how it works now will continue to work for the new cases. I think everything else in that comment matches my idea of how this should work.

The code here will change to something like:

def apply(self, deployment, app_spec):
        if self._secrets_image_set:
            self._generic_init.apply(deployment, app_spec)
        elif self._uses_strongbox_init_container(app_spec):
            self._strongbox.apply(deployment, app_spec)
        elif self._uses_secrets_init_contaner(app_spec):
            self._secrets_container.apply(deployment, app_spec)
        else:
            self._kubernetes.apply(deployment, app_spec)

def _uses_secrets_init_contaner(self, app_spec):
    if app_spec.extensions.secrets.keys().length > 0:
      return True
    if self.config.secrets['default']:
      return True
    return False

And this is how the logic would look, for just the new parts:

Screenshot 2020-03-31 at 13 39 47

Does that make sense?

oyvindio commented 4 years ago

Meaning, if there is only one secrets init container configured in fiaas-deploy-daemon's configuration, and it does not require any parameters, it should be used by default without any application configuration required.

I think this is fine as long as it is a specific key for that default (i.e. "default"), not just "if there is only one", as otherwise it's not possible to have the scenario where we provide an optional secrets-container to users.

Yes, I think that is what I meant, I just wrote something else 😄. So, if there is a default secrets init container configured in fiaas-deploy-daemon's configuration, it should be used by default without any application configuration required.

oyvindio commented 4 years ago
Screenshot 2020-03-31 at 13 39 47

Does that make sense?

Yes, I think it makes a lot of sense 👍

gregjones commented 4 years ago

I had missed these points previously:

I don't particularly like the no parameters case. On the other hand, I'm not sure how to solve it, so maybe it's fine anyway.

I think the alternative would be to have a key to enable/disable:

extensions:
  secrets:
    some-secret-provider:
      enabled: true

This is extra-noise for the providers that do require parameters, I'm not sure I like it much.

If the default is to not use an init-container at all, how is that configured?

If the namespace doesn't have a provider registered as 'default' then there won't be an init-container attached, and it will fallback to adding the k8s secret iff it's present.

mortenlj commented 4 years ago

This is extra-noise for the providers that do require parameters, I'm not sure I like it much.

Agreed. Your first suggestion is probably the best option.