Fully support generation of K8s RBAC resources #31797

Sgitario · 2023-03-13T08:48:21Z

These changes address a long-time issue regarding K8s RBAC resources (see related issues).

These changes allow generating the Roles, ClusterRoles, ServiceAccount, and RoleBindings resources. Plus, it will enable the Kubernetes Client and Kubernetes Config extensions to configure the role binding to generate.

Fix #16612
Fix #19286
Fix #15422

github-actions · 2023-03-13T09:20:26Z

🙈 The PR is closed and the preview is expired.

iocanel · 2023-03-13T09:36:27Z

There was an old abandoned pull request: #21595 that was related this.
With a quick glance the PR seems to be aligned with the views expressed while reviewing that pr and that is a good sign 😄

I will come back with a detailed review later today.

iocanel · 2023-03-13T13:56:26Z

The configuration feels more verbose than it needs to.

To deal with the listed issues all we have to do is allow customization of the actual permission the application has.

We don't need to support multiple service accounts, as the application can't have more than one.
We don't need to bind users to roles, as it's not in our scope.
We don't need support multiple namespaces
and so on.

So what do we need?

A way to tell what verbs are allowed per group of resources and if this relationship has cluster wide effect or not.
A way to specify existing roles that would be bound to the application.

Note: I don't intend to veto those changes, as it is a step forward over what we have and honestly last time I did in #21595 resulted in 18 months of doing nothing, but I am leaving this comment just to express my view.

Sgitario · 2023-03-13T15:03:06Z

The configuration feels more verbose than it needs to.

To deal with the listed issues all we have to do is allow customization of the actual permission the application has.

We don't need to support multiple service accounts, as the application can't have more than one.

We don't need to bind users to roles, as it's not in our scope.

We don't need support multiple namespaces
and so on.

So what do we need?

A way to tell what verbs are allowed per group of resources and if this relationship has cluster wide effect or not.

A way to specify existing roles that would be bound to the application.

Note: I don't intend to veto those changes, as it is a step forward over what we have and honestly last time I did in #21595 resulted in 18 months of doing nothing, but I am leaving this comment just to express my view.

Probably, it's more verbose than it needs to be, but it also gives much more freedom to generate complex RBAC resources (for example: for operator generation). So, it's up to the user to generate as many resources as they desire.

Sgitario · 2023-03-14T08:06:28Z

@iocanel are you ok with the changes part of this pull request as they are? I don't think we lose anything if we allow users generating as many RBAC resources as they need.

iocanel · 2023-03-14T08:31:59Z

@iocanel are you ok with the changes part of this pull request as they are? I don't think we lose anything if we allow users generating as many RBAC resources as they need.

We do loose simplicity and ease of use.

Anyway, this is clearly an improvement over what we have so yeah I am ok with it.

iocanel

This needs to be documented.

Sgitario · 2023-03-14T08:35:46Z

This needs to be documented.

+1

Sgitario · 2023-03-14T10:35:43Z

PR updated with updates on the documentation.

iocanel

The implementation overall looks good with some minor improvements that have been marked on the documentation. These are changes I'd like us to introduce to make things slightly less verbose (but without altering the nature of the pull request).

Also added some change requests on the structure of documentation, so that it's less intimidating and easier to digest.

docs/src/main/asciidoc/deploying-to-kubernetes.adoc

iocanel · 2023-03-15T10:54:47Z

docs/src/main/asciidoc/deploying-to-kubernetes.adoc

+quarkus.kubernetes.rbac.role-bindings.my-role-binding.cluster-wide=false
+
+# Use the service account "my-service-account" in the application
+quarkus.kubernetes.service-account=my-service-account <3>


I am wondering if this could be inferred.

We could possibly generated namespace when a namespace has been defined for the deployment or is referred by the rules.

The only challenge is what happens, if we want to refer to an externally created ServiceAccount. However, this is not our job and on top of that what we generate should be standalone.

TLDR: This should be optional.

I would actually avoid the "inferred" configuration, so it should be up to the user to use one service account or another.

We should always use the application service account, unless explicitly specified.
There is absolutely no reason specifying 2 full lines of configuration for something that should be the application service account for most of the cases.

docs/src/main/asciidoc/deploying-to-kubernetes.adoc

Sgitario · 2023-03-15T14:34:53Z

@iocanel I've updated the changes you suggested and making fully optional to configure the role binding:

