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.

Sign up for GitHub

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

Jump to bottom

✨ Adding a new field to clustermanager API to pass list of aws tags to be added to aws resources #358

Merged
merged 3 commits into from
Feb 26, 2025
Merged

✨ Adding a new field to clustermanager API to pass list of aws tags to be added to aws resources #358

merged 3 commits into from
Feb 26, 2025
+141 −26

Conversation

jaswalkiranavtar
Copy link
Member

Summary

Tags are required on all resources for cost attribution and identification purpose.

Related issue(s)

Fixes #open-cluster-management-io/ocm#514

@jaswalkiranavtar
Adding aa new field to clustermanager to pass list of aws tags to be …

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
Loading
Loading status checks…
3639134 
…added to aws resources

Signed-off-by: Gaurav Jaswal <jaswalkiranavtar@gmail.com>
@openshift-ci openshift-ci bot requested review from mdelder and qiujian16 February 24, 2025 16:50
mikeshng
mikeshng reviewed Feb 24, 2025
View reviewed changes
Copy link
Member

@mikeshng mikeshng left a comment

Choose a reason for hiding this comment

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

Left some nit comments.

I am ok with this but maybe consider the option of just use the cluster manager CR's labels/annotations (maybe follow some predefined regex/key). Let see what Qiu Jian thinks.

operator/v1/types_clustermanager.go Outdated
@@ -125,7 +125,7 @@ type RegistrationDriverHub struct {
// +kubebuilder:validation:Enum=csr;awsirsa
AuthType string `json:"authType,omitempty"`

// This represents the hub cluster ARN
// This represents the hub cluster ARN. Applicable to only awsirsa authentication type.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Could you group the Applicable to only awsirsa authentication type fields together.

To me the order should be: AuthType, AutoApprovedIdentities, HubClusterArn, Tags

Copy link
Member

Choose a reason for hiding this comment

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

I can't resolve this thread. Please feel free to resolve it if you are able to. Thanks.

operator/v1/types_clustermanager.go

// List of tags to be added to AWS resources created by hub while processing awsirsa registration request.
// Applicable to only awsirsa authentication type.
// Example - "product:v1:tenant:app-name=My-App"
Copy link
Member

Choose a reason for hiding this comment

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

I am just wondering is app-name=My-App close to the real usage of this field? If it is, please ignore this comment. Is better to have example like primary/secondary hub? Just guessing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes appname is the real usage, we have tags like appname, createdby, deptname etc

We are planning to reuse same roles for both primary and secondary, which is why we are not adding primary/secondary for now.

Copy link
Member

Choose a reason for hiding this comment

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

I can't resolve this thread. Please feel free to resolve it if you are able to. Thanks.

@jaswalkiranavtar
Copy link
Member Author

Left some nit comments.

I am ok with this but maybe consider the option of just use the cluster manager CR's labels/annotations (maybe follow some predefined regex/key). Let see what Qiu Jian thinks.

We thought about it. In our case the keys for k8s labels and aws tags and complete different to the extent that regex cannot be used to replace them. Otherwise it was easier for us to just copy the labels from registration controller and use them as tags.

@qiujian16
Copy link
Member

hrm I was thinking the API should look like:

drivers:
- type: csr
   csr:
       approveIdentities:
       - user1
- type: awsisra
   awsisra:
        approvedIdentities:
        - arn:.....
        tag:
        - .....

It means in RegistrationDriverHub we had a wrapped configuration struct for different drivers, which easily separated the configurations for different drivers.

@qiujian16
Copy link
Member

#359 as an example

@alex0chan
move configuration to each driver struct
Loading
Loading status checks…
a47331a 
Signed-off-by: Alex <alexchan2988@gmail.com>
mikeshng
mikeshng reviewed Feb 25, 2025
View reviewed changes
operator/v1/types_clustermanager.go

// AwsIrsa represents the configuration for awsisra driver.
// +optional
AwsIrsa *AwsIrsaConfig `json:"awsisra,omitempty"`
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
AwsIrsa *AwsIrsaConfig `json:"awsisra,omitempty"`
AwsIrsa *AwsIrsaDriverConfig `json:"awsisra,omitempty"`

To match the pattern of above CSRDriverConfig. I think both should either include "Driver" or both should omit it. Same comment for the other occurrences.

@alex0chan
standardizing registration driver names
Loading
Loading status checks…
b1c4e9c 
Signed-off-by: Alex <alexchan2988@gmail.com>
mikeshng
mikeshng approved these changes Feb 25, 2025
View reviewed changes
Copy link
Member

@mikeshng mikeshng left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Feb 25, 2025
@qiujian16
Copy link
Member

/approve

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Feb 26, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jaswalkiranavtar, mikeshng, qiujian16

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

@openshift-merge-bot openshift-merge-bot bot merged commit 8c97932 into open-cluster-management-io:main Feb 26, 2025
10 checks passed
@jaswalkiranavtar jaswalkiranavtar deleted the feature/GWCP-72203 branch March 20, 2025 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikeshng mikeshng

@mdelder mdelder

@qiujian16 qiujian16

Assignees

@mikeshng mikeshng

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jaswalkiranavtar @qiujian16 @mikeshng @alex0chan