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

test(integration): Added custom image pull secrets test #3808

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

chan-tim-sumo
Copy link
Contributor

@chan-tim-sumo chan-tim-sumo commented Jul 24, 2024

Overview

Checklist

  • Changelog updated or skip changelog label added
  • Documentation updated
  • Template tests added for new features
  • Integration tests added or modified for major features

@chan-tim-sumo chan-tim-sumo requested a review from a team as a code owner July 24, 2024 22:49
@chan-tim-sumo chan-tim-sumo force-pushed the chan-tim_imagePullSecretsIntegTest branch 7 times, most recently from 70b9285 to 324ee68 Compare July 24, 2024 23:24
Copy link
Contributor

@sumo-drosiek sumo-drosiek left a comment

Choose a reason for hiding this comment

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

I have an impression than some configuration has been added but not used (like for example for tracesGateway)

@chan-tim-sumo
Copy link
Contributor Author

chan-tim-sumo commented Jul 25, 2024

I have an impression than some configuration has been added but not used (like for example for tracesGateway)

this was exactly my gut feeling cause i didn't see pull secrets being used in tracesGateway, but when i ran the tests, got these errors:

        	Error Trace:	/Users/timothy.chan/Desktop/sumologic-kubernetes-collection/tests/helm/common_test.go:757
        	Error:      	Should be true
        	Test:       	TestCustomImagePullSecrets
        	Messages:   	Expected imagePullSecret customImagePullSecrets not found in Deployment col-test-kube-state-metrics
    common_test.go:729: Processing kind: Deployment
    common_test.go:757: 
        	Error Trace:	/Users/timothy.chan/Desktop/sumologic-kubernetes-collection/tests/helm/common_test.go:757
        	Error:      	Should be true
        	Test:       	TestCustomImagePullSecrets
        	Messages:   	Expected imagePullSecret customImagePullSecrets not found in Deployment col-test-opentelemetry-operator
    common_test.go:729: Processing kind: Deployment
    common_test.go:757: 
        	Error Trace:	/Users/timothy.chan/Desktop/sumologic-kubernetes-collection/tests/helm/common_test.go:757
        	Error:      	Should be true
        	Test:       	TestCustomImagePullSecrets
        	Messages:   	Expected imagePullSecret customImagePullSecrets not found in Deployment col-test-sumologic-traces-gateway
    common_test.go:729: Processing kind: Deployment
    common_test.go:757: 
        	Error Trace:	/Users/timothy.chan/Desktop/sumologic-kubernetes-collection/tests/helm/common_test.go:757
        	Error:      	Should be true
        	Test:       	TestCustomImagePullSecrets
        	Messages:   	Expected imagePullSecret customImagePullSecrets not found in Deployment col-test-sumologic-traces-sampler

which i thought maybe there was some internal knowledge i didn't know about (which is alot) and these were actually using pull secrets but just wasn't added to this repo yet 😆

@sumo-drosiek
Copy link
Contributor

Helm is very simple. It is just templates, and unless you don't use the content of values.yaml in templates directory, they are not used. You fixed the error by adding the following:

      {{- if $.Values.sumologic.pullSecrets }}
      imagePullSecrets:
{{- range $.Values.sumologic.pullSecrets }}
        - name: {{ .name }}
{{- end }}
      {{- end }} 

so configuration of tracesGateway is still not used so we can drop it from values.yaml

@chan-tim-sumo
Copy link
Contributor Author

chan-tim-sumo commented Jul 25, 2024

Helm is very simple. It is just templates, and unless you don't use the content of values.yaml in templates directory, they are not used. You fixed the error by adding the following:

      {{- if $.Values.sumologic.pullSecrets }}
      imagePullSecrets:
{{- range $.Values.sumologic.pullSecrets }}
        - name: {{ .name }}
{{- end }}
      {{- end }} 

so configuration of tracesGateway is still not used so we can drop it from values.yaml

ah got confused, found the documentation for pull secrets:

