You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "innerpeacez (via GitHub)" <gi...@apache.org> on 2023/03/09 02:38:01 UTC

[GitHub] [skywalking-kubernetes] innerpeacez opened a new pull request, #111: default rbac for oap

innerpeacez opened a new pull request, #111:
URL: https://github.com/apache/skywalking-kubernetes/pull/111

   When I used the otlp protocol to collect some metrics of k8s, I found that the clusterrole permission is also required, so I optimized the configuration of rbac, and it is no longer used exclusively for envoy als.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [skywalking-kubernetes] wankai123 commented on pull request #111: default rbac for oap

Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 commented on PR #111:
URL: https://github.com/apache/skywalking-kubernetes/pull/111#issuecomment-1461192586

   The permissions are authorized here, only when user needs to monitoring k8s:
   https://github.com/apache/skywalking-showcase/blob/main/deploy/platform/kubernetes/feature-kubernetes-monitor/permissions.yaml
   
   Should we add these permissions here by default?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [skywalking-kubernetes] wu-sheng commented on pull request #111: default rbac for oap

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #111:
URL: https://github.com/apache/skywalking-kubernetes/pull/111#issuecomment-1461189762

   @wankai123 I think we need some.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [skywalking-kubernetes] wu-sheng merged pull request #111: default rbac for oap

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng merged PR #111:
URL: https://github.com/apache/skywalking-kubernetes/pull/111


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [skywalking-kubernetes] kezhenxu94 commented on a diff in pull request #111: default rbac for oap

Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #111:
URL: https://github.com/apache/skywalking-kubernetes/pull/111#discussion_r1130597015


##########
chart/skywalking/values.yaml:
##########
@@ -19,6 +19,13 @@
 
 serviceAccounts:
   oap:
+    # By default, create SkyWalking's ServiceAccount. If set to false, you also need to change `serviceAccounts.oap.name` value to a custom ServiceAccount name.
+    create: true
+    # If set to false. Some functions will not be available, such as: envoy ALS, monitoring k8s(oltp)
+    # more envoy ALS, please refer to https://github.com/apache/skywalking/blob/master/docs/en/setup/envoy/als_setting.md#observe-service-mesh-through-als
+    # when needs to monitoring k8s (oltp), refer to https://skywalking.apache.org/docs/main/v9.0.0/en/setup/backend/backend-k8s-monitoring/
+    useClusterRole: true

Review Comment:
   I think we can't allow users to completely disable to cluster role? The OAP cluster coordinator need a role otherwise it may fail to start up



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [skywalking-kubernetes] kezhenxu94 commented on a diff in pull request #111: default rbac for oap

Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #111:
URL: https://github.com/apache/skywalking-kubernetes/pull/111#discussion_r1130738845


##########
chart/skywalking/templates/oap-clusterrole.yaml:
##########
@@ -13,7 +13,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-{{- if .Values.oap.envoy.als.enabled }}
+{{- if and .Values.serviceAccounts.oap.create }}

Review Comment:
   ```suggestion
   {{- if .Values.serviceAccounts.oap.create }}
   ```



##########
chart/skywalking/templates/oap-clusterrolebinding.yaml:
##########
@@ -13,7 +13,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-{{- if .Values.oap.envoy.als.enabled }}
+{{- if and .Values.serviceAccounts.oap.create }}

Review Comment:
   ```suggestion
   {{- if .Values.serviceAccounts.oap.create }}
   ```



##########
chart/skywalking/values.yaml:
##########
@@ -19,6 +19,9 @@
 
 serviceAccounts:
   oap:
+    # By default, create SkyWalking's ServiceAccount. If set to false, you also need to change `serviceAccounts.oap.name` value to a custom ServiceAccount name.
+    create: true
+    name: ""

Review Comment:
   Can we just keep 
   
   ```yaml
     serviceAccounts:
       oap: "" # If this is set to empty, SkyWalking creates the ServiceAccount, if you change `serviceAccounts.oap` value to a custom ServiceAccount name, SkyWalking won't create ServiceAccount, but please make sure the given ServiceAccount has correct permissions.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [skywalking-kubernetes] innerpeacez commented on a diff in pull request #111: default rbac for oap

Posted by "innerpeacez (via GitHub)" <gi...@apache.org>.
innerpeacez commented on code in PR #111:
URL: https://github.com/apache/skywalking-kubernetes/pull/111#discussion_r1130757731


##########
chart/skywalking/values.yaml:
##########
@@ -19,6 +19,9 @@
 
 serviceAccounts:
   oap:
+    # By default, create SkyWalking's ServiceAccount. If set to false, you also need to change `serviceAccounts.oap.name` value to a custom ServiceAccount name.
+    create: true
+    name: ""

Review Comment:
   ```
   serviceAccounts:
     oap:
       # By default, create SkyWalking's ServiceAccount. If set to false, you also need to change `serviceAccounts.oap.name` value to a custom ServiceAccount name.
       create: true
       name: ""
   ```
   I think this is more convenient. When we deploy multiple skywalking clusters in one k8s at the same time, multiple clusterRole may be created. At this time, we can use the name field to distinguish different SW clusterRole, so as not to cause clusterRole conflicts.
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [skywalking-kubernetes] innerpeacez commented on pull request #111: default rbac for oap

Posted by "innerpeacez (via GitHub)" <gi...@apache.org>.
innerpeacez commented on PR #111:
URL: https://github.com/apache/skywalking-kubernetes/pull/111#issuecomment-1461198252

   ![image](https://user-images.githubusercontent.com/37059529/223905554-c62e4905-d28a-448c-9707-1b07a58ce16c.png)
   I added some hints.  Of course, if we don't enable this permission by default, I can set it to false.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [skywalking-kubernetes] kezhenxu94 commented on a diff in pull request #111: default rbac for oap

Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #111:
URL: https://github.com/apache/skywalking-kubernetes/pull/111#discussion_r1130354336


##########
chart/skywalking/templates/oap-role.yaml:
##########
@@ -13,11 +13,15 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-{{- if not .Values.oap.envoy.als.enabled }}
+{{- if .Values.oap.rbac.enabled }}
 kind: Role
 apiVersion: rbac.authorization.k8s.io/v1
 metadata:
+  {{- if .Values.oap.rbac.useExistingRole }}
+  name: {{ .Values.oap.rbac.useExistingRole }}
+  {{- else }}

Review Comment:
   Isn't this an error when you try to create another role with the same name user already defined `{{ .Values.oap.rbac.useExistingRole }}`? Also this doesn't accord to what is said in the yaml file (`skipping role creating`):
   
   ```yaml
        # Set to a rolename to use existing role - skipping role creating - but still doing serviceaccount and rolebinding to it, rolename set here.
        # useExistingRole: your-existing-role
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [skywalking-kubernetes] kezhenxu94 commented on pull request #111: default rbac for oap

Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on PR #111:
URL: https://github.com/apache/skywalking-kubernetes/pull/111#issuecomment-1461216662

   The reason behind 👆 is that, ServiceAccount(SA) usually represents all needed RBAC permissions for a deployment/service, users should group the permissions into the SA and designate the SA to our deployment, or allow us to create all needed permissions by ourselves. Letting users configure one part and us configure the other part looks unreasonable and hard to maintain.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [skywalking-kubernetes] wankai123 commented on pull request #111: default rbac for oap

Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 commented on PR #111:
URL: https://github.com/apache/skywalking-kubernetes/pull/111#issuecomment-1461202252

   @kezhenxu94 WDYT?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [skywalking-kubernetes] innerpeacez commented on pull request #111: default rbac for oap

Posted by "innerpeacez (via GitHub)" <gi...@apache.org>.
innerpeacez commented on PR #111:
URL: https://github.com/apache/skywalking-kubernetes/pull/111#issuecomment-1461228786

   I'm trying to understand your mind, and I'll make some changes later. @kezhenxu94 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [skywalking-kubernetes] innerpeacez commented on pull request #111: default rbac for oap

Posted by "innerpeacez (via GitHub)" <gi...@apache.org>.
innerpeacez commented on PR #111:
URL: https://github.com/apache/skywalking-kubernetes/pull/111#issuecomment-1461183824

   I don't know if there are other oap capabilities that require such permissions. Can you give me some hints, and I can add some documents. @wu-sheng @kezhenxu94 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org