argoproj / argo-workflows

Workflow Engine for Kubernetes
https://argo-workflows.readthedocs.io/
Apache License 2.0
15.11k stars 3.21k forks source link

fix: bump minio-go to version that supports eks pod identity #13800 #13854

Closed blairdrummond closed 2 weeks ago

blairdrummond commented 2 weeks ago

Fixes #13800

Motivation

I deployed Argo Workflows with Pod Identity, then discovered that artifacts weren't working. Debugging led me to the same conclusion as #13800 . I rebuilt the argoexec image with this patch, and voila. Simply bumped to the minio-go version in argoproj/pkg.

Modifications

Verification

I pushed the argoexec image with this change into our dev environment and my artifacts started working.

tooptoop4 commented 2 weeks ago

@blairdrummond rebase to get lint fix

shuangkun commented 2 weeks ago

Can you give an example of using Pod Identity?

blairdrummond commented 2 weeks ago

For sure --- I'm using it to partition my s3 bucket by kubernetes namespace in a multi-tenant cluster. Every service account is allowed to access its own namespace folder within the bucket. The attribute based access control aspects of pod identities are great for this.

resource "aws_s3_bucket" "artifacts" {
  bucket = "<redacted>"
}

resource "aws_iam_role" "argo_artifact_manager" {
  name = "argo-artifact-manager"
  assume_role_policy = jsonencode({
    Version = "2012-10-17"
    Statement = [
      {
        Action = [
          "sts:AssumeRole",
          "sts:TagSession"
        ]
        Effect = "Allow"
        Principal = {
          Service = "pods.eks.amazonaws.com"
        }
      }
    ]
  })

  inline_policy {
    name = "argo-tenant-controller-policy"
    policy = jsonencode({
      Version = "2012-10-17"
      Statement = [
        {
            Sid = "AllowListS3ActionsOnSpecifiedBucket"
            Effect = "Allow"
            Action = [
                "s3:ListBucket",
                "s3:GetBucketVersioning",
                "s3:GetBucketAcl",
                "s3:GetBucketLogging",
                "s3:GetEncryptionConfiguration",
                "s3:GetBucketPolicy",
                "s3:GetBucketPublicAccessBlock",
            ]
          Resource = "arn:aws:s3:::${aws_s3_bucket.artifacts.bucket}"
        },
        {
            "Sid": "AllowGetAndPutS3ActionsOnSpecifiedBucketPath",
            "Effect": "Allow",
            "Action": [
                "s3:PutObject",
                "s3:GetObject"
            ],
          "Resource": "arn:aws:s3:::${aws_s3_bucket.artifacts.bucket}/artifacts/$${aws:PrincipalTag/kubernetes-namespace}/*"
        }
      ]
    })
  }
}
apiVersion: v1
data:
  artifactRepository: |
    archiveLogs: true
    s3:
      endpoint: s3.amazonaws.com
      bucket: <redacted>
      region: us-east-1
      insecure: false
      keyFormat: "artifacts/{{workflow.namespace}}\
        /{{workflow.creationTimestamp.Y}}\
        /{{workflow.creationTimestamp.m}}\
        /{{workflow.creationTimestamp.d}}\
        /{{workflow.name}}\
        /{{pod.name}}"
  workflowDefaults: |
    spec:
      serviceAccountName: workflow-executor
      ttlStrategy:
        secondsAfterSuccess: 5
      parallelism: 3
kind: ConfigMap
metadata:
  name: workflow-controller-configmap
  namespace: argo-workflows

I'm using a controller to automatically create the pod identity associations to this role for any k8s service account with a label. All the security is really implemented using the principal tags in the IAM policy, it's safe in this design to create the pod identity association to anyone who asks. If you want to create that pod identity association statically as a test, you can use this

resource "aws_eks_pod_identity_association" "argo_tenant_controller" {
  cluster_name    = var.cluster.cluster_name
  namespace       = var.namespace
  service_account = var.service_account
  role_arn        = aws_iam_role.argo_tenant_controller.arn
}

Something I haven't sorted out is how to grant the UI access to the artifacts yet. Will cross that bridge when I come to it

chubei-urus commented 2 weeks ago

Thank you @blairdrummond for the PR!