gitea/helm-chart
45
54
Fork 127
Code Issues 30 Pull Requests 8 Actions Releases 87 Activity

fix: correctly handle tls ingress #94

Merged
luhahn merged 1 commits from huww98/helm-chart:fix-tls-ingress into master 2022-07-28 08:29:33 +00:00
Conversation 14 Commits 1 Files Changed 1 +9 -9
huww98 commented 2020-12-21 04:23:53 +00:00
Contributor
Copy Link

This ensures that the the protocol in ROOT_URL is https when configuring TLS in ingress resource. Furthermore, we cannot rely on (index .Values.ingress.tls 0).hosts as it is used for certificates and may contain wildcards.

We also don't need to check .Values.ingress.hosts again, because we already set it to .Values.gitea.config.server.DOMAIN.

Fixes: #307

This ensures that the the protocol in `ROOT_URL` is `https` when configuring TLS in ingress resource. Furthermore, we cannot rely on `(index .Values.ingress.tls 0).hosts` as it is used for certificates and may contain wildcards. We also don't need to check .Values.ingress.hosts again, because we already set it to `.Values.gitea.config.server.DOMAIN`. Fixes: #307
huww98 commented 2020-12-21 04:29:10 +00:00
Author
Contributor
Copy Link

Sorry, It does not work, I will make another attempt soon.

Sorry, It does not work, I will make another attempt soon.
huww98 force-pushed fix-tls-ingress from 6f96c17363 to 7487fb72ce 2020-12-21 04:45:29 +00:00 Compare
huww98 force-pushed fix-tls-ingress from 7487fb72ce to 7c5c95a615 2020-12-21 05:05:17 +00:00 Compare
huww98 commented 2020-12-21 05:09:26 +00:00
Author
Contributor
Copy Link

I've tested it locally this time.

With values.yaml:

ingress:
  enabled: true
  hosts:
  - git.mydomain.com
  tls:
  - hosts:
    - "mydomain.com"
    - "*.mydomain.com"
    secretName: mydomain-cert

it generate:

[server]
APP_DATA_PATH = /data
DOMAIN = git.mydomain.com
HTTP_PORT = 3000
PROTOCOL = http
ROOT_URL = https://git.mydomain.com
SSH_DOMAIN = git.mydomain.com
SSH_LISTEN_PORT = 22
SSH_PORT = 22
I've tested it locally this time. With values.yaml: ```yaml ingress: enabled: true hosts: - git.mydomain.com tls: - hosts: - "mydomain.com" - "*.mydomain.com" secretName: mydomain-cert ``` it generate: ```ini [server] APP_DATA_PATH = /data DOMAIN = git.mydomain.com HTTP_PORT = 3000 PROTOCOL = http ROOT_URL = https://git.mydomain.com SSH_DOMAIN = git.mydomain.com SSH_LISTEN_PORT = 22 SSH_PORT = 22 ```
techknowlogick approved these changes 2020-12-31 05:20:59 +00:00
Dismissed
luhahn commented 2021-01-04 07:54:50 +00:00
Member
Copy Link

Sorry, for not responding so long, wasn't available during the holidays. I'll take a look at this issue today.

Sorry, for not responding so long, wasn't available during the holidays. I'll take a look at this issue today.
luhahn commented 2021-01-04 14:38:57 +00:00
Member
Copy Link

I don't really understand why this PR is needed. The protocol will already be set in the config if you enable tls.

It only defaults to http. But you can set it manually.
This is already included and will be integrated in the ROOT_URL

