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 user management #18

Merged
merged 1 commit into from
Sep 4, 2023
Merged

Add user management #18

merged 1 commit into from
Sep 4, 2023

Conversation

Kidswiss
Copy link
Collaborator

@Kidswiss Kidswiss commented Aug 30, 2023

Summary

  • Add user management
  • The users can't actually do anything, as a user without a policy has no permissions at all
  • The policy management is handled in a follow up

The minio admin sdk is under AGPL-3.0, so we need to adjust accordingly.

Checklist

  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog
  • Update tests.
  • Link this PR to related issues.

@Kidswiss Kidswiss marked this pull request as draft August 30, 2023 11:07
@Kidswiss Kidswiss force-pushed the add/user branch 3 times, most recently from 1a4399d to dcfe3ae Compare August 30, 2023 11:12
@Kidswiss Kidswiss added the enhancement New feature or request label Aug 30, 2023
@Kidswiss Kidswiss marked this pull request as ready for review August 30, 2023 11:15
@Kidswiss Kidswiss requested review from a team, TheBigLee, wejdross and zugao and removed request for a team August 30, 2023 11:56
With this commit the provider is able to manage users as well. The
users can't yet do anything as a user without any policy has no
permissions at all.
@Kidswiss Kidswiss changed the title Add/user Add user management Aug 31, 2023
Copy link
Contributor

@zugao zugao left a comment

Choose a reason for hiding this comment

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

LGTM, some nitpicks

// It can be used to assign a policy to a usser.
func NewMinioAdmin(ctx context.Context, c client.Client, config *providerv1.ProviderConfig) (*madmin.AdminClient, error) {

secret := &corev1.Secret{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could easily cash this value? Connect() method id always invoked before any reconciliation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We're using the cached kube client from the manager anyway. So this should already by cached by that.

Also with an additional cache we might miss updates to the secret.

u.emitCreationEvent(user)

annotations := user.GetAnnotations()
annotations[UserCreatedAnnotationKey] = "true"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this annotation is needed? If the resource is Ready that it would imply that it's created.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem is that we can't update the status during creation, so we need the annotation to mark it as created. We can then set the proper condition in the observe function. We've used this pattern to indicate that the creation function actually ran in the other providers as well.

Also see here for more information on how the crossplane provider framework works: https://kb.vshn.ch/app-catalog/explanations/crossplane_provider_mechanics.html

@Kidswiss Kidswiss merged commit 20fc9e3 into master Sep 4, 2023
4 checks passed
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