if one role is set, it will generate the role binding using the default service account

Also, I've created a logic that detects the cluster-wide flag when configuring the role binding.

Please, have another round to review again the changes.

Sgitario · 2023-03-16T12:49:09Z

@iocanel PR updated with:

Removed RBAC configuration from the Kubernetes Client extension. Now, the role binding will be only generated if and only if the user didn't provide any and the quarkus.kubernetes-client.generate-rbac is true (note that this is a breaking change, so I will note it in the migration guide after merging this pull request).

iocanel · 2023-03-16T13:12:59Z

Now, the role binding will be only generated if and only if the user didn't provide any and the quarkus.kubernetes-client.generate-rbac is true (note that this is a breaking change, so I will note it in the migration guide after merging this pull request).

Why do we have this change?

edit: I mean why do we change the behavior of quarkus.kubernetes-client.generate-rbac?

Sgitario · 2023-03-16T13:20:53Z

Now, the role binding will be only generated if and only if the user didn't provide any and the quarkus.kubernetes-client.generate-rbac is true (note that this is a breaking change, so I will note it in the migration guide after merging this pull request).

Why do we have this change?

edit: I mean why do we change the behavior of quarkus.kubernetes-client.generate-rbac?

The behavior change is that regardless of the value in quarkus.kubernetes-client.generate-rbac, the role binding with role "view" won't be generated if users provide their own role-binding from config (this config is new, so it should not be a problem).

However, when using the extension quarkus-kubernetes-config, this is also adding a role binding "view-secrets" (this is the default name), so in this scenario, the role binding from the kubernetes client won't be generated. (See the Kubernetes config tests that I had to modify). Note that we can change this if you think this is not correct).

iocanel · 2023-03-16T13:26:50Z

The behavior change is that regardless of the value in quarkus.kubernetes-client.generate-rbac, the role binding with role "view" won't be generated if users provide their own role-binding from config.

This is something I totally agree!

iocanel · 2023-03-16T13:33:30Z

However, when using the extension quarkus-kubernetes-config, this is also adding a role binding "view-secrets" (this is the default name), so in this scenario, the role binding from the kubernetes client won't be generated. (See the Kubernetes config tests that I had to modify). Note that we can change this if you think this is not correct).

I would approach it the other way around given that the kubernetes-client default rbac setup is less restrictive.

So, here's how I think that it should work:

kubernetes-client with no custom rbac config: generate the rbac we do today
kubernetes-client with custom rbac config or resources: generate what has been configured (no addtional things for kubernetes-client).
kubernetes-client with kubernetes-config: Generate the kubernetes-client staff (as if kubernetes-config was not even there).
kubenretes-client with kubenretes-config and custom rbac config: generate what has been configured (no addtional things for kubernetes-client or kubenretes-config).
kubernetes-config with no rbac: generate the rbac we do today (for kubernetes-config)
kubenretes-client with rbac config: generate what has been configured (no addtional things for kubenretes-config).

Sgitario · 2023-03-16T14:21:45Z

PR updated again by avoiding the breaking change between kubernetes-config and kubernetes-client extensions, this will keep working as before.

The only think to note in the migration guide is:

The behavior change is that regardless of the value in quarkus.kubernetes-client.generate-rbac, the role binding with role "view" won't be generated if users provide their own role-binding from config.

iocanel

lgtm

These changes address a long-time issue in regards of K8s RBAC resources (see related issues). These changes allow to generate custom Roles, ClusterRoles, ServiceAccount, and RoleBindings. Plus, it allows the Kubernetes Client and Kubernetes Config extensions to configure the role binding to generate. Fix quarkusio#16612 Fix quarkusio#19286 Fix quarkusio#15422

quarkus-bot · 2023-03-21T10:45:18Z

Failing Jobs - Building `ea9425b`

Status	Name	Step	Failures	Logs	Raw logs
✖	JVM Tests - JDK 11	`Build`	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 17
✔️	JVM Tests - JDK 19

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: integration-tests/opentelemetry-jdbc-instrumentation

📦 integration-tests/opentelemetry-jdbc-instrumentation

✖ io.quarkus.it.opentelemetry.OracleOpenTelemetryJdbcInstrumentationTest.testOracleQueryTraced line 14 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: JDBC insert statement was not traced. ==> expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)

Sgitario · 2023-03-21T11:38:19Z

The failure is unrelated. Merging.

