[WIP]: Remote Cluster

[Shrinjay]

No description provided.

[DerekTBrown]

Nit: I would be consistent with the method used for env fetching. I don't think there is a big difference, but I think it is wise to pick one or another.

Suggested change

	kubeconfig_path = os.environ.get('EG_REMOTE_CLUSTER_KUBECONFIG_PATH', '/etc/kube/config')
	context = os.environ.get('EG_REMOTE_CLUSTER_CONTEXT', None)
	kubeconfig_path = os.getenv('EG_REMOTE_CLUSTER_KUBECONFIG_PATH', '/etc/kube/config')
	context = os.getenv('EG_REMOTE_CLUSTER_CONTEXT', None)

[DerekTBrown]

Did you pick the /etc/kube/config default, or did this come from something existing?

[Shrinjay]

I picked that, honestly by random, I realize I forgot to set this as the default in the helm chart as well, but the idea was for the path to be the same in the helm chart and in code

[DerekTBrown]

Can you document what this does and also what the effect of get_remote_if_available is?

[DerekTBrown]

Nit: f-strings for the win

Suggested change

	"Created kubernetes client for host {host}".format(host=kubernetes_config.host)
	f"Created kubernetes client for host {kubernetes_config.host}"

[DerekTBrown]

(More of a meta point than a specific set of lines)

Slash continuations in Python are pretty evil. Was this something you put in manually, or something the formatter did for you?

You might want to put these on one (really long) line, and then use black to find a solution that doesn't involve slashes. I believe the project uses black anyway, so it is a good idea to run that over your code from the getgo.

[Shrinjay]

PyCharm did it when I tried to chain the invocations over multiple lines like I usually do in js/ts, honestly wasn't aware slash continuations were a problem until I just googled it, so thanks for pointing this one out

[DerekTBrown]

1. Super super good on you for using functional programming here. This is a senior dev move.

2. We can make this an even more big-headed by using a generator comprehension. The list/map method is on the order of n in terms of memory complexity and roughly 2n in terms of time complexity (we build a list of length n which takes n time, then we scan the list which takes n time). The lambda also requires constructing a function context, which can be expensive. The generator method is on the order of o(1) in terms of memory complexity and takes only n in terms of time complexity (we just perform a single scan of the other list without actually constructing anything).

Suggested change

	service_account_names_in_namespace: List[str] = list(map(lambda svcaccount: svcaccount.metadata.name, service_accounts_in_namespace))
	if service_account_name not in service_account_names_in_namespace:
	service_account_names_in_namespace = (svcaccount.metadata.name for svcaccount in service_accounts_in_namespace)
	if service_account_name not in service_account_names_in_namespace:

[DerekTBrown]

At this point, you are probably thinking "why do we care about performance, this is not that performance intensive?"

This has less to do with performance and more to do with code quality; we want the Juypter reviewers to think "damn, thats some slick code, they must know what they are doing."

[Shrinjay]

Oh I love finding out about optimizations like this, I honestly don't know enough about Python for how much I use it, so thanks for teaching me about this 😄

[Shrinjay]

Oh it's list comprehension... darn I should've remembered about that, I keep defaulting to the list-map combo, probably because I use .map a lot in TS

[DerekTBrown]

Same as above RE: generator comprehension

[DerekTBrown]

A few thoughts:

1. I don't think we (Lacework) want to grant a ClusterRole to JEG. At most, I think we want to grant roles to a handful of namespaces.
2. I don't think we should be copying a role from one place to another, this seems non-transparent.
3. I would recommend adding a static rolebinding to a specific namespace.
4. I would recommend adding an option to not create the service account, allowing the user to supply their own service account. This lets people customize as they choose.

[DerekTBrown]

I am not sure this works as expected. This will evaluate to true if EG_USE_REMOTE_CLUSTER is set to anything (including EG_USE_REMOTE_CLUSTER=False).

We should confirm this has the same semantics as other env options defined in JEG. Do they look for True/False or just Set/Unset. If True/False, we should add a more rigorous check. If set/unset, we should use EG_USE_REMOTE_CLUSTER=1 in the helm chart to denote this.

[Shrinjay]

The reason I added the copying was more as a first pass than anything. The issue is we want the role created in the remote cluster to be configurable via Helm. The only way to inject configurations from helm into runtime is environment variables. I’m not quite sure if we want to configure the role for the remote using environment variables, hence the copying. The role in the local space is defined by the helm chart so we just copy that over.

…

On Wed, Jan 25, 2023 at 4:23 PM Derek Brown ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In enterprise_gateway/services/processproxies/k8s.py <#1 (comment)> : > + if service_account_name not in service_account_names_in_namespace: + service_account_metadata = { + "name": service_account_name + } + service_account_to_create: client.V1ServiceAccount = client.V1ServiceAccount( + kind="ServiceAccount", + metadata=service_account_metadata + ) + + client\ + .CoreV1Api(api_client=kubernetes_client)\ + .create_namespaced_service_account(namespace=namespace, body=service_account_to_create) + + self.log.info(f"Created service account {service_account_name} in namespace {namespace}") + + def _forward_role_to_remote(self) -> None: A few thoughts: 1. I don't think we (Lacework) want to grant a ClusterRole to JEG. At most, I think we want to grant roles to a handful of namespaces. 2. I don't think we should be copying a role from one place to another, this seems non-transparent. 3. I would recommend adding a static rolebinding to a specific namespace. 4. I would recommend adding an option to *not* create the service account, allowing the user to supply their own service account. This lets people customize as they choose. — Reply to this email directly, view it on GitHub <#1 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AO5NQAWJ6YWC2OZ6RNLSOOTWUGYXTANCNFSM6AAAAAAUFVOHUI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[Shrinjay]

I realized I'm an idiot and configmaps exist... we can mount the local role resource definitions as a configmap and then create it in the remote rather than copying the role.