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

feat: Add support for IncludeLinkedAccounts Options #1466

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gtn3010
Copy link

@gtn3010 gtn3010 commented Jul 8, 2024

Add support for IncludeLinkedAccounts Options
Related to #861

@tuananh
Copy link

tuananh commented Jul 9, 2024

@cristiangreco could you help taking a look at this? if you have other idea how to do this, please let us know. we will update accordingly.

@tuananh
Copy link

tuananh commented Jul 25, 2024

@cristiangreco friendly ping. could you take a look at this?

@tektondeploy tektondeploy force-pushed the master branch 5 times, most recently from 5bb4f5d to 1b33efc Compare August 29, 2024 09:55
@tuananh
Copy link

tuananh commented Aug 29, 2024

@cristiangreco anything we need to update to get this in?

@l-baszak
Copy link

l-baszak commented Oct 2, 2024

@cristiangreco Any update on this? It is really needed.

@cristiangreco
Copy link
Contributor

This PR needs to be rebased before it can be reviewed. Additionally, please extend the documentation to i) describe the new config option and ii) describe more generally why/when this is useful.

@tektondeploy tektondeploy force-pushed the master branch 2 times, most recently from cb058f0 to 2ef0cd1 Compare October 3, 2024 09:22
@l-baszak
Copy link

l-baszak commented Nov 8, 2024

@cristiangreco Can this be deployed?

@gham-khaled
Copy link

@tuananh Thank you for the effort ! Anything we can help to get this merged in ? We really need it.

@tuananh
Copy link

tuananh commented Nov 22, 2024

@tuananh Thank you for the effort ! Anything we can help to get this merged in ? We really need it.

We need @cristiangreco to take a look at it. Or if he has idea for another way of doing, we're happy to update the PR accordingly.

We've been using this feature in-house because we need it for our aws org. Would be nice to have it merged upstream.

@cristiangreco
Copy link
Contributor

Hi, it is not clear to me how this problem is addressed:

Has anyone figured out if in this case it is possible to get resource tags for linked accounts via the monitoring account? It seems that only metric data itself when linked is obtainable via monitoring account but not resource tags, which means there's no easy way to associate resources from linked accounts.

From #861 (comment)

@gtn3010
Copy link
Author

gtn3010 commented Nov 27, 2024

Only metric data is obtained and no resource tags from linked accounts. For logic getting tagged resources, I return nil when enabling config IncludeLinkedAccounts, since querying resource tags from linked accounts requires more permissions in AWS cross-account.

@cristiangreco
Copy link
Contributor

Only metric data is obtained and no resource tags from linked accounts. For logic getting tagged resources, I return nil when enabling config IncludeLinkedAccounts, since querying resource tags from linked accounts requires more permissions in AWS cross-account.

This means that the metric data you get back is not associated with the correct resource? How's that useful? (genuine question)

@gtn3010
Copy link
Author

gtn3010 commented Nov 27, 2024

I think the metrics data (metrics and dimensions) from cloudwatch is enough for us. For linked accounts, no metrics _info of each resource is acceptable, we haven't used these _info metrics yet. Currently, we only clone the dashboard from cloudwatch to Grafana.

Example:
aws_applicationelb_healthy_host_count_minimum{cluster="nonprod", dimension_AvailabilityZone="", dimension_LoadBalancer="app/non-prod-xxxxxxxx/11111111111", dimension_TargetGroup="targetgroup/k8s-devitcor-backenda-24266686864/46ff4111174faacf", name="global", namespace="monitoring"}
I rely on the LB name dimension to aggregate data.

@GustavoGuima86
Copy link

Hey guys, sorry for the intrusion, but I'm right now in an implementation that would requires the Cloudwatch link + YACE and saw that this feature could solve me a lot of hustle configuration, Is there someone looking at it to release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants