feat: Add organization environment variables

[rocketeerbkw]

Draft while coordinating multiple PRs

[shreddedbacon]

In uselagoon/remote-controller#275 the proposal to add another field to the CRD to support organization variables ties the feature to the release of the remote-controller. I'd prefer to avoid this, and it would be possible to do so.

Since the build-deploy-tool merges everything down into one set of variables that it consumes during the build, it doesn't make a lot of sense to continue doing that and extending CRDs with new fields when we could send the data to the build in a different way. The variable fields in the CRD are just []bytes, which means we're not currently restricted here with what the payload looks like except for what consumes it.

Alternative 1

Do the consolidation on the API side and only send the resulting variables as LAGOON_ENVIRONMENT_VARIABLES (leaving project variables payload empty, essentially deprecating the field, with the side benefit of reducing data sent in the payload in the process). With the possibility of extending the data with a source field that contains where it was sourced from in the event this could be useful in the future (or if there is an existing check in the build-deploy-tool that does care about where it is sourced)

[
	{"name":"ENV_VAR","value":"envvalue","scope": "build","source":"environment"},
	{"name":"PROJ_VAR","value":"projvalue","scope": "build","source":"project"},
	{"name":"ORG_VAR","value":"orgvalue","scope": "build","source":"organization"},
	{"name":"LAGOON_SYSTEM_VAR","value":"sysvalue","scope": "internal_system","source":"internal_system"}
]

This way the merging function in the build-deploy-tool can remain mostly there for backwards compatibility, but would basically be unused.

This would make changes to the build-deploy-tool minimal if it was simply extending variables with the source field. No changes to the remote-controller required (except for maybe a deprecation notice on the project variables field)

Alternative 2

Instead of sending 2 (or 3 in this case) variable payloads for each source type, adjust the way the data is sent to the build from core. Instead of the current []EnvironmentVariable slice for each type in a separate field, use a custom type, or map[string][]EnvironmentVariable. This could make a single payload look something like this:

{
    "environment":[{"name":"ENV_VAR","value":"envvalue","scope": "build"}],
    "project":[{"name":"PROJ_VAR","value":"projvalue","scope": "build"}],
    "organization":[{"name":"ORG_VAR","value":"orgvalue","scope": "build"}],
    "internal_system":[{"name":"LAGOON_SYSTEM_VAR","value":"sysvalue","scope": "internal_system"}]
}

This would then get shipped to the build-deploy-tool as LAGOON_ENVIRONMENT_VARIABLES (again, leaving the project variables empty). All the logic in the build-deploy-tool can still merge variables as required, just the way it would do it would be a bit different with the map. We'd also be able to perform a check in the build to see if the variable payloads received are of the new single payload type, or the older dual payload type. May require a check to allow for backwards compatible processing (ie, if new payload use X merge, if old payloads use Y merge)

This would make changes to the build-deploy-tool still fairly extensive, mostly just modifying what you've already got to instead source from a map. No changes to the remote-controller required either.

My preference would probably be alternative 1, it puts the majority of the changes into core. The variable merging functionality from the build-deploy-tool could even be reproduced in the api-sidecar-handler and benefit from Go tests, and we just call it as required like we do with the ssh key functions (might need some thinking done here, because builds can be triggered by both api and webhooks2tasks. Except webhooks2tasks doesn't have access to this sidecar. The node commons functions would need to know how to communicate with it, meaning the chart would need some adjustments to load the sidecar into the webhooks2tasks service)

[rocketeerbkw]

Do the consolidation on the API side

We discussed this previously and we ruled it out because we also want support more complicated env vars that can't be calculated at pre-deployment time. I'm specifically thinking of service env vars, but the API can't know what services will exist until the deployment starts and parses the docker-compose.yml. I think it would be better to have it consolidated at the API, so maybe when we get NATS there can be a mid-deployment API request for env vars, that way all the environment info can be provided? I still have lots of open questions around this.

the proposal to add another field to the CRD to support organization variables ties the feature to the release of the remote-controller

I did account for this, so there isn't a hard dependency on a new remote-controller version for the next release, but eventually there would be a need to enforce a minimum version.

adjust the way the data is sent to the build from core...as LAGOON_ENVIRONMENT_VARIABLES (again, leaving the project variables empty)

I have no objections to this, from a technical perspective, to be done instead of changing the CRDs. I have philosophical questions about what the purpose of a CRD and it's version is if we can just change the data as we please. You've done more with those than I, I'll defer to your preference if you think it's a better way forward.

[shreddedbacon]

Do the consolidation on the API side

We discussed this previously and we ruled it out because we also want support more complicated env vars that can't be calculated at pre-deployment time. I'm specifically thinking of service env vars, but the API can't know what services will exist until the deployment starts and parses the docker-compose.yml. I think it would be better to have it consolidated at the API, so maybe when we get NATS there can be a mid-deployment API request for env vars, that way all the environment info can be provided? I still have lots of open questions around this.

Those conversations were a while ago now, and I don't remember ruling it out, I do remember we couldn't decide on how it would work which halted the conversation. However, I am fairly positive we can still do most of the consolidation API side with service env vars.

One of the nice things about builds is that they don't have a requirement to request information from the API (ignoring pre/post rollout tasks as these don't influence the build process directly). I'm mostly against a build requesting information from the API mid way, even with NATS, as it has the potential to make testing the build process even more complicated.

the proposal to add another field to the CRD to support organization variables ties the feature to the release of the remote-controller

I did account for this, so there isn't a hard dependency on a new remote-controller version for the next release, but eventually there would be a need to enforce a minimum version.

When I was saying do the consolidation on the API side, this is what I meant. Not modifying the API queries/mutations, or any database tables or how the data is stored in the API. Just doing what the build-deploy-tool is doing with those 2 fields, but only at the point of sending the build payload.

adjust the way the data is sent to the build from core...as LAGOON_ENVIRONMENT_VARIABLES (again, leaving the project variables empty)

I have no objections to this, from a technical perspective, to be done instead of changing the CRDs. I have philosophical questions about what the purpose of a CRD and it's version is if we can just change the data as we please. You've done more with those than I, I'll defer to your preference if you think it's a better way forward.

The data type for these two fields are []byte, there is no structured data. We could consider a move away from them being []byte and into an actual structure, but that could potentially happen when we start to consider a service scoped variable implementation. The fields currently being []byte gives us a bit of freedom to explore options though. As soon as we set a type on these fields, we are locked in and any changes to that type would be extremely painful. Additions are mostly fine, but changing or deleting parts of a type will be much more complicated, something we've been pretty shielded from due to the lack of major changes to the build and task CRDs.

Ignoring the purpose of a CRD though, it is only due to historical reasons that the environment and project variables are split payloads. Maybe in the past consolidating them wasn't even considered and we've just been running with that model because all the old bash required both. The build-deploy-tool only started to consolidate these variables in Go to avoid having to iterate over two sets of variables to compute what exists when they were being consumed like bash did. If I could go back in time, I would have pushed for the API to consolidate the variables sooner and only sent the one set of variables to builds.