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

Workload identity #2460

Merged
merged 151 commits into from
May 20, 2024
Merged

Workload identity #2460

merged 151 commits into from
May 20, 2024

Conversation

Adam-D-Lewis
Copy link
Member

@Adam-D-Lewis Adam-D-Lewis commented May 10, 2024

Reference Issues or PRs

Adds the ability to use workload identity on Azure similar to what we have with IRSA on the AWS provider. This allows us to tie a k8S service account to a Azure managed identity so we can get managed identity credentials into a pod simply by adding a label and service account to the pod and an annotation on the service account. I'm making mlflow Nebari plugin and I'm using it to give access to a blob storage bucket to mlflow.

Basically just adds a bit more possible customization to the AKS cluster, and this customization is disabled by default.

What does this implement/fix?

Put a x in the boxes that apply

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds a feature)
  • Breaking change (fix or feature that would cause existing features not to work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe):

Testing

  • Did you test the pull request on AKS?
  • Did you add new tests?

Any other comments?

src/_nebari/keycloak.py Outdated Show resolved Hide resolved
src/_nebari/config.py Outdated Show resolved Hide resolved
@@ -144,7 +153,7 @@ class Ingress(schema.Base):

class InputSchema(schema.Base):
domain: Optional[str] = None
Copy link
Member Author

Choose a reason for hiding this comment

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

revert changes to this file

src/nebari/schema.py Outdated Show resolved Hide resolved
src/nebari/plugins.py Outdated Show resolved Hide resolved
@Adam-D-Lewis
Copy link
Member Author

Adam-D-Lewis commented May 17, 2024

Looks like it's failing due to playwright tests due to the UI changes, but I'd like to merge while we work on fixing playwright tests.

@viniciusdc
Copy link
Contributor

viniciusdc commented May 17, 2024

Hi @Adam-D-Lewis, can you open an issue on docs to add a simple example on how to use this? I can add a PR later based on that.

@@ -144,7 +144,7 @@ resource "kubernetes_manifest" "forwardauth-middleware" {
apiVersion = "traefik.containo.us/v1alpha1"
kind = "Middleware"
metadata = {
name = "traefik-forward-auth"
name = var.forwardauth_middleware_name
Copy link
Contributor

@viniciusdc viniciusdc May 17, 2024

Choose a reason for hiding this comment

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

If I got this correctly, you just created an output for the traefik-forward-auth and you are calling the components here onwards, right?

Copy link
Member Author

@Adam-D-Lewis Adam-D-Lewis May 17, 2024

Choose a reason for hiding this comment

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

We previously hard coded the forward auth middleware name. I added a variable for it (not strictly necessary for what I needed with mlflow plugin). I also created an output for whatever name is chosen. This was necessary for an mlflow plugin. I needed to put authentication in front of the mlflow pod so I re-use the existing forward auth service, middleware, and pod.
See https://github.com/Quansight/nebari-mlflow-plugin/blob/0eeb522380e635ac4c5ac6d3ba843427acc6923f/src/nebari_plugin_mlflow_aws/__init__.py#L207-L221

Copy link
Contributor

Choose a reason for hiding this comment

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

cool! The reason I asked was because DaskGateway also uses it, so I just wanted to make sure that the service would still work

@viniciusdc viniciusdc added provider: Azure status: in review 👀 This PR is currently being reviewed by the team labels May 17, 2024
Copy link
Contributor

@viniciusdc viniciusdc left a comment

Choose a reason for hiding this comment

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

All looks good to me, though I just would like to confirm why the changes on the forward auth were required (liked how it is now), and a single doubt regarding the OIDC behavior

Comment on lines +21 to +24
output "cluster_oidc_issuer_url" {
description = "The OpenID Connect issuer URL that is associated with the AKS cluster"
value = azurerm_kubernetes_cluster.main.oidc_issuer_url
}
Copy link
Contributor

Choose a reason for hiding this comment

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

When oidc_issuer_enabled is False, does this output return null, or does terraform raises an error?

Copy link
Member Author

Choose a reason for hiding this comment

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

It returns null

@Adam-D-Lewis
Copy link
Member Author

Hi @Adam-D-Lewis, can you open an issue on docs to add a simple example on how to use this? I can add a PR later based on that.

Hey Vini, check this out. I explained the process of using workload identity. nebari-dev/nebari-docs#461

@Adam-D-Lewis
Copy link
Member Author

All looks good to me, though I just would like to confirm why the changes on the forward auth were required (liked how it is now), and a single doubt regarding the OIDC behavior

Cool I believe I addressed all this in the other comments. Let me know if there's anything else.

@Adam-D-Lewis Adam-D-Lewis requested a review from viniciusdc May 17, 2024 19:29
Copy link
Contributor

@viniciusdc viniciusdc left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @Adam-D-Lewis !

@Adam-D-Lewis Adam-D-Lewis merged commit 43fb770 into develop May 20, 2024
26 of 27 checks passed
@Adam-D-Lewis Adam-D-Lewis deleted the workload_identity branch May 20, 2024 21:21
aktech pushed a commit that referenced this pull request May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: review 👀 This PR is complete and ready for reviewing provider: Azure status: in review 👀 This PR is currently being reviewed by the team
Projects
Development

Successfully merging this pull request may close these issues.

4 participants