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

modify role to clusterrole #21

Merged
merged 3 commits into from
Oct 22, 2019
Merged

modify role to clusterrole #21

merged 3 commits into from
Oct 22, 2019

Conversation

innerpeacez
Copy link
Member

No description provided.

@innerpeacez
Copy link
Member Author

@wu-sheng @hanahmily help review

Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

LGTM.

@wu-sheng wu-sheng added the enhancement New feature or request label Oct 19, 2019
@hanahmily
Copy link
Contributor

Most of it looks great. Thanks @innerpeacez. We leverage oap permission here is because envoy recevier intends to access API server get information across the entire cluster. So a more fine grain method is to identify whether user opens this receiver then grant oap a appropriate permission. Well, this way is much simpler for our user. Can you document it in README. md

@wu-sheng
Copy link
Member

Most of it looks great. Thanks @innerpeacez. We leverage oap permission here is because envoy recevier intends to access API server get information across the entire cluster. So a more fine grain method is to identify whether user opens this receiver then grant oap a appropriate permission. Well, this way is much simpler for our user. Can you document it in README. md

@innerpeacez I think env varible description, helm guide and API server grant, should be added in this doc, https://github.com/apache/skywalking/blob/master/docs/en/setup/envoy/als_setting.md#observe-service-mesh-through-als

@wu-sheng
Copy link
Member

Change the document step 2-3 and guide the user to use the helm, please

@innerpeacez
Copy link
Member Author

innerpeacez commented Oct 20, 2019

I will add a oap.envoy.als.enabled key to the values.yaml file to control whether envoy sla is turned on. And when turned on, clusterrole will be used。 @hanahmily @wu-sheng

@innerpeacez
Copy link
Member Author

@hanahmily @wu-sheng Help review again,thanks .

@innerpeacez
Copy link
Member Author

If there are no problems, I will merge this PR

```yaml
oap:
envoy:
sla:
Copy link
Contributor

Choose a reason for hiding this comment

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

typo. should be als

Comment on lines 213 to 214
When envoy sla , will give ServerAccount clusterrole permission.
More envoy sla ,please refer to https://github.com/apache/skywalking/blob/master/docs/en/setup/envoy/als_setting.md#observe-service-mesh-through-als
Copy link
Contributor

Choose a reason for hiding this comment

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

from sla to als


Envoy ALS(access log service) provides fully logs about RPC routed, including HTTP and TCP.

If you want to open envoy sla, you can do this by modifying values.yaml.
Copy link
Contributor

Choose a reason for hiding this comment

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

should be als

Copy link
Contributor

Choose a reason for hiding this comment

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

and should be upper-case

@@ -56,6 +56,7 @@ The following table lists the configurable parameters of the Skywalking chart an
| `oap.nodeSelector` | OAP labels for master pod assignment | `{}` |
| `oap.tolerations` | OAP tolerations | `[]` |
| `oap.resources` | OAP node resources requests & limits | `{} - cpu limit must be an integer` |
| `oap.envoy.sla.enabled` | Open envoy sla | `false` |
Copy link
Contributor

Choose a reason for hiding this comment

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

als

envoy:
als:
enabled: false
# more envoy sla ,please refer to https://github.com/apache/skywalking/blob/master/docs/en/setup/envoy/als_setting.md#observe-service-mesh-through-als
Copy link
Contributor

Choose a reason for hiding this comment

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

should be ALS

@hanahmily
Copy link
Contributor

@innerpeacez after some typo is fixed I think it's fine to merge it.

@innerpeacez innerpeacez merged commit f0e0f13 into apache:master Oct 22, 2019
@innerpeacez innerpeacez deleted the modify branch October 22, 2019 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants