Dynamic validation of "EncryptionAtHost" feature subscription level registration at the RP. #3134
Conversation
schiruma
changed the title
schiruma
requested review from
jewzaam,
bennerv,
hawkowl,
rogbas,
petrkotas,
jharrington22,
cblecker,
facchettos,
cadenmarchese,
UlrichSchlueter,
s-amann,
SudoBrendan,
Shivkumar13,
yjst2012,
anshulvermapatel and
hlipsig
as code owners
|
Please rebase pull request. |
schiruma
force-pushed
the
frontend-validation-encryptionathost
branch
from
3010637 to
5641826
Compare
schiruma
force-pushed
the
frontend-validation-encryptionathost
branch
from
5641826 to
44c5b04
Compare
schiruma
force-pushed
the
frontend-validation-encryptionathost
branch
from
229927d to
0e64115
Compare
schiruma
force-pushed
the
frontend-validation-encryptionathost
branch
from
0e64115 to
83423f2
Compare
|
Please rebase pull request. |
schiruma
force-pushed
the
frontend-validation-encryptionathost
branch
from
83423f2 to
8af04f1
Compare
There was a problem hiding this comment.
The validation code looks good to me, I'd just make sure we have an approval from @kimorris27 before proceeding since it makes a change to the vendored azidentity.
| if err != nil { | ||
| return err | ||
| } | ||
| if *response.Properties.State == armfeatures.SubscriptionFeatureRegistrationStateRegistered { |
There was a problem hiding this comment.
Just to make sure we don't get any surprise nil pointer dereferences, let's check for nil pointers as needed before evaluating *response.Properties.State.
There was a problem hiding this comment.
@kimorris27, In the new commit, since we are returning nil by default it shouldn't cause any nil pointer dereferences. Let me know your thoughts.
There was a problem hiding this comment.
I see that you flipped the conditional in response to @cadenmarchese 's comment, but that won't stop us from potentially encountering a nil pointer dereference here. I would still check for nil pointers before trying if *response.Properties.State ....
There was a problem hiding this comment.
Looking good, but I hadn't noticed when I looked last week that you had imported the subscription features client from azure-sdk-for-go's sdk package. My requested change is to import the client from the services package, which I gave a link to in the more detailed comment.
|
|
||
| "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" | ||
| "github.com/Azure/azure-sdk-for-go/sdk/azidentity" | ||
| "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armfeatures" |
There was a problem hiding this comment.
Rather than importing the client from the sdk package, I think we should use the one from the services package (even though it's currently deprecated) to be consistent with the rest of the Azure clients: https://pkg.go.dev/github.com/Azure/azure-sdk-for-go@v63.1.0+incompatible/services/resources/mgmt/2021-07-01/features#SubscriptionFeatureRegistrationsClient.
Aside from keeping things consistent throughout the azureclient package, this will help make sure that we pass all of the correct options when instantiating the client in NewSubscriptionFeatureRegistrationsClient. For example, each Azure client needs a base URI telling it which Azure cloud it's working in (public, US Gov, German, etc.). (There may be other important settings, but this is just one that I noticed needs to be set.) It looks like on the clients from the sdk package, you could control these sorts of settings through the last argument (ClientOptions, I think?), but this PR as written wouldn't work in any cloud other than the public cloud, since the public cloud is the default and we're passing nil to that options argument.
For now, we can mimic how the other clients do things to make sure we do these types of things correctly. I think this will also allow you to pass an autorest.Authorizer rather a client certificate credential, which is another way to be consistent with the other clients.
For an example of what I'm talking about, see:
There was a problem hiding this comment.
As part of the track2 migration (which we haven't started but at some point will), we will move over clients one-by-one to the new track2. I would prefer any net-new work uses the newer track2 sdk package.
Otherwise we're going to come back to this and "reimplement" it using the newer track2 sdk.
There was a problem hiding this comment.
That makes sense. In that case, the only change needed here is to make sure that when the client gets instantiated, we pass options to make sure it uses the correct cloud, etc.
There was a problem hiding this comment.
@bennerv thank you for this context as it will make sense to anyone looking at this code going forward.
|
Please rebase pull request. |
cblecker
added
needs-rebase
|
Closing this and moving to an updated branch in #3826 |
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 in the RP's frontend.
Test plan for issue:
E2E
Is there any documentation that needs to be updated for this PR?
N/A