Dynamic validation of "EncryptionAtHost" feature subscription level registration at the RP. [DO NOT MERGE] #3065
Conversation
| @@ -46,3 +53,20 @@ func (dv *dynamic) validateEncryptionAtHostSupport(VMSize api.VMSize, path strin | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| func (dv *dynamic) IsRegisteredForFeature(sub *api.SubscriptionProperties) error { | |||
There was a problem hiding this comment.
minor nit:
The function name here doesn't make it clear which feature we are checking for registration for. Given that it's a method on the generic dynamic validator, I would expect it to either be named specifically for the EncryptionAtHost feature (e.g. IsSubRegisteredForEncryptionAtHostFeature), or take in the feature name as a parameter if there are places to reuse it.
| } | ||
| } | ||
|
|
||
| EncryptionAtHostEnabledOrDisabled := func(MasterEncryptionAtHost api.EncryptionAtHost, WorkerEncryptionAtHost api.EncryptionAtHost) *api.OpenShiftCluster { |
There was a problem hiding this comment.
I like the refactor here to keep the actual test cases focused on their intent, but I think the function name and syntax here is a little confusing and ends up obscuring some important information when looking at the test cases themselves, e.g. it's unclear from just reading EncryptionAtHostEnabledOrDisabled(api.EncryptionAtHostEnabled, api.EncryptionAtHostDisabled) what that test case is trying to set up.
I wonder if it might be cleaner to establish static master/workerprofile definitions, and have each test case assemble the cluster definition by composing those static members (e.g. func(masterProfile api.MasterProfile, workerProfile api.WorkerProfile)), or to just make a single, four-arg function that handles both cases.
|
Please rebase pull request. |
There was a problem hiding this comment.
This won't actually work.
The subscription document only holds features registered under the RP's namespace (Microsoft.RedHatOpenShift) and the Microsoft.Resources namespaces.
All AFEC features that the subscriptions has been registered under RP namespace and platform namespace (Microsoft.Resources).
There was a problem hiding this comment.
Quick summary of actions needed to be taken:
- Update our FPSP Built-In Service Role to include
Microsoft.Features/features/readandMicrosoft.Features/providers/features/read - The above follows SDP
- Update our local custom role in our dev environment to include the above permissions as well
- Update this PR to use the features client instead of reading from the subscription document
- Roll it out
schiruma
force-pushed
the
dynamic-validation-encryptionathost
branch
from
32ad18d
to
f12b002
Compare
schiruma
force-pushed
the
dynamic-validation-encryptionathost
branch
from
f12b002
to
eaa6175
Compare
schiruma
force-pushed
the
dynamic-validation-encryptionathost
branch
3 times, most recently
from
dc0e80c
to
230be57
Compare
schiruma
force-pushed
the
dynamic-validation-encryptionathost
branch
from
e5fb13a
to
ac075cb
Compare
|
Please rebase pull request. |
schiruma
changed the title
Which issue this PR addresses:
ARO-3211
What this PR does / why we need it:
When customers are attempting to set EncryptionAtHost, but that feature is not enabled for their Subscriptions, an error message describing about the issue and its resolution is displayed to them. This is achieved by dynamically validating their Subscription documents.
Test plan for issue:
E2E
Is there any documentation that needs to be updated for this PR?
N/A