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

Remove support for legacy user-provided service instance credentials #3520

Open
1 of 3 tasks
danail-branekov opened this issue Oct 15, 2024 · 1 comment
Open
1 of 3 tasks
Assignees
Milestone

Comments

@danail-branekov
Copy link
Member

danail-branekov commented Oct 15, 2024

In 0.12.0 we introduced support for objects in user-provided services credentials (#2900). When we have been working on that, we wanted to avoid workloads restart on korifi upgrade in order to ensure smooth upgrade path, and came up with some legacy support code.

With the upcoming release the smooth upgrade is not going to be possible as workloads have to restart, see

As there is no way around that, let's make use of it and get rid of the legacy secret format support:

  • log := logr.FromContextOrDiscard(ctx)
    log.Info("migrating legacy secret", "legacy-secret-name", credentialsSecret.Name)
    migratedSecret := &corev1.Secret{
    ObjectMeta: metav1.ObjectMeta{
    Name: cfServiceInstance.Name + "-migrated",
    Namespace: cfServiceInstance.Namespace,
    },
    }
    _, err := controllerutil.CreateOrPatch(ctx, r.k8sClient, migratedSecret, func() error {
    migratedSecret.Type = corev1.SecretTypeOpaque
    data := map[string]any{}
    for k, v := range credentialsSecret.Data {
    data[k] = string(v)
    }
    dataBytes, err := json.Marshal(data)
    if err != nil {
    log.Error(err, "failed to marshal legacy credentials secret data")
    return err
    }
    migratedSecret.Data = map[string][]byte{
    tools.CredentialsSecretKey: dataBytes,
    }
    return controllerutil.SetOwnerReference(cfServiceInstance, migratedSecret, r.scheme)
    })
    if err != nil {
    log.Error(err, "failed to create migrated credentials secret")
    return nil, err
    }
    return migratedSecret, nil

  • if isLegacyServiceBinding(cfServiceBinding, cfServiceInstance) {
    bindingSecret := &corev1.Secret{
    ObjectMeta: metav1.ObjectMeta{
    Name: cfServiceBinding.Status.Binding.Name,
    Namespace: cfServiceBinding.Namespace,
    },
    }
    // For legacy sevice bindings we want to keep the binding secret
    // unchanged in order to avoid unexpected app restarts. See ADR 16 for more details.
    err := r.k8sClient.Get(ctx, client.ObjectKeyFromObject(bindingSecret), bindingSecret)
    if err != nil {
    return err
    }
    return nil
    }

  • Ensure that user provided services that have been created with 0.12.0 are fine
  • Ensure upsis created with 0.11.0 and reconciled by 0.12.0 are fine
  • Check the behaviour of bumping 0.11.0 straigh into the release candidate. If broken, make sure not to forget to document that 0.12.0 is a prerequisite for this release.
@danail-branekov danail-branekov self-assigned this Oct 15, 2024
danail-branekov added a commit that referenced this issue Oct 15, 2024
In 0.12.0 we have extended upsi credentials secret to be represented by
arbitrary objects, not just plain maps of strings. That required certain
migration logic to migrate the secret to the new format and avoid
workloads restart during Korifi upgrade.

With the upcoming release, workload restart would be performed anyway
because of unrelated increments. Provided that 0.12.0 should have
migrated any user-provided service instances to the new credentials
format, we want to get rid of the migration code.

fixes #3520
@danail-branekov
Copy link
Member Author

Turns out that maybe this would not that trivial :(

Imagine the following scenario:

  • UPSI created with 0.11.0. The credentials secret referenced by the UPSI is in the old format

  • Korifi bumped to 0.12.0

    • As a result of the bump, the CFServiceInstance.Spec.SecretName resource still references the initial secret (in the old format)
    • However, as a result of the migration, its CFServiceInstance.Status.Credentials.Name is set to the migrated secret with the -migrated suffix). this migrated secret is in the new format
    • NOTE that the service instance spec is not changed at this point
  • Now Korifi is bumped to the release candidate

I have not tested yet, but I am pretty sure we would see the same error if we jump from 0.11.0 to the release candidate

@danail-branekov danail-branekov added this to the v1.0.0 milestone Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🧊 Icebox
Development

No branches or pull requests

1 participant