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

✨ Aws iam auth api change request #339

Conversation

suvaanshkumar
Copy link
Contributor

Summary

Related issue(s)

Fixes #

@suvaanshkumar suvaanshkumar changed the title Aws iam auth api change request ✨ Aws iam auth api change request Aug 23, 2024
@qiujian16
Copy link
Member

qiujian16 commented Aug 23, 2024

you would need to update the types.go at first, and then run make update to generate crd and client code. Ensure to run make verify after make update. Here https://github.com/open-cluster-management-io/api/blob/main/docs/development.md has some infos.

@EricaJ6
Copy link

EricaJ6 commented Aug 27, 2024

Thanks @qiujian16 for the suggestions. Please take a look at the updated changes with required changes.


// Contain the details required for registering with eks hub cluster using IAM roles for service account.
// This is required only when the authType is awsirsa.
AwsIrsa AwsIrsa `json:"awsIrsa,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

this should be the pointer

// The arn of the EKS hub cluster. This will be required to pass information to hub, which hub will use to create IAM identities for this klusterlet.
// Example - arn:eks:us-west-2:12345678910:cluster/hub-cluster1.
// +required
EksHubClusterArn string `json:"eksHubClusterArn"`
Copy link
Member

Choose a reason for hiding this comment

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

maybe just hubClusterArn

}

type AwsIrsa struct {
// The arn of the EKS hub cluster. This will be required to pass information to hub, which hub will use to create IAM identities for this klusterlet.
Copy link
Member

Choose a reason for hiding this comment

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

@qiujian16 do we still need to mention EKS here specifically after your field rename suggestion? Should we do something like: "The arn of the hub cluster (ie: an EKS cluster)."

Copy link

Choose a reason for hiding this comment

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

This arn is specific to the EKS cluster only. I can update the description to The arn of the hub cluster (ie: an EKS cluster).

// +kubebuilder:validation:Enum=csr;awsirsa
AuthType string `json:"authType"`

// Contain the details required for registering with eks hub cluster using IAM roles for service account.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Contain the details required for registering with eks hub cluster using IAM roles for service account.
// Contain the details required for registering with hub cluster (ie: an EKS cluster) using AWS IAM roles for service account.

I think it's better to be consistent and use EKS in upper case in all the places.

@EricaJ6 EricaJ6 force-pushed the AWS-IAM-Auth_API-change_request branch from 168d77a to 912d545 Compare August 28, 2024 14:25
@qiujian16
Copy link
Member

/approve

@mikeshng @elgnay would you take the review and tag?

Copy link
Contributor

openshift-ci bot commented Aug 29, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16, suvaanshkumar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@elgnay
Copy link
Contributor

elgnay commented Aug 30, 2024

LGTM

@qiujian16
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Aug 30, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit a19871c into open-cluster-management-io:main Aug 30, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants