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

Update kube-enforcer helm chart docs #121

Merged

Conversation

VineethReddy02
Copy link
Contributor

These changes are suggested as enhancements to docs.

@eranbibi @niso120b

This changes are suggested as enhancements to docs.
Copy link
Contributor

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Unsolicited review, but given I've made 15+ PRs to this repo, thought I'd leave some comments here since I happened to stumble upon this

@@ -2,7 +2,7 @@
imageCredentials:
# If aqua-registry already exists in the cluster. Make create to false. So it won't attempt to create a new registry secret.
create: true
name: csp-registry-secret # example
name: aqua-registry-secret # example
Copy link
Contributor

Choose a reason for hiding this comment

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

duplicates #110

## Configurable Variables

### KubeEnforcer

| Parameter | Description | Default |
| --------------------------------- | ------------------------------------ | ---------------------------------------------------------------------------- |
| `imageCredentials.create` | Set if to create new pull image secret | `true` |
| `imageCredentials.name` | Your Docker pull image secret name | `aqua-image-pull-secret` |
| `imageCredentials.name` | Your Docker pull image secret name | `aqua-registry-secret` |
Copy link
Contributor

Choose a reason for hiding this comment

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

duplicates #110

@@ -71,21 +81,25 @@ Optional flags:
--aquaSecret.kubeEnforcerToken default to "" you can find the KubeEnforcer token from aqua csp under enforcers tab in default/custom KubeEnforcer group or you can manually approve KubeEnforcer authentication from aqua CSP under default/custom KubeEnforcer group in enforcers tab.
```

## ClusterRole

KubeEnforcer needs a dedicated clusterrole with **get, list, watch** permissions on **pods, secrets, nodes, namespaces, deployments, replicasets, replicationcontrollers, statefulsets, daemonsets, jobs, cronjobs, clusterroles, clusterrolebindings, componentstatuses** to perform discovery on the cluster.
Copy link
Contributor

@agilgur5 agilgur5 Aug 11, 2020

Choose a reason for hiding this comment

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

This Chart already contains a ClusterRole, I would find this very confusing; this makes it sound like I need to add a ClusterRole myself.

I've also made a bunch of PRs to fix some very confusing and buggy inconsistencies and this adds a new inconsistency: the Server Chart also has a ClusterRole, but nothing is mentioned there...

It also doesn't say why it needs each of those permissions.

ToC also wasn't updated...

@VineethReddy02
Copy link
Contributor Author

Hi @agilgur5
The changes made in this PR are raised in the internal mail chain and I made changes accordingly the gaps mentioned to align with CSP helm chart. More information can be provided by @eranbibi @niso120b and Andreas Lambrecht (SA team).

@agilgur5
Copy link
Contributor

agilgur5 commented Aug 19, 2020

I made changes accordingly the gaps mentioned to align with CSP helm chart

I'm not sure what you mean because per my in-line comment the ClusterRole doc addition is not consistent with the CSP chart. I made a lot of PRs to fix consistency so I can say that inconsistencies are widespread and sometimes buggy between all the charts.

The changes made in this PR are raised in the internal mail chain

Ok well I only commented on what duplicated my existing PRs and the ClusterRole doc that I'm saying first-hand is confusing, inconsistent, and doesn't add new details (such as on why which is what someone looking for docs on ClusterRole would look for).

I'm not the only user that's found this repo incredibly difficult to use (see the issues and feedback from CS), but you all don't have to take mine or anyone's feedback if you don't want to.
That's all I'll say, probably won't be interacting with this repo anymore (c.f. #99 (comment))

@niso120b niso120b merged commit 32382a0 into aquasecurity:master Aug 19, 2020
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.

None yet

3 participants