## Using pull secrets with `sumologic-kubernetes-collection` helm chart
Full list of `user-values.yaml` keys for all the images that are used, can be found below:
| Image | `user-values.yaml` key |
| --------------------- | ----------------------------------------------- |
| setup job | `sumologic.setup.job.pullSecrets` |
| Sumo Logic OT distro | `sumologic.pullSecrets` |
| remote-write-proxy | `sumologic.pullSecrets` |
| kube-prometheus-stack | `kube-prometheus-stack.global.imagePullSecrets` |
| metrics-server | `metrics-server.image.pullSecrets` |
| telegraf-operator | `telegraf-operator.imagePullSecrets` |
| falco | `falco.image.pullSecrets` |

seems like only 7 images/objects that uses pull secrets, but currently since i was trying to use the same logic as previous integ test cases (running ALL objects) which is probably why it gave me the error for tracesGateway, kube-state-metrics, and others.

@chan-tim-sumo chan-tim-sumo force-pushed the chan-tim_imagePullSecretsIntegTest branch 6 times, most recently from 20eb23e to d5b362a Compare July 25, 2024 22:11
@sumo-drosiek
Copy link
Contributor

seems like only 7 images/objects that uses pull secrets, but currently since i was trying to use the same logic as previous integ test cases (running ALL objects) which is probably why it gave me the error for tracesGateway, kube-state-metrics, and others.

You should run test for ALL objects, and using sumologic.pullSecrets has been missing in some objects, so they should be added. And it's enough to add this change:

      {{- if $.Values.sumologic.pullSecrets }}
      imagePullSecrets:
{{- range $.Values.sumologic.pullSecrets }}
        - name: {{ .name }}
{{- end }}
      {{- end }} 

as this is using sumologic.pullSecrets from values.yaml to set imagePullSecrets

There is no need to add anything more to default values.yaml

@sumo-drosiek
Copy link
Contributor

sumo-drosiek commented Jul 26, 2024

This is needed (uses $.Values.sumologic.pullSecrets):

{{- if $.Values.sumologic.pullSecrets }}
imagePullSecrets:
{{- range $.Values.sumologic.pullSecrets }}
- name: {{ .name }}
{{- end }}
{{- end }}

This is not needed (this option is not used by any template):

@chan-tim-sumo
Copy link
Contributor Author

This is needed (uses $.Values.sumologic.pullSecrets):

{{- if $.Values.sumologic.pullSecrets }}
imagePullSecrets:
{{- range $.Values.sumologic.pullSecrets }}
- name: {{ .name }}
{{- end }}
{{- end }}

This is not needed (this option is not used by any template):

ah gotcha, was on the right track in the beginning but just don't need to add imagePullSecrets: [] to values.yaml

@chan-tim-sumo chan-tim-sumo force-pushed the chan-tim_imagePullSecretsIntegTest branch 4 times, most recently from 2d7e229 to 63e803c Compare July 26, 2024 19:00
@chan-tim-sumo chan-tim-sumo force-pushed the chan-tim_imagePullSecretsIntegTest branch from 63e803c to b3ac5fe Compare July 29, 2024 15:56
sumo-drosiek
sumo-drosiek previously approved these changes Jul 30, 2024
Copy link
Contributor

@sumo-drosiek sumo-drosiek left a comment

Choose a reason for hiding this comment

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

I would add also changelogs for the templates you modified, e.g. events statefulset

@sumo-drosiek sumo-drosiek dismissed their stale review July 30, 2024 08:18

Converting to comment 😅

@chan-tim-sumo chan-tim-sumo force-pushed the chan-tim_imagePullSecretsIntegTest branch 3 times, most recently from 366ae25 to b10a879 Compare August 1, 2024 00:08
@chan-tim-sumo chan-tim-sumo force-pushed the chan-tim_imagePullSecretsIntegTest branch from b10a879 to 54f7692 Compare August 1, 2024 00:10
Copy link
Contributor

@sumo-drosiek sumo-drosiek left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

We should enable all possible components (except windows as is experimental), but we can do it in another PR

@chan-tim-sumo chan-tim-sumo merged commit 6704458 into main Aug 1, 2024
55 checks passed
@chan-tim-sumo chan-tim-sumo deleted the chan-tim_imagePullSecretsIntegTest branch August 1, 2024 14:59
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.

3 participants