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: use pathType ImplementationSpecific for regex #272

Merged
merged 2 commits into from
Apr 25, 2024

Conversation

guilhem
Copy link
Contributor

@guilhem guilhem commented Apr 15, 2024

fix error

path /(/?|$)(.*) cannot be used with pathType Prefix

fix error
```
path /(/?|$)(.*) cannot be used with pathType Prefix
```

Signed-off-by: Guilhem Lettron <[email protected]>
@guilhem guilhem force-pushed the ImplementationSpecific branch from 7500eaf to 75d4cc4 Compare April 15, 2024 16:57
@klesh
Copy link
Contributor

klesh commented Apr 16, 2024

@JorgeGar Would you like to take a look? Thanks in advance.

@guilhem
Copy link
Contributor Author

guilhem commented Apr 24, 2024

gentle ping

@@ -63,8 +63,12 @@ spec:
- path: /{{ include "devlake.grafanaEndpointPrefix" . }}
{{- end }}
{{- if semverCompare ">=1.18-0" $.Capabilities.KubeVersion.GitVersion }}
{{- if .Values.ingress.useDefaultNginx }}
pathType: ImplementationSpecific
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be good enough, but why not making the pathType itself a variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in fact, I just copy paste exactly what was working before:

              {{- if semverCompare ">=1.19-0" $.Capabilities.KubeVersion.GitVersion }}
              service:
                name: {{ .Values.grafana.ingressServiceName | default ( include "grafana.fullname" (dict "Values" .Values.grafana "Chart" (dict "Name" "grafana") "Release" .Release ) ) }}
                port:
                  number: {{ .Values.grafana.ingressServicePort | default .Values.grafana.service.port }}
              {{- else }}
              serviceName: {{ .Values.grafana.ingressServiceName | default ( include "grafana.fullname" (dict "Values" .Values.grafana "Chart" (dict "Name" "grafana") "Release" .Release ) ) }}
              servicePort: {{ .Values.grafana.ingressServicePort | default .Values.grafana.service.port }}
              {{- end }}

@JorgeGar
Copy link
Contributor

gentle ping

So sorry, I missed the first notifications. Thanks for the ping.
I've added one comment, just let me know what you think about that :)

This simplify template and overriding.
@guilhem
Copy link
Contributor Author

guilhem commented Apr 24, 2024

@JorgeGar I change in last commit the way to manage ingress with 2 different ing resources.
I think it's easier to manage annotations specific in this way.

I also have that need because (/?|$)(.*) was far too much aggressive when wanting to do specific configuration for sub path (I have special rules for /api).

port:
number: 4000
number: {{ .Values.grafana.ingressServicePort | default .Values.grafana.service.port }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we have the value garafana.service.port in the values.yaml file. Since this is the default, coul you kindly add it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@guilhem if you could kindly take a look into this, the rest is looking great and I'm happy to approve it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JorgeGar I replied here #272 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

All good then :) Thanks

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 kept the exact same behavior not to change everything at once :)

Theses values come from grafana chart as dependancy.

@JorgeGar JorgeGar merged commit 392dc80 into apache:main Apr 25, 2024
3 checks passed
ZhangNing10 added a commit that referenced this pull request Apr 25, 2024
@ZhangNing10
Copy link
Contributor

hi @guilhem @JorgeGar , i have to revert this pr as having two ingresses can result in waste when the ingress class is alb, it would create two aws alb, i have created a pr #278 for this issue, could you check whether this pr solves your issue?

@guilhem
Copy link
Contributor Author

guilhem commented Apr 25, 2024

hi @guilhem @JorgeGar , i have to revert this pr as having two ingresses can result in waste when the ingress class is alb, it would create two aws alb, i have created a pr #278 for this issue, could you check whether this pr solves your issue?

Hi @ZhangNing10 this is not the case since 2020 and controller v2: https://aws.amazon.com/fr/blogs/containers/introducing-aws-load-balancer-controller/

you can use:

metadata:
  annotation:
    alb.ingress.kubernetes.io/group.name: devlake

which is supported by this PR as we are sharing the same .Values.ingress.annotations for both ingresses :)

@ZhangNing10
Copy link
Contributor

hi @guilhem @JorgeGar , i have to revert this pr as having two ingresses can result in waste when the ingress class is alb, it would create two aws alb, i have created a pr #278 for this issue, could you check whether this pr solves your issue?

Hi @ZhangNing10 this is not the case since 2020 and controller v2: https://aws.amazon.com/fr/blogs/containers/introducing-aws-load-balancer-controller/

you can use:

metadata:
  annotation:
    alb.ingress.kubernetes.io/group.name: devlake

which is supported by this PR as we are sharing the same .Values.ingress.annotations for both ingresses :)

Hi @guilhem , thanks for the info! As the grafana specifies root_url: "%(protocol)s://%(domain)s/grafana" with a subpath grafana, so i think for internal grafana, no need to rewrite target, while for an external grafana, rewrite target should be needed. So, i removed the rewrite target path for the first ingress, could you help review the pr #278 and check whether it includes the fix of the issue you found?

@JorgeGar
Copy link
Contributor

hi @guilhem @JorgeGar , i have to revert this pr as having two ingresses can result in waste when the ingress class is alb, it would create two aws alb, i have created a pr #278 for this issue, could you check whether this pr solves your issue?

Hi @ZhangNing10 this is not the case since 2020 and controller v2: https://aws.amazon.com/fr/blogs/containers/introducing-aws-load-balancer-controller/
you can use:

metadata:
  annotation:
    alb.ingress.kubernetes.io/group.name: devlake

which is supported by this PR as we are sharing the same .Values.ingress.annotations for both ingresses :)

Hi @guilhem , thanks for the info! As the grafana specifies root_url: "%(protocol)s://%(domain)s/grafana" with a subpath grafana, so i think for internal grafana, no need to rewrite target, while for an external grafana, rewrite target should be needed. So, i removed the rewrite target path for the first ingress, could you help review the pr #278 and check whether it includes the fix of the issue you found?

Hi @ZhangNing10. Sorry, I was a bit late to answer. I see the PR is already merged. Is the issue solved? Please, let me know how else I can help 😃

@ghost
Copy link

ghost commented May 2, 2024

Hi @guilhem,

Thank you for your contribution! I'm Joshua Poddoku, Committer at Apache DevLake. We'd love for you to join our Slack community or DM me incase you've joined already: https://join.slack.com/t/devlake-io/shared_invite/zt-18uayb6ut-cHOjiYcBwERQ8VVPZ9cQQw. Could you please share your full name there? We want to recognize your efforts with a certificate of contribution.

Best,
Joshua

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