{{- if not .Values.gitea.config.server.PROTOCOL -}}
{{- $_ := set .Values.gitea.config.server "PROTOCOL" "http" -}}
{{- end -}}
...
{{- if .Values.ingress.enabled -}}
    {{- if gt (len .Values.ingress.tls) 0 -}}
    {{- $_ := set .Values.gitea.config.server "ROOT_URL" (printf "%s://%s" .Values.gitea.config.server.PROTOCOL (index (index .Values.ingress.tls 0).hosts 0)) -}}
    {{- else -}}
    {{- $_ := set .Values.gitea.config.server "ROOT_URL" (printf "%s://%s" .Values.gitea.config.server.PROTOCOL (index .Values.ingress.hosts 0)) -}}
    {{- end -}}
    {{- else -}}
    {{- $_ := set .Values.gitea.config.server "ROOT_URL" (printf "%s://%s" .Values.gitea.config.server.PROTOCOL .Values.gitea.config.server.DOMAIN) -}}
    {{- end -}}
{{- end -}}
I don't really understand why this PR is needed. The protocol will already be set in the config if you enable tls. It only defaults to http. But you can set it manually. This is already included and will be integrated in the ROOT_URL ``` {{- if not .Values.gitea.config.server.PROTOCOL -}} {{- $_ := set .Values.gitea.config.server "PROTOCOL" "http" -}} {{- end -}} ... {{- if .Values.ingress.enabled -}} {{- if gt (len .Values.ingress.tls) 0 -}} {{- $_ := set .Values.gitea.config.server "ROOT_URL" (printf "%s://%s" .Values.gitea.config.server.PROTOCOL (index (index .Values.ingress.tls 0).hosts 0)) -}} {{- else -}} {{- $_ := set .Values.gitea.config.server "ROOT_URL" (printf "%s://%s" .Values.gitea.config.server.PROTOCOL (index .Values.ingress.hosts 0)) -}} {{- end -}} {{- else -}} {{- $_ := set .Values.gitea.config.server "ROOT_URL" (printf "%s://%s" .Values.gitea.config.server.PROTOCOL .Values.gitea.config.server.DOMAIN) -}} {{- end -}} {{- end -}} ```
luhahn requested changes 2021-01-04 14:39:39 +00:00
Dismissed
luhahn left a comment
Member
Copy Link

Please check again if this PR really is needed.

Please check again if this PR really is needed.
huww98 commented 2021-01-04 14:55:25 +00:00
Author
Contributor
Copy Link

@luhahn previously, the ROOT_URL is not set correctly. for values.yaml from above comment, it generate ROOT_URL = http://mydomain.com, which is wrong in 2 aspect:

  • The protocol should be https, instead of http, if TLS is enabled.
  • The domain part should be git.mydomain.com (.Values.ingress.hosts[0]), instead of mydomain.com (.Values.ingress.tls[0].hosts[0]), regardless whether TLS is enabled.
@luhahn previously, the `ROOT_URL` is not set correctly. for `values.yaml` from [above comment](https://gitea.com/gitea/helm-chart/issues/94#issuecomment-122715), it generate `ROOT_URL = http://mydomain.com`, which is wrong in 2 aspect: * The protocol should be `https`, instead of `http`, if TLS is enabled. * The domain part should be `git.mydomain.com` (.Values.ingress.hosts[0]), instead of `mydomain.com` (.Values.ingress.tls[0].hosts[0]), regardless whether TLS is enabled.
huww98 requested review from luhahn 2021-01-04 17:01:33 +00:00
luhahn requested changes 2021-01-07 15:31:51 +00:00
Dismissed
luhahn left a comment
Member
Copy Link

See my comment, I worry that this could be a breaking change, if someone is using only ingress to define the ROOT_URL

See my comment, I worry that this could be a breaking change, if someone is using only ingress to define the ROOT_URL
templates/gitea/config.yaml Outdated
@ -52,3 +46,1 @@
{{- else -}}
{{- $_ := set .Values.gitea.config.server "ROOT_URL" (printf "%s://%s" .Values.gitea.config.server.PROTOCOL .Values.gitea.config.server.DOMAIN) -}}
{{- end -}}
{{- $_ := set .Values.gitea.config.server "ROOT_URL" (printf "%s://%s" (include "public_protocol" .) .Values.gitea.config.server.DOMAIN) -}}
luhahn commented 2021-01-07 15:29:52 +00:00
Member
Copy Link

This change would be okay for domain. But if you're using the current functionality this would be a breaking change. For example, im not using the domain part, just configuring everything via ingress. We should keep the ingress part if no gitea.config.server.DOMAIN is set.

We could alter the current version so that your change would be asked first, and if no domain is set we default to ingress hosts.

This change would be okay for domain. But if you're using the current functionality this would be a breaking change. For example, im not using the domain part, just configuring everything via ingress. We should keep the ingress part if no gitea.config.server.DOMAIN is set. We could alter the current version so that your change would be asked first, and if no domain is set we default to ingress hosts.
huww98 commented 2021-03-17 09:53:41 +00:00
Author
Contributor
Copy Link

That should not be a breaking change, we have set .Values.gitea.config.server.DOMAIN to consider values from ingress earlier in this file (Line 40). So your config should just works.

That should not be a breaking change, we have set `.Values.gitea.config.server.DOMAIN` to consider values from ingress earlier in this file (Line 40). So your config should just works.
huww98 force-pushed fix-tls-ingress from 4a26e17afd to 60298cdad7 2021-03-17 10:01:11 +00:00 Compare
huww98 requested review from luhahn 2021-03-17 10:02:54 +00:00
huww98 commented 2021-03-17 10:04:11 +00:00
Author
Contributor
Copy Link

@luhahn Sorry, I don't recieve the notification. This should not be a breaking change. See my reply above. And I have rebased it on the master.

@luhahn Sorry, I don't recieve the notification. This should not be a breaking change. See my reply above. And I have rebased it on the master.
huww98 force-pushed fix-tls-ingress from 60298cdad7 to 300c52ec79 2021-03-17 10:10:21 +00:00 Compare
pat-s commented 2021-11-02 10:56:48 +00:00
Member
Copy Link

@huww98 There are quite some conflicts again unfortunately. Are you planning to continue/finish this?

@huww98 There are quite some conflicts again unfortunately. Are you planning to continue/finish this?
justusbunsi commented 2022-06-26 11:57:17 +00:00
Member
Copy Link

@huww98 There are quite some conflicts again unfortunately. Are you planning to continue/finish this?

@pat-s I have the feeling that this PR won't be finished by the original author. It's open for 2 years now and it looks like this PR got out of everyone's sight.

We really need to ensure that the ROOT_URL is set correctly. Especially with the newer versions of Gitea throwing errors to the frontend in case of mismatches.

If no one is against it I will close this PR next weekend (to give some response chances) and propose another PR based on this one.

cc: @luhahn

> @huww98 There are quite some conflicts again unfortunately. Are you planning to continue/finish this? @pat-s I have the feeling that this PR won't be finished by the original author. It's open for 2 years now and it looks like this PR got out of everyone's sight. We really need to ensure that the ROOT_URL is set correctly. Especially with the newer versions of Gitea throwing errors to the frontend in case of mismatches. If no one is against it I will close this PR next weekend (to give some response chances) and propose another PR based on this one. cc: @luhahn
👍 1
huww98 force-pushed fix-tls-ingress from 300c52ec79 to fe3dfb099c 2022-06-27 04:07:12 +00:00 Compare
huww98 commented 2022-06-27 04:11:25 +00:00
Author
Contributor
Copy Link

Rebased. Hopes the review process can be faster, or there will be only endless conflicts.

Rebased. Hopes the review process can be faster, or there will be only endless conflicts.
🎉 1
justusbunsi commented 2022-07-05 10:08:13 +00:00
Member
Copy Link

Probably closing #307.

Probably closing #307.
justusbunsi commented 2022-07-05 14:12:22 +00:00
Member
Copy Link

Hi @huww98. Thanks for your patience and for tackling this issue. I am currently evaluating unit testing for this Helm Chart, so I was curious if your changes would still pass the current main line test cases. You can find the test cases in my fork in a branch including usage instructions. I've included the test results in this comment as collapsed section.

Your changes do fix the wildcard tls + protocol issue for ROOT_URL which can be seen in the last test case (7). It's currently failing because it is designed to success on main line.
But it seems the current changes do not allow overwriting the ROOT_URL anymore (test case 5 of 7). That should still be possible.

@luhahn is right about this PR currently being a breaking change. The default settings rendering changed (test case 2 of 7). As we don't know how many users profit from the current "ingress-is-disabled-but-I-use-its-host-value-anyway" logic. There are two ways of handling this:

  • Remove the check for enabled ingress again.
  • Communicate it being a breaking change via README.md and describe how to mitigate it:
    • If not using built-in ingress but still expose it to the world, users need to define ROOT_URL and DOMAIN.

I'd prefer the first option, TBH.

Unit test results 
<assemblies>
	<assembly name="unittests/config-test.yaml" config-file="unittests/config-test.yaml" test-framework="helm-unittest" run-date="2022-07-05" run-time="16:01:45" time="0.100" total="8" passed="4" failed="4" skipped="0" errors="0">
		<environment>go1.16.9.linux-amd64</environment>
		<errors></errors>
		<collection name="Inline App Configuration Secret" time="0.100" total="8" passed="4" failed="4" skipped="0">
			<test name="renders expected object specs" type="Inline App Configuration Secret" method="Helm-Validation" time="0.012" result="Pass">
				<traits></traits>
			</test>
			<test name="renders default app.ini settings" type="Inline App Configuration Secret" method="Helm-Validation" time="0.013" result="Fail">
				<traits></traits>
				<failure exception-type="Helm-Validation">
					<message><![CDATA[Failed]]></message>
					<stack-trace><![CDATA[		 - asserts[0] `equal` fail 
			 Template:	gitea/templates/gitea/config.yaml 
			 DocumentIndex:	0 
			 Path:	stringData.server 
			 Expected to equal: 
			 	|- 
			 	  APP_DATA_PATH=/data 
			 	  DOMAIN=git.example.com 
			 	  ENABLE_PPROF=false 
			 	  HTTP_PORT=3000 
			 	  PROTOCOL=http 
			 	  ROOT_URL=http://git.example.com 
			 	  SSH_DOMAIN=git.example.com 
			 	  SSH_LISTEN_PORT=22 
			 	  SSH_PORT=22 
			 Actual: 
			 	|- 
			 	  APP_DATA_PATH=/data 
			 	  DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local 
			 	  ENABLE_PPROF=false 
			 	  HTTP_PORT=3000 
			 	  PROTOCOL=http 
			 	  ROOT_URL=http://gitea-unittests-gitea.testing.svc.cluster.local 
			 	  SSH_DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local 
			 	  SSH_LISTEN_PORT=22 
			 	  SSH_PORT=22 
			 Diff: 
			 	--- Expected 
			 	+++ Actual 
			 	@@ -2,3 +2,3 @@ 
			 	   APP_DATA_PATH=/data 
			 	-  DOMAIN=git.example.com 
			 	+  DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local 
			 	   ENABLE_PPROF=false 
			 	@@ -6,4 +6,4 @@ 
			 	   PROTOCOL=http 
			 	-  ROOT_URL=http://git.example.com 
			 	-  SSH_DOMAIN=git.example.com 
			 	+  ROOT_URL=http://gitea-unittests-gitea.testing.svc.cluster.local 
			 	+  SSH_DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local 
			 	   SSH_LISTEN_PORT=22 
]]></stack-trace>
				</failure>
			</test>
			<test name="without no ingress host specification given" type="Inline App Configuration Secret" method="Helm-Validation" time="0.014" result="Pass">
				<traits></traits>
			</test>
			<test name="with ingress enabled" type="Inline App Configuration Secret" method="Helm-Validation" time="0.013" result="Pass">
				<traits></traits>
			</test>
			<test name="with ingress enabled but no host specs given" type="Inline App Configuration Secret" method="Helm-Validation" time="0.013" result="Fail">
				<traits></traits>
				<failure exception-type="Helm-Validation">
					<message><![CDATA[Failed]]></message>
					<stack-trace><![CDATA[		 - asserts[0] `failedTemplate` fail 
			 Error: Invalid rendering: %!s(<nil>) 
]]></stack-trace>
				</failure>
			</test>
			<test name="allows an override for ROOT_URL" type="Inline App Configuration Secret" method="Helm-Validation" time="0.012" result="Fail">
				<traits></traits>
				<failure exception-type="Helm-Validation">
					<message><![CDATA[Failed]]></message>
					<stack-trace><![CDATA[		 - asserts[0] `equal` fail 
			 Template:	gitea/templates/gitea/config.yaml 
			 DocumentIndex:	0 
			 Path:	stringData.server 
			 Expected to equal: 
			 	|- 
			 	  APP_DATA_PATH=/data 
			 	  DOMAIN=git.example.com 
			 	  ENABLE_PPROF=false 
			 	  HTTP_PORT=3000 
			 	  PROTOCOL=http 
			 	  ROOT_URL=https://my.custom.gitea.url 
			 	  SSH_DOMAIN=git.example.com 
			 	  SSH_LISTEN_PORT=22 
			 	  SSH_PORT=22 
			 Actual: 
			 	|- 
			 	  APP_DATA_PATH=/data 
			 	  DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local 
			 	  ENABLE_PPROF=false 
			 	  HTTP_PORT=3000 
			 	  PROTOCOL=http 
			 	  ROOT_URL=https://my.custom.gitea.url 
			 	  SSH_DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local 
			 	  SSH_LISTEN_PORT=22 
			 	  SSH_PORT=22 
			 Diff: 
			 	--- Expected 
			 	+++ Actual 
			 	@@ -2,3 +2,3 @@ 
			 	   APP_DATA_PATH=/data 
			 	-  DOMAIN=git.example.com 
			 	+  DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local 
			 	   ENABLE_PPROF=false 
			 	@@ -7,3 +7,3 @@ 
			 	   ROOT_URL=https://my.custom.gitea.url 
			 	-  SSH_DOMAIN=git.example.com 
			 	+  SSH_DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local 
			 	   SSH_LISTEN_PORT=22 
]]></stack-trace>
				</failure>
			</test>
			<test name="allows an override for DOMAIN" type="Inline App Configuration Secret" method="Helm-Validation" time="0.012" result="Pass">
				<traits></traits>
			</test>
			<test name="with ingress enabled and tls configured" type="Inline App Configuration Secret" method="Helm-Validation" time="0.011" result="Fail">
				<traits></traits>
				<failure exception-type="Helm-Validation">
					<message><![CDATA[Failed]]></message>
					<stack-trace><![CDATA[		 - asserts[0] `equal` fail 
			 Template:	gitea/templates/gitea/config.yaml 
			 DocumentIndex:	0 
			 Path:	stringData.server 
			 Expected to equal: 
			 	|- 
			 	  APP_DATA_PATH=/data 
			 	  DOMAIN=git.example.com 
			 	  ENABLE_PPROF=false 
			 	  HTTP_PORT=3000 
			 	  PROTOCOL=http 
			 	  ROOT_URL=http://*.git.example.com 
			 	  SSH_DOMAIN=git.example.com 
			 	  SSH_LISTEN_PORT=22 
			 	  SSH_PORT=22 
			 Actual: 
			 	|- 
			 	  APP_DATA_PATH=/data 
			 	  DOMAIN=git.example.com 
			 	  ENABLE_PPROF=false 
			 	  HTTP_PORT=3000 
			 	  PROTOCOL=http 
			 	  ROOT_URL=https://git.example.com 
			 	  SSH_DOMAIN=git.example.com 
			 	  SSH_LISTEN_PORT=22 
			 	  SSH_PORT=22 
			 Diff: 
			 	--- Expected 
			 	+++ Actual 
			 	@@ -6,3 +6,3 @@ 
			 	   PROTOCOL=http 
			 	-  ROOT_URL=http://*.git.example.com 
			 	+  ROOT_URL=https://git.example.com 
			 	   SSH_DOMAIN=git.example.com 
]]></stack-trace>
				</failure>
			</test>
		</collection>
	</assembly>
</assemblies>

I've also noticed an edge case which isn't considered in main line and not in this PR: Enabled ingress but hosts spec explicitly set to empty array. This leads to a templating error. I've added a commented test case to the test suite covering it. I don't think it's scope of this PR, but important to be aware of.

Hi @huww98. Thanks for your patience and for tackling this issue. I am currently evaluating unit testing for this Helm Chart, so I was curious if your changes would still pass the current main line test cases. You can find the test cases [in my fork in a branch](https://gitea.com/justusbunsi/helm-chart/src/branch/unittesting/unittests) including usage instructions. I've included the test results in this comment as collapsed section. Your changes _do fix_ the wildcard tls + protocol issue for `ROOT_URL` which can be seen in the last test case (7). It's currently failing because it is designed to success on main line. But it seems the current changes do not allow overwriting the `ROOT_URL` anymore (test case 5 of 7). That should still be possible. @luhahn is right about this PR currently being a breaking change. The default settings rendering changed (test case 2 of 7). As we don't know how many users profit from the current "ingress-is-disabled-but-I-use-its-host-value-anyway" logic. There are two ways of handling this: - Remove the check for enabled ingress again. - Communicate it being a breaking change via README.md and describe how to mitigate it: - If not using built-in ingress but still expose it to the world, users need to define `ROOT_URL` and `DOMAIN`. I'd prefer the first option, TBH. <details> <summary>Unit test results</summary> ```xml <assemblies> <assembly name="unittests/config-test.yaml" config-file="unittests/config-test.yaml" test-framework="helm-unittest" run-date="2022-07-05" run-time="16:01:45" time="0.100" total="8" passed="4" failed="4" skipped="0" errors="0"> <environment>go1.16.9.linux-amd64</environment> <errors></errors> <collection name="Inline App Configuration Secret" time="0.100" total="8" passed="4" failed="4" skipped="0"> <test name="renders expected object specs" type="Inline App Configuration Secret" method="Helm-Validation" time="0.012" result="Pass"> <traits></traits> </test> <test name="renders default app.ini settings" type="Inline App Configuration Secret" method="Helm-Validation" time="0.013" result="Fail"> <traits></traits> <failure exception-type="Helm-Validation"> <message><![CDATA[Failed]]></message> <stack-trace><![CDATA[ - asserts[0] `equal` fail Template: gitea/templates/gitea/config.yaml DocumentIndex: 0 Path: stringData.server Expected to equal: |- APP_DATA_PATH=/data DOMAIN=git.example.com ENABLE_PPROF=false HTTP_PORT=3000 PROTOCOL=http ROOT_URL=http://git.example.com SSH_DOMAIN=git.example.com SSH_LISTEN_PORT=22 SSH_PORT=22 Actual: |- APP_DATA_PATH=/data DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local ENABLE_PPROF=false HTTP_PORT=3000 PROTOCOL=http ROOT_URL=http://gitea-unittests-gitea.testing.svc.cluster.local SSH_DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local SSH_LISTEN_PORT=22 SSH_PORT=22 Diff: --- Expected +++ Actual @@ -2,3 +2,3 @@ APP_DATA_PATH=/data - DOMAIN=git.example.com + DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local ENABLE_PPROF=false @@ -6,4 +6,4 @@ PROTOCOL=http - ROOT_URL=http://git.example.com - SSH_DOMAIN=git.example.com + ROOT_URL=http://gitea-unittests-gitea.testing.svc.cluster.local + SSH_DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local SSH_LISTEN_PORT=22 ]]></stack-trace> </failure> </test> <test name="without no ingress host specification given" type="Inline App Configuration Secret" method="Helm-Validation" time="0.014" result="Pass"> <traits></traits> </test> <test name="with ingress enabled" type="Inline App Configuration Secret" method="Helm-Validation" time="0.013" result="Pass"> <traits></traits> </test> <test name="with ingress enabled but no host specs given" type="Inline App Configuration Secret" method="Helm-Validation" time="0.013" result="Fail"> <traits></traits> <failure exception-type="Helm-Validation"> <message><![CDATA[Failed]]></message> <stack-trace><![CDATA[ - asserts[0] `failedTemplate` fail Error: Invalid rendering: %!s(<nil>) ]]></stack-trace> </failure> </test> <test name="allows an override for ROOT_URL" type="Inline App Configuration Secret" method="Helm-Validation" time="0.012" result="Fail"> <traits></traits> <failure exception-type="Helm-Validation"> <message><![CDATA[Failed]]></message> <stack-trace><![CDATA[ - asserts[0] `equal` fail Template: gitea/templates/gitea/config.yaml DocumentIndex: 0 Path: stringData.server Expected to equal: |- APP_DATA_PATH=/data DOMAIN=git.example.com ENABLE_PPROF=false HTTP_PORT=3000 PROTOCOL=http ROOT_URL=https://my.custom.gitea.url SSH_DOMAIN=git.example.com SSH_LISTEN_PORT=22 SSH_PORT=22 Actual: |- APP_DATA_PATH=/data DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local ENABLE_PPROF=false HTTP_PORT=3000 PROTOCOL=http ROOT_URL=https://my.custom.gitea.url SSH_DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local SSH_LISTEN_PORT=22 SSH_PORT=22 Diff: --- Expected +++ Actual @@ -2,3 +2,3 @@ APP_DATA_PATH=/data - DOMAIN=git.example.com + DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local ENABLE_PPROF=false @@ -7,3 +7,3 @@ ROOT_URL=https://my.custom.gitea.url - SSH_DOMAIN=git.example.com + SSH_DOMAIN=gitea-unittests-gitea.testing.svc.cluster.local SSH_LISTEN_PORT=22 ]]></stack-trace> </failure> </test> <test name="allows an override for DOMAIN" type="Inline App Configuration Secret" method="Helm-Validation" time="0.012" result="Pass"> <traits></traits> </test> <test name="with ingress enabled and tls configured" type="Inline App Configuration Secret" method="Helm-Validation" time="0.011" result="Fail"> <traits></traits> <failure exception-type="Helm-Validation"> <message><![CDATA[Failed]]></message> <stack-trace><![CDATA[ - asserts[0] `equal` fail Template: gitea/templates/gitea/config.yaml DocumentIndex: 0 Path: stringData.server Expected to equal: |- APP_DATA_PATH=/data DOMAIN=git.example.com ENABLE_PPROF=false HTTP_PORT=3000 PROTOCOL=http ROOT_URL=http://*.git.example.com SSH_DOMAIN=git.example.com SSH_LISTEN_PORT=22 SSH_PORT=22 Actual: |- APP_DATA_PATH=/data DOMAIN=git.example.com ENABLE_PPROF=false HTTP_PORT=3000 PROTOCOL=http ROOT_URL=https://git.example.com SSH_DOMAIN=git.example.com SSH_LISTEN_PORT=22 SSH_PORT=22 Diff: --- Expected +++ Actual @@ -6,3 +6,3 @@ PROTOCOL=http - ROOT_URL=http://*.git.example.com + ROOT_URL=https://git.example.com SSH_DOMAIN=git.example.com ]]></stack-trace> </failure> </test> </collection> </assembly> </assemblies> ``` </details> --- I've also noticed an edge case which isn't considered in main line and not in this PR: Enabled ingress but hosts spec explicitly set to empty array. This leads to a templating error. I've added a [commented test case to the test suite covering it](https://gitea.com/justusbunsi/helm-chart/src/commit/3c846c2be11b2a3259a1dc8565a1dec9c4257e3d/unittests/config-test.yaml#L70). I don't think it's scope of this PR, but important to be aware of.
justusbunsi added the
kind
bug
 label 2022-07-05 14:21:33 +00:00
luhahn commented 2022-07-07 10:51:29 +00:00
Member
Copy Link

I agree and also would prefer the first option.

I agree and also would prefer the first option.
huww98 force-pushed fix-tls-ingress from fe3dfb099c to dabeddae98 2022-07-24 07:12:01 +00:00 Compare
huww98 commented 2022-07-24 07:28:45 +00:00
Author
Contributor
Copy Link

Thanks for your review and detailed explanation. I was mislead by the if .Values.ingress.enabled branch in the original version, and was not aware of this use case.

Now I removed the check for enabled ingress. And verified the default settings rendering is not changed.

Sorry for the delay.

I'm curious if one would set DOMAIN and ROOT_URL to be inconsistent for any reason?

Thanks for your review and detailed explanation. I was mislead by the `if .Values.ingress.enabled` branch in the original version, and was not aware of this use case. Now I removed the check for enabled ingress. And verified the default settings rendering is not changed. Sorry for the delay. I'm curious if one would set DOMAIN and ROOT_URL to be inconsistent for any reason?
luhahn approved these changes 2022-07-26 07:14:13 +00:00
luhahn left a comment
Member
Copy Link

Thanks for hanging on so long, LGTM :)

Thanks for hanging on so long, LGTM :)
justusbunsi approved these changes 2022-07-27 16:46:39 +00:00
justusbunsi left a comment
Member
Copy Link

? it. Thank you for your patience.

? it. Thank you for your patience.
justusbunsi commented 2022-07-27 16:51:57 +00:00
Member
Copy Link

I've updated the PR description.

I've updated the PR description.
luhahn merged commit 58fc28f6d0 into master 2022-07-28 08:29:33 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
luhahn
justusbunsi
Labels
Clear labels   
has
backport

  
in progress

   
invalid
  
kind
breaking

  
kind
bug

  
kind
build

  
kind
dependency

  
kind
deployment

  
kind
docs

  
kind
enhancement

  
kind
feature

  
kind
lint

  
kind
proposal

  
kind
question

  
kind
refactor

  
kind
security

  
kind
testing

  
kind
translation

  
kind
ui

  
need
backport

  
priority
critical

  
priority
low

  
priority
maybe

  
priority
medium

  
reviewed
duplicate

  
reviewed
invalid

  
reviewed
wontfix

  
skip-changelog
  
status
blocked

  
status
needs-feedback

  
status
needs-reviews

  
status
wip

  
upstream
gitea

  
upstream
other

No Label
has
backport
in progress
invalid
kind
breaking
kind
bug
kind
build
kind
dependency
kind
deployment
kind
docs
kind
enhancement
kind
feature
kind
lint
kind
proposal
kind
question
kind
refactor
kind
security
kind
testing
kind
translation
kind
ui
need
backport
priority
critical
priority
low
priority
maybe
priority
medium
reviewed
duplicate
reviewed
invalid
reviewed
wontfix
skip-changelog
status
blocked
status
needs-feedback
status
needs-reviews
status
wip
upstream
gitea
upstream
other
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: gitea/helm-chart#94
No description provided.
Delete Branch "huww98/helm-chart:fix-tls-ingress"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?