zZHorizonZz · 2023-04-05T11:10:21Z

I'm currently experimenting with cluster role generation through the .properties config but, I can't get my head around how to specify the core API group for the cluster role. Because if I just left it blank the API group fields don't get generated at all and I get this:

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  name: conductor-role
rules:
    resources:
      - pods
      - namespaces
    verbs:
      - get
      - watch
      - list
      - create
      - update
      - patch
      - delete

If I specify this '' as per official K8s documentation I get this:

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  name: conductor-role
rules:
  - apiGroups:
      - ''''''
    resources:
      - pods
      - namespaces
    verbs:
      - get
      - watch
      - list
      - create
      - update
      - patch

And if I try "" then I get this:

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  name: conductor-role
rules:
  - apiGroups:
      - '""'
    resources:
      - pods
      - namespaces
    verbs:
      - get
      - watch
      - list
      - create
      - update
      - patch

I also tried just not specifying it at all but that resulted (Same error occurs for example 1 as well) in k8s error:

ClusterRole.rbac.authorization.k8s.io "test-role" is invalid: rules[0].apiGroups: Required value: resource rules must supply at least one api group

Sgitario · 2023-04-11T06:10:24Z

I'm currently experimenting with cluster role generation through the .properties config but, I can't get my head around how to specify the core API group for the cluster role. Because if I just left it blank the API group fields don't get generated at all and I get this:

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  name: conductor-role
rules:
    resources:
      - pods
      - namespaces
    verbs:
      - get
      - watch
      - list
      - create
      - update
      - patch
      - delete

If I specify this '' as per official K8s documentation I get this:

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  name: conductor-role
rules:
  - apiGroups:
      - ''''''
    resources:
      - pods
      - namespaces
    verbs:
      - get
      - watch
      - list
      - create
      - update
      - patch

And if I try "" then I get this:

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  name: conductor-role
rules:
  - apiGroups:
      - '""'
    resources:
      - pods
      - namespaces
    verbs:
      - get
      - watch
      - list
      - create
      - update
      - patch

I also tried just not specifying it at all but that resulted (Same error occurs for example 1 as well) in k8s error:

ClusterRole.rbac.authorization.k8s.io "test-role" is invalid: rules[0].apiGroups: Required value: resource rules must supply at least one api group

Thanks for spotting. This is indeed an issue. I've reported it in #32519.

zZHorizonZz · 2023-04-11T07:01:23Z

I'm currently experimenting with cluster role generation through the .properties config but, I can't get my head around how to specify the core API group for the cluster role. Because if I just left it blank the API group fields don't get generated at all and I get this:

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  name: conductor-role
rules:
    resources:
      - pods
      - namespaces
    verbs:
      - get
      - watch
      - list
      - create
      - update
      - patch
      - delete

If I specify this '' as per official K8s documentation I get this:

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  name: conductor-role
rules:
  - apiGroups:
      - ''''''
    resources:
      - pods
      - namespaces
    verbs:
      - get
      - watch
      - list
      - create
      - update
      - patch

And if I try "" then I get this:

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  name: conductor-role
rules:
  - apiGroups:
      - '""'
    resources:
      - pods
      - namespaces
    verbs:
      - get
      - watch
      - list
      - create
      - update
      - patch

I also tried just not specifying it at all but that resulted (Same error occurs for example 1 as well) in k8s error:

ClusterRole.rbac.authorization.k8s.io "test-role" is invalid: rules[0].apiGroups: Required value: resource rules must supply at least one api group

Thanks for spotting. This is indeed an issue. I've reported it in #32519.

Also, I have noticed something which I don't know if it's intended or not but I have been testing cluster role generation in kubernetes config it generated RoleBinging instead of ClusterRoleBinding, and because of that when I was using the k8s client it didn't seem like I had the role bound because I didn't have access to resources which I should have if that role was assigned.

Edit: After reading k8s documentation I came to the conclusion that this isn't probably a bug. Since you can bind via RoleBinging but it will make that role available for that specific namespace. But since I have been trying to create a new namespace that probably counts as accessing a new namespace so RoleBinging doesn't cover that.
But it would be nice to have a probably similar system to that of ClusterRole which would determine if it should generate ClusterRoleBinding or not.

Sgitario requested a review from iocanel March 13, 2023 08:48

quarkus-bot bot added area/documentation area/kubernetes labels Mar 13, 2023