Add DiscoveryKit features for Argo Rollout kinds. #229
Conversation
| # discovery.maxPodCount -- Skip listing pods, containers and hosts for deployments, statefulsets, etc. if there are more then the given pods. | ||
| maxPodCount: 50 | ||
| # discovery.disableArgoRollouts -- Disable discovery of Argo rollouts (requires argoproj.io rollout CRDs) | ||
| disableArgoRollouts: true |
There was a problem hiding this comment.
Line 195ff has already some values to disable discoveries
| AdviceSingleReplicaMinReplicas int `json:"adviceSingleReplicaMinReplicas" split_words:"true" required:"false" default:"2"` | ||
| DisableDiscoveryExcludes bool `required:"false" split_words:"true" default:"false"` | ||
| LogKubernetesHttpRequests bool `required:"false" split_words:"true" default:"false"` | ||
| DiscoveryDisabledArgoRollout bool `json:"discoveryDisabledArgoRollout" required:"false" split_words:"true" default:"true"` |
There was a problem hiding this comment.
We usually default to "false" here and do the real defaulting via the helm values. At least your current impl of the helm chart wouldn't ever set this flag to "false"
| - name: STEADYBIT_EXTENSION_DISCOVERY_MAX_POD_COUNT | ||
| value: "{{ .Values.discovery.maxPodCount }}" | ||
| {{- if .Values.discovery.disableArgoRollouts }} | ||
| - name: STEADYBIT_EXTENSION_DISCOVERY_DISABLE_ARGO_ROLLOUTS |
There was a problem hiding this comment.
The env var is STEADYBIT_EXTENSION_DISCOVERY_DISABLED_ARGO_ROLLOUTS (missing D in disableD)
| |------------------------------------------------------------------|---------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------|----------|----------------------------------------------------------------------| | ||
| | `STEADYBIT_EXTENSION_KUBERNETES_CLUSTER_NAME` | `kubernetes.clusterName` | The name of the kubernetes cluster | yes | | | ||
| | `STEADYBIT_EXTENSION_DISABLE_DISCOVERY_EXCLUDES` | `discovery.disableExcludes` | Ignore discovery excludes specified by `steadybit.com/discovery-disabled` | false | `false` | | ||
| | `STEADYBIT_EXTENSION_DISCOVERY_DISABLE_ARGO_ROLLOUTS` | `discovery.disableArgoRollouts` | Disable discovery of Argo rollouts | false | `true` | |
There was a problem hiding this comment.
The env var is STEADYBIT_EXTENSION_DISCOVERY_DISABLED_ARGO_ROLLOUTS (missing D in disableD)
(Also the helm value - see my comment in the helm values.yaml)
| {{- end }} | ||
| - name: STEADYBIT_EXTENSION_DISCOVERY_MAX_POD_COUNT | ||
| value: "{{ .Values.discovery.maxPodCount }}" | ||
| {{- if .Values.discovery.disableArgoRollouts }} |
There was a problem hiding this comment.
Could you add a test to charts/steadybit-extension-kubernetes/tests/deployment_test.yaml -> manifest should match snapshot with disabled discoveries
|
|
||
| // CreateClient is visible for testing | ||
| func CreateClient(clientset kubernetes.Interface, stopCh <-chan struct{}, rootApiPath string, permissions *PermissionCheckResult) *Client { | ||
| func CreateClient(clientset kubernetes.Interface, stopCh <-chan struct{}, rootApiPath string, permissions *PermissionCheckResult, config *rest.Config, dynamicClient dynamic.Interface) *Client { |
There was a problem hiding this comment.
config *rest.Config seems to be unused
| {group: "", resource: "pods", subresource: "eviction", verbs: []string{"create"}, allowGracefulFailure: true}, | ||
| {group: "", resource: "nodes", verbs: []string{"patch"}, allowGracefulFailure: true}, | ||
| {group: "", resource: "pods", subresource: "exec", verbs: []string{"create"}, allowGracefulFailure: true}, | ||
| {group: "argoproj.io", resource: "rollouts", verbs: []string{"get", "list", "watch"}, allowGracefulFailure: true}, |
There was a problem hiding this comment.
In line 76 you are adding these permission conditionally. Should that be removed from this list?
| } | ||
| } | ||
|
|
||
| // TODO: Get HPA if exists |
There was a problem hiding this comment.
What's about these TODOs? Kube-Score will not show anything for Argo Rollouts. I guess we can remove these lines.
2 participants
This pull request adds support for discovering and monitoring changes to Argo Rollout resources in a cluster.
It adds an
argoRollouttype to the K8SClientwrapper:RolloutLister,RolloutNamespaceLister, making use ofcache.Indexerto support the major use cases, toList()and getRollouts().Clientto retrieve Rollouts.Note that we hand-rolled the types to avoid bringing in a dependency on
argoprojv1. This also means that we are using adynamicclient for Kubernetes, which uses typeunstructured.Unstructred{}to support any generic kind of CRD.We also add permissions:
_permissions.tplto get/list/watch aroproj.io rolloutspermissions.go, to mark the resource permission as required (with graceful failure)It adds
testutil/dynamic_client.goto support creating a dynamic test-double (adding Argo Rollout and RolloutList to the schema). Because the wrapper Client is now using both aclientset(static types) and a dynamicclient, the faked dynamic client is used by existing tests (e.g. container, daemonset, ..., etc.) even where they do not test Rollout functionality.