search

kinvolk / lokomotive

🪦 DISCONTINUED Further Lokomotive development has been discontinued. Lokomotive is a 100% open-source, easy to use and secure Kubernetes distribution from the volks at Kinvolk
https://kinvolk.io/lokomotive-kubernetes/
Apache License 2.0
321 stars 49 forks source link

Create development guidelines for handling credentials in components and platforms code #424

Open invidian opened 4 years ago

invidian commented 4 years ago

As mentioned in #422, we often need to deal with credentials in our code (providers API tokens, etc.). We should decide and document if we want to e.g. validate that specific environment variable is set, or do we let Terraform do the validation, which negatively impacts the UX, as we cannot do the checks early (think runtime check instead of compile-time check).

johananl commented 4 years ago

IMO it probably makes sense to do such validations in a centralized place depending on the cluster config. For example, if the config contains a Packet cluster, ensure we have the Packet-related env var(s) in place.

Such logic fits naturally in a config package IMO. I don't see much difference between validating a config file, a flag and an env var. This makes the problem related to https://github.com/kinvolk/lokomotive/issues/23.

invidian commented 4 years ago

IMO it probably makes sense to do such validations in a centralized place depending on the cluster config. For example, if the config contains a Packet cluster, ensure we have the Packet-related env var(s) in place.

I don't fully agree with that. IMO environment variable is a "runtime" change, as opposite to the configuration which is static (think "source code"). The example would be terraform validate command. It validates the configuration, but does not require variable values to be defined or credentials configured.

Such logic fits naturally in a config package IMO.

If we decide to treat environment variables as part of the configuration, then indeed, configuration validation should trigger the logic, but the definitions should definitely be stored closely to the related subject, so packet package, externaldns package etc. not centrally in config package.

I think it would be nice to have a good separation of concerns here. E.g. packet package should not access any environment variables, if should only operate on it's own structures. Environment variables should probably be treated as part of UI, the same as configuration files. Perhaps config package could handle translating environment variables into "native" Go structures, together with HCL code.

johananl commented 4 years ago

I don't fully agree with that. IMO environment variable is a "runtime" change, as opposite to the configuration which is static (think "source code"). The example would be terraform validate command. It validates the configuration, but does not require variable values to be defined or credentials configured.

What don't you agree with? That we should validate configuration centrally?

johananl commented 4 years ago

To clarify, my main points were:

invidian commented 4 years ago

What don't you agree with? That we should validate configuration centrally?

I don't fully agree, that when validating the configuration, we should make sure that env variables are set. IMO they should be validated separately, as explained above.

johananl commented 4 years ago

Oh, I was not implying we want to validate env vars and the config in the same action.