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

Jump to bottom

feat(ingressAdvanced): use tc-opencors-chain if allow-cors is enabled #13240

Merged
merged 13 commits into from Nov 3, 2023
Merged

feat(ingressAdvanced): use tc-opencors-chain if allow-cors is enabled #13240

merged 13 commits into from Nov 3, 2023

Conversation

vandie
Copy link
Contributor

@vandie vandie commented Oct 4, 2023 •
edited

Description

⚒️ Allows for the optional use of the existing Traefik CORS setup for Apps as CORS is required to use the official jellyfin-vue client. Has been updated to rely on truecharts/library-charts#530

⚙️ Type of change

  • ⚙️ Feature/App addition
  • 🪛 Bugfix
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 🔃 Refactor of current code

🧪 How Has This Been Tested?
Manually edited the chart's files on my own TrueNas Scale Server.
Checked that jellyffin was using the tf-opencors-chain in Traefik rather than basic-chain and ensured I could connect to jellyfin via a custom app of jellyfin-vue client when the added config was enabled but not if disabled.

Evidence of the correct chain being used in Traefik
Added config option

📃 Notes:

This PR uses the same cors middleware setup as the existing audiobookshelf app to ensure conistancy. While it may be expected that the tf-opencors-headers middleware could be added within the UI this unfortunately results in the Access-Control-Allow-Origin header still being missing as the common chain conflicts.

✔️ Checklist:

  • ⚖️ My code follows the style guidelines of this project
  • 👀 I have performed a self-review of my own code
  • #️⃣ I have commented my code, particularly in hard-to-understand areas
  • 📄 I have made corresponding changes to the documentation
  • ⚠️ My changes generate no new warnings
  • 🧪 I have added tests to this description that prove my fix is effective or that my feature works
  • ⬆️ I increased versions for any altered app according to semantic versioning

➕ App addition

If this PR is an app addition please make sure you have done the following.

  • 🪞 I have opened a PR on truecharts/containers adding the container to TrueCharts mirror repo.
  • 🖼️ I have added an icon in the Chart's root directory called icon.png

Please don't blindly check all the boxes. Read them and only check those that apply.
Those checkboxes are there for the reviewer to see what is this all about and
the status of this PR with a quick glance.

@vandie vandie requested review from Ornias1993 and a team as code owners October 4, 2023 13:44
@CLAassistant
Copy link

CLAassistant commented Oct 4, 2023 •
edited

CLA assistant check
All committers have signed the CLA.

@stavros-k
Copy link
Member

I'm not a fan of hardcoding it just to cover an optional use case.
I'd prefer it to also be optional

@vandie
Copy link
Contributor Author

vandie commented Oct 4, 2023

I'm not a fan of hardcoding it just to cover an optional use case. I'd prefer it to also be optional

I think that's fair. The reason I didn't make it optional was that my assumption is that if this impacts one client, then I can see it affecting others as well. I'll look to add an option.

@vandie
Copy link
Contributor Author

vandie commented Oct 4, 2023

As requested, I've made this optional. Defaulted to false so as to maintain existing settings for current installs and tested on TrueNAS-SCALE-22.12.4.

Option for toggling CORS

@ksimm1
Copy link
Contributor

ksimm1 commented Oct 4, 2023

why not just add back the override default middlewares option

@Ornias1993
Copy link
Member

why not just add back the override default middlewares option

VETO'ed, that override was removed for good reasons.. It's unsafe and stupid.

@vandie
Copy link
Contributor Author

vandie commented Oct 6, 2023

why not just add back the override default middlewares option

VETO'ed, that override was removed for good reasons.. It's unsafe and stupid.

I did think it was removed for a reason 😅

@Ornias1993
Copy link
Member

@stavros-k I think we might want to move this (open vs closed cors in values.yaml) to common?

@vandie
Copy link
Contributor Author

vandie commented Oct 7, 2023

@stavros-k I think we might want to move this (open vs closed cors in values.yaml) to common?

You mean so that this can be toggled on any app?

@stavros-k
Copy link
Member

@stavros-k I think we might want to move this (open vs closed cors in values.yaml) to common?

Sounds good, but not exposed in the UI to all apps.
Toggled on apps that 100% need it, and for apps like this one, we expose the toggle.

@vandie
Copy link
Contributor Author

vandie commented Oct 7, 2023 •
edited

@stavros-k I think we might want to move this (open vs closed cors in values.yaml) to common?

Sounds good, but not exposed in the UI to all apps.
Toggled on apps that 100% need it, and for apps like this one, we expose the toggle.

Sounds good. So you want me to move the value itself andthe flag wrapped section of template to common and then leave the exposed toggle only on this one app for now?

@vandie vandie mentioned this pull request Oct 10, 2023
Merged
13 tasks
@vandie
Copy link
Contributor Author

vandie commented Oct 11, 2023

Updated this PR to make it use truecharts/library-charts#530

Ornias1993 pushed a commit to truecharts/library-charts that referenced this pull request Oct 29, 2023
@vandie
feat(ingress): Add allowCors to common values and enable the opencors…
1f54990 
… middleware if true (#530)

**Description**
As per the discussion on
truecharts/charts#13240, I've added the cors
enablement flag to common. Once everyone is happy with this PR, I'll
update the original to expose this flag instead of creating it's own.

**⚙️ Type of change**

- [x] ⚙️ Feature/App addition
- [ ] 🪛 Bugfix
- [ ] ⚠️ Breaking change (fix or feature that would cause existing
functionality to not work as expected)
- [ ] 🔃 Refactor of current code

**🧪 How Has This Been Tested?**
<!--
Please describe the tests that you ran to verify your changes. Provide
instructions so we can reproduce. Please also list any relevant details
for your test configuration
-->

**📃 Notes:**
<!-- Please enter any other relevant information here -->

**✔️ Checklist:**

- [x] ⚖️ My code follows the style guidelines of this project
- [x] 👀 I have performed a self-review of my own code
- [x] #️⃣ I have commented my code, particularly in hard-to-understand
areas
- [ ] 📄 I have made corresponding changes to the documentation
- [ ] ⚠️ My changes generate no new warnings
- [x] 🧪 I have added tests to this description that prove my fix is
effective or that my feature works
- [ ] ⬆️ I increased versions for any altered app according to semantic
versioning

**➕ App addition**

If this PR is an app addition please make sure you have done the
following.

- [ ] 🪞 I have opened a PR on
[truecharts/containers](https://github.com/truecharts/containers) adding
the container to TrueCharts mirror repo.
- [ ] 🖼️ I have added an icon in the Chart's root directory called
`icon.png`

---

_Please don't blindly check all the boxes. Read them and only check
those that apply.
Those checkboxes are there for the reviewer to see what is this all
about and
the status of this PR with a quick glance._
@Ornias1993
Copy link
Member

Updated this PR to make it use truecharts/library-charts#530

Common is just bumped on this afaik, so you should be able to do this now :)

@vandie
Copy link
Contributor Author

vandie commented Oct 29, 2023

Updated this PR to make it use truecharts/library-charts#530

Common is just bumped on this afaik, so you should be able to do this now :)

Thanks yeah, I saw my other PR had been merged. Thanks. Will get this updated.

@vandie
Merge branch 'master' into master
3705daf 
Signed-off-by: Michael Walter Van Der Velden <mikevdv@vandie.co.uk>
@vandie
Copy link
Contributor Author

vandie commented Oct 31, 2023

@Ornias1993 I think that should be good now then. Let me know if you want any further changes

@vandie vandie changed the title fix(jellyfin): use tc-opencors-chain for jellyfin so that it can work with the jellyfin-vue client fix(jellyfin): use tc-opencors-chain if allow-cors is enabled Nov 3, 2023
@vandie
Copy link
Contributor Author

vandie commented Nov 3, 2023

Updated as @Ornias1993 requested that it not just be available on jellyfin

@vandie vandie changed the title fix(jellyfin): use tc-opencors-chain if allow-cors is enabled feat(ingressAdvanced): use tc-opencors-chain if allow-cors is enabled Nov 3, 2023
@Ornias1993 Ornias1993 merged commit 7d15dc3 into truecharts:master Nov 3, 2023
15 checks passed
@truecharts-admin
Copy link
Collaborator

This PR is locked to prevent necro-posting on closed PRs. Please create a issue or contact staff on discord if you want to further discuss this

@truecharts truecharts locked as resolved and limited conversation to collaborators Nov 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Ornias1993 Ornias1993 Ornias1993 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@vandie @CLAassistant @stavros-k @ksimm1 @Ornias1993 @truecharts-admin