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.

Sign up for GitHub

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

Jump to bottom

Support user specified MySQLServer secrets #1625

Merged
merged 4 commits into from
Jul 11, 2021
Merged

Support user specified MySQLServer secrets #1625

merged 4 commits into from
Jul 11, 2021

Conversation

matthchr
Copy link
Member

@matthchr matthchr commented Jul 6, 2021

Closes #1590

  • The specified secret must be a Kubernetes secret.
  • The specified secret must contain a "username" and "password" field.
  • The specified secret must be in the same namespace as the MySQLServer.
  • If the specified secret doesn't exist, reconciliation will be blocked
    until the secret does exist. Once the secret is created, reconciliation
    will continue as normal.
  • The operator does not make the user specified secret owned by
    the MySQLServer.
  • The operator still creates a secret containing connection string details
    and username/password for the server. This secret is named as it was
    before. This means that the customer specified username and password
    are consumed to create this secret, but other resources such as MySQLUser
    still consume the generated secret file.

If applicable:

  • this PR contains documentation
  • this PR contains tests

@matthchr matthchr force-pushed the feature/mysql-secrets branch from d804ee2 to f3b7fd5 Compare July 6, 2021 18:20
@codecov-commenter
Copy link

codecov-commenter commented Jul 6, 2021 •
edited
Loading

Codecov Report

Merging #1625 (f56cdfb) into master (5df37e7) will increase coverage by 1.96%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1625      +/-   ##
==========================================
+ Coverage   64.65%   66.61%   +1.96%     
==========================================
  Files         203      204       +1     
  Lines       12696    14760    +2064     
==========================================
+ Hits         8209     9833    +1624     
- Misses       3760     4170     +410     
- Partials      727      757      +30
Impacted Files Coverage Δ
hack/generated/pkg/testcommon/kube_test_context.go 83.33% <0.00%> (-6.67%) ⬇️
...enerator/pkg/astmodel/storage_package_reference.go 85.71% <0.00%> (-4.29%) ⬇️
...l/armconversion/create_empty_arm_value_function.go 79.16% <0.00%> (-4.17%) ⬇️
hack/generated/pkg/genruntime/property_bag.go 75.86% <0.00%> (-4.14%) ⬇️
hack/generator/pkg/functions/hub_function.go 77.27% <0.00%> (-3.98%) ⬇️
...k/generator/pkg/functions/original_gvk_function.go 86.11% <0.00%> (-3.89%) ⬇️
...nerator/pkg/functions/original_version_function.go 80.76% <0.00%> (-3.45%) ⬇️
...or/pkg/conversions/writable_conversion_endpoint.go 92.00% <0.00%> (-3.24%) ⬇️
hack/generator/pkg/astmodel/array_type.go 85.18% <0.00%> (-3.06%) ⬇️
hack/generator/pkg/astmodel/map_type.go 88.88% <0.00%> (-2.78%) ⬇️
... and 194 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5df37e7...f56cdfb. Read the comment docs.

@matthchr matthchr force-pushed the feature/mysql-secrets branch 2 times, most recently from a65f826 to 090cddd Compare July 7, 2021 00:19
@matthchr matthchr added this to the operator-v1.7 milestone Jul 7, 2021
theunrepentantgeek
theunrepentantgeek approved these changes Jul 7, 2021
View reviewed changes
Copy link
Member

@theunrepentantgeek theunrepentantgeek left a comment

Choose a reason for hiding this comment

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

Looks good, only minor comments.

babbageclunk
babbageclunk approved these changes Jul 8, 2021
View reviewed changes
Copy link
Member

@babbageclunk babbageclunk left a comment

Choose a reason for hiding this comment

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

This looks great. I wonder whether there's a way to have a change to the secret trigger a reconcile?

controllers/mysql_combined_test.go Outdated Show resolved Hide resolved
pkg/resourcemanager/mysql/mysqlaaduser/reconcile.go Outdated Show resolved Hide resolved
pkg/resourcemanager/mysql/mysqluser/mysqluser.go Show resolved Hide resolved
pkg/resourcemanager/mysql/server/reconcile.go
if len(instance.Spec.ReplicaProperties.SourceServerId) == 0 {
instance.Status.Message = "Replica requested but source server unspecified"
return true, nil
}
}

adminCreds, err := m.GetUserProvidedAdminCredentials(ctx, instance)
Copy link
Member

Choose a reason for hiding this comment

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

Should we make this resource reconcile when the secret is updated? Are people going to expect the user details to be updated in that case? (Is updating the credentials like that supported in ARM?)

Copy link
Member Author

@matthchr matthchr Jul 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We probably should, yes. It's a bit of feature creep because we didn't support rolling credentials before (since the user couldn't specify them). I was thinking to avoid it as part of this PR based on the "feature creep" logic but probably should do better.

I'll look into implementing this but may do it as a separate PR as it'll have another set of tests ensuring that the update actually works, and the actual watching of that secret may add some complexity.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've filed #1634 - will follow up with a separate PR tackling that.

pkg/resourcemanager/mysql/server/reconcile.go Show resolved Hide resolved
matthchr added 2 commits July 8, 2021 14:51
@matthchr
Support user specified MySQLServer secrets
1576777 
 - The specified secret must be a Kubernetes secret.
 - The specified secret must contain a "username" and "password" field.
 - The specified secret must be in the same namespace as the MySQLServer.
 - If the specified secret doesn't exist, reconciliation will be blocked
   until the secret does exist. Once the secret is created, reconciliation
   will continue as normal.
 - The operator does not make the user specified secret owned by
   the MySQLServer.
 - The operator still creates a secret containing connection string details
   and username/password for the server. This secret is named as it was
   before. This means that the customer specified username and password
   are consumed to create this secret, but other resources such as MySQLUser
   still consume the generated secret file.
@matthchr
PR feedback
4b696e4
@matthchr matthchr force-pushed the feature/mysql-secrets branch from 090cddd to 4b696e4 Compare July 8, 2021 21:51
@matthchr matthchr merged commit 81091cf into Azure:master Jul 11, 2021
@matthchr matthchr deleted the feature/mysql-secrets branch July 11, 2021 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theunrepentantgeek theunrepentantgeek theunrepentantgeek approved these changes

@babbageclunk babbageclunk babbageclunk approved these changes

@davefellows davefellows Awaiting requested review from davefellows

@Porges Porges Awaiting requested review from Porges

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
operator-v1.7
Development

Successfully merging this pull request may close these issues.

MySQL Server username and password should be able to be specified by the user
4 participants
@matthchr @codecov-commenter @babbageclunk @theunrepentantgeek