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

Allow authentication through environment variables #41

Merged
merged 6 commits into from
Dec 11, 2024

Conversation

baurmatt
Copy link
Contributor

@baurmatt baurmatt commented Dec 6, 2024

This allows the usage of standardized OS_* variables to authenticate against OpenStack. It's helpful when running the plugin in Kubernetes, because you "only" need to inject environment variables into the pod, not a full YAML file.

Copy link
Contributor

@vooon vooon left a comment

Choose a reason for hiding this comment

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

You anyway should provide config.toml, right?
Maybe easier just to directly add all that vars to it?

provider.go Outdated

var err error
endpointOps = gophercloud.EndpointOpts{Region: os.Getenv("OS_REGION_NAME")}
authOptions, err = openstack.AuthOptionsFromEnv()
Copy link
Contributor

Choose a reason for hiding this comment

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

It not the best, as it more identity v2 compatible than v3. In one of utilities i had to reinvent the wheel to have proper operation, like with python lib.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would be the v3 version of it? 🤔 Currently this is the documented way shown in the gophercloud README.md.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whatever they say, it's incompatible with openrc generated for v3. And even more, it breaks python agents, when they see OS_DOMAIN for example.

Here few parts of code, which do a proper support for v3:
https://gist.github.com/vooon/911c13af31c4aff53477536d8d6afe65

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I think it depends on what environment variables you configure. If you're supposed to talk to a v3 API, it could be very well be that configuring OS_DOMAIN breaks the tooling, because it's not supported. Which is definitely wrong, but I hesitate to add >100 lines of code just to catch those cases. How about you open a bug for gophercloud? Or would you prefer me to add those code lines from you Gist?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think GC would not break existing calls, as v2-rc time passed... Easier if you add similar code here. Not necessary the same way.
I've used interface because i reuse that common code in a lot of places, and it's helpful to embed into Kong cli...

Copy link
Contributor

Choose a reason for hiding this comment

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

github.com/caarlos0/env/v10 works (almost) the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see an option for dom there 🤔 Did I miss something?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's not necessary to support xor on env parsing, you may do the check later.
It just handy to error-exit early during cli opt validation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, implemented something similar. Hope that's fine! :)

@baurmatt
Copy link
Contributor Author

baurmatt commented Dec 9, 2024

You anyway should provide config.toml, right? Maybe easier just to directly add all that vars to it?

Yes, but only the [[runners]] part, which is almost the same for all regions/environments. I'd prefer the environment variable credential lookup, as it's easier to handle the secret management. Putting them into the config.toml would mix up things.

This allows the usage of standardized OS_* variables to authenticate
against OpenStack. It's helpful when running the plugin in Kubernetes,
because you "only" need to inject environment variables into the
pod, not a full YAML file.
provider.go Outdated Show resolved Hide resolved
internal/openstackclient/openstackclient.go Outdated Show resolved Hide resolved
Copy link
Contributor

@vooon vooon left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

@vooon vooon merged commit 2a2678a into sardinasystems:main Dec 11, 2024
5 checks passed
@vooon vooon added the enhancement New feature or request label Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants