Skip to content

Commit

Permalink
use route instead of ingress for oauth endpoint
Browse files Browse the repository at this point in the history
Signed-off-by: Kevin <[email protected]>
  • Loading branch information
KPostOffice authored and openshift-merge-bot[bot] committed Jan 16, 2024
1 parent 0feab0f commit 8a5ea19
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 123 deletions.
10 changes: 0 additions & 10 deletions poetry.lock

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
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 @@ -557,10 +555,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
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
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
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 @@ -91,6 +91,7 @@
read_template,
enable_local_interactive,
)
import codeflare_sdk.utils.openshift_oauth as sdk_oauth

import openshift
from openshift.selector import Selector
Expand All @@ -110,6 +111,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 @@ -536,17 +555,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 @@ -2751,7 +2767,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 @@ -2765,35 +2780,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 @@ -2807,9 +2804,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 @@ -2823,16 +2817,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 @@ -2846,21 +2834,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

0 comments on commit 8a5ea19

Please sign in to comment.