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

Add support to define application credentials #1230

Merged
merged 2 commits into from
Jun 28, 2022

Conversation

NotTheEvilOne
Copy link
Contributor

@NotTheEvilOne NotTheEvilOne commented Jun 1, 2022

What this PR does / why we need it:
This change implements support to enter application credentials for OpenStack as an alternative for the use of a technical user with password.

Which issue(s) this PR fixes:
Closes #1049

Special notes for your reviewer:
Error messages should reflect required inputs taking precedence of the underlying gardener-extension-provider-openstack implementation.

Added support to enter application credentials for OpenStack as an alternative for the use of a technical user with password

@gardener-robot
Copy link

@NotTheEvilOne Thank you for your contribution.

@gardener-robot gardener-robot added needs/review Needs review size/s Size of pull request is small (see gardener-robot robot/bots/size.py) labels Jun 1, 2022
@gardener-robot-ci-3
Copy link
Contributor

Thank you @NotTheEvilOne for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below.

@gardener-robot
Copy link

@grolu, @holgerkoser You have pull request review open invite, please check

@grolu
Copy link
Contributor

grolu commented Jun 8, 2022

@NotTheEvilOne Thank you for your contribution.
It looks good at first glance, however I think the error handling needs to be improved:

  • Currently both - username / password as well as application credentials - can be defined at the same time. Does this make sense / is this a valid scenario?
  • If a password and no username but instead credential id / name are given, the validation succeeds but gardener returns an error: admission webhook "validation.openstack.provider.extensions.gardener.cloud" denied the request: 'username' is required if 'password' is given in garden-lukas/my-openstack-secret4
  • In this PR add support for application credentials gardener-extension-provider-openstack#300 @kayrus mentioned that "when you're specifying application credentials, especially by id and a password, there is no need to request domain/project/username. application credentials are project scoped by default" this is currently also not reflected in the implementation

Let's discuss the open questions here. After that you can improve your implementation or I can take care of it if you want.

@NotTheEvilOne
Copy link
Contributor Author

Thanks for your review.

@NotTheEvilOne Thank you for your contribution. It looks good at first glance, however I think the error handling needs to be improved:

* Currently both - username / password as well as application credentials - can be defined at the same time. Does this make sense / is this a valid scenario?

I don't think so.

* If a password and no username but instead credential id / name are given, the validation succeeds but gardener returns an error: `admission webhook "validation.openstack.provider.extensions.gardener.cloud" denied the request: 'username' is required if 'password' is given in garden-lukas/my-openstack-secret4`

Looks like I read the code here wrong.

* In this PR [add support for application credentials gardener-extension-provider-openstack#300](https://github.com/gardener/gardener-extension-provider-openstack/pull/300) @kayrus mentioned that _"when you're specifying application credentials, especially by id and a password, there is no need to request domain/project/username. application credentials are project scoped by default"_ this is currently also not reflected in the implementation

Here I'm not sure and need to check again. As far as I can see, domain and tenant are always required code wise. Could be that they are prefilled automatically somewhere else.

Let's discuss the open questions here. After that you can improve your implementation or I can take care of it if you want.

@grolu
Copy link
Contributor

grolu commented Jun 10, 2022

Here I'm not sure and need to check again. As far as I can see, domain and tenant are always required code wise. Could be that they are prefilled automatically somewhere else.

  • Currently both - username / password as well as application credentials - can be defined at the same time. Does this make sense / is this a valid scenario?

@MartinWeindel maybe you can clarify the open questions

@MartinWeindel
Copy link
Member

MartinWeindel commented Jun 28, 2022

Currently both - username / password as well as application credentials - can be defined at the same time. Does this make sense / is this a valid scenario?

No, this makes no sense and it is also not allowed to provide both password and applicationCredentialSecret. Either you provide username/password or application credentials

Here` I'm not sure and need to check again. As far as I can see, domain and tenant are always required code wise. Could be that they are prefilled automatically somewhere else.

With the current implementation, domain and tenant name need to be provided always. There are too much combinations which may be valid, but are not tested.

@grolu
Copy link
Contributor

grolu commented Jun 28, 2022

@NotTheEvilOne I will incorporate the required changes and make a new PR based on this one if this is ok for you

@NotTheEvilOne NotTheEvilOne force-pushed the pr/openstack-app-creds-support branch from d5292d8 to 0920ad3 Compare June 28, 2022 09:05
@gardener-robot gardener-robot added size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) and removed size/s Size of pull request is small (see gardener-robot robot/bots/size.py) labels Jun 28, 2022
@NotTheEvilOne
Copy link
Contributor Author

I've changed the PR based on the feedback given. If you find any further mistakes please go ahead as you suggested.

@NotTheEvilOne NotTheEvilOne force-pushed the pr/openstack-app-creds-support branch from 0920ad3 to 41c6e0f Compare June 28, 2022 09:12
Copy link
Contributor

@grolu grolu left a comment

Choose a reason for hiding this comment

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

/lgtm

There is still a minor glitch but most of the PR is fine . As I want to do some validation logic cleanup anyway. Let's merge it now. I will add a separate PR later

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels Jun 28, 2022
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 28, 2022
@grolu grolu merged commit 49a00e5 into gardener:master Jun 28, 2022
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for application credentials for creating Openstack provider secrets
6 participants