*: update to use the new msi-dataplane library #4047
Conversation
Signed-off-by: Steve Kuznetsov <[email protected]>
stevekuznetsov
requested review from
jewzaam,
bennerv,
hawkowl,
rogbas,
petrkotas,
jharrington22,
cblecker,
cadenmarchese,
UlrichSchlueter,
SudoBrendan,
yjst2012,
jaitaiwan,
anshulvermapatel,
hlipsig,
mociarain,
kimorris27,
tsatam,
bitoku,
fahlmant and
sankur-codes
as code owners
|
Update to use Azure/msi-dataplane#34 |
| } | ||
| var msiDataplane dataplane.ClientFactory | ||
| if _env.FeatureIsSet(env.FeatureUseMockMsiRp) { | ||
| msiDataplane = _env.MockMSIResponses(msiResourceId) |
There was a problem hiding this comment.
We need to mock out the MSI dataplane, so we can just provide a different implementation for the interface. The previous stub approach of injecting data through an HTTP transport was fragile - it, for one, did not make any distinction between which method was being called!
| } else if azcoreErr, ok := err.(*azcore.ResponseError); !ok || azcoreErr.StatusCode != http.StatusNotFound { | ||
| return err | ||
| } | ||
| // TODO: why are we fetching from the keyvault but ignoring the output? |
There was a problem hiding this comment.
Kipp, could you speak to this?
Signed-off-by: Steve Kuznetsov <[email protected]>
stevekuznetsov
force-pushed
the
skuznets/update-msi-dataplane
branch
from
b000883 to
feb1d5e
Compare
| @@ -41,6 +41,7 @@ type Manager interface { | |||
| GetCertificatePolicy(ctx context.Context, certificateName string) (azkeyvault.CertificatePolicy, error) | |||
| GetCertificateSecret(context.Context, string) (*rsa.PrivateKey, []*x509.Certificate, error) | |||
| GetSecret(context.Context, string) (azkeyvault.SecretBundle, error) | |||
| DeleteSecret(ctx context.Context, secretName string) (azkeyvault.DeletedSecretBundle, error) | |||
There was a problem hiding this comment.
I didn't fully understand all the different levels of wrapping and mocking going on here since I'm new to the repo, so please let me know if this is not the correct way to go about this. I needed a KeyVault client + mock since msi-dataplane lets you bring your own now.
| //go:generate mockgen -destination=../mocks/$GOPACKAGE/client_factory.go github.com/Azure/msi-dataplane/pkg/dataplane ClientFactory | ||
| //go:generate goimports -local=github.com/Azure/ARO-RP -e -w ../mocks/$GOPACKAGE/client_factory.go | ||
|
|
||
| // mockgen unfortunately walks the type aliases in the dataplane package, and tries to import their |
There was a problem hiding this comment.
This is an unfortunate limitation of mockgen, so we incur some manual work to update the mock for the client in the future if it changes the interface.
| @@ -12,14 +12,12 @@ import ( | |||
|
|
|||
| sdkmsi "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/msi/armmsi" | |||
| "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork/v2" | |||
| "github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets" | |||
There was a problem hiding this comment.
I don't fully understand why the msi-dataplane repo used to use a client based on this azsecrets package, whereas this repo uses the clients based on the keyvault one.
| } | ||
|
|
||
| msiCredObj, err := m.msiDataplane.GetUserAssignedIdentities(ctx, uaMsiRequest) | ||
| msiCredObj, err := client.GetUserAssignedIdentitiesCredentials(ctx, uaMsiRequest) |
There was a problem hiding this comment.
Kipp, the names of every client method and the names of the underlying types are up in the air - I couldn't get over how un-helpful NestedCredentialObject was, and I hope I've improved the situation, but please feel free to give your preference for any of these names. Field names and their JSON encodings are of course locked down, but the top-level type names and method names are not.
| var _ dataplane.Client = (*mockClient)(nil) | ||
|
|
||
| func (m *mockClient) DeleteSystemAssignedIdentity(ctx context.Context) error { | ||
| panic("not yet implemented") |
There was a problem hiding this comment.
The previous transport-based approach hid that the mock MSI could not correctly respond to these other client calls. Up to you what we do with these.
|
Please rebase pull request. |
go.mod: bump to new Azure/msi-dataplane
Signed-off-by: Steve Kuznetsov [email protected]
*: update to use the new msi-dataplane library
Signed-off-by: Steve Kuznetsov [email protected]