Skip to content

Comments

Application developer self managed CRDs#1716

Merged
robinAwallace merged 2 commits intomainfrom
rw/user-crds
Sep 7, 2023
Merged

Application developer self managed CRDs#1716
robinAwallace merged 2 commits intomainfrom
rw/user-crds

Conversation

@robinAwallace
Copy link
Contributor

@robinAwallace robinAwallace commented Aug 11, 2023

What kind of PR is this?

Required: Mark one of the following that is applicable:

  • kind/feature
  • Adds support for user to self manage CRDs.
  • Adds a chart to install required clusterroles/bindings for operators for self management.
  • Adds possibility to create a service account and kube config to act as a end user.

Which issue this PR fixes: fixes #1613

Public facing documentation PR (if applicable)

Special notes for reviewer:

I would say we probably also need to look at adding pre-defined webhooks for some operators.
Also for us to manage these per-defined/installed roles we probably should define what versions of operators we support. So end users can actually install the operator.

Add a screenshot or an example to illustrate the proposed solution:

Checklist:

  • Added relevant notes to WIP-CHANGELOG.md
  • Proper commit message prefix on all commits
  • Updated the public facing documentation
  • Is this changeset backwards compatible for existing clusters? Applying:
    • is completely transparent, will not impact the workload in any way.
    • requires running a migration script.
    • will create noticeable cluster degradation.
      E.g. logs or metrics are not being collected or Kubernetes API server
      will not be responding while upgrading.
    • requires draining and/or replacing nodes.
    • will change any APIs.
      E.g. removes or changes any CK8S config options or Kubernetes APIs.
    • will break the cluster.
      I.e. full cluster migration is required.
  • Chart checklist (pick exactly one):
    • I upgraded no Chart.
    • I upgraded a Chart and determined that no migration steps are needed.
    • I upgraded a Chart and added migration steps.

Pipeline config (if applicable)
If you change some config options (e.g. add/rename variable or change the default value) you may need to update the config used by the pipeline in pipeline/config.

Copy link

@llarsson llarsson left a comment

Choose a reason for hiding this comment

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

I love where this is going! 😄

Copy link
Contributor

@viktor-f viktor-f left a comment

Choose a reason for hiding this comment

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

Nice, work. Good addition with the service account kubeconfig.

Copy link
Contributor

@cristiklein cristiklein left a comment

Choose a reason for hiding this comment

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

So cool to see userCRDs moving forward. I love how you even included tests for the most security-critical aspect of the implementation, i.e., the OPA policy.

My main concerns are about how we communicate this feature to the outside:

  • The PR needs to be harmonized with our Glossary.
  • The PR is too valuable to be "hidden" in apps's GitHub repo. Let's make it easy to find on elastisys.io. This fits nicely with the concept of self-managed services.

Anyway, really happy to see this happening!

@viktor-f
Copy link
Contributor

Since you already are working on the kubeconfig script. Feel free to fix this as well: #1723. It should be fairly small.

@robinAwallace robinAwallace changed the title End user self managed CRDs Application developer self managed CRDs Aug 24, 2023
@Xartos Xartos linked an issue Aug 25, 2023 that may be closed by this pull request
@robinAwallace
Copy link
Contributor Author

I feel that I have now fixed the comments you guys gave me, thanks 👍
The docs PR can be found here https://github.com/elastisys/compliantkubernetes/pull/660
Please take another look.

@robinAwallace robinAwallace force-pushed the rw/user-crds branch 2 times, most recently from 2b602d2 to 27717fc Compare August 31, 2023 06:09
@robinAwallace robinAwallace force-pushed the rw/user-crds branch 2 times, most recently from af2b020 to 8ba39b4 Compare August 31, 2023 06:21
Copy link
Contributor

@cristiklein cristiklein left a comment

Choose a reason for hiding this comment

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

I have one more concern. Can you clarify why we can't use resourceNames?

rules:
- apiGroups: ["apiextensions.k8s.io"]
resources: ["customresourcedefinitions"]
verbs: ["get", "list", "watch", "create", "update", "patch", "delete"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use resourceNames? If not, can you please leave a comment explaining to the next person why we cannot use resourceNames and reassure them that OPA will deny requests of unsuitable CRDs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know I tried resourceNames for something else and it did not work. So i dropped that thought. But now I look at this, yeah it should be possible. I will try it out.
But if that works then the OPA policy maybe is not needed? 😢

One reason that opa can be better, is that it can check on multiple variables instead of just the name.
At the moment the name and group need to match.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see no reason to through the nice OPA script out the window.

A bit like with PSAs, I think it would be great if we could go for a "security in depth" approach here too. resourceNames for "course-grained" filtering, OPA for "fine-grained" filtering.

Copy link
Contributor Author

@robinAwallace robinAwallace Sep 4, 2023

Choose a reason for hiding this comment

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

Okay, I found why resourceNames does not work here. It is not possible to restrict create or delete with resourceNames. So we must rely on OPA for this.

Note: You cannot restrict create or deletecollection requests by their resource name. For create, this limitation is because the name of the new object may not be known at authorization time.

Ref: https://kubernetes.io/docs/reference/access-authn-authz/rbac/#referring-to-resources

Copy link
Contributor

Choose a reason for hiding this comment

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

Mkey, and I guess pre-creating the CRD or not allowing its deletion would lead to poor DX, right? If so, please just add a comment.

Copy link

Choose a reason for hiding this comment

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

Pre-creating and preventing deletion it would lead to inability to follow normal "here's how you add X to your cluster" instructions, so if we can enable people to just follow normal "helm install"-types of guides, it would be very nice from a DX perspective.

@cristiklein
Copy link
Contributor

@robinAwallace Sorry to be a PITA.

I slept over it. I understand that we cannot use resourceNames for create. However, I think we should use resourceNames for patch, update and delete. We can leave get, watch, list un-resourceNames-ed. Sorry for the poor grammar. 😄


## Open policy agent configuration
gatekeeper:
restrictUserCRDs:
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit torn on the usage of allow and restrict.
Because by default devs can't create crds, but I'm not sure on what to do

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel allowUserCRDs (default false) would better capture the intention of this flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I also prefer allow actually. As you say it is "restricted" by default. And this allow users to use CRDs.

@robinAwallace
Copy link
Contributor Author

robinAwallace commented Sep 5, 2023

@robinAwallace Sorry to be a PITA.

I slept over it. I understand that we cannot use resourceNames for create. However, I think we should use resourceNames for patch, update and delete. We can leave get, watch, list un-resourceNames-ed. Sorry for the poor grammar. 😄

No problems 👍 Yes I could add it to patch, update. Unsure about delete as it also says "You cannot restrict ... deletecollection". Unsure if that could cause an issue when uninstalling a chart with multiple CRDs.

Delete and deletecollection are two different verbs, so no issues.

Copy link
Contributor

@cristiklein cristiklein left a comment

Choose a reason for hiding this comment

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

Awesome! No more comments from my end.

@robinAwallace robinAwallace merged commit 1bb1167 into main Sep 7, 2023
@robinAwallace robinAwallace deleted the rw/user-crds branch September 7, 2023 07:40
@llarsson
Copy link

llarsson commented Sep 7, 2023

🎉

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.

Overriding baseDomain in wc-config generates faulty user kubeconfigs [3] UserAdmin predefined allowed list of CRDs

7 participants