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

feat: Replace nginx snippet annotation with custom header annotation #106

Merged
merged 11 commits into from
Aug 29, 2024

Conversation

HerrSpeck
Copy link
Contributor

Our cluster does not allow snippet annotations by default and changing the configuration would be complicated.

But the snippet is not actually needed, since the same thing can be achieve using a config map and the custom-headers annotation.

Copy link
Contributor

@santiagon610 santiagon610 left a comment

Choose a reason for hiding this comment

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

Overall, looks good. I would just worry about breaking functionality for other users that are using Nginx Ingress with your changes.

Also, since this is a feature release, could you update the Chart.yaml to increment the minor version of the release?

charts/vaultwarden/templates/ingress.yaml Outdated Show resolved Hide resolved
charts/vaultwarden/templates/ingress.yaml Outdated Show resolved Hide resolved
@HerrSpeck
Copy link
Contributor Author

HerrSpeck commented Aug 20, 2024

Overall, looks good. I would just worry about breaking functionality for other users that are using Nginx Ingress with your changes.

Also, since this is a feature release, could you update the Chart.yaml to increment the minor version of the release?

Thanks :) I changed the version :)
I'm not sure if it would interfere with other users. I think there was no way to really set any custom headers before. So this shouldn't override any settings? Also, the Request-Id header is still only added if the nginxIngressAnnotations are activated, which is the same behavior as before.

@guerzon
Copy link
Owner

guerzon commented Aug 22, 2024

Hallo @HerrSpeck, danke für die PR

I am adding some review points.

charts/vaultwarden/templates/ingress.yaml Outdated Show resolved Hide resolved
charts/vaultwarden/templates/ingress.yaml Outdated Show resolved Hide resolved
@HerrSpeck
Copy link
Contributor Author

HerrSpeck commented Aug 26, 2024

Thanks for the response! :)

You're totally right, I moved the config map to the configmap.yaml file :) and I also fixed the typo, not sure how that happened 😅

I did some digging into the global-allowed-response-headers flag and from what I can tell this is not actually a key in the custom-headers config map, but is instead part of the nginx-configuration config map. As far as I can tell, it controls which headers are allowed in general and as soon as you would like to set custom headers, you also have to specify these headers in the global-allowed-response-headers to "whitelist" them.

So I added another config map for the nginx-configuration and I put it in the same namespace as the ingress configuration. Kind of lost at this point though, because I'm not sure if that's enough for the value to be applied or if we still have to add a reference to this config map somewhere. My current understanding is that this should suffice, but maybe you can help me out if I do in fact have to reference it somewhere else :) I'm also pretty sure that setting the value of global-allowed-response-headers to just "Request-Id" would be enough, but I added the remaining default headers that you provided in the example as well for now. It might also possible to set this value directly in the values.yaml instead?

@HerrSpeck
Copy link
Contributor Author

HerrSpeck commented Aug 26, 2024

Hi there @guerzon !

I had a colleague take a look at the problem as well and we found some more information that I would like to share:

It looks like this is a flag/setting on the ingress-controller and I couldn't find any way to configure this using the project-specific charts at all. Instead, as the name suggests, this is a global configuration (like the allow-snippet-annotations flag that caused this whole issue on our side) that has to be applied directly to the ingress-controller. Therefore, I reverted the changes where I introduced an additional config map that I tried to use to allow the correct headers.

BUT: I also found out that the allow-snippet-annotations configuration is deprecated and should not be used, because of security issues.
Therefore, I still think this is the correct way to solve this issue and I would suggest that we add a notice/warning to the README, that tells readers to configure the global-allowed-response-headers field of their ingress-controller to allow the Request-Id header. What do you think?

guerzon
guerzon previously approved these changes Aug 27, 2024
@guerzon
Copy link
Owner

guerzon commented Aug 27, 2024

Thanks for the feedback @HerrSpeck. I had more time to test this and I can see no issue with not adding global-allowed-response-headers.

PR looks good and ready to merge, but please rebase and fix the merge conflict. Thanks.

@HerrSpeck
Copy link
Contributor Author

HerrSpeck commented Aug 28, 2024

@guerzon Alright, sounds great :) I rebased the pr and fixed the merge conflicts. Setting 0.25.0 as the new version is alright? Or should I just increase the patch version instead?

I'm also wondering what the Request-Id header actually does? I feel like it's not actually required for the application to work. That's probably also why no issues arise if the global-allowed-response-headers field is not set.

@guerzon guerzon merged commit 6c1cf6a into guerzon:main Aug 29, 2024
1 check passed
@RealKelsar
Copy link

This broke my Setup. Not exactly sure why yet. Gives me 503 now, but other then in the nginx log I can not see any errors. Need to investigate after work.

@guerzon
Copy link
Owner

guerzon commented Aug 29, 2024

This broke my Setup. Not exactly sure why yet. Gives me 503 now, but other then in the nginx log I can not see any errors. Need to investigate after work.

Hi @RealKelsar, please submit a new issue and include your config and logs. Thanks.

dc-tec referenced this pull request in dc-tec/k8s-gitops Sep 1, 2024
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [vaultwarden](https://redirect.github.com/guerzon/vaultwarden) |
HelmChart | minor | `0.24.1` -> `0.25.2` |

---

### Release Notes

<details>
<summary>guerzon/vaultwarden (vaultwarden)</summary>

###
[`v0.25.2`](https://redirect.github.com/guerzon/vaultwarden/releases/tag/v0.25.2)

[Compare
Source](https://redirect.github.com/guerzon/vaultwarden/compare/v0.25.1...v0.25.2)

vaultwarden is an unofficial Bitwarden-compatible server written in Rust

#### What's Changed

- fix: incomplete fix, additional by
[@&#8203;guerzon](https://redirect.github.com/guerzon) in
[https://github.com/guerzon/vaultwarden/pull/113](https://redirect.github.com/guerzon/vaultwarden/pull/113)

**Full Changelog**:
guerzon/vaultwarden@v0.25.1...v0.25.2

###
[`v0.25.1`](https://redirect.github.com/guerzon/vaultwarden/releases/tag/v0.25.1)

[Compare
Source](https://redirect.github.com/guerzon/vaultwarden/compare/v0.25.0...v0.25.1)

vaultwarden is an unofficial Bitwarden-compatible server written in Rust

#### What's Changed

- fix: make customHeadersConfigMap optional by
[@&#8203;guerzon](https://redirect.github.com/guerzon) in
[https://github.com/guerzon/vaultwarden/pull/112](https://redirect.github.com/guerzon/vaultwarden/pull/112)

**Full Changelog**:
guerzon/vaultwarden@v0.25.0...v0.25.1

###
[`v0.25.0`](https://redirect.github.com/guerzon/vaultwarden/releases/tag/v0.25.0)

[Compare
Source](https://redirect.github.com/guerzon/vaultwarden/compare/v0.24.4...v0.25.0)

vaultwarden is an unofficial Bitwarden-compatible server written in Rust

#### What's Changed

- feat: Replace nginx snippet annotation with custom header annotation
by [@&#8203;HerrSpeck](https://redirect.github.com/HerrSpeck) in
[https://github.com/guerzon/vaultwarden/pull/106](https://redirect.github.com/guerzon/vaultwarden/pull/106)

#### New Contributors

- [@&#8203;HerrSpeck](https://redirect.github.com/HerrSpeck) made their
first contribution in
[https://github.com/guerzon/vaultwarden/pull/106](https://redirect.github.com/guerzon/vaultwarden/pull/106)

**Full Changelog**:
guerzon/vaultwarden@v0.24.4...v0.25.0

###
[`v0.24.4`](https://redirect.github.com/guerzon/vaultwarden/releases/tag/v0.24.4)

[Compare
Source](https://redirect.github.com/guerzon/vaultwarden/compare/v0.24.3...v0.24.4)

vaultwarden is an unofficial Bitwarden-compatible server written in Rust

#### What's Changed

- service: use selectorLabels helper by
[@&#8203;craigcabrey](https://redirect.github.com/craigcabrey) in
[https://github.com/guerzon/vaultwarden/pull/95](https://redirect.github.com/guerzon/vaultwarden/pull/95)

#### New Contributors

- [@&#8203;craigcabrey](https://redirect.github.com/craigcabrey) made
their first contribution in
[https://github.com/guerzon/vaultwarden/pull/95](https://redirect.github.com/guerzon/vaultwarden/pull/95)

**Full Changelog**:
guerzon/vaultwarden@v0.24.3...v0.24.4

###
[`v0.24.3`](https://redirect.github.com/guerzon/vaultwarden/releases/tag/v0.24.3)

[Compare
Source](https://redirect.github.com/guerzon/vaultwarden/compare/v0.24.2...v0.24.3)

vaultwarden is an unofficial Bitwarden-compatible server written in Rust

#### What's Changed

- fix: add flag to enable notifications service by
[@&#8203;guerzon](https://redirect.github.com/guerzon) in
[https://github.com/guerzon/vaultwarden/pull/110](https://redirect.github.com/guerzon/vaultwarden/pull/110)

**Full Changelog**:
guerzon/vaultwarden@v0.24.2...v0.24.3

###
[`v0.24.2`](https://redirect.github.com/guerzon/vaultwarden/releases/tag/v0.24.2)

[Compare
Source](https://redirect.github.com/guerzon/vaultwarden/compare/v0.24.1...v0.24.2)

vaultwarden is an unofficial Bitwarden-compatible server written in Rust

#### What's Changed

- Push Notifications existing secret by
[@&#8203;Yannik26](https://redirect.github.com/Yannik26) in
[https://github.com/guerzon/vaultwarden/pull/107](https://redirect.github.com/guerzon/vaultwarden/pull/107)

#### New Contributors

- [@&#8203;Yannik26](https://redirect.github.com/Yannik26) made their
first contribution in
[https://github.com/guerzon/vaultwarden/pull/107](https://redirect.github.com/guerzon/vaultwarden/pull/107)

**Full Changelog**:
guerzon/vaultwarden@v0.24.1...v0.24.2

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://redirect.github.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC41OS4yIiwidXBkYXRlZEluVmVyIjoiMzguNTkuMiIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsicmVub3ZhdGUiXX0=-->

Co-authored-by: Renovate Bot <[email protected]>
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