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

Support Any Additional resource #416

Closed
oliverbaehler opened this issue Sep 9, 2021 · 7 comments · Fixed by #644
Closed

Support Any Additional resource #416

oliverbaehler opened this issue Sep 9, 2021 · 7 comments · Fixed by #644
Assignees
Labels
enhancement New feature or request helm
Milestone

Comments

@oliverbaehler
Copy link
Collaborator

Describe the feature

I would like to have a generic approach on resources, which are created for each new namespace belonging to a tenant. The same behavior that .spec.additionalRoleBindings and .spec.networkPolicies already implement, but for any kubernetes resource.

As of why:

We are currently moving to Cilium Network Policies, and we would require each namespace to implement those policies (same could also apply for calico resources or really anything else). So instead of writing an own workaround, it would make sense to be able to declare something like this:

---
apiVersion: capsule.clastix.io/v1beta1
kind: Tenant
metadata:
  name: gas
spec:
  additionalResources:
    -  apiVersion: "cilium.io/v2"
       kind: CiliumNetworkPolicy 
       metadata:
         name: "l3-rule"
      spec:
        endpointSelector:
          matchLabels:
             role: backend
       ingress:
       - fromEndpoints:
         - matchLabels:
             role: frontend

What would the new user story look like?

How would the new interaction with Capsule look like? E.g.

  1. A new Tenant is created with additional resources
  2. The resources could be validated on tenant creation and reject the tenant creation if there's an error, if not the tenant is created.
  3. A new User creates/assigns a new namespace to the previously created tenant.
  4. the additional resources are added to the namespace once the operator has reconciled.
  5. Everyone is happy.

Expected behavior

A clear and concise description of what you expect to happen.

When I have defined .spec.additionalResources on a tenant, those resources are created for each namespace that is assigned or created in that tenant.

@oliverbaehler oliverbaehler added the blocked-needs-validation Issue need triage and validation label Sep 9, 2021
@bsctl
Copy link
Member

bsctl commented Sep 19, 2021

@oliverbaehler thanks for suggesting this improvement. I think this will require some sort of refactoring of the code. Let's to see what @prometherion says.

@bsctl bsctl added enhancement New feature or request needs-discussion No outline on the feature, discussion is welcome labels Sep 19, 2021
@prometherion
Copy link
Member

I'm a bit late on this, thanks for the patience!

This feature would be awesome and I couldn't agree more, despite some limitations I'd like to evaluate with the community.

We have to keep in mind that CRDs are, essentially, OpenAPI v3 specifications: we got a schema that must stick to it and that's great, I love it when everything is strictly typed since it's less error-prone and safer, especially at compile time.
The first challenge would be how to structure the /spec/additionalResources field: although from a Go struct type point of view a []interface{} type could do the trick, the resulting OpenAPI v3 wouldn't be so happy (ndr: not sure, should be tested), and even if it wouldn't be the case, we would have to deal with another issue, how to apply these resources.

I'm really happy with the unversioned client set offered by controller-runtime since we just need to register the schemes and everything is done under the hoods, and this simplicity is a trade-off on the flexibility required by this feature request: that would be impossible using this client, or at least would be required the dynamic client that we're not using.

The dynamic client setup looks easy as pie, also considering we already have whatever is needed: I'm more concerned regarding the naming of the resources that must be prefixed to avoid collision (especially for the cluster-scoped resources) and the set of the Namespace field.

tl;dr; we would be required to deal with https://k8s.io/apimachinery/pkg/apis/meta/v1/unstructured.Unstructured type that seems able to set Namespace and Name.

A lot of words and a simple question: should we implement this on our own or, rather, take advantage of HNC? We would just need to point the Tenant to a template Namespace and that controller would start replicating the resources without hacking so much our code-base, although adding a tough dependency since an external component would be required, not sure if we can grab their code and start it using our Manager, need to be investigated.

@oliverbaehler
Copy link
Collaborator Author

Another Solution to this would be just using generate Functionalities of e.g Kyverno (https://kyverno.io/docs/writing-policies/generate/). You could write policies which would generate the resources based on namespace labels. As far as I am concerned this would also cover the use-case. Now you would have to assume that kyverno is already installed and therefor would have kinda a dependency. But I would also say that such use cases occur in complexer setup where the probability is high, that such policy management is already deployed. This would not required to hack your codebase and also would not require a template namespace you mentioned. But it would mostly not fit in the incrementing name convention etc. But i don't know if that's a big thing.

Actually Kyverno implements a clone feature which kinda does the same thing as your proposed with the template namespace:

https://kyverno.io/docs/writing-policies/generate/#clone-a-configmap-and-propagate-changes

I am mentioning that because it could make sense to not inflate the capsule project to a all use-case resolving solution. But that's up to you guys if you think that feature should be covered by capsule.

Some Examples.

Let's say you would want to add a ciliumPolicy for each Namespace that's part of the capsule

policy.yaml

apiVersion: kyverno.io/v1
kind: ClusterPolicy
metadata:
  name: tenancy-cilium-inject
spec:
  rules:
  - name: tenancy-cilium-inject
    match:
      resources:
        kinds:
        - Namespace
      selector:
        matchExpressions:
          - key: "tenant.label"
            operator: Exists
    exclude:
      resources:
        namespaces:
        - capsule-system
    generate:
      synchronize: true
      kind: CiliumNetworkPolicy 
      data:
        apiVersion: "cilium.io/v2"
        metadata:
          name: "l3-rule"
          namespace: "{{request.object.metadata.name}}"
        spec:
          endpointSelector:
            matchLabels:
              role: backend
          ingress:
           - fromEndpoints:
             - matchLabels:
                 role: frontend

i didn't test it, but just that you get the idea of what i am talking.

@oliverbaehler
Copy link
Collaborator Author

@prometherion Still the same thoughts on this? I mean we could start a very simple feature where it just renders all the resources and validation for the basic kubernetes fields (kind,metadata) and then go from there

@prometherion prometherion self-assigned this Feb 4, 2022
@prometherion prometherion removed blocked-needs-validation Issue need triage and validation needs-discussion No outline on the feature, discussion is welcome labels Feb 4, 2022
@oliverbaehler
Copy link
Collaborator Author

After some thoughts i may have an extra idea for this feature.

What if we would also add a selector, with which we could target only specific namespaces which create the additional resource (if no selector set create in all)

So the an extra resource would look something like this

apiVersion: capsule.clastix.io/v1beta1
kind: Tenant
metadata:
  name: gas
spec:
  additionalResources:
    - selector:
        # For any namespace that matches the regexp 
        regex: "^.*-reserved$"
        # For any namespace the has the team "marketing" label
        matchLabels:
           team: "marketing"
      template: | 
         apiVersion: "cilium.io/v2"
         kind: CiliumNetworkPolicy 
         metadata:
           name: "l3-rule"
         spec:
           endpointSelector:
             matchLabels:
               role: backend
         ingress:
         - fromEndpoints:
           - matchLabels:
               role: frontend

This might also help with building a tenant based rbac, since you could assign rolebinding based on the namespace's attributes. I don't know if that would help your team @MaxFedotov.

It's just an idea, not yet exactly sure how much sense it makes.

@prometherion
Copy link
Member

@oliverbaehler JFI there's a feature request (#525 ) that is pretty similar to this. Could you provide feedback on that?

gernest added a commit to gernest/capsule that referenced this issue Apr 18, 2022
This attempts to address  projectcapsule#525 which is related to projectcapsule#416.

problem this commit is trying to solve
----

```
Alice would like to create several resources on each of their Namespaces.
Upon the creation of each Namespace, they have to create the desired resources on each Namespace
```

On the above linked tickets there are two proposed approaches

approach 01
----

Create a new resource `TenantResource`  something like this

```yaml
apiVersion: capsule.clastix.io/v1beta2
kind: TenantResource
metadata:
  name: database
  namespace: solar-management
spec:
  resyncPeriod: 60s
  additionalResources:
    namespaceSelector:
      matchLabels:
        tier: one
    items:
    - apiVersion: presslabs.io/v1beta1
      kind: MySQL
      spec:
        foo: bar
  clusterRoles:
    namespaceSelector:
      matchLabels:
        tier: one
    items:
      - name: database-admin
        subjects:
        - kind: Group
          name: developers
```

approach 02
-----

Extend `Tenant` to  support addtional resources

```yaml
apiVersion: capsule.clastix.io/v1beta1
kind: Tenant
metadata:
  name: gas
spec:
  additionalResources:
    -  apiVersion: "cilium.io/v2"
       kind: CiliumNetworkPolicy
       metadata:
         name: "l3-rule"
      spec:
        endpointSelector:
          matchLabels:
             role: backend
       ingress:
       - fromEndpoints:
         - matchLabels:
             role: frontend
```

This commit implements approach `02` due to the following reasons

- The namespaces belong to the tenant already, extra `TenantResource` seems redundant given that the lifecycle of the additional resources are tied to the `Tenant`.

How does the crd look like now ?
----

```yaml
apiVersion: capsule.clastix.io/v1beta1
kind: Tenant
metadata:
  name: oil
spec:
  resyncPeriod: 60s
  additionalResources:
    namespaceSelector:
      matchLabels:
        tier: one
    items:
      - |
        apiVersion: v1
        kind: Pod
        metadata:
          name: nginx
          labels:
            app.kubernetes.io/name: proxy
        spec:
          containers:
            - name: nginx
              image: nginx:11.14.2
              ports:
                - containerPort: 80
                  name: http-web-svc
      - |
        apiVersion: v1
        kind: Service
        metadata:
          name: nginx-service
          labels:
            app.kubernetes.io/name: proxy
        spec:
          selector:
            app.kubernetes.io/name: proxy
          ports:
            - name: name-of-service-port
              protocol: TCP
              port: 80
              targetPort: http-web-svc
  owners:
    - name: alice
      kind: User
```

The difference with the proposed crd is, items are strings of k8s objects in yaml
format. I ran through decoding issues when I tried to use `Unstructured`  so I
decided to settle on strings instead.

How it works ?
----

We search for namespaces  specified by `namespaceSelector` that are owned by the
tenant. For each matched namespace, apply all resources specified in `additionalResources.items`
on any error, reschedule next reconciliation by `resyncPeriod`.

What is missing ?
----

- [ ] Tests
- [ ] What happens when a tenant is deleted ?
- [ ] What happens when a tenant is deleted ?
- [ ] Does `additionalRoleBindings` cover for  `clusterRoles` defined in approach 01?

I will wait for feedback/discussion on how to proceed from here.
gernest added a commit to gernest/capsule that referenced this issue Apr 18, 2022
This attempts to address  projectcapsule#525 which is related to projectcapsule#416.

problem this commit is trying to solve
----

```
Alice would like to create several resources on each of their Namespaces.
Upon the creation of each Namespace, they have to create the desired resources on each Namespace
```

On the above linked tickets there are two proposed approaches

approach 01
----

Create a new resource `TenantResource`  something like this

```yaml
apiVersion: capsule.clastix.io/v1beta2
kind: TenantResource
metadata:
  name: database
  namespace: solar-management
spec:
  resyncPeriod: 60s
  additionalResources:
    namespaceSelector:
      matchLabels:
        tier: one
    items:
    - apiVersion: presslabs.io/v1beta1
      kind: MySQL
      spec:
        foo: bar
  clusterRoles:
    namespaceSelector:
      matchLabels:
        tier: one
    items:
      - name: database-admin
        subjects:
        - kind: Group
          name: developers
```

approach 02
-----

Extend `Tenant` to  support addtional resources

```yaml
apiVersion: capsule.clastix.io/v1beta1
kind: Tenant
metadata:
  name: gas
spec:
  additionalResources:
    -  apiVersion: "cilium.io/v2"
       kind: CiliumNetworkPolicy
       metadata:
         name: "l3-rule"
      spec:
        endpointSelector:
          matchLabels:
             role: backend
       ingress:
       - fromEndpoints:
         - matchLabels:
             role: frontend
```

This commit implements approach `02` due to the following reasons

- The namespaces belong to the tenant already, extra `TenantResource` seems redundant given that the lifecycle of the additional resources are tied to the `Tenant`.

How does the crd look like now ?
----

```yaml
apiVersion: capsule.clastix.io/v1beta1
kind: Tenant
metadata:
  name: oil
spec:
  resyncPeriod: 60s
  additionalResources:
    namespaceSelector:
      matchLabels:
        tier: one
    items:
      - |
        apiVersion: v1
        kind: Pod
        metadata:
          name: nginx
          labels:
            app.kubernetes.io/name: proxy
        spec:
          containers:
            - name: nginx
              image: nginx:11.14.2
              ports:
                - containerPort: 80
                  name: http-web-svc
      - |
        apiVersion: v1
        kind: Service
        metadata:
          name: nginx-service
          labels:
            app.kubernetes.io/name: proxy
        spec:
          selector:
            app.kubernetes.io/name: proxy
          ports:
            - name: name-of-service-port
              protocol: TCP
              port: 80
              targetPort: http-web-svc
  owners:
    - name: alice
      kind: User
```

The difference with the proposed crd is, items are strings of k8s objects in yaml
format. I ran through decoding issues when I tried to use `Unstructured`  so I
decided to settle on strings instead.

How it works ?
----

We search for namespaces  specified by `namespaceSelector` that are owned by the
tenant. For each matched namespace, apply all resources specified in `additionalResources.items`
on any error, reschedule next reconciliation by `resyncPeriod`.

What is missing ?
----

- [ ] Tests
- [ ] What happens when a tenant is deleted ?
- [ ] What happens when a tenant is deleted ?
- [ ] Does `additionalRoleBindings` cover for  `clusterRoles` defined in approach 01?

I will wait for feedback/discussion on how to proceed from here.
gernest added a commit to gernest/capsule that referenced this issue Apr 18, 2022
This attempts to address  projectcapsule#525 which is related to projectcapsule#416.

problem this commit is trying to solve
----

```
Alice would like to create several resources on each of their Namespaces.
Upon the creation of each Namespace, they have to create the desired resources on each Namespace
```

On the above linked tickets there are two proposed approaches

approach 01
----

Create a new resource `TenantResource`  something like this

```yaml
apiVersion: capsule.clastix.io/v1beta2
kind: TenantResource
metadata:
  name: database
  namespace: solar-management
spec:
  resyncPeriod: 60s
  additionalResources:
    namespaceSelector:
      matchLabels:
        tier: one
    items:
    - apiVersion: presslabs.io/v1beta1
      kind: MySQL
      spec:
        foo: bar
  clusterRoles:
    namespaceSelector:
      matchLabels:
        tier: one
    items:
      - name: database-admin
        subjects:
        - kind: Group
          name: developers
```

approach 02
-----

Extend `Tenant` to  support addtional resources

```yaml
apiVersion: capsule.clastix.io/v1beta1
kind: Tenant
metadata:
  name: gas
spec:
  additionalResources:
    -  apiVersion: "cilium.io/v2"
       kind: CiliumNetworkPolicy
       metadata:
         name: "l3-rule"
      spec:
        endpointSelector:
          matchLabels:
             role: backend
       ingress:
       - fromEndpoints:
         - matchLabels:
             role: frontend
```

This commit implements approach `02` due to the following reasons

- The namespaces belong to the tenant already, extra `TenantResource` seems redundant given that the lifecycle of the additional resources are tied to the `Tenant`.

How does the crd look like now ?
----

```yaml
apiVersion: capsule.clastix.io/v1beta1
kind: Tenant
metadata:
  name: oil
spec:
  resyncPeriod: 60s
  additionalResources:
    namespaceSelector:
      matchLabels:
        tier: one
    items:
      - |
        apiVersion: v1
        kind: Pod
        metadata:
          name: nginx
          labels:
            app.kubernetes.io/name: proxy
        spec:
          containers:
            - name: nginx
              image: nginx:11.14.2
              ports:
                - containerPort: 80
                  name: http-web-svc
      - |
        apiVersion: v1
        kind: Service
        metadata:
          name: nginx-service
          labels:
            app.kubernetes.io/name: proxy
        spec:
          selector:
            app.kubernetes.io/name: proxy
          ports:
            - name: name-of-service-port
              protocol: TCP
              port: 80
              targetPort: http-web-svc
  owners:
    - name: alice
      kind: User
```

The difference with the proposed crd is, items are strings of k8s objects in yaml
format. I ran through decoding issues when I tried to use `Unstructured`  so I
decided to settle on strings instead.

How it works ?
----

We search for namespaces  specified by `namespaceSelector` that are owned by the
tenant. For each matched namespace, apply all resources specified in `additionalResources.items`
on any error, reschedule next reconciliation by `resyncPeriod`.

What is missing ?
----

- [ ] Tests
- [ ] What happens when a tenant is deleted ?
- [ ] What happens when a tenant is deleted ?
- [ ] Does `additionalRoleBindings` cover for  `clusterRoles` defined in approach 01?

I will wait for feedback/discussion on how to proceed from here.
@prometherion prometherion added this to the v0.2.0 milestone Oct 5, 2022
@prometherion prometherion modified the milestones: v0.2.0, v0.1.3 Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request helm
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants