[Bug] Fail to create ingress due to the deprecation of the ingress.class annotation

[kevin85421]

Why are these changes needed?

This issue is not totally same as #441, and thus I opened a new issue. Users cannot create ingress with the instructions in https://ray-project.github.io/kuberay/guidance/ingress/. The operator also reports the error message:

2022-10-18T00:34:38.187Z	ERROR	controllers.RayCluster	Ingress create error!	{"Ingress.Error": "Ingress.extensions \"raycluster-ingress-head-ingress\" is invalid: annotations.kubernetes.io/ingress.class: Invalid value: \"nginx\": can not be set when the class field is also set", "error": "Ingress.extensions \"raycluster-ingress-head-ingress\" is invalid: annotations.kubernetes.io/ingress.class: Invalid value: \"nginx\": can not be set when the class field is also set"}
github.com/ray-project/kuberay/ray-operator/controllers/ray.(*RayClusterReconciler).reconcileIngress
	/workspace/controllers/ray/raycluster_controller.go:256
github.com/ray-project/kuberay/ray-operator/controllers/ray.(*RayClusterReconciler).rayClusterReconcile
	/workspace/controllers/ray/raycluster_controller.go:192
github.com/ray-project/kuberay/ray-operator/controllers/ray.(*RayClusterReconciler).Reconcile
	/workspace/controllers/ray/raycluster_controller.go:97
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile
	/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:114
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler
	/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:311
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem
	/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:266
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2
	/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:227

As shown in the following code snippet, the error message is reported by the function ValidateIngressCreate (Link) in Kubernetes. When both annotations.kubernetes.io/ingress.class and spec.IngressClassName are set at the same time, the validation function will report the error "can not be set when the class field is also set". This PR only sets spec.IngressClassName and avoids setting annotations.kubernetes.io/ingress.class.

if annotationIsSet && ingress.Spec.IngressClassName != nil {
   annotationPath := field.NewPath("annotations").Child(annotationIngressClass)
   allErrs = append(allErrs, field.Invalid(annotationPath, annotationVal, "can not be set when the class field is also set"))
}

In doc, it indicates that annotations.kubernetes.io/ingress.class was deprecated in Kubernetes 1.18, and spec.ingressClassName is a replacement for this annotation.

Related issue number

Closes #645
#441

Checks

