gocardless / theatre

GoCardless' collection of Kubernetes extensions
MIT License
23 stars 17 forks source link

Add UTOPIA_CONSOLE_USER environment variable to console containers #334

Closed mbfisher closed 7 hours ago

mbfisher commented 2 months ago

In FY25, BPNG will have a Hero Metric that measures our ability to process payments without manual intervention. For more info see here.

We'd like to automate the way this is measured, so @stephenbinns and I are looking in to how to instrument the different ways we intervene in payment processing. A very common one is the use of consoles, for example to restart Paysvc banking pipelines:

$>  utopia consoles create --environment live-production --template readwrite --timeout 8h --no-attach --reason "Restarting GBP Payouts Generation pipeline" --service payments-service

# Authorise console...

[1] PS[live-production](main)> Jobs::Pipelines::GBPPayoutsGeneration.run_from("...")

We would like to instrument this code to record the intervention, including the engineer who performed it. To do this we need to know which engineer has authorised the console from within the Rails console.

This change introduces a UTOPIA_CONSOLE_USER environment variable to all consoles, set to the email address of the authorised user.

Neither of us are subject matter experts in either Kubernetes, Utopia or Anu, but we know enough to cobble together a change. Any and all advice about better ways to achieve this would be very welcome!

For more on how we're approaching this project, here's our scoping doc, though fair warning that it's changing fast 🙏

bogvak commented 2 months ago

Hi Mike. @mbfisher Just a quick question - have you already tested that addition to Console controller somewhere in real workload beside integration test? Does that do job that you expected from those changes? Thanks!

mbfisher commented 2 months ago

Hi Mike. @mbfisher Just a quick question - have you already tested that addition to Console controller somewhere in real workload beside integration test? Does that do job that you expected from those changes? Thanks!

Hey @bogvak, I haven't done any testing like that, no. I wanted to get a sense check of the direction before setting up the local k8s cluster and everything!

0x0013 commented 2 months ago

Hi @mbfisher

In general, seems like this would do what you expect. I'm trying to think if this might have any further implications.

Meanwhile, I wonder if you considered exposing the environment variable from the label, similar to how it is done for the session recording pod, for consistency? This would set it to the username e.g. jdoe.

mbfisher commented 2 months ago

Hi @mbfisher

In general, seems like this would do what you expect. I'm trying to think if this might have any further implications.

Meanwhile, I wonder if you considered exposing the environment variable from the label, similar to how it is done for the session recording pod, for consistency? This would set it to the username e.g. jdoe.

No I hadn't considered that - didn't know it existed!! It would be better to keep it consistent across the containers and just have one way to do it. This is why you ask for advice 👌

Would this work because metadata.labels['user'] is set on the PodSpec, so we can do the envVarSource trick on any container within the Pod?

0x0013 commented 2 months ago

Would this work because metadata.labels['user'] is set on the PodSpec, so we can do the envVarSource trick on any container within the Pod?

Pretty much, yes. Technically, it's not part of the PodSpec even, but of object's metadata (object, in this case, being a Pod). The environment variable value from field is actually a kubernetes feature. If you inspect the pubsubtle container of a console pod, you can see:

- name: LABEL_USER
  valueFrom:
    fieldRef:
      apiVersion: v1
      fieldPath: metadata.labels['user']

So, instead of encoding a specific string to the environment variable via the code in Theatre, we would encode a reference to a label field.

Also, I was informed that Theatre is apparently also available publicly, so we should best avoid using the UTOPIA_ prefix, which is specific for our usage. Following the same approach as for the console recorder, with the same consistent naming, would avoid this as well.

mbfisher commented 2 months ago

Thanks for the explanation @0x0013!

Am I right that I could use the fieldRef environment variable in the ConsoleTemplate, instead of setting it in the controller? Like this?

If that would work, do you think either implementation is better than the other? I'm inclined to use the ConsoleTemplate features where possible, and it means we can add the environment variable to the consoles of services that we need it for, rather than all of them. That said, doing it in the controller means doing it just once, and you can't typo the variable name or remove it accidentally or without knowing its importance.

🙏

0x0013 commented 1 month ago

Hi @mbfisher I seem to have missed your reply/question.

Am I right that I could use the fieldRef environment variable in the ConsoleTemplate, instead of setting it in the controller? Like this?

The link doesn't work for me, maybe the branch has been deleted? But from context, if I understand correctly, I think this would work.

If that would work, do you think either implementation is better than the other?

I think this could be a valuable addition for other services as well, as long as the above suggestions are followed, namely, not using a GC-specific naming (UTOPIA_). Immediately, I don't see a downside of exposing the console author to any workload running in all consoles, so unless I'm missing something that makes this a risk, I think adding it to the controller would be a good choice.

mbfisher commented 1 month ago

@0x0013 ah dang I went the other way to get us unblocked! https://github.com/gocardless/anu/pull/30890

We'll need to make the same change for other services, so when we're ready for that I'll revive this PR.