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

add create-kubeconfigs command #545

Closed

Conversation

mplsgrant
Copy link
Collaborator

I transmuted the setup_user_contexts.sh script into python code.

I also made it so that it would query all namespaces that start "warnet-" looking for service accounts,
and then create token credentials for those accounts.

Currently, user namespaces must start with "warnet-".

Also, I included the commits from #544 and #543 because they are necessary for this.

Copy link
Collaborator

@josibake josibake left a comment

Choose a reason for hiding this comment

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

+1 for replacing another bash script! LGTM, I think we can merge as is in the interest of time, but would be nice to leave some TODO hints for follow ups.

@@ -1,4 +1,5 @@
import os
import subprocess
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need subprocess? iirc, we have a warnet command runner (run_command, or stream_command?)



# Get all namespaces that start with "warnet-"
def get_warnet_namespaces():
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we are going to start having conventions around namespace strings (e.g., warnet-), might be more useful to make this a "get_namespaces_by_prefix(prefix: str)" function. not urgent, though

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @josibake but more strongly. I think we should avoid using any naming conventions like we did before helm (if possible) -- can we use kubernetes metadata "labels" instead?

# Create a kubeconfig file for the user
kubeconfig_file = os.path.join(kubeconfig_dir, f"{sa}-{namespace}-kubeconfig")

kubeconfig_content = f"""apiVersion: v1
Copy link
Collaborator

Choose a reason for hiding this comment

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

works for now, but ideally we keep the k8s yamls out of the python code. maybe just leave a #TODO comment to move this at some point ?

@bdp-DrahtBot
Copy link
Collaborator

bdp-DrahtBot commented Sep 7, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Users need access to a number of resources so that they can run scenarios.
I transmuted the setup_user_contexts.sh script into python code.

I also made it so that it would query all namespaces that start "warnet-" looking for service accounts,
and then create token credentials for those accounts.

Currently, user namespaces must start with "warnet-".
@mplsgrant mplsgrant force-pushed the 2024-09-create-kubeconfigs branch from 3d223cb to 1d583d0 Compare September 7, 2024 15:34
@mplsgrant
Copy link
Collaborator Author

I added your comments as TODOs

@pinheadmz
Copy link
Contributor

Pretty sure I understand what this is doing but it would be nice to have a test or docs to read so I know who to try it out myself, especially this week with our pre-tabconf trials

@josibake
Copy link
Collaborator

josibake commented Sep 9, 2024

Pretty sure I understand what this is doing but it would be nice to have a test or docs to read

I'll take these tomorrow and 1) add the changes I proposed in #543 and 2) add a README on how to deploy namespaces for multiple teams.

As an fyi, this all works today, these PRs are just setting/documenting sane defaults in the yaml files, so should be a very easy change.

@josibake
Copy link
Collaborator

will rebase this on master now that #543 is merged and address the outstanding comments from @pinheadmz and @me

@josibake
Copy link
Collaborator

This is continued in #598

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants