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

fixup(httpd & mirrorbits-lite) remove all mocks from packaged chart #825

Merged

Conversation

dduportal
Copy link
Contributor

This PR is a fixup of #817 and #818.

These past PRs did not break the current update-jenkins-io but the new feature to use global PVC/ingress from parent chart had some hiccups.

Namely, the mocks used for unit tests are packaged with the subchart and adding the pattern *mock* to the .helmignore results in unit test failing because, with the current 0.3.5 version of unit tests, it does not access files ignores by the.helmignore.

The result is, when trying on real life, the mocked templates takes over the expected template from the parent chart leading to unexpected results.

The proposed solution (2 commits, 1 per chart) proposes to use a combination of:

  • An idea by @lemeurherve around passing the PVC name through the global value from parent chart to subchart. I've initially denied this proposal as using a static value forbids multiple installations of the chart (or at least makes it tedious to maintain).
  • BUT I found this issue: Implement chart library testing helm-unittest/helm-unittest#171 about helm library chart tests which is the same kind of problem (mocking templates between charts). The proposed (hackish) solution uses the tpl engine to pass the template to underlying subcharts.

Tested in "production" with the helmfile diff and helm/helm#9987 and the results looks really well.

However I'm opening this in draft until we evaluate the risks of this solution as it could be (ab)used to extract credentials of the PVCs with helmfile diff (as this is a user input).

@dduportal dduportal force-pushed the fix/httpd_and_mirrorbits-lite/no_mocks branch from 773289e to be2bf93 Compare October 4, 2023 08:25
@dduportal
Copy link
Contributor Author

OK, ready to review as I found a way to sanitize the tpl (using a combination of printf to force to a string, trimming to remove whitespaces around and then truncating to 63 chars as claimname must have this size).

@dduportal dduportal marked this pull request as ready for review October 4, 2023 08:27
@dduportal dduportal added bug Something isn't working mirrorbits-lite httpd labels Oct 4, 2023
Copy link
Member

@lemeurherve lemeurherve left a comment

Choose a reason for hiding this comment

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

👏

@@ -10,6 +10,7 @@ set:
global:
storage:
enabled: true
claimNameTpl: '{{ default "parent-chart-shared-data" }}'
Copy link
Member

Choose a reason for hiding this comment

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

I didn't realize we could use templating in tests too, good to know!

Copy link
Member

Choose a reason for hiding this comment

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

Not sure why you're not specifying "parent-chart-shared-data" directly though, what could be the other value than the default one here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't realize we could use templating in tests too, good to know!

Neither did I until I realized that we can use the tpl function to pass custom data through values. Credits due to your idea which led me to helm-unittest/helm-unittest#171 where I learnt the trick.

⚠️ Remember that using values is dangerous for this (hence the sanitization of the rendered template to avoid user input garbage)

Not sure why you're not specifying "parent-chart-shared-data" directly though, what could be the other value than the default one here?

Oh I did it initially (and it works) but I wanted to show explicitly the fact that we can insert a template.

@dduportal dduportal enabled auto-merge October 4, 2023 10:30
@dduportal dduportal merged commit d34ac4b into jenkins-infra:main Oct 4, 2023
2 checks passed
@dduportal dduportal deleted the fix/httpd_and_mirrorbits-lite/no_mocks branch October 4, 2023 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working httpd mirrorbits-lite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants