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 support for archival #425

Merged
merged 4 commits into from
Aug 9, 2023
Merged

Conversation

alexandrevilain
Copy link
Owner

@alexandrevilain alexandrevilain commented Jul 19, 2023

This PR closes #364

@alexandrevilain alexandrevilain changed the title Add support for archival WIP: Add support for archival Jul 19, 2023
@alexandrevilain alexandrevilain force-pushed the feat/add-support-for-archival branch 2 times, most recently from 5130a1d to 7dbf245 Compare July 24, 2023 17:01
@yujunz
Copy link
Contributor

yujunz commented Jul 26, 2023

@alexandrevilain do you have an ETA for archival support? It is mandatory for our production deployment.

Alternatively, we can implement #418 but I guess that will take longer.

@alexandrevilain
Copy link
Owner Author

Hi @yujunz ! Happy to read that you’re deploying this project in production!

Even if we are using this operator in production at work, it’s not my daily job to maintain it.

So I’m sorry but I can’t give you any ETA. Maybe during the following weeks, but not 100% confident about that.

@yujunz
Copy link
Contributor

yujunz commented Jul 26, 2023

Understood. We just need to make decisions on either turning to helm chart for flexibility and features completeness or sticking to operator to reduce maintenance overhead in the long term.

A few weeks fit our schedule well. Let me know if anything I can help to accelerate the delivery.

@alexandrevilain alexandrevilain force-pushed the feat/add-support-for-archival branch from 7dbf245 to 96d69ae Compare July 26, 2023 18:12
@alexandrevilain
Copy link
Owner Author

@yujunz Are you using EKS ? I'll need some testers to check if IAM authentication will work for archival :)

@yujunz
Copy link
Contributor

yujunz commented Jul 27, 2023

@yujunz Are you using EKS ? I'll need some testers to check if IAM authentication will work for archival :)

Yeah, glad to help. Let me know when it is ready for testing.

@alexandrevilain alexandrevilain force-pushed the feat/add-support-for-archival branch 3 times, most recently from e10ec82 to 4075a9c Compare July 29, 2023 13:34
@alexandrevilain alexandrevilain force-pushed the feat/add-support-for-archival branch from 4075a9c to 8961bc4 Compare July 29, 2023 15:12
@DodgeCamaro
Copy link
Contributor

DodgeCamaro commented Jul 31, 2023

@alexandrevilain I can help you with test IAM at AWS EKS

@alexandrevilain
Copy link
Owner Author

Hi @yujunz and @DodgeCamaro !
I’ve tried this feature with OVHCloud’s s3 works like a charm!
If you have time could you please try it with EKS + IAM role?
If it works could you please provide me a role example so I can provide it in the documentation? Thanks!

@DodgeCamaro
Copy link
Contributor

DodgeCamaro commented Aug 3, 2023

example of trust_relationships

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Principal": {
                "Federated": "arn:aws:iam::<account_id>:oidc-provider/oidc.eks.<aws_region>.amazonaws.com/id/<cluster_id>"
            },
            "Action": "sts:AssumeRoleWithWebIdentity",
            "Condition": {
                "StringEquals": {
                    "oidc.eks.<aws_region>.amazonaws.com/id/<cluster_id>:sub": "system:serviceaccount:temporal:temporal-frontend"
                }
            }
        }
    ]
}

s3 policy example

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Principal": {
                "AWS": "arn:aws:iam::<account_id>:role/<iam_role>"
            },
            "Action": [
                "s3:PutObjectAcl",
                "s3:PutObject",
                "s3:GetObjectVersion",
                "s3:GetObject",
                "s3:DeleteObject"
            ],
            "Resource": "arn:aws:s3:::<bucket_name>/*"
        },
        {
            "Effect": "Allow",
            "Principal": {
                "AWS": "arn:aws:iam::<account_id>:role/<iam_role>"
            },
            "Action": "s3:ListBucket",
            "Resource": "arn:aws:s3:::<bucket_name>"
        }
    ]
}

@DodgeCamaro
Copy link
Contributor

DodgeCamaro commented Aug 3, 2023

Update example at this PR #445
Currently, I've testing archival and waiting when retention period move my workflow to archival

UPD: At k8s serviceAccount have same Annotations
eks.amazonaws.com/role-arn=arn:aws:iam::<account-id>:role/temporal-frontend-role
It won't work, because trust_relationships have conditions only from namespace - temporal, and SA temporal-frontend

@alexandrevilain
Copy link
Owner Author

alexandrevilain commented Aug 5, 2023

It won't work, because trust_relationships have conditions only from namespace - temporal, and SA temporal-frontend

I don't understand. What's missing on the operator side ? A service account per service type ?

@DodgeCamaro
Copy link
Contributor

DodgeCamaro commented Aug 7, 2023

All temporal services have their own ServiceAccount.
It can be fixed by adding SA to IAM Role trust_relationships
Example for add to doc

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Principal": {
                "Federated": "arn:aws:iam::<account_id>:oidc-provider/oidc.eks.<aws_region>.amazonaws.com/id/<cluster_id>"
            },
            "Action": "sts:AssumeRoleWithWebIdentity",
            "Condition": {
                "StringEquals": {
                    "oidc.eks.<aws_region>.amazonaws.com/id/<cluster_id>:sub": ["system:serviceaccount:<temporal_ns>:<temporal_sa>"]
                }
            }
        }
    ]
}

@alexandrevilain
Copy link
Owner Author

Alright, so with both iam role on ServiceAccount + trust relationship it works well ? No need to update the operator code ?

@DodgeCamaro
Copy link
Contributor

Alright, so with both iam role on ServiceAccount + trust relationship it works well ? No need to update the operator code ?

Don't need any update from operator code

@alexandrevilain
Copy link
Owner Author

Thanks, last test step for me is GCS

@alexandrevilain alexandrevilain changed the title WIP: Add support for archival Add support for archival Aug 9, 2023
@alexandrevilain alexandrevilain marked this pull request as ready for review August 9, 2023 11:33
@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@alexandrevilain alexandrevilain merged commit 2da8530 into main Aug 9, 2023
@alexandrevilain alexandrevilain deleted the feat/add-support-for-archival branch August 9, 2023 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Archival for backs up Workflow Execution Event Histories and Visibility data
4 participants