• I've made sure the tests are passing.
• Testing Strategy
  • Unit tests
  • Manual tests
  • This PR is not tested :(

Update

Thank @rokiyer for pointing this out!

Based on @rokiyer comments, we need to add additional annotations and update the path-matching pattern in the ingress. To elaborate,

(1) ingress.go: Path: "/" + cluster.Name => Path: "/" + cluster.Name + "/(.*)" (Note: (.*) is regex.)
(2) YAML file: nginx.ingress.kubernetes.io/rewrite-target: /$1 (rewrite-target doc)

For example, the ingress definition above will result in the following rewrites:

• $IP/$CLUSTER_NAME/ rewrites to $IP/ ($1 maps to nothing)
• $IP/$CLUSTER_NAME/#/actors rewrites to $IP/#/actors ($1 maps to #/actors)
• $IP/$CLUSTER_NAME/#/node rewrites to $IP/#/node ($1 maps to #/node)

Checks

# Step1: Create a KinD cluster with `extraPortMappings` and `node-labels`
cat <<EOF | kind create cluster --config=-
kind: Cluster
apiVersion: kind.x-k8s.io/v1alpha4
nodes:
- role: control-plane
  kubeadmConfigPatches:
  - |
    kind: InitConfiguration
    nodeRegistration:
      kubeletExtraArgs:
        node-labels: "ingress-ready=true"
  extraPortMappings:
  - containerPort: 80
    hostPort: 80
    protocol: TCP
  - containerPort: 443
    hostPort: 443
    protocol: TCP
EOF
# Step2: Install NGINX ingress
kubectl apply -f https://raw.githubusercontent.com/kubernetes/ingress-nginx/main/deploy/static/provider/kind/deploy.yaml
kubectl wait --namespace ingress-nginx \
  --for=condition=ready pod \
  --selector=app.kubernetes.io/component=controller \
  --timeout=90s
# Step3: Install KubeRay operator
pushd helm-chart/kuberay-operator
helm install kuberay-operator .
# Step4: Install RayCluster with Nginx ingress. See https://github.com/ray-project/kuberay/pull/646
#        for the explanations of `ray-cluster.ingress.yaml`. Some fields are worth to discuss further:
#
#        (1) metadata.annotations.kubernetes.io/ingress.class: nginx => required
#        (2) spec.headGroupSpec.enableIngress: true => required
#        (3) metadata.annotations.nginx.ingress.kubernetes.io/rewrite-target: /$1 => required for nginx.
popd
kubectl apply -f ray-operator/config/samples/ray-cluster.ingress.yaml
# Step5: Check ingress created by Step4.
kubectl describe ingress raycluster-ingress-head-ingress
# [Example]
# ...
# Rules:
# Host        Path  Backends
# ----        ----  --------
# *
#             /raycluster-ingress/(.*)   raycluster-ingress-head-svc:8265 (10.244.0.11:8265)
# Annotations:  nginx.ingress.kubernetes.io/rewrite-target: /$1
# Step6: Check `<ip>/raycluster-ingress/` on your browser. You will see the Ray Dashboard.
#        [Note] The forward slash at the end of the address is necessary. `<ip>/raycluster-ingress`
#               will report "404 Not Found".

• Set up an ingress controller on KinD cluster

[kevin85421]

cc @rokiyer @brucez-anyscale

[Jeffwan]

technically, we don't like to support version < 1.19 since ingress api has been updated. We internally have similar issue but make some customization. https://opensource.googleblog.com/2020/09/kubernetes-ingress-goes-ga.html

BTW, in public cloud, I don't think there're still 1.18 clusters?

[kevin85421]

technically, we don't like to support version < 1.19 since ingress api has been updated. We internally have similar issue but make some customization. https://opensource.googleblog.com/2020/09/kubernetes-ingress-goes-ga.html

BTW, in public cloud, I don't think there're still 1.18 clusters?

Thank @Jeffwan for this information!

I am a little confused about your comments. This PR does not support Kubernetes < 1.19. Can you explain more about your concerns? Thank you!

[Jeffwan]

Sorry for unclear comments. I mean the user should not use the deprecated annotation in earlier versions and this validation logic looks good to me. I was wondering why user still give the annotation on > 1.19 clusters? probably forget to clean them up after upgrade

[kevin85421]

Sorry for unclear comments. I mean the user should not use the deprecated annotation in earlier versions and this validation logic looks good to me. I was wondering why user still give the annotation on > 1.19 clusters? probably forget to clean them up after upgrade

Thank you for your explanation! In my opinion, some users followed the KubeRay doc to add the ingress.class annotation in their YAML files. To keep backward compatibility, I decided to use the annotation to set the value of spec.IngressClassName.

Honestly, I really agreed with you that the logic is weird. Do you have any thoughts on the following two methods?

(1) Keep backward compatibility with weird logic (deprecated annotation) => current logic
(2) Break backward compatibility with rational logic (Maybe we can have a new field IngressClassName in RayCluster CRD)

[rokiyer]

cc @rokiyer @brucez-anyscale

@kevin85421 Thanks for fast bugfix. I have tried the code changes and ingress can be created.

However, when I visit http://{domain}/cluster_name, "404 Not Found" shows up.
I have to make some changes in ingress before I can visit ray dashboard. Changes:

• ingress path: change /cluster_name to /cluster_name(/|$)(.*)
• ingress annotation: add new annotation, nginx.ingress.kubernetes.io/rewrite-target: /$2

[Jeffwan]

(1) Keep backward compatibility with weird logic (deprecated annotation) => current logic
(2) Break backward compatibility with rational logic (Maybe we can have a new field IngressClassName in RayCluster CRD)

As @rokiyer said, user needs to give additional annotations to make Ingress work. for short term, I think current PR should be ok. for 2nd option, I suggest we use a specific annotation instead a new field to present the class name.

[kevin85421]

(1) Keep backward compatibility with weird logic (deprecated annotation) => current logic
(2) Break backward compatibility with rational logic (Maybe we can have a new field IngressClassName in RayCluster CRD)

As @rokiyer said, user needs to give additional annotations to make Ingress work. for short term, I think current PR should be ok. for 2nd option, I suggest we use a specific annotation instead a new field to present the class name.

Maybe I can open an issue to track the ingress support in KubeRay. We can discuss more details about ingress in next community sync meeting:

(1) All annotations in RayCluster will be passed to Ingress
(2) Is the exposed interface general enough for users to configure their custom ingress?
(3) Do we need to keep the backward compatibility?

cc @DmitriGekhtman

[DmitriGekhtman]

TBH, I don't think it makes sense for the operator to have been built-in ingress support; details of ingress set-up are sensitive to cloud provider context and user requirements.
It is straightforward enough have users kubectl apply

ingress
---
raycluster

Some discussion of how to set up an ingress in the docs would be great.

My opinions:

(2) Is the exposed interface general enough for users to configure their custom ingress?

We don't need to prioritize this. Given that ingress support is already there, we should get things to a state where basic use-cases are covered. Then we can document the limitations of the built-in support and explain how to set up an ingress yourself.

(3) Do we need to keep the backward compatibility?

I also don't think we should stress about this -- just don't introduce any config structure incompatibilities (don't delete fields).

Either new field or a custom annotation seems fine to me. A custom annotation might be better initially since we're feeling pretty iffy about the ingress support in general at the moment. The only important thing is to clearly document the solution we choose.

[kevin85421]

Thank @DmitriGekhtman for your reply!

I will write the documents about:

(1) Limitations of KubeRay's built-in ingress support: I still need to figure out the limitation. Currently, I just understand Nginx, and I will start to work on #637 to study the support of AWS LoadBalancer.

(2) Tell users how to set up ingress by themselves.

By the way, do we need to add "custom annotation" support in this PR? Thank you!

[DmitriGekhtman]

This PR is fine as is, I think.

[Jeffwan]

By the way, do we need to add "custom annotation" support in this PR? Thank you!

This one looks good. We can have a separate issue/PR for further improvement.