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.

Sign up for GitHub

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

Jump to bottom

Allow Shared Namespace Ownership for Projects #1654

Open
3 tasks done
isugimpy opened this issue Mar 20, 2024 · 4 comments · May be fixed by #1667
Open
3 tasks done

Allow Shared Namespace Ownership for Projects #1654

isugimpy opened this issue Mar 20, 2024 · 4 comments · May be fixed by #1667
Assignees
@krancour @isugimpy
Labels
area/controller area/security area/webhooks kind/enhancement priority/normal
Milestone
v0.7.0

Comments

@isugimpy
Copy link

isugimpy commented Mar 20, 2024 •
edited

Checklist

  • I've searched the issue queue to verify this is not a duplicate feature request.
  • I've pasted the output of kargo version, if applicable.
  • I've pasted logs, if applicable.

Proposed Feature

Allow the Project resource to share ownership of a Namespace by appending to the OwnerReferences instead of giving up.

Motivation

As a platform owner, I've already got a managed mechanism for creation of Namespaces. This directly conflicts with the Project implementation in Kargo because Kargo will only adopt a namespace if the OwnerReferences field has no items in it.

Suggested Implementation

Add additional control flow to

kargo/internal/controller/management/projects/projects.go

Lines 240 to 254 in 33247fe

if ns.Labels != nil &&
ns.Labels[kargoapi.ProjectLabelKey] == kargoapi.LabelTrueValue &&
len(ns.OwnerReferences) == 0 {
logger.Debug(
"namespace exists, but is not owned by this Project, but has the " +
"project label; Project will adopt it",
)
ns.OwnerReferences = []metav1.OwnerReference{*ownerRef}
controllerutil.AddFinalizer(ns, kargoapi.FinalizerName)
if err = r.updateNamespaceFn(ctx, ns); err != nil {
return status, fmt.Errorf("error updating namespace %q: %w", project.Name, err)
}
logger.Debug("updated namespace with Project as owner")
return status, nil
}
to append to the OwnerReferences list if it's present. This could potentially be gated by a boolean field on the Project resource, or a CLI flag for whichever Kargo controller implements this behavior, but the gate doesn't seem strictly necessary.

Additional Notes

I'm willing to toss a contribution for this if it'd be accepted. I'll openly admit, I haven't tried Kargo yet, hoping to do so yet this week, but the current behavior is a potential blocker for my organization because we manage namespaces via an in-house controller that maintains ownership.
@krancour
Copy link
Member

@isugimpy thanks for the suggestion. I can see how this is a problem for you.

The reason for the current behavior is that it is meant to prevent "hijacking" an existing namespace.

I'd love if we could do what you're suggesting, but I'm not sure yet how we could mitigate that risk.

Happy to hear your thoughts.

@isugimpy
Copy link
Author

The concern is totally valid! That's actually why I suggested a gate on the CR saying that Project is allowed to share ownership of the namespace, so that it isn't the default, but rather an opt-in behavior. I could also be fine with a label or annotation on the namespace that is a cue to Kargo to accept that shared ownership model. That may actually be safer, because then it's preventing an unprivileged user from doing something like creating a Project that could assume ownership of kube-system or something like that.

@krancour
Copy link
Member

That's actually why I suggested a gate on the CR saying that Project is allowed to share ownership of the namespace, so that it isn't the default, but rather an opt-in behavior.

My concern there was that it only prevents an accidental hijacking and not an intentional one.

I could also be fine with a label or annotation on the namespace that is a cue to Kargo to accept that shared ownership model. That may actually be safer, because then it's preventing an unprivileged user from doing something like creating a Project that could assume ownership of kube-system or something like that.

I quite like this idea. It's actually what we do for Argo CD Applications as well. An annotation on an Application indicates consent for Kargo to interact with it, provided by someone who obviously had sufficient permission to add that annotation.

I'm willing to toss a contribution for this if it'd be accepted.

Given the revised approach -- we'd absolutely love to have you contribute that! Thank you!

@isugimpy isugimpy linked a pull request Mar 21, 2024 that will close this issue
Open
@isugimpy
Copy link
Author

Opened #1667 with the proposed solution!

@krancour krancour added this to the v0.6.0 milestone Mar 21, 2024
@krancour krancour modified the milestones: v0.6.0, v0.7.0 Apr 22, 2024
@krancour krancour self-assigned this May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@krancour krancour

@isugimpy isugimpy

Labels
area/controller area/security area/webhooks kind/enhancement priority/normal
Projects
None yet
Milestone
v0.7.0
Development

Successfully merging a pull request may close this issue.

feat: add support for shared namespace ownership isugimpy/kargo
2 participants
@krancour @isugimpy