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

fix(test): add securityContext for Helm test #930

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

fty4
Copy link
Contributor

@fty4 fty4 commented Jul 27, 2023

When running tests with Helm the same securityContext also should apply.
Therefore this change will add the securityContext from the values file also to the Helm tests.

The change will use the securityContext of the server statefulSet.
This results from the fact that the same image is used in the test.


Marco Lecheler marco.lecheler@mercedes-benz.com Mercedes-Benz Tech Innovation GmbH (ProviderInformation)

When running tests with Helm the same securityContext also should apply.
Therefore this change will add the securityContext from the values file also to the Helm tests.

The change will use the securityContext of the server statefulSet.
This results from the fact that the same image is used in the test.

Signed-off-by: Marco Lecheler <marco.lecheler@mercedes-benz.com>
@fty4 fty4 requested a review from a team July 27, 2023 10:20
@fty4
Copy link
Contributor Author

fty4 commented Aug 10, 2023

@KhizerJaan could you please take a look here?

{{/*
securityContext for the test pod template.
*/}}
{{- define "server.test.securityContext.pod" -}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @fty4 , why do we need to define new template , as its supposed to use the securityContext of the server statefulSet. why not use "server.statefulSet.securityContext.pod" for server-test pod?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @KhizerJaan
I did that because of the indentation of the template.
But now I just added a new commit to use nindent to fix the indentation in the template files.

I hope this is the desired solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fty4 Thank you , but can you please check indentation , it does not look right for both server-statefulset and and server-test .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I run a diff between the templated yaml from main and after the fix I get this change:

381a382
>       
414a416
>           
557a560,565
>   
>   securityContext:
>     runAsNonRoot: true
>     runAsGroup: 1000
>     runAsUser: 100
>     fsGroup: 1000
585a594,596
>       
>       securityContext:
>         allowPrivilegeEscalation: false

This means the indentation should be fine.
The only issue is see here is that two new lines are beeing added...
But this is a issue in the whole chart and I am not sure how to fix this.

I guess you mean the indentation in the helpers file - I was required to remove the indentation here to later use different indentation on the statefulset and the test.
This is because both use different indentation length (6 and 10 for the statefulset and 2 and 6 for the server-test).

Do you agree, @KhizerJaan?

Copy link
Contributor

Choose a reason for hiding this comment

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

this line
&
this
would add to indent 14 and same is with others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this does not result in a diff in the templated resources - but for sure it is better to remove them when the indentation is set in another place.

I've added commit 43ca102 for this.

@fty4 fty4 requested a review from a team as a code owner August 16, 2023 06:33
@fty4 fty4 requested a review from KhizerJaan August 16, 2023 06:36
Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

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

This looks good to me, and it seems sensible to reuse the server's security context for the same image. Please could we just add some unit tests to have some enforced validation on the output?

@fty4
Copy link
Contributor Author

fty4 commented Aug 18, 2023

I updated the requested changes - also I noticed that a unit-test failed (wondering why this test will not be triggered in a PR).
There was an issue with the line break when specifying separate securityContext values.

I hope the tests implemented fit your needs.

@tvoran tvoran modified the milestones: v0.27.0, v0.28.0 Nov 14, 2023
@tvoran tvoran modified the milestones: v0.28.0, v0.29.0 Apr 8, 2024
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.

4 participants