Skip to content

use route instead of ingress for oauth endpoint #437

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 16, 2024
Merged

use route instead of ingress for oauth endpoint #437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions poetry.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 0 additions & 6 deletions src/codeflare_sdk/utils/generate_yaml.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@
from base64 import b64encode
from urllib3.util import parse_url

from .kube_api_helpers import _get_api_host


def read_template(template):
with open(template, "r") as stream:
Expand Down Expand Up @@ -539,10 +537,6 @@ def enable_openshift_oauth(user_yaml, cluster_name, namespace):
tls_secret_name = f"{cluster_name}-proxy-tls-secret"
tls_volume_name = "proxy-tls-secret"
port_name = "oauth-proxy"
host = _get_api_host(k8_client)
host = host.replace(
"api.", f"{gen_dashboard_ingress_name(cluster_name)}-{namespace}.apps."
)
oauth_sidecar = _create_oauth_sidecar_object(
namespace,
tls_mount_location,
Expand Down
4 changes: 0 additions & 4 deletions src/codeflare_sdk/utils/kube_api_helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,3 @@ def _kube_api_error_handling(
elif e.reason == "Conflict":
raise FileExistsError(exists_msg)
raise e


def _get_api_host(api_client: client.ApiClient): # pragma: no cover
return parse_url(api_client.configuration.host).host
109 changes: 48 additions & 61 deletions src/codeflare_sdk/utils/openshift_oauth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,47 +1,51 @@
from urllib3.util import parse_url
from .generate_yaml import gen_dashboard_ingress_name
from .kube_api_helpers import _get_api_host
from base64 import b64decode
import yaml

from ..cluster.auth import config_check, api_config_handler

from kubernetes import client
from kubernetes import dynamic


def _route_api_getter():
return dynamic.DynamicClient(
api_config_handler() or client.ApiClient()
).resources.get(api_version="route.openshift.io/v1", kind="Route")


def create_openshift_oauth_objects(cluster_name, namespace):
config_check()
api_client = api_config_handler() or client.ApiClient()
oauth_port = 8443
oauth_sa_name = f"{cluster_name}-oauth-proxy"
tls_secret_name = _gen_tls_secret_name(cluster_name)
service_name = f"{cluster_name}-oauth"
port_name = "oauth-proxy"
host = _get_api_host(api_client)

# replace "^api" with the expected host
host = f"{gen_dashboard_ingress_name(cluster_name)}-{namespace}.apps" + host.lstrip(
"api"
)

_create_or_replace_oauth_sa(namespace, oauth_sa_name, host)
_create_or_replace_oauth_sa(namespace, oauth_sa_name, cluster_name)
_create_or_replace_oauth_service_obj(
cluster_name, namespace, oauth_port, tls_secret_name, service_name, port_name
)
_create_or_replace_oauth_ingress_object(
cluster_name, namespace, service_name, port_name, host
_create_or_replace_oauth_route_object(
cluster_name,
namespace,
service_name,
port_name,
)
_create_or_replace_oauth_rb(cluster_name, namespace, oauth_sa_name)


def _create_or_replace_oauth_sa(namespace, oauth_sa_name, host):
def _create_or_replace_oauth_sa(namespace, oauth_sa_name, cluster_name):
oauth_sa = client.V1ServiceAccount(
api_version="v1",
kind="ServiceAccount",
metadata=client.V1ObjectMeta(
name=oauth_sa_name,
namespace=namespace,
annotations={
"serviceaccounts.openshift.io/oauth-redirecturi.first": f"https://{host}"
"serviceaccounts.openshift.io/oauth-redirectreference.first": '{"kind":"OAuthRedirectReference","apiVersion":"v1","reference":{"kind":"Route","name":"'
+ "ray-dashboard-"
+ cluster_name
+ '"}}'
},
),
)
Expand Down Expand Up @@ -98,15 +102,14 @@ def delete_openshift_oauth_objects(cluster_name, namespace):
# for an existing cluster before calling this => the objects should never be deleted twice
oauth_sa_name = f"{cluster_name}-oauth-proxy"
service_name = f"{cluster_name}-oauth"
v1_routes = _route_api_getter()
client.CoreV1Api(api_config_handler()).delete_namespaced_service_account(
name=oauth_sa_name, namespace=namespace
)
client.CoreV1Api(api_config_handler()).delete_namespaced_service(
name=service_name, namespace=namespace
)
client.NetworkingV1Api(api_config_handler()).delete_namespaced_ingress(
name=f"{cluster_name}-ingress", namespace=namespace
)
v1_routes.delete(name=f"ray-dashboard-{cluster_name}", namespace=namespace)
client.RbacAuthorizationV1Api(api_config_handler()).delete_cluster_role_binding(
name=f"{cluster_name}-rb"
)
Expand Down Expand Up @@ -161,52 +164,36 @@ def _create_or_replace_oauth_service_obj(
raise e


def _create_or_replace_oauth_ingress_object(
def _create_or_replace_oauth_route_object(
cluster_name: str,
namespace: str,
service_name: str,
port_name: str,
host: str,
) -> client.V1Ingress:
ingress = client.V1Ingress(
api_version="networking.k8s.io/v1",
kind="Ingress",
metadata=client.V1ObjectMeta(
annotations={"route.openshift.io/termination": "passthrough"},
name=f"{cluster_name}-ingress",
namespace=namespace,
),
spec=client.V1IngressSpec(
rules=[
client.V1IngressRule(
host=host,
http=client.V1HTTPIngressRuleValue(
paths=[
client.V1HTTPIngressPath(
backend=client.V1IngressBackend(
service=client.V1IngressServiceBackend(
name=service_name,
port=client.V1ServiceBackendPort(
name=port_name
),
)
),
path_type="ImplementationSpecific",
)
]
),
)
]
),
)
):
route = f"""
apiVersion: route.openshift.io/v1
kind: Route
metadata:
name: ray-dashboard-{cluster_name}
namespace: {namespace}
spec:
port:
targetPort: {port_name}
tls:
termination: passthrough
to:
kind: Service
name: {service_name}
"""
route_data = yaml.safe_load(route)
v1_routes = _route_api_getter()
try:
client.NetworkingV1Api(api_config_handler()).create_namespaced_ingress(
namespace=namespace, body=ingress
existing_route = v1_routes.get(
name=f"ray-dashboard-{cluster_name}", namespace=namespace
)
except client.ApiException as e:
if e.reason == "Conflict":
client.NetworkingV1Api(api_config_handler()).replace_namespaced_ingress(
namespace=namespace, body=ingress, name=f"{cluster_name}-ingress"
)
else:
raise e
route_data["metadata"]["resourceVersion"] = existing_route["metadata"][
"resourceVersion"
]
v1_routes.replace(body=route_data)
except dynamic.client.ApiException:
v1_routes.create(body=route_data)
66 changes: 24 additions & 42 deletions tests/unit_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
aw_dir = os.path.expanduser("~/.codeflare/appwrapper/")
sys.path.append(str(parent) + "/src")

from kubernetes import client, config
from kubernetes import client, config, dynamic
from codeflare_sdk.cluster.awload import AWManager
from codeflare_sdk.cluster.cluster import (
Cluster,
Expand Down Expand Up @@ -90,6 +90,7 @@
read_template,
enable_local_interactive,
)
import codeflare_sdk.utils.openshift_oauth as sdk_oauth

import openshift
from openshift.selector import Selector
Expand All @@ -109,6 +110,24 @@
fake_res = openshift.Result("fake")


def mock_routes_api(mocker):
mocker.patch.object(
sdk_oauth,
"_route_api_getter",
return_value=MagicMock(
resources=MagicMock(
get=MagicMock(
return_value=MagicMock(
create=MagicMock(),
replace=MagicMock(),
delete=MagicMock(),
)
)
)
),
)


def arg_side_effect(*args):
fake_res.high_level_operation = args
return fake_res
Expand Down Expand Up @@ -535,17 +554,14 @@ def test_delete_openshift_oauth_objects(mocker):
mocker.patch.object(client.CoreV1Api, "delete_namespaced_service")
mocker.patch.object(client.NetworkingV1Api, "delete_namespaced_ingress")
mocker.patch.object(client.RbacAuthorizationV1Api, "delete_cluster_role_binding")
mock_routes_api(mocker)
delete_openshift_oauth_objects("test-cluster", "test-namespace")

client.CoreV1Api.delete_namespaced_service_account.assert_called_with(
name="test-cluster-oauth-proxy", namespace="test-namespace"
)
client.CoreV1Api.delete_namespaced_service.assert_called_with(
name="test-cluster-oauth", namespace="test-namespace"
)
client.NetworkingV1Api.delete_namespaced_ingress.assert_called_with(
name="test-cluster-ingress", namespace="test-namespace"
)
client.RbacAuthorizationV1Api.delete_cluster_role_binding.assert_called_with(
name="test-cluster-rb"
)
Expand Down Expand Up @@ -2675,7 +2691,6 @@ def test_create_openshift_oauth(mocker: MockerFixture):
create_namespaced_service_account = MagicMock()
create_cluster_role_binding = MagicMock()
create_namespaced_service = MagicMock()
create_namespaced_ingress = MagicMock()
mocker.patch.object(
client.CoreV1Api,
"create_namespaced_service_account",
Expand All @@ -2689,35 +2704,17 @@ def test_create_openshift_oauth(mocker: MockerFixture):
mocker.patch.object(
client.CoreV1Api, "create_namespaced_service", create_namespaced_service
)
mocker.patch.object(
client.NetworkingV1Api, "create_namespaced_ingress", create_namespaced_ingress
)
mocker.patch(
"codeflare_sdk.utils.openshift_oauth._get_api_host", return_value="foo.com"
)
mock_routes_api(mocker)
create_openshift_oauth_objects("foo", "bar")
create_ns_sa_args = create_namespaced_service_account.call_args
create_crb_args = create_cluster_role_binding.call_args
create_ns_serv_args = create_namespaced_service.call_args
create_ns_ingress_args = create_namespaced_ingress.call_args
assert (
create_ns_sa_args.kwargs["namespace"] == create_ns_serv_args.kwargs["namespace"]
)
assert (
create_ns_serv_args.kwargs["namespace"]
== create_ns_ingress_args.kwargs["namespace"]
)
assert isinstance(create_ns_sa_args.kwargs["body"], client.V1ServiceAccount)
assert isinstance(create_crb_args.kwargs["body"], client.V1ClusterRoleBinding)
assert isinstance(create_ns_serv_args.kwargs["body"], client.V1Service)
assert isinstance(create_ns_ingress_args.kwargs["body"], client.V1Ingress)
assert (
create_ns_serv_args.kwargs["body"].spec.ports[0].name
== create_ns_ingress_args.kwargs["body"]
.spec.rules[0]
.http.paths[0]
.backend.service.port.name
)


def test_replace_openshift_oauth(mocker: MockerFixture):
Expand All @@ -2731,9 +2728,6 @@ def test_replace_openshift_oauth(mocker: MockerFixture):
create_namespaced_service = MagicMock(
side_effect=client.ApiException(reason="Conflict")
)
create_namespaced_ingress = MagicMock(
side_effect=client.ApiException(reason="Conflict")
)
mocker.patch.object(
client.CoreV1Api,
"create_namespaced_service_account",
Expand All @@ -2747,16 +2741,10 @@ def test_replace_openshift_oauth(mocker: MockerFixture):
mocker.patch.object(
client.CoreV1Api, "create_namespaced_service", create_namespaced_service
)
mocker.patch.object(
client.NetworkingV1Api, "create_namespaced_ingress", create_namespaced_ingress
)
mocker.patch(
"codeflare_sdk.utils.openshift_oauth._get_api_host", return_value="foo.com"
)
mocker.patch.object(dynamic.ResourceList, "get", return_value=True)
replace_namespaced_service_account = MagicMock()
replace_cluster_role_binding = MagicMock()
replace_namespaced_service = MagicMock()
replace_namespaced_ingress = MagicMock()
mocker.patch.object(
client.CoreV1Api,
"replace_namespaced_service_account",
Expand All @@ -2770,21 +2758,15 @@ def test_replace_openshift_oauth(mocker: MockerFixture):
mocker.patch.object(
client.CoreV1Api, "replace_namespaced_service", replace_namespaced_service
)
mocker.patch.object(
client.NetworkingV1Api, "replace_namespaced_ingress", replace_namespaced_ingress
)
mock_routes_api(mocker)
create_openshift_oauth_objects("foo", "bar")
replace_namespaced_service_account.assert_called_once()
replace_cluster_role_binding.assert_called_once()
replace_namespaced_service.assert_called_once()
replace_namespaced_ingress.assert_called_once()


def test_gen_app_wrapper_with_oauth(mocker: MockerFixture):
mocker.patch("kubernetes.client.ApisApi.get_api_versions")
mocker.patch(
"codeflare_sdk.utils.generate_yaml._get_api_host", return_value="foo.com"
)
mocker.patch(
"codeflare_sdk.cluster.cluster.get_current_namespace",
return_value="opendatahub",
Expand Down