Add new interfaces to support operations on nested credentials

[gouthamMN]

JIRA link:
https://issues.redhat.com/browse/ARO-11172

What does this PR do?
This PR adds following interfaces to support operations on nested credentials as outlined in the DDR.

• Interfaces:
  • GetNestedCredentialsObject
  • GetDeletedNestedCredentialsObject
  • SetNestedCredentialsObject
• Objects:
  • secretObject
  • deletedSecretObject

Testing:

• New unit tests are added to cover the new interfaces and objects.

[stevekuznetsov]

Unless this method and its siblings is adding some value for the caller - e.g. taking in the credentials object and calculating the storage path - we should omit the method entirely.

[gouthamMN]

type MsiKeyVaultStore struct {
	kvClient KeyVaultClient
}

I agree with your point, but since the kvClient is private and won't be able to access outside the store package this might have to be redesigned altogether. I think original author of this was @carvalhe and tagging him here to understand the rationale behind this.

@stevekuznetsov - So, If I understand your ask properly then the possible solution is to make kvClient a public as this would let the client to directly access KV interface methods. But with this we might introduce inconsistency in the way we let the client access our methods.

[stevekuznetsov]

I think all of the users of this library will need to impose some keyvault key naming convention by which a specific credential or nested credential can be stored and retrieved by knowing only some identifying characteristics of the MSI. I imagine both the refresher and the clusters-service will furthermore need to agree on this pattern. We can either omit the storage piece from this library and leave it up to the end-clients or we can add value to the storage piece by amending our interfaces not to act over opaque secretNames.

[gouthamMN]

We can either omit the storage piece from this library and leave it up to the end-clients.

This approach needs to be discussed with RH as this will impact Classic MIWI clusters changes.

we can add value to the storage piece by amending our interfaces not to act over opaque secretNames.

Is the ask is to create a separate interface for cred obj and nested cred obj? eg:DeleteCredentialsObject() & DeleteNestedCredentialsObject()?

[stevekuznetsov]

Let's see how the clients use the library and decide what makes the most sense for the interface.

[kimorris27]

For MIWI classic, we're currently doing exactly what @stevekuznetsov described in an earlier comment; we impose a naming convention for the KV keys by using a utility function that takes in the cluster doc and returns the secret name, call the utility function to get the name, and then delete by name: https://github.com/Azure/ARO-RP/blob/b534984ce591b208ded342d8e1538f1fce69d837/pkg/cluster/delete.go#L372

In classic we only need to delete CredentialsObjects and not NestedCredentialsObjects though. Maybe someone from HCP would be able to provide more valuable input here.

[bennerv]

Do we ever use the result of the deletion call? I would assume not because the backing credential is actually deleted.

For HCP, CS wanted to prefix the secret name with either nco or co representing nested credential object and credential object respectively.

ARO Classic is storing the raw credentials object with the below contents, where the docID is a guid and the name is the name of the resource ID of the managed identity.

	return fmt.Sprintf("%s-%s", m.doc.ID, clusterMsi.Name), nil

Do we need to force consistent naming across the two, or can we have the refresher and ARO HCP (cluster service) leverage nco and assume everything else is a credentials object? Also is there any way to abstract the deletion to exist for both types and the response is either just a string or we don't care about it?

If we'd like consistent prefix/suffix across 3 repos (cluster service, aro classic, and msi refresher) should we move the naming prefix into this library and return the secret name back, or do we not care as a client?

[stevekuznetsov]

If we'd like consistent prefix/suffix across 3 repos (cluster service, aro classic, and msi refresher) should we move the naming prefix into this library and return the secret name back

That is my question. I assumed the answer was yes - please provide your opinions :)

[bennerv]

I think we still want to expose a function in pkg/client to support authenticating as a NestedCredentialsObject instead of a CredentialsObject. By moving the authentication logic into a package here, we can ensure consistency for authenticating (i.e. setting regional endpoint, disabling instance metadata discovery, etc.)

[gouthamMN]

I think we still want to expose a function in pkg/client to support authenticating as a NestedCredentialsObject instead of a CredentialsObject. By moving the authentication logic into a package here, we can ensure consistency for authenticating (i.e. setting regional endpoint, disabling instance metadata discovery, etc.)

@bennerv - Is it ok, If I do this in a separate PR to keep this PR simple/small?

[bennerv]

@bennerv - Is it ok, If I do this in a separate PR to keep this PR simple/small?

Sgtm, I think @bryan-cox will be interested in those when they come assuming he bases the shared function off of that functionality.

[gouthamMN]

Sgtm, I think @bryan-cox will be interested in those when they come assuming he bases the shared function off of that functionality.

I wasn't able to find you under reviewers using your GitHub ID but here is the PR #30