feat(discovery): implement All Namespaces discovery #213
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7 tasks
tthvo
reviewed
Nov 30, 2024
There was a problem hiding this comment.
I agree with cryostatio/cryostat-operator#976 (comment) here, this might introduce more elevated access than intended.
| portNames: [] | ||
| ## @param core.discovery.kubernetes.builtInPortNumbersDisabled When false and `portNumbers` is empty, the Cryostat application will use the default port number `9091` to look for JMX connectable targets. | ||
| builtInPortNumbersDisabled: false | ||
| ## @param core.discovery.kubernetes.portNumbers [array] List of port numbers that the Cryostat application should look for in order to consider a target as JMX connectable | ||
| ## @param core.discovery.kubernetes.portNumbers [array] List of port numbers that the Cryostat application should look for in order to consider a target as JMX connectable. |
There was a problem hiding this comment.
nit: we can remove the periods for consistency with other descriptions :D
| @@ -75,15 +75,17 @@ core: | |||
| enabled: true | |||
| ## @param core.discovery.kubernetes.installNamespaceDisabled When false and `namespaces` is empty, the Cryostat application will default to discovery targets in the install namespace (i.e. `{{ .Release.Namespace }}`) | |||
| installNamespaceDisabled: false | |||
| ## @param core.discovery.kubernetes.namespaces [array] List of namespaces whose workloads the Cryostat application should be permitted to access and profile | |||
| ## @param core.discovery.kubernetes.allNamespaces When true, this overrides the `namespaces` list and configures Cryostat to monitor all namespaces in the cluster. This requires elevated permissions to create a ClusterRole and ClusterRoleBinding, which will be done automatically if the rbac.create value is true. WARNING: this allows Cryostat to discover, and potentially connect to and collect data from, applications in *any* Namespace in the cluster. ALL users with access to this Cryostat instance will be able to read data from potentially any application in the cluster. For data security and isolation concerns it is best to leave this setting disabled, and instead install multiple Cryostat instances with lists of target namespaces, and apply sensible access controls for users to each Cryostat instance as needed. | |||
There was a problem hiding this comment.
Suggested change
| ## @param core.discovery.kubernetes.allNamespaces When true, this overrides the `namespaces` list and configures Cryostat to monitor all namespaces in the cluster. This requires elevated permissions to create a ClusterRole and ClusterRoleBinding, which will be done automatically if the rbac.create value is true. WARNING: this allows Cryostat to discover, and potentially connect to and collect data from, applications in *any* Namespace in the cluster. ALL users with access to this Cryostat instance will be able to read data from potentially any application in the cluster. For data security and isolation concerns it is best to leave this setting disabled, and instead install multiple Cryostat instances with lists of target namespaces, and apply sensible access controls for users to each Cryostat instance as needed. | |
| ## @param core.discovery.kubernetes.allNamespaces When true, this overrides the `core.discovery.kubernetes.namespaces` list and configures the Cryostat application to monitor all namespaces in the cluster. This requires elevated permissions to create a ClusterRole and ClusterRoleBinding, which will be done automatically if the `rbac.create` value is `true`. WARNING: This allows Cryostat to discover, and potentially connect to and collect data from, applications in *any* Namespace in the cluster. ALL users with access to this Cryostat instance will be able to read data from potentially any application in the cluster. For data security and isolation concerns it is recommended to leave this setting disabled, and instead install multiple Cryostat instances with lists of target namespaces, and apply sensible access controls for users to each instance as needed. |
nit: just some formatting that I think will make a bit clearer :D I suggested the word recommended to be consistent with other descriptions.
Comment on lines
120
to
133
| value: "true" | ||
| {{- if .Values.core.discovery.kubernetes.allNamespaces }} | ||
| - name: CRYOSTAT_DISCOVERY_KUBERNETES_NAMESPACES | ||
| value: '*' | ||
| {{- else }} | ||
| {{- with .Values.core.discovery.kubernetes }} | ||
| - name: CRYOSTAT_DISCOVERY_KUBERNETES_NAMESPACES | ||
| value: {{ include "cryostat.commaSepList" (list .namespaces $.Release.Namespace .installNamespaceDisabled) }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- with .Values.core.discovery.kubernetes }} | ||
| - name: CRYOSTAT_DISCOVERY_KUBERNETES_PORT_NAMES | ||
| value: {{ include "cryostat.commaSepList" (list .portNames "jfr-jmx" .builtInPortNamesDisabled) }} | ||
| - name: CRYOSTAT_DISCOVERY_KUBERNETES_PORT_NUMBERS |
There was a problem hiding this comment.
nit: I think these configurations can be refactored in a single {{- with .Values.core.discovery.kubernetes }}.
| @@ -75,15 +75,17 @@ core: | |||
| enabled: true | |||
| ## @param core.discovery.kubernetes.installNamespaceDisabled When false and `namespaces` is empty, the Cryostat application will default to discovery targets in the install namespace (i.e. `{{ .Release.Namespace }}`) | |||
| installNamespaceDisabled: false | |||
| ## @param core.discovery.kubernetes.namespaces [array] List of namespaces whose workloads the Cryostat application should be permitted to access and profile | |||
| ## @param core.discovery.kubernetes.allNamespaces When true, this overrides the `namespaces` list and configures Cryostat to monitor all namespaces in the cluster. This requires elevated permissions to create a ClusterRole and ClusterRoleBinding, which will be done automatically if the rbac.create value is true. WARNING: this allows Cryostat to discover, and potentially connect to and collect data from, applications in *any* Namespace in the cluster. ALL users with access to this Cryostat instance will be able to read data from potentially any application in the cluster. For data security and isolation concerns it is best to leave this setting disabled, and instead install multiple Cryostat instances with lists of target namespaces, and apply sensible access controls for users to each Cryostat instance as needed. | |||
| allNamespaces: false | |||
There was a problem hiding this comment.
nit: How about a field like (i.e. just feel a bit k8s-like, such as allowPrivilegeEscalation, runAsNonRoot)?
allowAllNamespaces: false
# or allNamespacesAllowed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #212
Depends on cryostatio/cryostat#725
To test:
cd cryostat-operatorfor ns in apps1 apps2; do oc new-project $ns ; make sample_app ; doneoc new-project testcd cryostat-helmhelm install cryostat --set core.image.repository=quay.io/andrewazores/cryostat --set core.image.tag=k8s-all-namespaces-7 cryostat ./charts/cryostatthis is an image built from feat(k8s): implement wildcard All Namespaces discovery cryostat#725