-
Notifications
You must be signed in to change notification settings - Fork 128
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
Lets to get security credentials if run on AWS FarGate #2174
Conversation
anonymous seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @petlitskiy,
thank you for your contribution.
Could you please sign the CLA, add to description what this PR is about and how it will help you?
I left a few comments what can be improved
Thank you
managed/services/agents/rds.go
Outdated
p, err := procfs.NewProc(1) | ||
if err != nil { | ||
log.Fatalf("could not get process: %s", err) | ||
} | ||
|
||
envs, err := p.Environ() | ||
if err != nil { | ||
log.Fatalf("could not get process stat: %s", err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p, err := procfs.NewProc(1) | |
if err != nil { | |
log.Fatalf("could not get process: %s", err) | |
} | |
envs, err := p.Environ() | |
if err != nil { | |
log.Fatalf("could not get process stat: %s", err) | |
} | |
env := os.Environ() | |
if err != nil { | |
log.Fatalf("could not get process stat: %s", err) | |
} |
managed/services/agents/rds.go
Outdated
@@ -65,6 +71,16 @@ func mergeLabels(node *models.Node, agent *models.Agent) (model.LabelSet, error) | |||
return res, nil | |||
} | |||
|
|||
func contains(s []string, str string) string { | |||
for _, v := range s { | |||
match, err := regexp.MatchString(str, v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be replaced with just strings.HasPrefix
managed/services/agents/rds.go
Outdated
log.Fatalf("could not get process stat: %s", err) | ||
} | ||
|
||
result := contains(envs, "AWS_CONTAINER_CREDENTIALS_RELATIVE_URI.*") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know the exact environment variable? we can use os.GetEnv
for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exactly env variable ("AWS_CONTAINER_CREDENTIALS_RELATIVE_URI") ALWAYS is present only on AWS Fargate for PID 1 - i.e. only for "supervisord" process. It not available for "managed" process - that is why i use procfs but not os.GetEnv(). It is my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But yes - we can use os.GetEnv. I checked. It works well.
managed/services/agents/rds.go
Outdated
if result != "" { | ||
return &agentpb.SetStateRequest_AgentProcess{ | ||
Type: inventorypb.AgentType_RDS_EXPORTER, | ||
TemplateLeftDelim: tdp.Left, | ||
TemplateRightDelim: tdp.Right, | ||
Args: args, | ||
Env: []string{ | ||
fmt.Sprintf("AWS_CONTAINER_CREDENTIALS_RELATIVE_URI=%s", result), | ||
}, | ||
TextFiles: map[string]string{ | ||
"config": "---\n" + string(b), | ||
}, | ||
RedactWords: words, | ||
}, nil | ||
} else { | ||
return &agentpb.SetStateRequest_AgentProcess{ | ||
Type: inventorypb.AgentType_RDS_EXPORTER, | ||
TemplateLeftDelim: tdp.Left, | ||
TemplateRightDelim: tdp.Right, | ||
Args: args, | ||
TextFiles: map[string]string{ | ||
"config": "---\n" + string(b), | ||
}, | ||
RedactWords: words, | ||
}, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if result != "" { | |
return &agentpb.SetStateRequest_AgentProcess{ | |
Type: inventorypb.AgentType_RDS_EXPORTER, | |
TemplateLeftDelim: tdp.Left, | |
TemplateRightDelim: tdp.Right, | |
Args: args, | |
Env: []string{ | |
fmt.Sprintf("AWS_CONTAINER_CREDENTIALS_RELATIVE_URI=%s", result), | |
}, | |
TextFiles: map[string]string{ | |
"config": "---\n" + string(b), | |
}, | |
RedactWords: words, | |
}, nil | |
} else { | |
return &agentpb.SetStateRequest_AgentProcess{ | |
Type: inventorypb.AgentType_RDS_EXPORTER, | |
TemplateLeftDelim: tdp.Left, | |
TemplateRightDelim: tdp.Right, | |
Args: args, | |
TextFiles: map[string]string{ | |
"config": "---\n" + string(b), | |
}, | |
RedactWords: words, | |
}, nil | |
} | |
var env []string | |
if result != "" { | |
env = []string{fmt.Sprintf("AWS_CONTAINER_CREDENTIALS_RELATIVE_URI=%s", result)} | |
} | |
return &agentpb.SetStateRequest_AgentProcess{ | |
Type: inventorypb.AgentType_RDS_EXPORTER, | |
TemplateLeftDelim: tdp.Left, | |
TemplateRightDelim: tdp.Right, | |
Args: args, | |
Env: env, | |
TextFiles: map[string]string{ | |
"config": "---\n" + string(b), | |
}, | |
RedactWords: words, | |
}, nil |
To propagate AWS_CONTAINER_CREDENTIALS_RELATIVE_URI environment variable to rds exporter child process. Reduced as maximum as i can.
Hi @petlitskiy , sorry for the delay. Are you still interested in this contribution? We would like to merge this change, can you please reopen this PR? |
Yes. I'll reopen. But i confused with your CLA assistant plugin. I have no
idea what it wants from me.
ср, 28 черв. 2023, 15:58 користувач Artem Gavrilov ***@***.***>
пише:
… Hi @petlitskiy <https://github.com/petlitskiy> , sorry for the delay. Are
you still interested in this contribution? We would like to merge this
change, can you please reopen this PR?
—
Reply to this email directly, view it on GitHub
<#2174 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ATLN5D6O4PZ46ULYXRWIUMTXNQS73ANCNFSM6AAAAAAYNEB5QU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@petlitskiy Please check that your email in git setting matches email specified in Github account, most likely this is the case. |
Created again. #2334
чт, 29 черв. 2023 р. о 11:59 Artem Gavrilov ***@***.***> пише:
… @petlitskiy <https://github.com/petlitskiy> Please check that your email
in git setting matches email specified in Github account, most likely this
is the case.
—
Reply to this email directly, view it on GitHub
<#2174 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ATLN5D3PFOXUTFZU23KDIXTXNU7Y3ANCNFSM6AAAAAAYNEB5QU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
This a pretty small but meaningful improvement can make our work much more
convenient. P.S. my colleague/chief like to update from PMM console. But to
update from PMM console it the rds agent has to work inside ECS Fargate.
пт, 30 черв. 2023 р. о 11:22 Олексій Петлицький ***@***.***>
пише:
… Created again. #2334
чт, 29 черв. 2023 р. о 11:59 Artem Gavrilov ***@***.***>
пише:
> @petlitskiy <https://github.com/petlitskiy> Please check that your email
> in git setting matches email specified in Github account, most likely this
> is the case.
>
> —
> Reply to this email directly, view it on GitHub
> <#2174 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ATLN5D3PFOXUTFZU23KDIXTXNU7Y3ANCNFSM6AAAAAAYNEB5QU>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
PMM-0
Link to the Feature Build: SUBMODULES-0
If this PR adds or removes or alters one or more API endpoints, please review and add or update the relevant API documents as well:
If this PR is related to some other PRs in this or other repositories, please provide links to those PRs: