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

Can't include $ in permanent-redirect URL annotation #11175

Open
artlogic opened this issue Mar 29, 2024 · 7 comments
Open

Can't include $ in permanent-redirect URL annotation #11175

artlogic opened this issue Mar 29, 2024 · 7 comments
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/backlog Higher priority than priority/awaiting-more-evidence. triage/accepted Indicates an issue or PR is ready to be actively worked on. triage/needs-information Indicates an issue needs more information in order to work on it.

Comments

@artlogic
Copy link

What happened:

When attempting to add a permanent-redirect annotation with an nginx variable, the admission controller wouldn't allow it.

nginx.ingress.kubernetes.io/permanent-redirect: https://redirectedto.com$request_uri

Results in:

admission webhook "validate.nginx.ingress.kubernetes.io" denied the request: annotation nginx.ingress.kubernetes.io/permanent-redirect contains invalid value

What you expected to happen:

In earlier versions this syntax was allowed. I have been using up until recently.

NGINX Ingress controller version (exec into the pod and run nginx-ingress-controller --version.):

Version 1.9.6 installed via helm chart 4.9.1

Kubernetes version (use kubectl version):

v1.27.11

Environment:

  • Cloud provider or hardware configuration: Linode
  • OS (e.g. from /etc/os-release): Debian GNU/Linux 11 (bullseye)
  • Kernel (e.g. uname -a): 5.10.0-27-cloud-amd64
  • Install tools: Linode LKE
    • Please mention how/where was the cluster created like kubeadm/kops/minikube/kind etc.
  • Basic cluster related info:
    • kubectl version: v1.27.11
    • kubectl get nodes -o wide
NAME                         STATUS   ROLES    AGE   VERSION   INTERNAL-IP       EXTERNAL-IP      OS-IMAGE                         KERNEL-VERSION          CONTAINER-RUNTIME
lke5626-81148-63f064cd35e1   Ready    <none>   36d   v1.27.9   192.168.195.76    XXXX   Debian GNU/Linux 11 (bullseye)   5.10.0-27-cloud-amd64   containerd://1.6.28
lke5626-81148-63f819f9ea7b   Ready    <none>   36d   v1.27.9   192.168.200.108   XXXX     Debian GNU/Linux 11 (bullseye)   5.10.0-28-cloud-amd64   containerd://1.6.28
lke5626-8369-5dd46ac20000    Ready    <none>   36d   v1.27.9   192.168.129.108   XXXX     Debian GNU/Linux 11 (bullseye)   5.10.0-27-cloud-amd64   containerd://1.6.28
lke5626-8369-5ef41c553db8    Ready    <none>   36d   v1.27.9   192.168.156.2     XXXX     Debian GNU/Linux 11 (bullseye)   5.10.0-27-cloud-amd64   containerd://1.6.28
lke5626-8369-5ef41c556858    Ready    <none>   36d   v1.27.9   192.168.156.25    XXXX    Debian GNU/Linux 11 (bullseye)   5.10.0-27-cloud-amd64   containerd://1.6.28
lke5626-8369-5fab04523d82    Ready    <none>   36d   v1.27.9   192.168.156.37    XXXX   Debian GNU/Linux 11 (bullseye)   5.10.0-27-cloud-amd64   containerd://1.6.28
lke5626-8369-5fc5b7ebc61b    Ready    <none>   36d   v1.27.9   192.168.156.39    XXXX   Debian GNU/Linux 11 (bullseye)   5.10.0-27-cloud-amd64   containerd://1.6.28
lke5626-8369-62d5aa13807b    Ready    <none>   36d   v1.27.9   192.168.156.49    XXXX   Debian GNU/Linux 11 (bullseye)   5.10.0-27-cloud-amd64   containerd://1.6.28
  • How was the ingress-nginx-controller installed:
    • If helm was used then please show output of helm ls -A | grep -i ingress
    • If helm was used then please show output of helm -n <ingresscontrollernamespace> get values <helmreleasename>
    • If helm was not used, then copy/paste the complete precise command used to install the controller, along with the flags and options used
    • if you have more than one instance of the ingress-nginx-controller installed in the same cluster, please provide details for all the instances
ingress-nginx-app	default     	13      	2024-02-21 03:31:52.789669193 -0500 EST	deployed	ingress-nginx-4.9.1  	1.9.6 
USER-SUPPLIED VALUES:
controller:
  enableAnnotationValidations: true
  replicaCount: 3
  service:
    annotations:
      service.beta.kubernetes.io/linode-loadbalancer-default-proxy-protocol: v2
      service.beta.kubernetes.io/linode-loadbalancer-hostname-only-ingress: true
defaultBackend:
  enabled: true
  • Current State of the controller:
    • kubectl describe ingressclasses
    • kubectl -n <ingresscontrollernamespace> get all -A -o wide
    • kubectl -n <ingresscontrollernamespace> describe po <ingresscontrollerpodname>
    • kubectl -n <ingresscontrollernamespace> describe svc <ingresscontrollerservicename>
Name:         nginx
Labels:       app.kubernetes.io/component=controller
              app.kubernetes.io/instance=ingress-nginx-volt
              app.kubernetes.io/managed-by=Helm
              app.kubernetes.io/name=ingress-nginx
              app.kubernetes.io/part-of=ingress-nginx
              app.kubernetes.io/version=1.9.6
              helm.sh/chart=ingress-nginx-4.9.1
Annotations:  meta.helm.sh/release-name: ingress-nginx-volt
              meta.helm.sh/release-namespace: default
Controller:   k8s.io/ingress-nginx
Events:       <none>

Please let me know if the rest is needed.

  • Current state of ingress object, if applicable:
    • kubectl -n <appnamespace> get all,ing -o wide
    • kubectl -n <appnamespace> describe ing <ingressname>
    • If applicable, then, your complete and exact curl/grpcurl command (redacted if required) and the reponse to the curl/grpcurl command with the -v flag

This occurs with any ingress.

  • Others:
    • Any other related information like ;
      • copy/paste of the snippet (if applicable)
      • kubectl describe ... of any custom configmap(s) created and in use
      • Any other related information that may help

How to reproduce this issue:

Try to add an ingress that uses permanent-redirect with a $ in the URL.

Anything else we need to know:

@artlogic artlogic added the kind/bug Categorizes issue or PR as related to a bug. label Mar 29, 2024
@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels Mar 29, 2024
@longwuyuan
Copy link
Contributor

/triage accepted
/priority backlog
/assign

Discussed here https://kubernetes.slack.com/archives/CANQGM8BA/p1711559130696059

cc @rikatz @tao12345666333 @strongjz @cpanato

It is almost certain that the work on validations set the $/dollar sign as high risk and hence unacceptable. Was discussed in community meeting with Ricardo.

Next step is for me to peruse code and confirm the high-risk classification and exclusion of dollar/$ sign character from the list of allowed characters in the permanent-redirect annotation.

@tao12345666333 any help or comments you can provide is appreciated.

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels Mar 29, 2024
@strongjz
Copy link
Member

Whats the pathType of the ingress object? We have added strict regex to Exact and Prefix, you may need to use ImplementationSpecfic

/triage needs-information

@k8s-ci-robot k8s-ci-robot added the triage/needs-information Indicates an issue needs more information in order to work on it. label Apr 16, 2024
@apiwat-chantawibul
Copy link
Contributor

I encountered the same problem.

  • @strongjz This happens even if pathType: ImplementationSpecific is set.
  • Here is something similar to what I used:
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: redirect-legacy
  annotations:
    nginx.ingress.kubernetes.io/permanent-redirect: https://redirect-target.com/v2/$1?mode=legacy
spec:
  ingressClassName: nginx
  rules:
  - host: legacy-api.com
    http: &HTTP
      paths:
      - path: /([a-f0-9]{32})/?$
        pathType: ImplementationSpecific
        backend:
          # `backend` field is ignored by NGINX ingress controller when `permanent-redirect` is active.
          # It's only filled because k8s API requirements.
          service:
            name: upstream-service
            port:
              name: http

Using this version

$ helm list -n ingress-nginx
NAME         	NAMESPACE    	REVISION	UPDATED                                	STATUS  	CHART               	APP VERSION
ingress-nginx	ingress-nginx	1       	2024-03-03 21:56:29.187687019  0700  07	deployed	ingress-nginx-4.10.0	1.10.0 

@artlogic
Copy link
Author

artlogic commented Apr 26, 2024

Whats the pathType of the ingress object? We have added strict regex to Exact and Prefix, you may need to use ImplementationSpecfic

My pathType is set to Prefix. I'm currently OOO but I will see if ImplementationSpecific resolves the issue next week.

Copy link

This is stale, but we won't close it automatically, just bare in mind the maintainers may be busy with other tasks and will reach your issue ASAP. If you have any question or request to prioritize this, please reach #ingress-nginx-dev on Kubernetes Slack.

@github-actions github-actions bot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label May 27, 2024
@Starefossen
Copy link

We are facing the same issue in most recent version of ingress-nginx with ImplementationSpecific ingress as well.

@longwuyuan
Copy link
Contributor

After looking at the redirect destination posted here, it seems there could be a contention regarding what values are valid as destination and what values are not.

Currently, if a FQDN or a FDN suffixed with a known path is configured as a value to the redirect annotations, then there is no problem at all. So it seems that this issue is reporting a fail of redirect annotation only and only when a nginx variable is used in the value for the redirect annotation.

After checking here, it looks like a regexp group can be used as the regexp does not call for extrapolating a nginx var. So I doubt that a nginx var is a acceptable valid value for the redirect annotations. Hence I think there is not much that can be done on this problem as nginx vars are nowhere visibly documented as a standard.

The project has taken on too many custom features unique to the controller, that are not defined in either the K8S KEP specs or the docs of the upstream Nginx reverseproxy/webserver. It has caused security problems and maintenance problems and hence the project is moving towards healthy and reliable design by removing less used and edge-case use features & functionalities. Hence working on using nginx vars as part of the value for the redirect annotations does not seem like a viable approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/backlog Higher priority than priority/awaiting-more-evidence. triage/accepted Indicates an issue or PR is ready to be actively worked on. triage/needs-information Indicates an issue needs more information in order to work on it.
Projects
Development

No branches or pull requests

6 participants