Add AuthContext to Kubernetes commands #1126
Conversation
source/Octopus.Tentacle/Kubernetes/KubernetesScriptPodCreator.cs
Outdated
Show resolved
Hide resolved
|
|
||
| namespace Octopus.Tentacle.Contracts | ||
| { | ||
| public interface IAuthContext |
There was a problem hiding this comment.
Just had the thought, is IAuthContext too generic? Would IScriptAuthContext be a bit clearer (also thinking about how it manifests in Server)
There was a problem hiding this comment.
I'm not sure I see the value of having this interface for extension with only one implementation. Are we already expecting more different kinds of auth contexts for Kubernates commands? Is it hard to add this extension point later if we need it?
There was a problem hiding this comment.
The only reason we may want an IAuthContext is based on how deep into the execution pipeline we need to leak the fact that this is a K8S specific resource.
Based on some other exploration that @eddymoulton made with the K8S specific property
#1133
https://github.com/OctopusDeploy/OctopusDeploy/pull/36074
Then it perhaps isn't necessary. As noted, we could add it to other non-k8s usages if and when we actually need it in future.
|
|
||
| public bool IsRawScript { get; } | ||
|
|
||
| public IAuthContext? AuthContext { get; } |
There was a problem hiding this comment.
Yep this looks like where I expected you to add the property
| public string? ProjectSlug { get; } | ||
| public string? EnvironmentSlug { get; } | ||
| public string? TenantSlug { get; } | ||
| public string? StepSlug { get; } |
There was a problem hiding this comment.
This makes tentacle aware of projects, environments, tenants and steps. This creates a lot of coupling. Its the kind of thing I was suggesting we would want to avoid. Is there a good reason to keep these properties all separated instead of just combining them into a natural identifier?
There was a problem hiding this comment.
On the other side of this functionality, we are matching against these properties one by one.
In addition to that, the object we are matching against is a user configured object - so simple, human readable labels and values are desirable.
There was a problem hiding this comment.
On the other side of this functionality
I'm not quite sure what this means? As in you use them in calamari or something?
I think creating coupling to these modelling domain concepts in tentacle is a definite con here and I'm not sure what other options we have so its hard for me to see this as the preferred way forward. Maybe @zentron or @APErebus can help me understand?
There was a problem hiding this comment.
We are setting some labels on the Kubernetes script pod with these values.
By "The other side of this functionality", I'm referring to a new component we are creating that will run in Kubernetes and watch for our script pods with these new labels.
There was a problem hiding this comment.
A single label would need to then be some kind of conglomerate of the 4 labels above.
These labels are matched up with configuration defined for a new component (which is configured with the slugs).
These aren't used by Calamari and can't be used by Calamari because they are used in setting up the host pod that Calamari runs in.
There was a problem hiding this comment.
To reiterate what is said above, we need the resource to be an object rather than single string since we would otherwise end up having to serialize and deserialize some object just to cross the boundary, and at that point we lose some of the benefits of strongly typed properties.
There was a problem hiding this comment.
Ah, I just reviewed @eddymoulton's new PR before I read this comment....
I think having to serialise is worth it to avoid coupling. I dont think we do loose the benefits of a strongly typed property where we need to because in these places we specifically do not want it to be strongly typed so we can't use that type...
There was a problem hiding this comment.
I'm not sure I fully agree on the coupling, a single encoded string seems worse than the property bag that we wanted to originally avoid.
That being said it's not a hill I feel strongly enough to die on, so if this minimizes the rest of the impacts to the pipeline then im happy to 👍
|
@eddymoulton are you happy to instead replace this PR for consideration with @jimmyp with your non IAuthContext approach in |
Yep, I'm happy with those replacement PRs. I'll close this and the related server PR. |
Background
For deployment tasks run on the Kubernetes agent, we require additional context about the project/tenant/environment/step that initiated the script currently being run as a mechanism to provide granular permissions.
Results
The
ExecuteKubernetesScriptCommandnow has an additional property to provide this context in the form ofIAuthContext. An interface was used here to ensure a clean separation of concerns for Kubernetes deployments from all others. While this feature is targetted purely at Kubernetes agent deployments right now, the concepts could be applied more broadly - if that was to happen thenIAuthContextis the seam that we'd like to see that done on.For the granular permissions functionality, we are using labels on the created script pod to act as our interaction point to apply correct permissions where needed.
How to review this PR
Quality ✔️
Pre-requisites