Skip to content
This repository has been archived by the owner on Jun 29, 2022. It is now read-only.

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

Open
invidian opened this issue May 13, 2020 · 6 comments
Labels
area/feature-parity Ensuring things are similar on all platforms area/security Security related stuff area/ux User Experience kind/documentation Issues about documentation

Comments

@invidian
Copy link
Member

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).

@invidian invidian added area/security Security related stuff kind/documentation Issues about documentation area/ux User Experience area/feature-parity Ensuring things are similar on all platforms labels May 13, 2020
@johananl
Copy link
Member

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 #23.

@invidian
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

To clarify, my main points were:

  • We want to error out if a required env var isn't set.
  • We don't want to check for a Packet API key when deploying on AWS.

@invidian
Copy link
Member Author

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
Copy link
Member

johananl commented May 19, 2020

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/feature-parity Ensuring things are similar on all platforms area/security Security related stuff area/ux User Experience kind/documentation Issues about documentation
Projects
None yet
Development

No branches or pull requests

2 participants