Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mbarnes The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
TenantId is optional according to the Resource Provider Contract but the ARO-HCP backend relies on it for cluster creation. The frontend tries to ensure its presence, cherry-picking from request headers if necessary.
mbarnes
force-pushed
the
1p/ensure-tenantid-in-subscriptions
branch
from
50040f6 to
f9ccb18
Compare
| // TenantId is optional according to the Resource Provider Contract but the | ||
| // ARO-HCP backend relies on it for cluster creation. The frontend tries to | ||
| // ensure its presence, cherry-picking from request headers if necessary. | ||
| errs = append(errs, validate.RequiredValue(ctx, op, fldPath.Child("tenantId"), &newObj.TenantId, nil)...) |
There was a problem hiding this comment.
are there ways newObj can be null?
i know you populate it in frontend.go but other paths might lead to here eventually via ValidateSubscriptionCreate
There was a problem hiding this comment.
check for RequiredPointer as well
| if requestSubscription.Properties == nil { | ||
| requestSubscription.Properties = &arm.SubscriptionProperties{} | ||
| } | ||
| if requestSubscription.Properties.TenantId == nil || len(*requestSubscription.Properties.TenantId) == 0 { |
There was a problem hiding this comment.
so the request data can send various invalid states? properties nil but also properties not nil and tenant nil or tenant empty? this this a known ARM inconsistency?
There was a problem hiding this comment.
The RPC states that Properties is a required field in subscription payloads but Properties.TenantId is optional. For better or worse we have the subscription model defined entirely as pointer fields, so I'm just being careful here to avoid a panic. Such inconsistencies would be more likely, I think, in a test scenario than an actual request from ARM.
|
@mbarnes: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Followup to a comment in #4784
What
This changes subscription validation to require the
TenantIdfield.Why
In a subscription PUT request, the
TenantIdfield is optional according to the Resource Provider Contract but the ARO-HCP backend relies on it for cluster creation. Therefore we want to make sure we never write a subscription document to Cosmos without aTenantIdvalue. The frontend tries to ensure its presence before validating, filling it in from theX-Ms-Home-Tenant-Idrequest header if necessary.It's unclear whether a subscription registration without a
TenantIdvalue would ever actually occur outside of test environments. This may be purely academic, but it ensures an invariant in our Cosmos DB data.Testing
Several existing integration tests were updated to reflect the automatic addition of a tenant ID value from request headers.