Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add UTOPIA_CONSOLE_USER environment variable to console containers #334

Closed
wants to merge 2 commits into from

Conversation

mbfisher
Copy link

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 🙏

@mbfisher mbfisher marked this pull request as draft May 17, 2024 14:30
@@ -68,7 +68,7 @@ manifests: generate
install-tools:
go install github.com/onsi/ginkgo/ginkgo@v1.16.5
go install sigs.k8s.io/controller-tools/cmd/controller-gen@v0.10.0
go install sigs.k8s.io/controller-runtime/tools/setup-envtest@latest
go install sigs.k8s.io/controller-runtime/tools/setup-envtest@v0.0.0-20230216140739-c98506dc3b8e
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the latest version of this supports golang >1.22. Rather than try to upgrade go I've just pinned to an older version that works without changing anything else

@asanord asanord self-requested a review May 22, 2024 13:26
@bogvak bogvak self-requested a review May 29, 2024 11:03
@bogvak
Copy link

bogvak commented May 29, 2024

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!

@bogvak bogvak requested a review from 0x0013 May 29, 2024 12:31
@mbfisher
Copy link
Author

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

0x0013 commented May 29, 2024

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

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

0x0013 commented May 30, 2024

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

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

0x0013 commented Jun 25, 2024

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

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

@mbfisher mbfisher closed this Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants