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

Refactor Custom Secrets and AzureSQLUser #1153

Closed
wants to merge 17 commits into from

Conversation

WilliamMortlMicrosoft
Copy link
Contributor

@WilliamMortlMicrosoft WilliamMortlMicrosoft commented Jun 9, 2020

What this PR does / why we need it:
Secrets Pkg:

  • Return error when trying to use flatten on kube client (+test)

AzureSqlUser:

  • Refactor and move custom secret formatting and reconciliation into its own function (out of Ensure)
  • Clean up and standardize NamespacedName generation
  • Clean up overall azuresqluser variable naming for code clarity

The flatten option has been kept in the implementation because we need to have a way to ask the secrets package to create individual secrets rather than the default behavior of working with a map/json array. The kube client now returns an error when asked to flatten because that doesn't make sense in-context (Kube only works with map objects for secrets).

Closes #887

Special notes for your reviewer:

How does this PR make you feel:
gif

If applicable:

  • this PR contains documentation
  • this PR contains tests

Comment on lines +286 to +295
if strings.Contains(err.Error(), "FlattenedSecretsNotSupported") { // kube client does not support Flatten
err = secretClient.Upsert(ctx,
types.NamespacedName{Namespace: userSecretNamespacedName.Namespace, Name: instance.Name},
formattedSecrets)
if err != nil {
return fmt.Errorf("Upsert into KubeClient without flatten failed")
}
instance.Status.FlattenedSecrets = false
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jananivMS I followed your lead on this

@WilliamMortlMicrosoft
Copy link
Contributor Author

Waiting for this to pass and then Ill pull the WIP off

@WilliamMortlMicrosoft
Copy link
Contributor Author

Will make sure to rebase and merge when ready so that Denis gets credit for his contributions

Comment on lines +177 to +180
keyVaultEnabled := reflect.TypeOf(secretClient).Elem().Name() == "KeyvaultSecretClient"
if !keyVaultEnabled {
return nil
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I remove this @jananivMS

@WilliamMortlMicrosoft WilliamMortlMicrosoft changed the title WIP: Refactor Custom Secrets and AzureSQLUser Refactor Custom Secrets and AzureSQLUser Jun 9, 2020
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.

A couple of minor documentation nits.

docs/services/azuresql/azuresql.md Outdated Show resolved Hide resolved
docs/services/azuresql/azuresql.md Outdated Show resolved Hide resolved
docs/services/azuresql/azuresql.md Outdated Show resolved Hide resolved
@frodopwns frodopwns self-assigned this Jun 10, 2020
@jananivMS
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jananivMS
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@matthchr matthchr removed the XL label Jun 24, 2020
@matthchr
Copy link
Member

matthchr commented Feb 4, 2021

Closing this given its age and the fact that an alternative approach is about to merge here #1358

@matthchr matthchr closed this Feb 4, 2021
@Porges Porges deleted the secret-refactor branch June 23, 2021 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Task: refactor sql user secret formatting
6 participants