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

automatically mount service account tokens when needed #3888

Merged
merged 6 commits into from
Jun 12, 2024
Merged
4 changes: 3 additions & 1 deletion paasta_tools/generate_authenticating_services.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.
"""
A simple script to enumerate all services partecipating in authenticated
A simple script to enumerate all services participating in authenticated
communications, and list them in a YAML/JSON file.
"""
import argparse
Expand All @@ -27,6 +27,7 @@
import yaml

from paasta_tools.utils import DEFAULT_SOA_DIR
from paasta_tools.utils import load_system_paasta_config
from paasta_tools.utils import write_json_configuration_file
from paasta_tools.utils import write_yaml_configuration_file

Expand All @@ -53,6 +54,7 @@ def enumerate_authenticating_services() -> Dict[str, List[str]]:
config_path_pattern = os.path.join(DEFAULT_SOA_DIR, "*", AUTHORIZATION_CONFIG_FILE)
for authz_config in glob.glob(config_path_pattern):
result.update(list_services_in_authz_config(authz_config))
result.update(load_system_paasta_config().get_always_authenticating_services())
return {"services": sorted(result)}


Expand Down
40 changes: 40 additions & 0 deletions paasta_tools/kubernetes_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import re
from datetime import datetime
from enum import Enum
from functools import lru_cache
from inspect import currentframe
from pathlib import Path
from typing import Any
Expand Down Expand Up @@ -2552,6 +2553,13 @@ def get_topology_spread_constraints(
"topology_spread_constraints", default_pod_topology_spread_constraints
)

def get_projected_sa_volumes(self) -> List[ProjectedSAVolume]:
return add_volumes_for_authenticating_services(
service_name=self.service,
config_volumes=super().get_projected_sa_volumes(),
soa_dir=self.soa_dir,
)


def get_kubernetes_secret_hashes(
environment_variables: Mapping[str, str], service: str, namespace: str
Expand Down Expand Up @@ -4406,3 +4414,35 @@ def get_kubernetes_secret_volumes(
] = secret_contents

return secret_volumes


@lru_cache()
def get_authenticating_services(soa_dir: str = DEFAULT_SOA_DIR) -> Set[str]:
"""Load list of services participating in authenticated traffic"""
authenticating_services_conf_path = os.path.join(soa_dir, "authenticating.yaml")
config = service_configuration_lib.read_yaml_file(authenticating_services_conf_path)
return set(config.get("services", []))


def add_volumes_for_authenticating_services(
service_name: str,
config_volumes: List[ProjectedSAVolume],
soa_dir: str = DEFAULT_SOA_DIR,
) -> List[ProjectedSAVolume]:
"""Add projected service account volume to the list of volumes if service
participates in authenticated traffic. In case of changes, a new list is returned,
no updates in-place.

:param str service_name: name of the service
:param List[ProjectedSAVolume] config_volumes: existing projected volumes from service config
:param str soa_dir: path to SOA configurations directory
:return: updated list of projected service account volumes
"""
token_config = load_system_paasta_config().get_service_auth_token_volume_config()
if (
token_config
and service_name in get_authenticating_services(soa_dir)
and not any(volume == token_config for volume in config_volumes)
Copy link
Member

Choose a reason for hiding this comment

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

i think it might be worth doing a more generic dedupe in the future - presumably having user-supplied duplicates is also something we'd like to avoid on top of preventing folks from colliding with the auto-mounted token-config volume

i.e., we probably wanna figure out a central place to dedupe all volume configs and handle all of them consistently :p

def get_pod_volumes(
has us dedupe a couple different types of volume mounts, but not all of them ;_;

):
config_volumes = [token_config, *config_volumes]
return config_volumes
13 changes: 13 additions & 0 deletions paasta_tools/tron_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
from paasta_tools.utils import validate_pool
from paasta_tools.utils import PoolsNotConfiguredError
from paasta_tools.utils import DockerVolume
from paasta_tools.utils import ProjectedSAVolume

from paasta_tools import spark_tools

Expand All @@ -71,6 +72,7 @@
from paasta_tools.secret_tools import is_shared_secret_from_secret_name
from paasta_tools.secret_tools import get_secret_name_from_ref
from paasta_tools.kubernetes_tools import get_paasta_secret_name
from paasta_tools.kubernetes_tools import add_volumes_for_authenticating_services
from paasta_tools.secret_tools import SHARED_SECRET_SERVICE

from paasta_tools import monitoring_tools
Expand Down Expand Up @@ -627,6 +629,14 @@ def get_spark_executor_pool(self) -> str:
def get_service_account_name(self) -> Optional[str]:
return self.config_dict.get("service_account_name")

def get_projected_sa_volumes(self) -> Optional[List[ProjectedSAVolume]]:
projected_volumes = add_volumes_for_authenticating_services(
service_name=self.service,
config_volumes=super().get_projected_sa_volumes(),
soa_dir=self.soa_dir,
)
return projected_volumes if projected_volumes else None


class TronJobConfig:
"""Represents a job in Tron, consisting of action(s) and job-level configuration values."""
Expand Down Expand Up @@ -958,6 +968,9 @@ def format_tron_action_dict(action_config: TronActionConfig):
dry_run=action_config.for_validation,
)

# service account token volumes for service authentication
result["projected_sa_volumes"] = action_config.get_projected_sa_volumes()

extra_volumes = action_config.get_extra_volumes()
if executor == "spark":
is_mrjob = action_config.config_dict.get("mrjob", False)
Expand Down
8 changes: 8 additions & 0 deletions paasta_tools/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -2020,6 +2020,8 @@ class SystemPaastaConfigDict(TypedDict, total=False):
eks_cluster_aliases: Dict[str, str]
secret_sync_delay_seconds: float
use_multiple_log_readers: Optional[List[str]]
service_auth_token_settings: ProjectedSAVolume
always_authenticating_services: List[str]


def load_system_paasta_config(
Expand Down Expand Up @@ -2725,6 +2727,12 @@ def get_spark_kubeconfig(self) -> str:
def get_kube_clusters(self) -> Dict:
return self.config_dict.get("kube_clusters", {})

def get_service_auth_token_volume_config(self) -> ProjectedSAVolume:
return self.config_dict.get("service_auth_token_settings", {})
Comment on lines +2730 to +2731
Copy link
Contributor

Choose a reason for hiding this comment

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

lol this difference in name is going to trip me up in the future, I just know 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.

lol, apologies, I'm a terrible human being. I created this config first thinking that each JSON file is loaded in a different "namespace", but then realized that everything gets flattened out, so started adding words over words to the key.


def get_always_authenticating_services(self) -> List[str]:
return self.config_dict.get("always_authenticating_services", [])


def _run(
command: Union[str, List[str]],
Expand Down
29 changes: 26 additions & 3 deletions tests/test_generate_authenticating_services.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,17 +56,39 @@ def mock_soa_configs(tmpdir):
yield


def test_enumerate_authenticating_services(mock_soa_configs):
@pytest.fixture
def mock_paasta_config():
with patch(
"paasta_tools.generate_authenticating_services.load_system_paasta_config"
) as mock_load:
mock_load.return_value.get_always_authenticating_services.return_value = [
"service_always"
]
yield mock_load.return_value


def test_enumerate_authenticating_services(mock_soa_configs, mock_paasta_config):
assert generate_authenticating_services.enumerate_authenticating_services() == {
"services": ["service_a", "service_b", "service_c", "service_d"],
"services": [
"service_a",
"service_always",
"service_b",
"service_c",
"service_d",
],
}


@patch("paasta_tools.utils.socket")
@patch("paasta_tools.utils.datetime")
@patch("paasta_tools.generate_authenticating_services.parse_args")
def test_main_yaml_config(
mock_parse_args, mock_datetime, mock_socket, tmpdir, mock_soa_configs
mock_parse_args,
mock_datetime,
mock_socket,
tmpdir,
mock_soa_configs,
mock_paasta_config,
):
output = tmpdir / "authenticating.yaml"
mock_datetime.datetime.now().isoformat.return_value = "$SOME_TIME"
Expand All @@ -85,6 +107,7 @@ def test_main_yaml_config(
---
services:
- service_a
- service_always
- service_b
- service_c
- service_d
Expand Down
61 changes: 61 additions & 0 deletions tests/test_kubernetes_tools.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import functools
from base64 import b64encode
from copy import deepcopy
from typing import Any
from typing import Dict
from typing import List
Expand Down Expand Up @@ -84,6 +85,7 @@
from paasta_tools.contrib.get_running_task_allocation import (
get_pod_pool as task_allocation_get_pod_pool,
)
from paasta_tools.kubernetes_tools import add_volumes_for_authenticating_services
from paasta_tools.kubernetes_tools import allowlist_denylist_to_requirements
from paasta_tools.kubernetes_tools import create_custom_resource
from paasta_tools.kubernetes_tools import create_deployment
Expand Down Expand Up @@ -906,6 +908,9 @@ def test_get_kubernetes_containers(self, prometheus_port, expected_ports):
"paasta_tools.kubernetes_tools.KubernetesDeploymentConfig.get_sidecar_containers",
autospec=True,
return_value=["mock_sidecar"],
), mock.patch(
"paasta_tools.kubernetes_tools.load_system_paasta_config",
autospec=True,
):
if prometheus_port:
self.deployment.config_dict["prometheus_port"] = prometheus_port
Expand Down Expand Up @@ -1611,6 +1616,10 @@ def test_format_kubernetes_app_dict(self, _):
in ret.spec.template.metadata.labels.__setitem__.mock_calls
)

@mock.patch(
"paasta_tools.kubernetes_tools.load_system_paasta_config",
autospec=True,
)
@mock.patch(
"paasta_tools.kubernetes_tools.KubernetesDeploymentConfig.get_volumes",
autospec=True,
Expand Down Expand Up @@ -1695,6 +1704,7 @@ def test_get_pod_template_spec(
mock_get_pod_volumes,
mock_get_kubernetes_containers,
mock_get_volumes,
mock_load_system_paasta_config,
in_smtstk,
routable_ip,
pod_topology,
Expand Down Expand Up @@ -4905,3 +4915,54 @@ def test_get_kubernetes_secret_volumes_single_file():
assert ret == {
"/the/container/path/the_secret_name": "secret_contents",
}


@pytest.mark.parametrize(
"service,existing_config,expected",
(
(
"service_auth",
[],
[{"audience": "foo.bar", "container_path": "/var/secret/something"}],
),
("service_noauth", [], []),
(
"service_auth",
[{"audience": "foo.bar", "container_path": "/var/secret/something"}],
[{"audience": "foo.bar", "container_path": "/var/secret/something"}],
),
(
"service_auth",
[{"audience": "foo.bar", "container_path": "/var/secret/whatever"}],
[
{"audience": "foo.bar", "container_path": "/var/secret/something"},
{"audience": "foo.bar", "container_path": "/var/secret/whatever"},
],
),
(
"service_noauth",
[{"audience": "foo.bar", "container_path": "/var/secret/whatever"}],
[{"audience": "foo.bar", "container_path": "/var/secret/whatever"}],
),
),
)
@mock.patch("paasta_tools.kubernetes_tools.load_system_paasta_config", autospec=None)
@mock.patch("paasta_tools.kubernetes_tools.get_authenticating_services", autospec=None)
def test_add_volumes_for_authenticating_services(
mock_get_auth_services, mock_system_config, service, existing_config, expected
):
mock_get_auth_services.return_value = {"service_auth", "service_foobar"}
mock_system_config.return_value.get_service_auth_token_volume_config.return_value = {
"audience": "foo.bar",
"container_path": "/var/secret/something",
}
existing_config_copy = deepcopy(existing_config)
assert (
add_volumes_for_authenticating_services(
service, existing_config, "/mock/soa/dir"
)
== expected
)
mock_get_auth_services.assert_called_once_with("/mock/soa/dir")
# verifying that the method does not do in-place updates
assert existing_config == existing_config_copy
37 changes: 37 additions & 0 deletions tests/test_tron_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -817,6 +817,10 @@ def test_format_tron_action_dict_default_executor(self):
), mock.patch(
"paasta_tools.tron_tools.load_system_paasta_config",
autospec=True,
), mock.patch(
"paasta_tools.tron_tools.add_volumes_for_authenticating_services",
autospec=True,
return_value=[],
):
result = tron_tools.format_tron_action_dict(action_config)
assert result["executor"] == "kubernetes"
Expand Down Expand Up @@ -873,6 +877,12 @@ def test_format_tron_action_dict_paasta(self):
), mock.patch(
"paasta_tools.tron_tools.load_system_paasta_config",
autospec=True,
), mock.patch(
"paasta_tools.tron_tools.add_volumes_for_authenticating_services",
autospec=True,
return_value=[
{"audience": "foo.bar.com", "container_path": "/var/foo/bar"}
],
):
result = tron_tools.format_tron_action_dict(action_config)

Expand All @@ -899,6 +909,9 @@ def test_format_tron_action_dict_paasta(self):
"extra_volumes": [
{"container_path": "/nail/tmp", "host_path": "/nail/tmp", "mode": "RW"}
],
"projected_sa_volumes": [
{"audience": "foo.bar.com", "container_path": "/var/foo/bar"},
],
"field_selector_env": {"PAASTA_POD_IP": {"field_path": "status.podIP"}},
"node_selectors": {"yelp.com/pool": "special_pool"},
"labels": {
Expand Down Expand Up @@ -993,6 +1006,10 @@ def test_format_tron_action_dict_spark(self):
"paasta_tools.tron_tools.load_system_paasta_config",
autospec=True,
return_value=MOCK_SYSTEM_PAASTA_CONFIG,
), mock.patch(
"paasta_tools.tron_tools.add_volumes_for_authenticating_services",
autospec=True,
return_value=[],
), mock.patch(
"paasta_tools.tron_tools.get_k8s_url_for_cluster",
autospec=True,
Expand Down Expand Up @@ -1295,6 +1312,10 @@ def test_format_tron_action_dict_paasta_k8s_service_account(self):
), mock.patch(
"paasta_tools.tron_tools.load_system_paasta_config",
autospec=True,
), mock.patch(
"paasta_tools.tron_tools.add_volumes_for_authenticating_services",
autospec=True,
return_value=[],
):
result = tron_tools.format_tron_action_dict(action_config)

Expand Down Expand Up @@ -1415,6 +1436,10 @@ def test_format_tron_action_dict_paasta_k8s(
"paasta_tools.secret_tools.is_shared_secret_from_secret_name",
autospec=True,
return_value=False,
), mock.patch(
"paasta_tools.tron_tools.add_volumes_for_authenticating_services",
autospec=True,
return_value=[],
):
result = tron_tools.format_tron_action_dict(action_config)

Expand Down Expand Up @@ -1522,6 +1547,10 @@ def test_format_tron_action_dict_paasta_no_branch_dict(self):
), mock.patch(
"paasta_tools.tron_tools.load_system_paasta_config",
autospec=True,
), mock.patch(
"paasta_tools.tron_tools.add_volumes_for_authenticating_services",
autospec=True,
return_value=[],
):
result = tron_tools.format_tron_action_dict(action_config)
assert result == {
Expand Down Expand Up @@ -1682,6 +1711,10 @@ def test_create_complete_config_e2e(self, tmpdir):
"paasta_tools.utils.load_system_paasta_config",
autospec=True,
return_value=MOCK_SYSTEM_PAASTA_CONFIG,
), mock.patch(
"paasta_tools.tron_tools.add_volumes_for_authenticating_services",
autospec=True,
return_value=[],
):
tronfig = tron_tools.create_complete_config(
service="fake_service",
Expand Down Expand Up @@ -1732,6 +1765,10 @@ def test_override_default_pool_override(self, tmpdir):
"paasta_tools.utils.load_system_paasta_config",
autospec=True,
return_value=MOCK_SYSTEM_PAASTA_CONFIG_OVERRIDES,
), mock.patch(
"paasta_tools.tron_tools.add_volumes_for_authenticating_services",
autospec=True,
return_value=[],
):
tronfig = tron_tools.create_complete_config(
service="fake_service",
Expand Down
Loading