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

feat(database/rds): Add password constraints #1734

Closed

Conversation

MisterMX
Copy link
Collaborator

Description of your changes

This adds the ability to set additional password constraints for DBCluster.docdb, DBInstance.rds, DBCluster.rds and RDSInstance.database.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Unit tests.

Signed-off-by: Maximilian Blatt <maximilian.blatt-extern@deutschebahn.com>
(external expert on behalf of DB Netz AG)
Signed-off-by: Maximilian Blatt <maximilian.blatt-extern@deutschebahn.com>
(external expert on behalf of DB Netz AG)
Signed-off-by: Maximilian Blatt (external expert on behalf of DB Netz) <maximilian.blatt-extern@deutschebahn.com>
@@ -141,6 +143,10 @@ type CustomDBClusterParameters struct {
// Constraints: Must contain from 8 to 100 characters.
MasterUserPasswordSecretRef *xpv1.SecretKeySelector `json:"masterUserPasswordSecretRef,omitempty"`

// MasterUserPaswordConstraints specifies additional constraints
// that apply to the password referenced by MasterUserPasswordSecretRef.
MasterUserPaswordConstraints *common.PasswordConstraints `json:"masterUserPaswordConstraints,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo, two "s" in password

@chlunde
Copy link
Collaborator

chlunde commented Apr 25, 2023

I wonder if it would be better to just make the default passwords really good (enough for 95% of users), and have the final 5% just pre-generate a password (provider-secret-password? 😄) with features like this.

@MisterMX
Copy link
Collaborator Author

MisterMX commented Apr 26, 2023

provider-secret-password

I really like that thought. However, we do allow our users to specify their own passwords so we need it to be checked. And I think the provider is the best place to do that. Would it still be possible to get theses changes merged?

@chlunde
Copy link
Collaborator

chlunde commented Apr 26, 2023

However, we do allow our users to specify their own passwords so we need it to be checked.
[...]
Would it still be possible to get theses changes merged?

I think this is a fairly special case, I'd leave it to @haarchri to decide. I think the best practise is to generate a random master password and never expose it to humans, only use it in other tooling to create less privileged users, so this feature is normally not a requirement.
On the other hand, it's not a lot of code inside each controller.

@haarchri
Copy link
Member

have the same feeling than @chlunde

@MisterMX
Copy link
Collaborator Author

Closing it since this use case is too specific.

@MisterMX MisterMX closed this Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants