Add additional types and providers #24

jonasdemoor · 2021-11-30T15:23:21Z

This PR adds the following types and providers for managing local MinIO resources:

minio_user
minio_group
minio_bucket
minio_policy

These work for the most part; there are still some caveats:

It's not possible to update the secret key of a user, since the CLI doesn't report the secret key when fetching users.
Assigning policies only works reliably when doing so via either minio_user or minio_group. Attempting to mix those two providers for policy assignment might result in assigned policies being overwritten.
The minio_group provider processes updates by recreating the whole group to keeps things simple. A proper update would require removing and/or adding members via the CLI in a certain order.
A client alias with the name puppet is hardcoded into the provider code, since the MinIO CLI needs an alias to retrieve and create MinIO resources. This alias would need to be created before running the providers.

I'm running the providers on a Debian 11 VM against the following MinIO versions:

$ sudo mcli admin info puppet
●  127.0.0.1:9000
   Uptime: 1 week 
   Version: 2021-11-03T03:36:36Z
   Network: 1/1 OK 

3.1 MiB Used, 7 Buckets, 1 Object

$ sudo mcli --version
mcli version RELEASE.2021-10-07T04-19-58Z

$ cat /etc/os-release 
PRETTY_NAME="Debian GNU/Linux 11 (bullseye)"
NAME="Debian GNU/Linux"
VERSION_ID="11"
VERSION="11 (bullseye)"
VERSION_CODENAME=bullseye
ID=debian
HOME_URL="https://www.debian.org/"
SUPPORT_URL="https://www.debian.org/support"
BUG_REPORT_URL="https://bugs.debian.org/"

I'm still working on my Ruby skills, feedback is always welcome :)

ZloeSabo · 2021-12-07T15:07:36Z

Thank you for your contribution @jonasdemoor . I will check the changes once I have a chance, that's a pretty big PR to check!

ZloeSabo · 2021-12-17T09:43:36Z

lib/puppet/provider/minio_bucket/minio_bucket.rb

+require 'puppet/resource_api/simple_provider'
+require 'puppet_x/minio/client'
+
+DEFAUlT_TARGET_ALIAS ||= 'puppet'.freeze


I suggest removing the hardcoded alias and adding it as a parameter here and in other types you're adding. I'm sure it would work as it is for certain use-cases, but it's always better to be flexible and allow module consumers to decide what alias they want to use.

This is bit of a chicken-egg problem unfortunately, as we need an alias to query MinIO for state.

I was thinking about managing a small JSON file (e.g. /usr/local/etc/minio_state.json) that would hold the currently selected alias by the user. We can manage this from the manifests or we can write a small provider for that, for example.

I was also thinking about using this file for saving user passwords to from the minio_user provider, then we could update a user's password without having to (manually) delete said user.

Does this sound okay for you? Just thinking out loud here :)

I solved this by setting /root/.minio_default_alias in the manifests and retrieving the contents of that file in the providers (via the alias method in puppet_x/minio/client

ZloeSabo · 2021-12-17T09:48:53Z

lib/puppet/type/minio_bucket.rb

+      desc:      'The name of the resource you want to manage.',
+      behaviour: :namevar,
+    },
+  },


Please add autorequire here and in other new types, as I did in aliases:

autorequire: { file: '/root/.minioclient', }

Otherwise, puppet sometimes tries to create resources before minio client is set and that fails for obvious reasons:

Error: minio_bucket[testbucket]: Creating: Failed after 0.000269 seconds: Symlink to minio client does not exist at /root/.minioclient. Make sure you installed the client before managing minio resources. Error: /Stage[main]/Main/Minio_bucket[testbucket]: Could not evaluate: Execution encountered an error

I forgot those indeed, I will add them to the types.

ZloeSabo · 2021-12-17T09:51:54Z

lib/puppet/type/minio_bucket.rb

+      desc:    'Whether this resource should be present or absent on the target system.',
+      default: 'present',
+    },
+    name: {


If that's possible, please also add parameters for region and locking.

I added some additional parameters for region and locking

ZloeSabo · 2021-12-17T09:58:40Z

lib/puppet/provider/minio_group/minio_group.rb

+    operations << "admin group add #{DEFAUlT_TARGET_ALIAS} #{name} #{should[:members].join(' ')}"
+
+    operations << "admin group disable #{DEFAUlT_TARGET_ALIAS} #{name}" unless should[:enabled]
+    operations << "admin policy set #{DEFAUlT_TARGET_ALIAS} #{should[:policies].join(',')} group=#{name}" unless should[:policies].nil?


I suggest moving the policy assignment to a dedicated type that would only assign an existing policy to a user or to a group.

In my experience, as soon as a type started doing more than one thing, module consumers start having problems using the module. So it's better to give consumers smaller simple building blocks and let them decide how they want to combine those. Of course given the client supports a dedicated command for assignment (in the case of minio, policies have the dedicated command, but groups do not).

This makes more sense indeed, and would simplify the user and group providers.

Policy assignment is now done with a dedicated minio_policy_assignment provider. Since there's no real API endpoint for retrieving policy assignments, the get method returns resources with <subject_type>_<subject> as composite namevar. So it's only possible to update those resources.

ZloeSabo · 2021-12-17T10:00:30Z

lib/puppet/type/minio_user.rb

+      behaviour: :namevar,
+    },
+    secret_key: {
+      type: 'Variant[Sensitive[String], String]',


Could you please add length restrictions to the string types? The secret key should be between 8 and 40 characters in length.

I added some length restrictions like you described.

ZloeSabo · 2021-12-17T10:00:50Z

lib/puppet/type/minio_user.rb

+    },
+    access_key: {
+      type:      'String',
+      desc:      'The API access key',


Please add a mention that it's also a username.

I modified the description a bit

ZloeSabo · 2021-12-17T10:12:43Z

lib/puppet/provider/minio_policy/minio_policy.rb

+
+    f = Tempfile.new(["#{name}-policy", '.json'])
+    begin
+      json_policy = Hash[:Version => DEFAULT_POLICY_VERSION, :Statement => should[:statement]].to_json


You could just do {:Version => ...} with curly brackets without Hash

you don't really need f.rewind. For further closing and deleting the file, it's not important where the internal cursor for the file is.

I adjusted the Hash syntax to the one you described

When I didn't rewind the file, mcli couldn't read the JSON file with the policy object, because the CLI read an empty string. After some searching, I found that I had to rewind the file for it to work.

ZloeSabo · 2021-12-17T10:29:17Z

lib/puppet/type/minio_user.rb

+      type:    'Optional[Array[String]]',
+      desc:    'List of MinIO PBAC policies to set for this user.',
+    },
+    member_of: {


Do you really need it? I'd just remove it unless sure why would we need it.

Could you please also add enabled parameter here?

This isn't really needed indeed, it's mostly cosmetic, so I removed it

I added an additional enabled parameter

ZloeSabo · 2021-12-17T10:34:09Z

lib/puppet/type/minio_policy.rb

+    version: {
+      type:      'String',
+      desc:      'Specifies the language syntax rules that are to be used to process a policy.',
+      behaviour: :read_only,


Why don't you just add a default value here instead of making it read only? In such a case, consumers could continue using the module when the version ever changes without making changes to the module code.

I added a default value for the version

ZloeSabo · 2021-12-17T10:40:15Z

@jonasdemoor thank you again for your PR. I finally had time to look at it.

I understand that some of my requests gonna require a lot of time to implement, so I suggest you fix whatever you have time for and let me know, so I take over and finish the remaining stuff myself.

jonasdemoor · 2022-03-15T11:21:01Z

Hello @ZloeSabo ,

My apologies for taking so long to respond. Thank you very much for your detailed feedback, I'm reworking this PR based on your feedback.

Instead of hardcoding the alias.

This has been moved to a dedicated minio_policy_assignment provider.

Moved to minio_policy_assignment.

Instead of setting a global variable in the constructor. Otherwise, catalog compilation fails at the first resource.

jonasdemoor · 2022-04-13T09:17:06Z

Hi @ZloeSabo,

I reworked the providers and types based on your feedback. Feel free to let me know if there's anything else that could be improved or handled differently :)

jonasdemoor · 2022-04-13T09:17:42Z

I'm still working on the unit tests, so CI may still fail

jonasdemoor added 5 commits November 26, 2021 17:17

providers/minio_client_alias: Move to unwrap_maybe_sensitive to utils

a92b395

Add minio_bucket provider

065eb38

Add minio_user provider

4f65e40

Add minio_group provider

c497a9c

Add minio_policy provider

31a6abb

ZloeSabo self-requested a review December 7, 2021 16:47

ZloeSabo added the enhancement label Dec 7, 2021

ZloeSabo reviewed Dec 17, 2021

View reviewed changes

jonasdemoor added 14 commits April 12, 2022 10:26

manifests: Add parameter for setting a default client alias

bb4395e

lib/puppet_x/minio/client: Add helper method for using default alias

b6e4b05

providers: Use helper alias method from client

7709e88

Instead of hardcoding the alias.

Add minio_policy_assignment provider

4694bab

lib/puppet/minio_user: Remove policy assignment

bce3f3e

This has been moved to a dedicated minio_policy_assignment provider.

lib/puppet/type/minio_group: Remove policy assignment

869cdd8

Moved to minio_policy_assignment.

lib/puppet/type/minio_user: Add secret key length restrictions

44d0f7e

lib/puppet/type/minio_user: Clarify parameter descriptions

1b08458

lib/puppet/minio_user: Add enabled param

0fdb959

lib/puppet/provider/minio_policy: Also sanitize non-symbolizes keys

0311ba8

lib/puppet/minio_policy: Use default for version parameter

a79bedc

lib/puppet/minio_bucket: Add region and object lock params

21526e8

lib/puppet/minio_bucket: Add insync? method

3bb8830

providers: Use client alias method directly for retrieving alias

bf9a2ad

Instead of setting a global variable in the constructor. Otherwise, catalog compilation fails at the first resource.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add additional types and providers #24

Add additional types and providers #24

jonasdemoor commented Nov 30, 2021

ZloeSabo commented Dec 7, 2021

ZloeSabo Dec 17, 2021

jonasdemoor Mar 15, 2022

jonasdemoor Apr 13, 2022

ZloeSabo Dec 17, 2021

jonasdemoor Mar 15, 2022

ZloeSabo Dec 17, 2021

jonasdemoor Apr 13, 2022

ZloeSabo Dec 17, 2021

jonasdemoor Mar 15, 2022

jonasdemoor Apr 13, 2022

ZloeSabo Dec 17, 2021

jonasdemoor Apr 13, 2022

ZloeSabo Dec 17, 2021

jonasdemoor Apr 13, 2022

ZloeSabo Dec 17, 2021

jonasdemoor Apr 13, 2022

ZloeSabo Dec 17, 2021

jonasdemoor Apr 13, 2022

ZloeSabo Dec 17, 2021

jonasdemoor Apr 13, 2022

ZloeSabo commented Dec 17, 2021

jonasdemoor commented Mar 15, 2022

jonasdemoor commented Apr 13, 2022

jonasdemoor commented Apr 13, 2022

Add additional types and providers #24

Are you sure you want to change the base?

Add additional types and providers #24

Conversation

jonasdemoor commented Nov 30, 2021

ZloeSabo commented Dec 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZloeSabo commented Dec 17, 2021

jonasdemoor commented Mar 15, 2022

jonasdemoor commented Apr 13, 2022

jonasdemoor commented Apr 13, 2022