-
Notifications
You must be signed in to change notification settings - Fork 191
Dynamic Validator Refactor for Operator Compatibility #1244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
mjudeikis
merged 10 commits into
Azure:master
from
bennerv:dynamic-validator-exploration
Jan 20, 2021
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
c8e4747
revert #1234
54884d4
remove authorizer refreshing from validateActions
a3e72ff
simplify: add findSubnet
6fd876b
optimisation: check each route table once in validateRouteTablesPermi…
335b599
optimisation: use map instead of slice for uniqueness
840b256
remove out of date comment
690c767
WIP
4255328
WIP - first pass on dynamic still has oc document dependency
bennerv b1e398e
Remove oc document dependencies from dynamic so it can be called in o…
bennerv 7d91fe9
Refactor dynamic and ocp validate dynamic tests to pass
bennerv File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| package validate | ||
|
|
||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the Apache License 2.0. | ||
|
|
||
| import ( | ||
| "context" | ||
|
|
||
| mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-07-01/network" | ||
| ) | ||
|
|
||
| type virtualNetworksGetClient interface { | ||
| Get(context.Context, string, string, string) (mgmtnetwork.VirtualNetwork, error) | ||
| } | ||
|
|
||
| type virtualNetworksCacheKey struct { | ||
| resourceGroupName string | ||
| virtualNetworkName string | ||
| expand string | ||
| } | ||
|
|
||
| type virtualNetworksCache struct { | ||
| c virtualNetworksGetClient | ||
| m map[virtualNetworksCacheKey]mgmtnetwork.VirtualNetwork | ||
| } | ||
|
|
||
| func (vnc *virtualNetworksCache) Get(ctx context.Context, resourceGroupName string, virtualNetworkName string, expand string) (mgmtnetwork.VirtualNetwork, error) { | ||
| if _, ok := vnc.m[virtualNetworksCacheKey{resourceGroupName, virtualNetworkName, expand}]; !ok { | ||
| vnet, err := vnc.c.Get(ctx, resourceGroupName, virtualNetworkName, expand) | ||
| if err != nil { | ||
| return vnet, err | ||
| } | ||
|
|
||
| vnc.m[virtualNetworksCacheKey{resourceGroupName, virtualNetworkName, expand}] = vnet | ||
| } | ||
|
|
||
| return vnc.m[virtualNetworksCacheKey{resourceGroupName, virtualNetworkName, expand}], nil | ||
| } | ||
|
|
||
| // newVirtualNetworksCache returns a new virtualNetworksCache. It knows nothing | ||
| // about updates and is not thread-safe, but it does retry in the face of | ||
| // errors. | ||
| func newVirtualNetworksCache(c virtualNetworksGetClient) virtualNetworksGetClient { | ||
| return &virtualNetworksCache{ | ||
| c: c, | ||
| m: map[virtualNetworksCacheKey]mgmtnetwork.VirtualNetwork{}, | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,228 @@ | ||
| package validate | ||
|
|
||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the Apache License 2.0. | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "net/http" | ||
| "sort" | ||
| "strings" | ||
| "time" | ||
|
|
||
| mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-07-01/network" | ||
| "github.com/Azure/go-autorest/autorest" | ||
| "github.com/Azure/go-autorest/autorest/azure" | ||
| "github.com/sirupsen/logrus" | ||
| "k8s.io/apimachinery/pkg/util/wait" | ||
|
|
||
| "github.com/Azure/ARO-RP/pkg/api" | ||
| "github.com/Azure/ARO-RP/pkg/env" | ||
| "github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/authorization" | ||
| "github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/network" | ||
| utilpermissions "github.com/Azure/ARO-RP/pkg/util/permissions" | ||
| "github.com/Azure/ARO-RP/pkg/util/refreshable" | ||
| "github.com/Azure/ARO-RP/pkg/util/steps" | ||
| "github.com/Azure/ARO-RP/pkg/util/subnet" | ||
| ) | ||
|
|
||
| // SlimDynamic validate in the operator context. | ||
| type SlimDynamic interface { | ||
| ValidateVnetPermissions(ctx context.Context) error | ||
| ValidateRouteTablesPermissions(ctx context.Context) error | ||
| ValidateVnetDns(ctx context.Context) error | ||
| // etc | ||
| // does Quota code go in here too? | ||
| } | ||
|
|
||
| type dynamic struct { | ||
| log *logrus.Entry | ||
| vnetr *azure.Resource | ||
| masterSubnetID string | ||
| workerSubnetIDs []string | ||
|
|
||
| code string | ||
| typ string | ||
|
|
||
| permissions authorization.PermissionsClient | ||
| virtualNetworks virtualNetworksGetClient | ||
| } | ||
|
|
||
| func NewValidator(log *logrus.Entry, env env.Interface, masterSubnetID string, workerSubnetIDs []string, subscriptionID string, authorizer refreshable.Authorizer, code string, typ string) (*dynamic, error) { | ||
| vnetID, _, err := subnet.Split(masterSubnetID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| vnetr, err := azure.ParseResourceID(vnetID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &dynamic{ | ||
| log: log, | ||
| vnetr: &vnetr, | ||
| masterSubnetID: masterSubnetID, | ||
| workerSubnetIDs: workerSubnetIDs, | ||
|
|
||
| code: code, | ||
| typ: typ, | ||
|
|
||
| permissions: authorization.NewPermissionsClient(env.Environment(), subscriptionID, authorizer), | ||
| virtualNetworks: newVirtualNetworksCache(network.NewVirtualNetworksClient(env.Environment(), subscriptionID, authorizer)), | ||
| }, nil | ||
| } | ||
|
|
||
| func (dv *dynamic) ValidateVnetPermissions(ctx context.Context) error { | ||
| dv.log.Printf("ValidateVnetPermissions (%s)", dv.typ) | ||
|
|
||
| err := dv.validateActions(ctx, dv.vnetr, []string{ | ||
| "Microsoft.Network/virtualNetworks/join/action", | ||
| "Microsoft.Network/virtualNetworks/read", | ||
| "Microsoft.Network/virtualNetworks/write", | ||
| "Microsoft.Network/virtualNetworks/subnets/join/action", | ||
| "Microsoft.Network/virtualNetworks/subnets/read", | ||
| "Microsoft.Network/virtualNetworks/subnets/write", | ||
| }) | ||
|
|
||
| if err == wait.ErrWaitTimeout { | ||
| return api.NewCloudError(http.StatusBadRequest, dv.code, "", "The %s does not have Network Contributor permission on vnet '%s'.", dv.typ, dv.vnetr) | ||
| } | ||
| if detailedErr, ok := err.(autorest.DetailedError); ok && | ||
| detailedErr.StatusCode == http.StatusNotFound { | ||
| return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedVNet, "", "The vnet '%s' could not be found.", dv.vnetr) | ||
| } | ||
| return err | ||
| } | ||
|
|
||
| func (dv *dynamic) ValidateRouteTablesPermissions(ctx context.Context) error { | ||
| vnet, err := dv.virtualNetworks.Get(ctx, dv.vnetr.ResourceGroup, dv.vnetr.ResourceName, "") | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| m := map[string]string{} | ||
|
|
||
| rtID, err := getRouteTableID(&vnet, "properties.masterProfile.subnetId", dv.masterSubnetID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if rtID != "" { | ||
| m[strings.ToLower(rtID)] = "properties.masterProfile.subnetId" | ||
| } | ||
|
|
||
| for i, s := range dv.workerSubnetIDs { | ||
| path := fmt.Sprintf("properties.workerProfiles[%d].subnetId", i) | ||
|
|
||
| rtID, err := getRouteTableID(&vnet, path, s) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if _, ok := m[strings.ToLower(rtID)]; ok || rtID == "" { | ||
| continue | ||
| } | ||
| m[strings.ToLower(rtID)] = path | ||
| } | ||
|
|
||
| rts := make([]string, 0, len(m)) | ||
| for rt := range m { | ||
| rts = append(rts, rt) | ||
| } | ||
|
|
||
| sort.Slice(rts, func(i, j int) bool { return strings.Compare(m[rts[i]], m[rts[j]]) < 0 }) | ||
|
|
||
| for _, rt := range rts { | ||
| err := dv.validateRouteTablePermissions(ctx, rt, m[rt]) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (dv *dynamic) validateRouteTablePermissions(ctx context.Context, rtID string, path string) error { | ||
| dv.log.Printf("validateRouteTablePermissions(%s, %s)", dv.typ, path) | ||
|
|
||
| rtr, err := azure.ParseResourceID(rtID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| err = dv.validateActions(ctx, &rtr, []string{ | ||
| "Microsoft.Network/routeTables/join/action", | ||
| "Microsoft.Network/routeTables/read", | ||
| "Microsoft.Network/routeTables/write", | ||
| }) | ||
| if err == wait.ErrWaitTimeout { | ||
| return api.NewCloudError(http.StatusBadRequest, dv.code, "", "The %s does not have Network Contributor permission on route table '%s'.", dv.typ, rtID) | ||
| } | ||
| if detailedErr, ok := err.(autorest.DetailedError); ok && | ||
| detailedErr.StatusCode == http.StatusNotFound { | ||
| return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedRouteTable, "", "The route table '%s' could not be found.", rtID) | ||
| } | ||
| return err | ||
| } | ||
|
|
||
| func (dv *dynamic) ValidateVnetDNS(ctx context.Context) error { | ||
| dv.log.Print("validateVnetDns") | ||
|
|
||
| vnet, err := dv.virtualNetworks.Get(ctx, dv.vnetr.ResourceGroup, dv.vnetr.ResourceName, "") | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if vnet.DhcpOptions != nil && | ||
| vnet.DhcpOptions.DNSServers != nil && | ||
| len(*vnet.DhcpOptions.DNSServers) > 0 { | ||
| return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedVNet, "", "The provided vnet '%s' is invalid: custom DNS servers are not supported.", *vnet.ID) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (dv *dynamic) validateActions(ctx context.Context, r *azure.Resource, actions []string) error { | ||
| timeoutCtx, cancel := context.WithTimeout(ctx, 2*time.Minute) | ||
| defer cancel() | ||
|
|
||
| return wait.PollImmediateUntil(10*time.Second, func() (bool, error) { | ||
| permissions, err := dv.permissions.ListForResource(ctx, r.ResourceGroup, r.Provider, "", r.ResourceType, r.ResourceName) | ||
|
|
||
| if detailedErr, ok := err.(autorest.DetailedError); ok && | ||
| detailedErr.StatusCode == http.StatusForbidden { | ||
| return false, steps.ErrWantRefresh | ||
| } | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| for _, action := range actions { | ||
| ok, err := utilpermissions.CanDoAction(permissions, action) | ||
| if !ok || err != nil { | ||
| // TODO(jminter): I don't understand if there are genuinely | ||
| // cases where CanDoAction can return false then true shortly | ||
| // after. I'm a little skeptical; if it can't happen we can | ||
| // simplify this code. We should add a metric on this. | ||
| return false, err | ||
| } | ||
| } | ||
|
|
||
| return true, nil | ||
| }, timeoutCtx.Done()) | ||
| } | ||
|
|
||
| func getRouteTableID(vnet *mgmtnetwork.VirtualNetwork, path, subnetID string) (string, error) { | ||
| s := findSubnet(vnet, subnetID) | ||
| if s == nil { | ||
| return "", api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedVNet, path, "The subnet '%s' could not be found.", subnetID) | ||
| } | ||
|
|
||
| if s.RouteTable == nil { | ||
| return "", nil | ||
| } | ||
|
|
||
| return *s.RouteTable.ID, nil | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please raise follow-up to try to get this generalized so this would be set by called instead in the initiation