-
Notifications
You must be signed in to change notification settings - Fork 39
feat(access-token): add ephemeral access-token resource #1068
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
base: main
Are you sure you want to change the base?
feat(access-token): add ephemeral access-token resource #1068
Conversation
Signed-off-by: Mauritz Uphoff <[email protected]>
| RoundTripper http.RoundTripper | ||
| ServiceAccountEmail string // Deprecated: ServiceAccountEmail is not required and will be removed after 12th June 2025. | ||
|
|
||
| PrivateKey string |
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.
Could we have a seperate struct for that? E.g. by using https://gobyexample.com/struct-embedding
type EphermalProviderData struct {
ProviderData
PrivateKey string
PrivateKeyPath string
ServiceAccountKey string
ServiceAccountKeyPath string
}The fields like PrivateKey, ... don't belong into regular resources and datasources. You don't even set them there (what is good of course 😅). So some abstraction won't hurt here to keep things clean.
| return | ||
| } | ||
|
|
||
| ctx = tflog.SetField(ctx, "access_token", accessToken) |
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.
This is a HUGE no no, please don't ever (!!) log credentials
| func resolvePrivateKey(ctx context.Context, cfgValue, cfgPath string, key *clients.ServiceAccountKeyResponse) (string, diag.Diagnostics) { | ||
| var diags diag.Diagnostics | ||
|
|
||
| env := os.Getenv("STACKIT_PRIVATE_KEY") |
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.
shouldn't this be done already by the provider logic? I'm not willing to read env variables here...
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 it's not done there, it belongs there. This doesn't belong into some resource implementation for sure.
| func loadServiceAccountKey(ctx context.Context, cfgValue, cfgPath string) (*clients.ServiceAccountKeyResponse, diag.Diagnostics) { | ||
| var diags diag.Diagnostics | ||
|
|
||
| env := os.Getenv("STACKIT_SERVICE_ACCOUNT_KEY") |
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.
shouldn't this be done already by the provider logic? I'm not willing to read env variables here...
| ) | ||
|
|
||
| // #nosec G101 tokenUrl is a public endpoint, not a hardcoded credential | ||
| const tokenUrl = "https://service-account.api.stackit.cloud/token" |
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.
No need to duplicate, this is already the default
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.
Other question, what's with custom endpoints?
Description
Internal Issue: STACKITTPR-353
Checklist
make fmtexamples/directory)make generate-docs(will be checked by CI)make test(will be checked by CI)make lint(will be checked by CI)