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

Specify the specific API and resource for owners #32

Open
sftim opened this issue Feb 13, 2023 · 5 comments
Open

Specify the specific API and resource for owners #32

sftim opened this issue Feb 13, 2023 · 5 comments
Labels
blocked-needs-validation Issue need tirage and validation enhancement New feature or request help wanted Extra attention is needed lifecycle/frozen question Further information is requested

Comments

@sftim
Copy link

sftim commented Feb 13, 2023

Describe the feature

This should change:

...
spec:
  owners:
    - name: dev-team@example.com
      kind: Group

A kind is not sufficient to definitively identify which API group we mean here. Instead, we should specify owners more like:

...
spec:
  owners:
    - name: dev-team@example.com
      apiGroup:  rbac.authorization.k8s.io
      apiResources: groups # yes, the lowercase plural form that you see at the HTTP layer
    - name: bot@example.net
      apiGroup:  some.other.example
      apiResources: externalserviceaccounts # for this example, it could be an aggregated API
    - name: phippy@example.net
      apiGroup:  avatars.some.other.example
      apiResources: users # ambiguous name, disambiguated by API group

We also shouldn't assume that Kubernetes RBAC is the only game in town. You can have a conformant Kubernetes cluster that uses a different access control mechanism and runs with RBAC disabled.

Expected behavior

There's never ambiguity about how we refer to owners.

@smileisak
Copy link
Member

@sftim Thank you for your proposition.

Actually, the owners spec is a list of Subject []v1.Subject that contains already the APIGroup and Kind.

// Subject contains a reference to the object or user identities a role binding applies to.  This can either hold a direct API object reference,
// or a value for non-objects such as user and group names.
// +structType=atomic
type Subject struct {
	// Kind of object being referenced. Values defined by this API group are "User", "Group", and "ServiceAccount".
	// If the Authorizer does not recognized the kind value, the Authorizer should report an error.
	Kind string `json:"kind" protobuf:"bytes,1,opt,name=kind"`
	// APIGroup holds the API group of the referenced subject.
	// Defaults to "" for ServiceAccount subjects.
	// Defaults to "rbac.authorization.k8s.io" for User and Group subjects.
	// +optional
	APIGroup string `json:"apiGroup,omitempty" protobuf:"bytes,2,opt.name=apiGroup"`
	// Name of the object being referenced.
	Name string `json:"name" protobuf:"bytes,3,opt,name=name"`
	// Namespace of the referenced object.  If the object kind is non-namespace, such as "User" or "Group", and this value is not empty
	// the Authorizer should report an error.
	// +optional
	Namespace string `json:"namespace,omitempty" protobuf:"bytes,4,opt,name=namespace"`
}

You can as well refer to the api reference.


Although, I think that we need to update the documentation and samples to add explicitly the APIGroup.

@sftim
Copy link
Author

sftim commented Feb 13, 2023

https://nauticus.edixos.com/0.1.0/crds-apis/#spacespecownersindex doesn't mention an API group. I missed it.

I don't recommend allowing even optional references to a kind, as these can be ambiguous with CRDs. They shouldn't be but they can.

// APIGroup holds the API group of the referenced subject.
// Defaults to "" for ServiceAccount subjects.
// Defaults to "rbac.authorization.k8s.io" for User and Group subjects.
// +optional
APIGroup string json:"apiGroup,omitempty" protobuf:"bytes,2,opt.name=apiGroup"

That defaulting is hard-coding implementation details of Kubernetes RBAC. Watch out for compatibility issues with Kubernetes clusters that use an alternative mechanism (RBAC is not part of certified Kubernetes, and some clusters use different mechanisms).
Defaulting via a MutatingAdmissionWebhook works better if it's essential, because then someone can omit deploying the webhook or replace it with their own.

@smileisak
Copy link
Member

Yeah I agree, this can be managed by a MutatingWebhook if you omit it.
@sftim Do you have examples of Kubernetes distributions that does not use RBAC by default ?

@sftim
Copy link
Author

sftim commented Feb 13, 2023

Distributions? No. Although OpenShift has its own User and Group APIs that aren't the ones from RBAC.

@smileisak smileisak added enhancement New feature or request help wanted Extra attention is needed question Further information is requested blocked-needs-validation Issue need tirage and validation labels Feb 19, 2023
@github-actions
Copy link

This is stale, but we won't close it automatically, just bare in mind the maintainers may be busy with other tasks and will reach your issue ASAP. If you have any question or request to prioritize this, please reach #nauticus on Kubernetes Slack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked-needs-validation Issue need tirage and validation enhancement New feature or request help wanted Extra attention is needed lifecycle/frozen question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants