Skip to content
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.

Sign up for GitHub

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

ARO-6425 v20240812preview validation #3563

Merged
merged 19 commits into from
May 21, 2024
Merged

ARO-6425 v20240812preview validation #3563

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
9f47ce3
convert OpenShiftClusterProperties.ServicePrincipalProfile to a pointer
yithian Apr 23, 2024
f772a1b
add a util function to check if a string is formatted like a resource ID
yithian Apr 19, 2024
4d615b3
validate PlatformWorkloadIdentityProfile for v20240812preview
yithian Apr 18, 2024
a37d25f
validate the provided PlatformWorkloadIdentities are identities
yithian Apr 19, 2024
fbae7d7
validate that service principal credentials aren't used with an identity
yithian Apr 23, 2024
4587b60
validate cluster identity and an operator identity appear together
yithian Apr 24, 2024
a1e43cc
validate that identities are user assigned identities
yithian Apr 30, 2024
f7a880b
validate the operator name isn't empty
yithian Apr 30, 2024
ebd39bf
Clarify validateIdentityXORServiceCredentials logic
yithian May 10, 2024
f54d10e
combine IdentityXORServiceCredentials and PlatformIdentitiesExist checks
yithian May 10, 2024
ca9536a
remove functions only used in one place
yithian May 13, 2024
babec42
simplify and dedup static validation for operatorname presence
yithian May 15, 2024
e12a882
avoid a possible nil pointer dereference
yithian May 15, 2024
8cf3fd9
use the ArmResource util instead of a bespoke regex to parse resource…
yithian May 16, 2024
ba316f7
Update pkg/util/arm/resources.go
yithian May 17, 2024
2d5e5d5
update terminology to match API
yithian May 17, 2024
e61cb86
update arm resource unit tests to reflect deeper subresource support
yithian May 17, 2024
8d688d8
use Azure sdk to parse resource IDs in v20240812preview static valida…
yithian May 17, 2024
294c6b1
remove duplicate ServicePrincipalProfile copy from ToExternal()
yithian May 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions pkg/api/v20240812preview/openshiftcluster_convert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,12 @@ func (c openShiftClusterConverter) ToExternal(oc *api.OpenShiftCluster) interfac
}
}

if oc.Properties.ServicePrincipalProfile != nil {
yithian marked this conversation as resolved.
Show resolved Hide resolved
out.Properties.ServicePrincipalProfile = &ServicePrincipalProfile{}
out.Properties.ServicePrincipalProfile.ClientID = oc.Properties.ServicePrincipalProfile.ClientID
out.Properties.ServicePrincipalProfile.ClientSecret = string(oc.Properties.ServicePrincipalProfile.ClientSecret)
}

if oc.Properties.PlatformWorkloadIdentityProfile != nil && oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities != nil {
out.Properties.PlatformWorkloadIdentityProfile = &PlatformWorkloadIdentityProfile{}
out.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities = make([]PlatformWorkloadIdentity, len(oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities))
Expand Down Expand Up @@ -339,8 +345,10 @@ func (c openShiftClusterConverter) ExternalNoReadOnly(_oc interface{}) {
for i := range oc.Properties.IngressProfiles {
oc.Properties.IngressProfiles[i].IP = ""
}
for i := range oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities {
oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities[i].ClientID = ""
oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities[i].ObjectID = ""
if oc.Properties.PlatformWorkloadIdentityProfile != nil {
for i := range oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities {
oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities[i].ClientID = ""
oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities[i].ObjectID = ""
}
}
}
56 changes: 56 additions & 0 deletions pkg/api/v20240812preview/openshiftcluster_validatestatic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"net"
"net/http"
"net/url"
"regexp"
"strings"

"github.com/Azure/go-autorest/autorest/azure"
Expand Down Expand Up @@ -78,6 +79,10 @@ func (sv openShiftClusterStaticValidator) validate(oc *OpenShiftCluster, isCreat
return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, "location", "The provided location '%s' is invalid.", oc.Location)
}

if err := sv.validatePlatformIdentities(oc); err != nil {
return err
}

return sv.validateProperties("properties", &oc.Properties, isCreate, architectureVersion)
}

Expand Down Expand Up @@ -110,6 +115,9 @@ func (sv openShiftClusterStaticValidator) validateProperties(path string, p *Ope
if err := sv.validateAPIServerProfile(path+".apiserverProfile", &p.APIServerProfile); err != nil {
return err
}
if err := sv.validatePlatformWorkloadIdentityProfile(path+".platformWorkloadIdentityProfile", p.PlatformWorkloadIdentityProfile); err != nil {
return err
}

if isCreate {
if len(p.WorkerProfilesStatus) != 0 {
Expand Down Expand Up @@ -412,3 +420,51 @@ func (sv openShiftClusterStaticValidator) validateDelta(oc, current *OpenShiftCl

return nil
}

func (sv openShiftClusterStaticValidator) validatePlatformWorkloadIdentityProfile(path string, pwip *PlatformWorkloadIdentityProfile) error {
// PlatformWorkloadIdentityProfile being empty is acceptable
if pwip == nil {
return nil
}

resourceIDPattern, _ := regexp.Compile("/subscriptions/.*/resourceGroups/.*/providers/.*/.*/.*")
yithian marked this conversation as resolved.
Show resolved Hide resolved

// Validate the PlatformWorkloadIdentities
for n, p := range pwip.PlatformWorkloadIdentities {
kimorris27 marked this conversation as resolved.
Show resolved Hide resolved
// the regexp is statically defined and hard-coded, we shouldn't need to check for errors
match := resourceIDPattern.Match([]byte(p.ResourceID))
if !match {
return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, fmt.Sprintf("%s.PlatformWorkloadIdentities[%d].resourceID", path, n), "ResourceID %s formatted incorrectly.", p.ResourceID)
}

if p.OperatorName == "" {
return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, fmt.Sprintf("%s.PlatformWorkloadIdentities[%d].resourceID", path, n), "Operator name is empty.")
yithian marked this conversation as resolved.
Show resolved Hide resolved
}

if strings.Split(p.ResourceID, "/")[7] != "userAssignedIdentities" {
yithian marked this conversation as resolved.
Show resolved Hide resolved
return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, fmt.Sprintf("%s.PlatformWorkloadIdentities[%d].resourceID", path, n), "Resource must be a user assigned identity.")
}
}
return nil
}

func (sv openShiftClusterStaticValidator) validatePlatformIdentities(oc *OpenShiftCluster) error {
pwip := oc.Properties.PlatformWorkloadIdentityProfile
spp := oc.Properties.ServicePrincipalProfile

if pwip == nil && spp == nil {
return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, "properties.servicePrincipalProfile", "Must provide either an identity or service principal credentials.")
}

if pwip != nil && spp != nil && (spp.ClientID != "" || spp.ClientSecret != "") {
return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, "properties.servicePrincipalProfile", "Cannot use identities and service principal credentials at the same time.")
}

clusterIdentityPresent := oc.Identity != nil
operatorRolePresent := pwip != nil

if clusterIdentityPresent != operatorRolePresent {
return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, "identity", "Cluster identity and operator roles require each other.")
yithian marked this conversation as resolved.
Show resolved Hide resolved
}
return nil
}
166 changes: 166 additions & 0 deletions pkg/api/v20240812preview/openshiftcluster_validatestatic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1161,3 +1161,169 @@ func TestOpenShiftClusterStaticValidateDelta(t *testing.T) {

runTests(t, testModeUpdate, tests)
}

func TestOpenShiftClusterStaticValidatePlatformWorkloadIdentityProfile(t *testing.T) {
createTests := []*validateTest{
{
name: "valid empty workloadIdentityProfile",
modify: func(oc *OpenShiftCluster) {
oc.Properties.PlatformWorkloadIdentityProfile = nil
},
},
{
name: "valid workloadIdentityProfile",
modify: func(oc *OpenShiftCluster) {
oc.Properties.PlatformWorkloadIdentityProfile = &PlatformWorkloadIdentityProfile{
PlatformWorkloadIdentities: []PlatformWorkloadIdentity{
{
OperatorName: "FAKE-OPERATOR",
ResourceID: "/subscriptions/12345678-1234-1234-1234-123456789012/resourceGroups/a-fake-group/providers/Microsoft.RedHatOpenShift/userAssignedIdentities/fake-cluster-name",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be Microsoft.ManagedIdentity/userAssignedIdentities?
same for other test cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regexp the resourceID is being compared against is at pkg/api/v20240812preview/openshiftcluster_validatestatic.go line 430

The specific provider for the resource ID isn't being checked, just that the string is formatted like a resource ID at all. I can change the value in the test cases to Microsoft.ManagedIdentity if you'd like

},
},
}
oc.Identity = &Identity{
UserAssignedIdentities: UserAssignedIdentities{
"first": {
ClientID: "11111111-1111-1111-1111-111111111111",
PrincipalID: "SOMETHING",
},
},
}
oc.Properties.ServicePrincipalProfile = nil
},
},
{
name: "invalid resourceID",
modify: func(oc *OpenShiftCluster) {
oc.Identity = &Identity{
UserAssignedIdentities: UserAssignedIdentities{
"first": {
ClientID: "11111111-1111-1111-1111-111111111111",
PrincipalID: "SOMETHING",
},
},
}
oc.Properties.PlatformWorkloadIdentityProfile = &PlatformWorkloadIdentityProfile{
PlatformWorkloadIdentities: []PlatformWorkloadIdentity{
{
OperatorName: "FAKE-OPERATOR",
ResourceID: "BAD",
},
},
}
oc.Properties.ServicePrincipalProfile = nil
},
wantErr: "400: InvalidParameter: properties.platformWorkloadIdentityProfile.PlatformWorkloadIdentities[0].resourceID: ResourceID BAD formatted incorrectly.",
},
{
name: "wrong resource type",
modify: func(oc *OpenShiftCluster) {
oc.Properties.PlatformWorkloadIdentityProfile = &PlatformWorkloadIdentityProfile{
PlatformWorkloadIdentities: []PlatformWorkloadIdentity{
{
OperatorName: "FAKE-OPERATOR",
ResourceID: "/subscriptions/12345678-1234-1234-1234-123456789012/resourceGroups/a-fake-group/providers/Microsoft.RedHatOpenShift/otherThing/fake-cluster-name",
},
},
}
oc.Properties.ServicePrincipalProfile = nil
oc.Identity = &Identity{
UserAssignedIdentities: UserAssignedIdentities{
"first": {
ClientID: "11111111-1111-1111-1111-111111111111",
PrincipalID: "SOMETHING",
},
},
}
},
wantErr: "400: InvalidParameter: properties.platformWorkloadIdentityProfile.PlatformWorkloadIdentities[0].resourceID: Resource must be a user assigned identity.",
},
{
name: "no credentials with identities",
modify: func(oc *OpenShiftCluster) {
oc.Properties.PlatformWorkloadIdentityProfile = &PlatformWorkloadIdentityProfile{
PlatformWorkloadIdentities: []PlatformWorkloadIdentity{
{
OperatorName: "FAKE-OPERATOR",
ResourceID: "/subscriptions/12345678-1234-1234-1234-123456789012/resourceGroups/a-fake-group/providers/Microsoft.RedHatOpenShift/userAssignedIdentities/fake-cluster-name",
},
},
}
oc.Identity = &Identity{
UserAssignedIdentities: UserAssignedIdentities{
"first": {
ClientID: "11111111-1111-1111-1111-111111111111",
PrincipalID: "SOMETHING",
},
},
}
oc.Properties.ServicePrincipalProfile = &ServicePrincipalProfile{
ClientID: "11111111-1111-1111-1111-111111111111",
ClientSecret: "BAD",
}
},
wantErr: "400: InvalidParameter: properties.servicePrincipalProfile: Cannot use identities and service principal credentials at the same time.",
},
{
name: "cluster identity missing operator identity",
modify: func(oc *OpenShiftCluster) {
oc.Identity = &Identity{
UserAssignedIdentities: UserAssignedIdentities{
"first": {
ClientID: "11111111-1111-1111-1111-111111111111",
PrincipalID: "SOMETHING",
},
},
}
},
wantErr: "400: InvalidParameter: identity: Cluster identity and operator roles require each other.",
},
{
name: "operator identity missing cluster identity",
modify: func(oc *OpenShiftCluster) {
oc.Properties.PlatformWorkloadIdentityProfile = &PlatformWorkloadIdentityProfile{
PlatformWorkloadIdentities: []PlatformWorkloadIdentity{
{
OperatorName: "operator_name",
},
},
}
oc.Properties.ServicePrincipalProfile = nil
},
wantErr: "400: InvalidParameter: identity: Cluster identity and operator roles require each other.",
},
{
name: "operator name missing",
modify: func(oc *OpenShiftCluster) {
oc.Identity = &Identity{
UserAssignedIdentities: UserAssignedIdentities{
"first": {
ClientID: "11111111-1111-1111-1111-111111111111",
PrincipalID: "SOMETHING",
},
},
}
oc.Properties.PlatformWorkloadIdentityProfile = &PlatformWorkloadIdentityProfile{
PlatformWorkloadIdentities: []PlatformWorkloadIdentity{
{
ResourceID: "/subscriptions/12345678-1234-1234-1234-123456789012/resourceGroups/a-fake-group/providers/Microsoft.RedHatOpenShift/userAssignedIdentities/fake-cluster-name",
OperatorName: "",
},
},
}
oc.Properties.ServicePrincipalProfile = nil
},
wantErr: "400: InvalidParameter: properties.platformWorkloadIdentityProfile.PlatformWorkloadIdentities[0].resourceID: Operator name is empty.",
},
{
name: "identity and service principal missing",
modify: func(oc *OpenShiftCluster) {
oc.Properties.PlatformWorkloadIdentityProfile = nil
oc.Properties.ServicePrincipalProfile = nil
},
wantErr: "400: InvalidParameter: properties.servicePrincipalProfile: Must provide either an identity or service principal credentials.",
},
}

runTests(t, testModeCreate, createTests)
}
4 changes: 3 additions & 1 deletion pkg/util/stringutils/stringutils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ package stringutils
// Copyright (c) Microsoft Corporation.
// Licensed under the Apache License 2.0.

import "strings"
import (
"strings"
)

// LastTokenByte splits s on sep and returns the last token
func LastTokenByte(s string, sep byte) string {
Expand Down
Loading