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

feat: add ktutil and a helper script to our images #720

Merged
merged 4 commits into from
Jan 14, 2025

Conversation

mathis-marcotte
Copy link
Contributor

@mathis-marcotte mathis-marcotte commented Jan 3, 2025

Description

for https://jirab.statcan.ca/browse/BTIS-700

Tests / Quality Checks

Can be tested by running "ktutil-keytab" in a terminal in a running notebook

Are there breaking changes?

Ask yourself the next question;

  • Do we want to maintain the previous image from which we had to do breaking changes from?

If no, then carry on. If yes, there is a breaking change and we want to maintain the previous image do the following

  • Create a new branch for the current version (ex v1) based off the current master/main branch
  • Increment the tag in the CI for pushes to master/main (v1 to v2)
  • Change the CI that on pushes to the newly created "v1" branch (the name of the newly created branch we want to maintain is) it will push to the ACR.

Automated Testing/build and deployment

  • Does the image pass CI successfully (build, pass vulnerability scan, and pass automated test suite)?
  • If new features are added (new image, new binary, etc), have new automated tests been added to cover these?
  • If new features are added that require in-cluster testing (e.g. a new feature that needs to interact with kubernetes), have you added the auto-deploy tag to the PR before pushing in order to build and push the image to ACR so you can test it in cluster as a custom image?

JupyterLab extensions

  • Are all extensions "enabled" (jupyter labextension list from inside the notebook)?

VS Code tests

  • Does VS Code open?
  • Can you install extensions?

Code review

  • Have you added the auto-deploy tag to your PR before your most recent push to this repo? This causes CI to build the image and push to our ACR, letting reviewers access the built image without having to create it themselves
  • Have you chosen a reviewer, attached them as a reviewer to this PR, and messaged them with the SHA-pinned image name for the final image to test on the dev cluster (e.g. k8scc01covidacrdev.azurecr.io/jupyterlab-cpu:746d058e2f37e004da5ca483d121bfb9e0545f2b)?

@mathis-marcotte mathis-marcotte added the auto-deploy Trigger manual CI steps for this PR label Jan 3, 2025
@mathis-marcotte mathis-marcotte marked this pull request as ready for review January 9, 2025 19:07
Copy link
Contributor

@wg102 wg102 left a comment

Choose a reason for hiding this comment

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

Just one comment and if you could confirm the following:
This will trigger for everyone and every notebook?

@mathis-marcotte
Copy link
Contributor Author

This script command will be available on every notebook type

Copy link
Contributor

@wg102 wg102 left a comment

Choose a reason for hiding this comment

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

Random not a specific place to write comments:

  1. What if the user enters the wrong password? Does it update?
  2. What if they use the keytab with a wrong password, what happens
  3. is one keytab valid for all notebooks, (I think so) if so, does the file appear/is visible without the user needing to do the command.
  4. If not, does it mean only one is valid at a time
  5. I don't think this is related to your ticket but any chance some of this can be cleaned up :
    image
  6. What if the notebook is not persistent?


# add script for kerberos keytab creation
COPY ktutil-keytab.sh /usr/local/bin/ktutil-keytab
RUN chmod +x /usr/local/bin/ktutil-keytab
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a prompt/instructions for the user to know to run this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a ticket to define those instructions in the docs to let users know what to do
https://jirab.statcan.ca/browse/BTIS-686

@mathis-marcotte
Copy link
Contributor Author

  1. What if the user enters the wrong password? Does it update?
    Yes it does. This script doesn't do any actual authentication. I don't know if there is a way to even do that. This basically just creates their credentials that will be used when it is time to authenticate

  2. What if they use the keytab with a wrong password, what happens
    Same question as 1? So same answer

  3. is one keytab valid for all notebooks, (I think so) if so, does the file appear/is visible without the user needing to do the command.
    Yes. Once the command is run, it creates the file under /~/krb5, so yes it can be visible. This new command doesn't display the keytab. (Users shouldn't need to see this file anyways)

  4. If not, does it mean only one is valid at a time
    Irrelevant based on previous question's answer. But one thing to note, the keytab becomes invalid if a user's password changes. So when their netA password changes, they will need to run this again.

  5. I don't think this is related to your ticket but any chance some of this can be cleaned up
    Possibly.

  6. What if the notebook is not persistent?
    Not sure what this means. The command creates a k8s secret with the keytab value. We have big problems if somehow, k8s secrets aren't persisted.

@mathis-marcotte mathis-marcotte merged commit ad268e9 into master-2.0 Jan 14, 2025
3 checks passed
@mathis-marcotte mathis-marcotte deleted the keytab-shell-script branch January 14, 2025 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-deploy Trigger manual CI steps for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants