Change Environment tag to something else?

[toast-gear]

Hello again,

Great module, it's working really well so far from our testing.

One thing that confuses me slightly is that as far as I can tell, you are expected to deploy many stacks of this module if you are making use of runner groups, 1 stack per runner group. The GitHub Actions platform offers few server-side security features currently and so it's difficult to not make use of runner groups if security is important to your organisation. As a result of this, you often end up with multiple runner groups being implemented for any given environment (production being the most common). The Environment input causes problems because it not only gets used to namespace the stack, but also to create / set a tag called Environment. If you have to deploy multiple instances of this stack the set value for this tag can't be what someone would traditionally call an "environment" e.g. "production", as it needs to be unique to namespace the instance from other instances of the stack. Furthermore, the tag Environment is extremely common and so you end up with a clash between your internal tagging standard, and this modules use of the tag, both in terms of what this tag ends up representing as well as a duplicate key clash.

Would it be better to rename this key to Namespace and have the tag it creates also have the key Namespace so that people can still apply an Environment tag as well as having its value make a bit more sense. Perhaps it seems like semantics but I would think Environment is an extremely common key resulting in a clash in lots of organisations. Namespace might also cause clashing issues but I would think for fewer. You could use something a bit more utilitarian instead like ActionsStackPrefix or even continue with using Environment but have the tag creation as a configurable option if it isn't used by the module for something? If none of those work perhaps add a namespace to this tags key to differentiate it from an internal Environment tag like philips-labs/Environment? You could argue that you should namespace your internal tags however that is much harder to implement once a strategy is in place already.

Interested to hear your thoughts if this is a problem from your perspective. This modules concept of an Environment seems very blurry to me if you need to deploy multiple copies of the stack to support multiple runner groups. Perhaps in isolation it makes sense but when deploying this solution into an AWS organisation or running multiple instances of this stack due to the need for multiple runner groups to target the same "environment" e.g. production, this modules concept of an Environment breaks down.

[toast-gear]

@npalm Any thoughts on this? Has the horse bolted at this point or is it worth considering moving to a more generic and less opinionated word?

[npalm]

@toast-gear not yet had the time to make up my mind. Will do my best to come back asap

[sammort]

This is actually something that is biting us currently. We have a global environment tag that is added to all our resources. When we try to apply this module we get the below error:

Error: Error creating IAM Role test-runner-action-webhook-lambda-role: InvalidInput: Duplicate tag keys found. Please note that Tag keys are case insensitive.
│ 	status code: 400, request id: 3ac70edf-d44a-4410-ada8-41c786a1593b
│
│   with module.test-github-runner.module.webhook.aws_iam_role.webhook_lambda,
│   on .terraform/modules/test-github-runner/modules/webhook/webhook.tf line 51, in resource "aws_iam_role" "webhook_lambda":
│   51: resource "aws_iam_role" "webhook_lambda" {

I believe this is because IAM Roles are one of the few resources that don't follow the case sensitive nature of pretty much all other resource tagging in AWS (i.e. we only receive this error on aws_iam_role resources) and the failure is because both an Environment and environment tag are trying to be added to the Role.

[npalm]

Would it be an idea to see if we make the key used for the tag environment configurable?

[sammort]

From my perspective the Environment tag isn't going to be used by all organizations in the same way and, specifically in the case of IAM Users and Roles, causes a problem when an environment tag is also being used due to the clash of Environment and environment keys for these resources.

Would one approach be to have a toggle for whether the Environment key for this module should be added as a tag to the created resources? One drawback I can see for this, is it wouldn't be clear to a new adopter of this module that they need to enable this behaviour if they have a global environment tag.

[toast-gear]

From my pespective the word environment when talking about runner infrastructure (including the runners) is problamatic and I would consider moving to something more neutral like namespace. Runner infrastructure doesn't really fit into the traditional definitions of an environment. If I had to say I would say that most runner infrastructure is production as it is either deploying / touching systems that are production or developers rely on it to do their job thus it is production. This is exacerbated by the fact that runner groups are one of only a few server-side security features GitHub provide for managing access to runners (and thus access) so you are likely to end up with multiple runner groups all targeting various systems that are are all production.

If you do want to stick to environment then it makes sense for the tag creation to be configurable with off being the default as that is the most compatible configuration. If someone is relying on it currently they'll see the diff in their plan and either enable the tag creation or add the tag at a more appropriate place. Terraform makes releasing this kind of potentially (I think this would only be for a small percentage of users) breaking change in the module fairly safe to do after the fact.

[npalm]

@toast-gear I agree on most parts of you concerns, but just renaming environment to namespace would cause clashed for users allready applying that tag to rerources.

I would first like to give a bit of background. When we started years ago using terraform we simply used environment to ensure names are unique and all resources can be easy looked up. For some reason, we choose the name environment and so today we still use that one. In retrospective I think namespace is much better one and can fullfill the same needs. You also addresses the use of runner-groups. Any suggestion how to better support the rgroups are very welcom. We have written teh module shortly after self-hosted runners became available. At that moment groups doesn't even exists.

So I would prefer that we update the module in such way that we make it possible to define the key we for the tag used as namespace (or environment). This won't be a simple update of the terraform code since some lamda's and the installation script of runner also using the namespace.

@toast-gear are you interessted to help us with a PR?

[toast-gear]

@toast-gear are you interessted to help us with a PR?

I've never written Typescript before so I can't promise anything but I can certainly try. The lambdas aren't massively complicated so I don't think you need to be a Typescript expert to do the PR, we'll see though! I'll try to get some time to take a look but if someone else knows a bit of Typescript feel free to do the PR!

[sammort]

Closed #1328 following discussion that would break module. The approach described in that PR sounds sensible but not something I can look at right now.

Opened #1346 which tags perhaps a more pragmatic approach by not tagging the IAM Roles with Environment as these are the only resources which would fail to create in the event that environment tag is trying to be created as well.

@npalm

[ScottGuymer]

In #1444 I have started to enable the transition to tags prefixed with ghr: to prevent this sort of clashing.

Currently it has both option to preserve any existing functionality and allow a smoother transition and removal of the Environment tag (and others)

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs. Thank you for your contributions.

[npalm]

solved in PR #1858