-
Notifications
You must be signed in to change notification settings - Fork 25
[VC-35377] Reduce memory by excluding Helm release Secrets and some standard Secret types #554
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
Changes from all commits
737eb5a
6eed6f4
7f45cfb
e3d1c98
29efc5d
15998fc
72450b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,7 +64,7 @@ resource referenced in the `kind` for that datagatherer. | |
| There is an example `ClusterRole` and `ClusterRoleBinding` which can be found in | ||
| [`./deployment/kubernetes/base/00-rbac.yaml`](./deployment/kubernetes/base/00-rbac.yaml). | ||
|
|
||
| # Secrets | ||
| ## Secrets | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Drive-by fix. This was wrong heading level. |
||
|
|
||
| Secrets can be gathered using the following config: | ||
|
|
||
|
|
@@ -79,4 +79,30 @@ Secrets can be gathered using the following config: | |
|
|
||
| Before Secrets are sent to the Preflight backend, they are redacted so no secret data is transmitted. See [`fieldfilter.go`](./../../pkg/datagatherer/k8s/fieldfilter.go) to see the details of which fields are filteres and which ones are redacted. | ||
|
|
||
| > **All resource other than Kubernetes Secrets are sent in full, so make sure that you don't store secret information on arbitrary resources.** | ||
| > **All resource other than Kubernetes Secrets are sent in full, so make sure that you don't store secret information on arbitrary resources.** | ||
|
|
||
|
|
||
| ## Field Selectors | ||
|
|
||
| You can use [field selectors](https://kubernetes.io/docs/concepts/overview/working-with-objects/field-selectors/#list-of-supported-fields) | ||
| to include or exclude certain resources. | ||
| For example, you can reduce the memory usage of the agent and reduce the load on the Kubernetes | ||
| API server by omitting various common [Secret types](https://kubernetes.io/docs/concepts/configuration/secret/#secret-types) | ||
| when listing Secrets. | ||
|
|
||
| ```yaml | ||
| - kind: "k8s-dynamic" | ||
| name: "k8s/secrets" | ||
| config: | ||
| resource-type: | ||
| version: v1 | ||
| resource: secrets | ||
| field-selectors: | ||
| - type!=kubernetes.io/service-account-token | ||
| - type!=kubernetes.io/dockercfg | ||
| - type!=kubernetes.io/dockerconfigjson | ||
| - type!=kubernetes.io/basic-auth | ||
| - type!=kubernetes.io/ssh-auth, | ||
| - type!=bootstrap.kubernetes.io/token | ||
| - type!=helm.sh/release.v1 | ||
| ``` | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| # one-shot-secret.yaml | ||
| # | ||
| # An example configuration file which can be used for local testing. | ||
| # It gathers only secrets and it does not attempt to upload to Venafi. | ||
| # For example: | ||
| # | ||
| # builds/preflight agent \ | ||
| # --agent-config-file examples/one-shot-secret.yaml \ | ||
| # --one-shot \ | ||
| # --output-path output.json | ||
| # | ||
| organization_id: "my-organization" | ||
| cluster_id: "my_cluster" | ||
| period: 1m | ||
| data-gatherers: | ||
| - kind: "k8s-dynamic" | ||
| name: "k8s/secrets" | ||
| config: | ||
| resource-type: | ||
| version: v1 | ||
| resource: secrets | ||
| field-selectors: | ||
| - type!=kubernetes.io/service-account-token | ||
| - type!=kubernetes.io/dockercfg | ||
| - type!=kubernetes.io/dockerconfigjson | ||
| - type!=kubernetes.io/basic-auth | ||
| - type!=kubernetes.io/ssh-auth, | ||
| - type!=bootstrap.kubernetes.io/token | ||
| - type!=helm.sh/release.v1 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,8 @@ type ConfigDynamic struct { | |
| ExcludeNamespaces []string `yaml:"exclude-namespaces"` | ||
| // IncludeNamespaces is a list of namespaces to include. | ||
| IncludeNamespaces []string `yaml:"include-namespaces"` | ||
| // FieldSelectors is a list of field selectors to use when listing this resource | ||
| FieldSelectors []string `yaml:"field-selectors"` | ||
| } | ||
|
|
||
| // UnmarshalYAML unmarshals the ConfigDynamic resolving GroupVersionResource. | ||
|
|
@@ -52,6 +54,7 @@ func (c *ConfigDynamic) UnmarshalYAML(unmarshal func(interface{}) error) error { | |
| } `yaml:"resource-type"` | ||
| ExcludeNamespaces []string `yaml:"exclude-namespaces"` | ||
| IncludeNamespaces []string `yaml:"include-namespaces"` | ||
| FieldSelectors []string `yaml:"field-selectors"` | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a bit weird that the yaml field name hints are added in two places, but I decided to follow the existing pattern. |
||
| }{} | ||
| err := unmarshal(&aux) | ||
| if err != nil { | ||
|
|
@@ -64,6 +67,7 @@ func (c *ConfigDynamic) UnmarshalYAML(unmarshal func(interface{}) error) error { | |
| c.GroupVersionResource.Resource = aux.ResourceType.Resource | ||
| c.ExcludeNamespaces = aux.ExcludeNamespaces | ||
| c.IncludeNamespaces = aux.IncludeNamespaces | ||
| c.FieldSelectors = aux.FieldSelectors | ||
|
|
||
| return nil | ||
| } | ||
|
|
@@ -79,6 +83,16 @@ func (c *ConfigDynamic) validate() error { | |
| errors = append(errors, "invalid configuration: GroupVersionResource.Resource cannot be empty") | ||
| } | ||
|
|
||
| for i, selectorString := range c.FieldSelectors { | ||
| if selectorString == "" { | ||
| errors = append(errors, fmt.Sprintf("invalid field selector %d: must not be empty", i)) | ||
| } | ||
| _, err := fields.ParseSelector(selectorString) | ||
| if err != nil { | ||
| errors = append(errors, fmt.Sprintf("invalid field selector %d: %s", i, err)) | ||
| } | ||
| } | ||
|
Comment on lines
+86
to
+94
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The selectors are validated here but the validation is not very good. fields.Requirement {
Op: "!=",
Field: "type " // with trailing space
Value: " foo" // with leading space
}And then later the client-go Reflector will fail with an error like "unknown field: type " but you won't be able to see the trailing space, because it's not quoted. |
||
|
|
||
| if len(errors) > 0 { | ||
| return fmt.Errorf(strings.Join(errors, ", ")) | ||
| } | ||
|
|
@@ -150,7 +164,15 @@ func (c *ConfigDynamic) newDataGathererWithClient(ctx context.Context, cl dynami | |
| return nil, err | ||
| } | ||
| // init shared informer for selected namespaces | ||
| fieldSelector := generateFieldSelector(c.ExcludeNamespaces) | ||
| fieldSelector := generateExcludedNamespacesFieldSelector(c.ExcludeNamespaces) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I gave this function a much more specific name to avoid any confusion about how it fits in with the user-supplied field-selectors. |
||
|
|
||
| // Add any custom field selectors to the excluded namespaces selector | ||
| // The selectors have already been validated, so it is safe to use | ||
| // ParseSelectorOrDie here. | ||
| for _, selectorString := range c.FieldSelectors { | ||
| fieldSelector = fields.AndSelectors(fieldSelector, fields.ParseSelectorOrDie(selectorString)) | ||
| } | ||
|
|
||
| // init cache to store gathered resources | ||
| dgCache := cache.New(5*time.Minute, 30*time.Second) | ||
|
|
||
|
|
@@ -159,7 +181,7 @@ func (c *ConfigDynamic) newDataGathererWithClient(ctx context.Context, cl dynami | |
| cl: cl, | ||
| k8sClientSet: clientset, | ||
| groupVersionResource: c.GroupVersionResource, | ||
| fieldSelector: fieldSelector, | ||
| fieldSelector: fieldSelector.String(), | ||
| namespaces: c.IncludeNamespaces, | ||
| cache: dgCache, | ||
| } | ||
|
|
@@ -177,7 +199,7 @@ func (c *ConfigDynamic) newDataGathererWithClient(ctx context.Context, cl dynami | |
| 60*time.Second, | ||
| informers.WithNamespace(metav1.NamespaceAll), | ||
| informers.WithTweakListOptions(func(options *metav1.ListOptions) { | ||
| options.FieldSelector = fieldSelector | ||
| options.FieldSelector = fieldSelector.String() | ||
| })) | ||
| newDataGatherer.nativeSharedInformer = factory | ||
| informer := informerFunc(factory) | ||
|
|
@@ -200,7 +222,7 @@ func (c *ConfigDynamic) newDataGathererWithClient(ctx context.Context, cl dynami | |
| cl, | ||
| 60*time.Second, | ||
| metav1.NamespaceAll, | ||
| func(options *metav1.ListOptions) { options.FieldSelector = fieldSelector }, | ||
| func(options *metav1.ListOptions) { options.FieldSelector = fieldSelector.String() }, | ||
| ) | ||
| resourceInformer := factory.ForResource(c.GroupVersionResource) | ||
| informer := resourceInformer.Informer() | ||
|
|
@@ -420,17 +442,17 @@ func namespaceResourceInterface(iface dynamic.NamespaceableResourceInterface, na | |
| return iface.Namespace(namespace) | ||
| } | ||
|
|
||
| // generateFieldSelector creates a field selector string from a list of | ||
| // namespaces to exclude. | ||
| func generateFieldSelector(excludeNamespaces []string) string { | ||
| fieldSelector := fields.Nothing() | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there was a bug in this function. This initial |
||
| // generateExcludedNamespacesFieldSelector creates a field selector string from | ||
| // a list of namespaces to exclude. | ||
| func generateExcludedNamespacesFieldSelector(excludeNamespaces []string) fields.Selector { | ||
| var selectors []fields.Selector | ||
| for _, excludeNamespace := range excludeNamespaces { | ||
| if excludeNamespace == "" { | ||
| continue | ||
| } | ||
| fieldSelector = fields.AndSelectors(fields.OneTermNotEqualSelector("metadata.namespace", excludeNamespace), fieldSelector) | ||
| selectors = append(selectors, fields.OneTermNotEqualSelector("metadata.namespace", excludeNamespace)) | ||
| } | ||
| return fieldSelector.String() | ||
| return fields.AndSelectors(selectors...) | ||
| } | ||
|
|
||
| func isIncludedNamespace(namespace string, namespaces []string) bool { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -105,8 +105,12 @@ func sortGatheredResources(list []*api.GatheredResource) { | |
| func TestNewDataGathererWithClientAndDynamicInformer(t *testing.T) { | ||
| ctx := context.Background() | ||
| config := ConfigDynamic{ | ||
| IncludeNamespaces: []string{"a"}, | ||
| ExcludeNamespaces: []string{"kube-system"}, | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The validate function prevents you having both Include and Exclude namespaces, so I removed the IncludeNamespaces in this test because I wanted to see how the exclude namespace field selector got merged with the user-supplied field selectors. |
||
| GroupVersionResource: schema.GroupVersionResource{Group: "foobar", Version: "v1", Resource: "foos"}, | ||
| FieldSelectors: []string{ | ||
| "type!=kubernetes.io/service-account-token", | ||
| "type!=kubernetes.io/dockercfg", | ||
| }, | ||
| } | ||
| cl := fake.NewSimpleDynamicClient(runtime.NewScheme()) | ||
| dg, err := config.newDataGathererWithClient(ctx, cl, nil) | ||
|
|
@@ -121,7 +125,8 @@ func TestNewDataGathererWithClientAndDynamicInformer(t *testing.T) { | |
| groupVersionResource: config.GroupVersionResource, | ||
| // it's important that the namespaces are set as the IncludeNamespaces | ||
| // during initialization | ||
| namespaces: config.IncludeNamespaces, | ||
| namespaces: config.IncludeNamespaces, | ||
| fieldSelector: "metadata.namespace!=kube-system,type!=kubernetes.io/service-account-token,type!=kubernetes.io/dockercfg", | ||
| } | ||
|
|
||
| gatherer := dg.(*DataGathererDynamic) | ||
|
|
@@ -150,6 +155,9 @@ func TestNewDataGathererWithClientAndDynamicInformer(t *testing.T) { | |
| if gatherer.nativeSharedInformer != nil { | ||
| t.Errorf("unexpected nativeSharedInformer value: %v. should be nil", gatherer.nativeSharedInformer) | ||
| } | ||
| if !reflect.DeepEqual(gatherer.fieldSelector, expected.fieldSelector) { | ||
| t.Errorf("expected %v, got %v", expected.fieldSelector, gatherer.fieldSelector) | ||
| } | ||
| } | ||
|
|
||
| func TestNewDataGathererWithClientAndSharedIndexInformer(t *testing.T) { | ||
|
|
@@ -216,6 +224,8 @@ exclude-namespaces: | |
| # from the config file | ||
| include-namespaces: | ||
| - default | ||
| field-selectors: | ||
| - type!=kubernetes.io/service-account-token | ||
| ` | ||
|
|
||
| expectedGVR := schema.GroupVersionResource{ | ||
|
|
@@ -231,6 +241,10 @@ include-namespaces: | |
|
|
||
| expectedIncludeNamespaces := []string{"default"} | ||
|
|
||
| expectedFieldSelectors := []string{ | ||
| "type!=kubernetes.io/service-account-token", | ||
| } | ||
|
|
||
| cfg := ConfigDynamic{} | ||
| err := yaml.Unmarshal([]byte(textCfg), &cfg) | ||
| if err != nil { | ||
|
|
@@ -251,6 +265,9 @@ include-namespaces: | |
| if got, want := cfg.IncludeNamespaces, expectedIncludeNamespaces; !reflect.DeepEqual(got, want) { | ||
| t.Errorf("IncludeNamespaces does not match: got=%+v want=%+v", got, want) | ||
| } | ||
| if got, want := cfg.FieldSelectors, expectedFieldSelectors; !reflect.DeepEqual(got, want) { | ||
| t.Errorf("FieldSelectors does not match: got=%+v want=%+v", got, want) | ||
| } | ||
| } | ||
|
|
||
| func TestConfigDynamicValidate(t *testing.T) { | ||
|
|
@@ -275,17 +292,42 @@ func TestConfigDynamicValidate(t *testing.T) { | |
| }, | ||
| ExpectedError: "cannot set excluded and included namespaces", | ||
| }, | ||
| { | ||
| Config: ConfigDynamic{ | ||
| GroupVersionResource: schema.GroupVersionResource{ | ||
| Group: "", | ||
| Version: "v1", | ||
| Resource: "secrets", | ||
| }, | ||
| FieldSelectors: []string{""}, | ||
| }, | ||
| ExpectedError: "invalid field selector 0: must not be empty", | ||
| }, | ||
| { | ||
| Config: ConfigDynamic{ | ||
| GroupVersionResource: schema.GroupVersionResource{ | ||
| Group: "", | ||
| Version: "v1", | ||
| Resource: "secrets", | ||
| }, | ||
| FieldSelectors: []string{"foo"}, | ||
| }, | ||
| ExpectedError: "invalid field selector 0: invalid selector: 'foo'; can't understand 'foo'", | ||
| }, | ||
| } | ||
|
|
||
| for _, test := range tests { | ||
| err := test.Config.validate() | ||
| if !strings.Contains(err.Error(), test.ExpectedError) { | ||
| if err == nil && test.ExpectedError != "" { | ||
| t.Errorf("expected error: %q, got: nil", test.ExpectedError) | ||
| } | ||
| if err != nil && !strings.Contains(err.Error(), test.ExpectedError) { | ||
| t.Errorf("expected %s, got %s", test.ExpectedError, err.Error()) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestGenerateFieldSelector(t *testing.T) { | ||
| func TestGenerateExcludedNamespacesFieldSelector(t *testing.T) { | ||
| tests := []struct { | ||
| ExcludeNamespaces []string | ||
| ExpectedFieldSelector string | ||
|
|
@@ -300,19 +342,19 @@ func TestGenerateFieldSelector(t *testing.T) { | |
| ExcludeNamespaces: []string{ | ||
| "kube-system", | ||
| }, | ||
| ExpectedFieldSelector: "metadata.namespace!=kube-system,", | ||
| ExpectedFieldSelector: "metadata.namespace!=kube-system", | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the trailing comma bug that I referred to in an earlier comment. |
||
| }, | ||
| { | ||
| ExcludeNamespaces: []string{ | ||
| "kube-system", | ||
| "my-namespace", | ||
| }, | ||
| ExpectedFieldSelector: "metadata.namespace!=my-namespace,metadata.namespace!=kube-system,", | ||
| ExpectedFieldSelector: "metadata.namespace!=kube-system,metadata.namespace!=my-namespace", | ||
| }, | ||
| } | ||
|
|
||
| for _, test := range tests { | ||
| fieldSelector := generateFieldSelector(test.ExcludeNamespaces) | ||
| fieldSelector := generateExcludedNamespacesFieldSelector(test.ExcludeNamespaces).String() | ||
| if fieldSelector != test.ExpectedFieldSelector { | ||
| t.Errorf("ExpectedFieldSelector does not match: got=%+v want=%+v", fieldSelector, test.ExpectedFieldSelector) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hawksight I added
ignoredSecretTypesto the Helm chart values, as you suggested.