Merge Azure MSI Node Resolver into Attestor #3272
Conversation
Signed-off-by: Andrew Harding <[email protected]>
azdagron
requested review from
evan2645,
amartinezfayo,
MarcosDY and
rturner3
as code owners
Signed-off-by: Andrew Harding <[email protected]>
Signed-off-by: Andrew Harding <[email protected]>
|
Many thanks to the folks from UFCG (@SilvaMatteus, Fernando, Eduardo and Anderson) for testing these changes! |
There was a problem hiding this comment.
This looks great, thanks @azdagron!
I only have some small suggestions. Could we also update server_full.conf with a note in the NodeResolver "azure_msi sample indicating that it has been deprecated?
| // If credentials are not configured and selectors won't be gathered. | ||
| // TODO: make this an error condition in a future release | ||
| if client == nil { | ||
| p.log.Warn("No client credentials available for tenant. Selectors will not be produced by the node attestor for this node. This will be an error in a future release.", |
There was a problem hiding this comment.
Not a blocker, but I think that it would be great to have this covered in the tests. Other misconfigurations like the use of both MSI and app authentication could also gain more coverage.
There was a problem hiding this comment.
Good call. I've expanded coverage to test that in the absence of creds:
- This warning gets logged
- Attestation still succeeds
I also added the test for misconfiguration when both creds are supplied.
There was a problem hiding this comment.
Oh, and also fixed flakiness due to nondeterministic ordering of processing of the tenant map.
Signed-off-by: Andrew Harding <[email protected]>
Signed-off-by: Andrew Harding <[email protected]>
Signed-off-by: Andrew Harding <[email protected]>
This PR merges the selector gathering functionality from the Azure MSI NodeResolver into the NodeAttestor in preparation for the removal of the NodeResolver in 1.5.0.
There are supported two cases:
If the Node Resolver is configured, deprecation warnings will be emitted. If both the NodeAttestor and NodeResolver produce selectors, they will be merged without duplication.
There is one small change to the configuration of the NodeAttestor as it existed in the NodeResolver plugin. The
use_msiconfigurable has now been moved into the per-tenant configuration. Since each tenant must be specified anyway (a departure from the NodeResolver), and the MSI credentials will only work with at most one tenant, it made sense to configure which tenant should authenticate with MSI token.