GoogleCloudPlatform / deploymentmanager-samples

Deployment Manager samples and templates.
Apache License 2.0
948 stars 716 forks source link

Unable to update a database user with sqladmin-v1beta4:users #142

Open jiang-wei opened 6 years ago

jiang-wei commented 6 years ago

The gcp-types/sqladmin-v1beta4:users interfact does not have a GET method.

When updating a big DM configuration that contains a database user resource like this:

resources:
# No GET method for user, resulting update error
- name: {{ env['name'] }}
  type: gcp-types/sqladmin-v1beta4:users
  properties:
    name: {{ properties['userName'] }}
    instance: {{ properties['instanceName'] }}
    host: ""
    password: {{ properties['password'] }}

There must be an update error:

ERROR: (gcloud.deployment-manager.deployments.update) Error in Operation [operation-1522961826387-569202eab1438-48567386-60db63f3]: errors:
- code: MISSING_METHOD_IN_COLLECTION
  message: Method 'get' does not exists for collection 'users' in descriptor url 'https://www.googleapis.com/discovery/v1/apis/sqladmin/v1beta4/rest'

I have to remove the user configuration from that bit DM configuration

munutzer commented 6 years ago

DM does not support updating resources that don't expose a GET method. As a workaround, you could change the user name along with whatever other change you want to make. That would delete the old user resource and create a new one with the updated settings.

jiang-wei commented 6 years ago

and then add the original user back?

quite annoying :(

munutzer commented 6 years ago

Yes, we don't have a good way to update this resource in DM because it does not have a GET method in the resource API.

gjferrier commented 6 years ago

Why was this issue closed? Its a live issue, no?

takeda commented 5 years ago

@munutzer please reopen this ticket, this is a major flaw, and merely including the resource renders the entire config non updatable. Removing and adding a database user not only is a hassle, but most of the time impossible to do.

It is crazy that this issue is still present nearly a year later.

vagababov commented 5 years ago

In general GoogleSQL API is not very fitting for DM operations. As description above states it was merely impossible to implement.

takeda commented 5 years ago

I see, but GoogleSQL and DM are both part of Google Cloud, I'm sure there must be a way to provide the call.

AWS CloudFormation has its warts, but I haven't seen issues like that.

vinayan3 commented 5 years ago

This should be clearly documented somewhere because if we can't do updates after creating it renders configs un-updatable which is a huge problem.

vinayan3 commented 5 years ago

@takeda I've figured out a work around. Remove the user from the config but use --delete-policy=abandon which will cause it to be removed from the deployment. Then you can manage the user using SQL Files etc... An example is below:

gcloud deployment-manager deployments update db-test --template db.yaml.jinja --delete-policy=abandon

I had trouble deleting my user because it was being used to grant privileges to the roles.

takeda commented 5 years ago

@vinayan3 thanks, but this only changes behavior and won't remove the user when needed + it is required every time and affects other operations.

This and other issues made me just switch to terraform. I generally prefer to use tool provided by the cloud provider (e.g. Cloud Formation), because it generally supposed to be better integrated with the platform, but Deployment Manager is a joke.

konrad-garus commented 5 years ago

I see, but GoogleSQL and DM are both part of Google Cloud, I'm sure there must be a way to provide he call.

I couldn't agree more. As a consumer, I don't want to know or care about Google org hierarchy. I only expect the end result, the platform, to be reliable, user-friendly and free of such surprises. That's why I'm using Google Cloud in the first place, tired of AWS low-level ops and warts.

themethod commented 5 years ago

Seems that deployment-manager can deploy ok but needs work on the management side. A big part of managing deployments is being able to update existing components to a state that matches a declarative specification.

judas-christ commented 5 years ago

I'm having this problem now and as far as I understand, the users resource does have a GET method: https://cloud.google.com/sql/docs/mysql/admin-api/v1beta4/users/list

It's also in the discovery doc (https://www.googleapis.com/discovery/v1/apis/sqladmin/v1beta4/rest search for sql.users.list), so how come DM doesn't see it?

nsa-itv commented 4 years ago

Regarding the password, from the database point of view I expect we should only be able to change a password if we know the existing password, unless we use a database user/login with certain privileges to override that.

So in general we would have to have password and newPassword fields and supply these to update the password - we shouldn't expect Deployment Manager to know/store passwords.

And on that latter note, I wish it would not store - and certainly not display the password in plaintext in the web console (and possibly yaml configuration description dumps).

rick-pri commented 4 years ago

Regarding the password, from the database point of view I expect we should only be able to change a password if we know the existing password, unless we use a database user/login with certain privileges to override that.

If you have access to the admin user then you can arbitrarily change the password in the database so shouldn't need the original password.

And on that latter note, I wish it would not store - and certainly not display the password in plaintext in the web console (and possibly yaml configuration description dumps).

Also, the users should not automatically be essentially super users, but allow us to specify the databases that they have access to.

But, at the moment, I can't even get the API to create the users for me, as I have databases specified. It just returns a 400

edit: This was because I wasn't passing a password with the name of the user and this wasn't tested in PostgreSQL. The example has been fixed with the password field and tests are now run for Postgres.