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

chore: remove duplicated logic by function call #1080

Merged
merged 3 commits into from
Jun 26, 2024
Merged

chore: remove duplicated logic by function call #1080

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
51194cc
chore: remove duplicated logic by function call
zdtsw Jun 25, 2024
11fb6b6
update:refactor some more
zdtsw Jun 25, 2024
fe4e0e5
update: code review
zdtsw Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 31 additions & 42 deletions pkg/cluster/cert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,27 +32,9 @@ func CreateSelfSignedCertificate(ctx context.Context, c client.Client, secretNam
if err := ApplyMetaOptions(certSecret, metaOptions...); err != nil {
return err
}
existingSecret := &corev1.Secret{}
err = c.Get(ctx, client.ObjectKey{Name: secretName, Namespace: namespace}, existingSecret)
if err != nil {
if k8serr.IsNotFound(err) {
// Secret does not exist, create it
if createErr := c.Create(ctx, certSecret); createErr != nil {
return fmt.Errorf("failed creating certificate secret: %w", createErr)
}
} else {
return fmt.Errorf("failed getting certificate secret: %w", err)
}
} else if existingSecret.Type != certSecret.Type {
// Secret exists but with a different type, delete and recreate it
if err := c.Delete(ctx, existingSecret); err != nil {
return fmt.Errorf("failed deleting existing secret: %w", err)
}
if createErr := c.Create(ctx, certSecret); client.IgnoreAlreadyExists(createErr) != nil {
return fmt.Errorf("failed creating certificate secret: %w", createErr)
}
if err = generateCertSecret(ctx, c, certSecret, secretName, namespace); err != nil {
return fmt.Errorf("failed update self-signed certificate secret: %w", err)
}

return nil
}

Expand Down Expand Up @@ -194,29 +176,9 @@ func copySecretToNamespace(ctx context.Context, c client.Client, secret *corev1.
Data: secret.Data,
Type: secret.Type,
}

existingSecret := &corev1.Secret{}
err := c.Get(ctx, client.ObjectKey{Name: newSecretName, Namespace: namespace}, existingSecret)
if k8serr.IsNotFound(err) { // create if not found
if err = c.Create(ctx, newSecret); err != nil {
return fmt.Errorf("failed to create new secret: %w", err)
}
} else if err != nil {
return fmt.Errorf("failed to get existing secret: %w", err)
}

if existingSecret.Type != newSecret.Type { // recreate if found with mismatched type
if recreateSecret(ctx, c, existingSecret, newSecret) != nil {
return errors.New("failed to recreate secret with type corrected")
}
}

if isSecretOutdated(existingSecret.Data, newSecret.Data) {
if err = c.Update(ctx, newSecret); err != nil { // update data if found with same type but outdated content
return fmt.Errorf("failed to update secret: %w", err)
}
if err := generateCertSecret(ctx, c, newSecret, newSecretName, namespace); err != nil {
return fmt.Errorf("failed to deploy default cert secret to namespace %s: %w", namespace, err)
}

return nil
}

Expand All @@ -231,6 +193,33 @@ func recreateSecret(ctx context.Context, c client.Client, existingSecret, newSec
return nil
}

// generateCertSecret creates a secret if it does not exist; recreate this secret if type not match; update data if outdated.
func generateCertSecret(ctx context.Context, c client.Client, certSecret *corev1.Secret, secretName, namespace string) error {
existingSecret := &corev1.Secret{}
err := c.Get(ctx, client.ObjectKey{Name: secretName, Namespace: namespace}, existingSecret)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this chain of if if if else elseif if a bit more confusing than the proposal, but fine for me if it's better for you, clear enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

My way of thinking behind using switch is that we have 3 cases for Get: existing object, non-existing object, error of fetching. Not 2 as for if-else.

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 was not very fond of using switch in this case, thats why tried to avoid it.
but after reading Luca's comments, maybe switch is much better for a quick return.
let me do some updates again

if k8serr.IsNotFound(err) {
// Secret does not exist, create it
if createErr := c.Create(ctx, certSecret); createErr != nil {
return fmt.Errorf("failed creating certificate secret: %w", createErr)
}
} else {
return fmt.Errorf("failed getting certificate secret: %w", err)
}
} else if existingSecret.Type != certSecret.Type {
// Secret exists but with a different type, delete and create it again
if recreateSecret(ctx, c, existingSecret, certSecret) != nil {
return errors.New("failed to recreate secret with type corrected")
}
}
if isSecretOutdated(existingSecret.Data, certSecret.Data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, in the original code it was part of only one function, just kept the functionality as is, had a thought that it should be in both :) Thanks!

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 had the same thoughts, why at first we did not have outdated secret updated, unless some reason behind it.
Just wanna @VaishnaviHire take a look in case it should not be.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I properly understood the code, the isSecretOutdated function is invoked even if the secret is created or recreated but since the existingSecret is never updated (and there is no reason to do it), then it could lead to a spurious Update so the two if/else should probably return early.

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right.

  • in the successful Create() case, it should return immediately
  • in the recreateSecret() case, it does not need to check isSecretOutdated() as well

if err = c.Update(ctx, certSecret); err != nil { // update data if found with same type but outdated content
return fmt.Errorf("failed to update secret: %w", err)
}
}
return nil
}

// isSecretOutdated compares two secret data of type map[string][]byte and returns true if they are not equal.
func isSecretOutdated(existingSecretData, newSecretData map[string][]byte) bool {
if len(existingSecretData) != len(newSecretData) {
Expand Down
Loading