Add Azure resource ID to protos#15673
Conversation
GavinFrazar
changed the base branch from
gavinfrazar/azure_mysql_postgres_auto_discovery_config_create
to
gavinfrazar/azure_mysql_postgres_auto_discovery_configuration
github-actions
Bot
added
the
tsh
| // Region is an Azure cloud region. | ||
| string Region = 2 [ (gogoproto.jsontag) = "region,omitempty" ]; | ||
| // ResourceID is the Azure fully qualified ID for the resource. | ||
| AzureResourceID ResourceID = 3 |
There was a problem hiding this comment.
nit: ResourceID name indicate that this will be some kind of uniq identifier but instead this filed contains struct with other field that makes a resourceID so would would you say about renaming this field to AzureResource
There was a problem hiding this comment.
Actually, I think for future-proof we should just make this ResourceID a string. I like keeping the fields separate for ease of use and readability, but as I looked at the arm.ResourceID definition I realized that the ID can be nested.
There was a problem hiding this comment.
The armmysql.Server and armpostgresql.Server have comments that imply the ID can only be of the form /subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/{resourceProviderNamespace}/{resourceType}/{resourceName} though. I'm guessing that the nested form doesn't apply to databases then - what do you guys think?
There was a problem hiding this comment.
I think the current way is fine once the struct name is updated. If one day we run into the nested ID, it can be dealt with separately.
Though I think the real question is what we need these values for, after types.Database is created. I checked the big PR I don't see any use for it yet.
So I may suggest adding these later when there is a use case.
There was a problem hiding this comment.
I needed the resource group for making the metadata watcher, since armmysql.ServersClient and armpostgresql.ServersClient require the resource name and group to fetch a resource. The resource group is only specified in the resource ID string, which requires parsing. I added all the relevant fields from parsing the ID so I could just parse once, but the only one I need so far is that resource group. I also added the region, but this has no use right now either.
All that being said I'm not sure if we even have a use-case for metadata watcher service for Azure. We do need the server to have an Azure Active Directory admin set to enable AAD auth. If a user ever unsets the AD admin on the database server then AAD is effectively disabled. So this metadata watcher could act like it does for RDS and detect if AAD auth is disabled. I'm just not sure if that's useful since a user must still login manually and create the user/role in the database identified by the service principal ID - and if we "reenable" AAD auth by setting an AD admin, then we would have to set the admin to the service principal itself...
There was a problem hiding this comment.
The metadata service is not designed as a watcher currently. The metadata service is only run once per new database (with the exception of ElastiCache and MemoryDB but you can ignore). I believe the metadata service was mainly designed to fetch metadata for non-auto-discovered databases (e.g. from static files), since auto-discovered databases usually come with proper metadata. The auto-discovery watchers should detect any metadata change and the reconciler can push a new database.
Do we have a plan on "auto" enabling AAD auth? Maybe we can revisit this once we work on that.
Is anything else requiring the ResourceID apart from the metadata service?
There was a problem hiding this comment.
I don't think anything else needs the resource ID. I'll just hold off on changing the protobuf if we want to revisit this in the future. I think we should eventually automate the creation of a role/user in the database itself, but we don't do that for AWS right now either.
|
Based on discussions with @smallinsky, I'm going to add the meta data fetching for Azure in meta.go, to check if Azure AD Auth is enabled. This will require querying for whether the Azure AD Admin is set (to anyone). Eventually I will need to automate setting the AD admin to enable AAD auth. I changed the protobuf in this PR to just include the resource id string itself, since that is the most flexible future-proof option imo. If you look at the AWS protos, we just use the string ARN instead of parsing it into metadata fields. edit: I also removed the "Region" metadata since it serves no foreseeable purpose in Azure for meta data fetching. |
GavinFrazar
deleted the
gavinfrazar/azure_mysql_postgres_auto_discovery_proto
branch
* Add Azure auto-discovery configuration fields * Init databases if azure matchers are in config * Use AzureMatchers in db service * Use all azure subscriptions/resource groups if omitted in matcher * Add azure config tests * Update protobuf and fix database serialization * Update azure database spec/status * Change proto to use resource id string * Fix database serialization test
* Azure mysql postgres auto discovery configuration (#15629) * Add Azure auto-discovery configuration fields * Init databases if azure matchers are in config * Use AzureMatchers in db service * Use all azure subscriptions/resource groups if omitted in matcher * Add azure config tests * Update lib/services/matchers.go Co-authored-by: Krzysztof Skrzętnicki <krzysztof.skrzetnicki@goteleport.com> * Update lib/config/fileconf.go Co-authored-by: Marek Smoliński <marek@goteleport.com> * Update lib/config/fileconf.go Co-authored-by: Marek Smoliński <marek@goteleport.com> * Update lib/services/matchers.go Co-authored-by: Marek Smoliński <marek@goteleport.com> * Remove superfluous cmp option for diffing azure matcher * Rename AzureMatchers Tags to ResourceTags * Deduplicate subscription/resource groups and add tests * Remove azure matcher config fixup Co-authored-by: Krzysztof Skrzętnicki <krzysztof.skrzetnicki@goteleport.com> Co-authored-by: Marek Smoliński <marek@goteleport.com> * Azure mysql postgres auto discovery config create (#15630) * Add Azure auto-discovery configuration fields * Init databases if azure matchers are in config * Use AzureMatchers in db service * Use all azure subscriptions/resource groups if omitted in matcher * Add azure config tests * Add config create flags for azure matchers * Add config create tests for azure * Move discovery flags for azure below aws * Fixup merge * Add Azure resource ID to protos (#15673) * Add Azure auto-discovery configuration fields * Init databases if azure matchers are in config * Use AzureMatchers in db service * Use all azure subscriptions/resource groups if omitted in matcher * Add azure config tests * Update protobuf and fix database serialization * Update azure database spec/status * Change proto to use resource id string * Fix database serialization test Co-authored-by: Krzysztof Skrzętnicki <krzysztof.skrzetnicki@goteleport.com> Co-authored-by: Marek Smoliński <marek@goteleport.com>
3 participants
This PR updates the protobuf for Azure database metadata to include the full resource ID, which will be parsed from the resource ID in form
/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/{resourceProviderNamespace}/{resourceType}/{resourceName}Mostly protobuf only changes, but I did have to fix a test for serialization which was affected by the metadata change.
This is a part of a larger issue to implement Azure MySQL/PostgreSQL auto-discovery: #14688
This PR branched off #15629 so github will automatically retarget the base branch to master when that PR merges.