Expose IsEqual for AccessRequests (#64141)

`(AccessRequestV3) IsEqual` was added with a partially goderived
implementation to remove go-cmp comparisons outside test files.
This was not as straightforward as other resources due to
goderive not being able to handle interfaces. Due to the
oneof in AccessRequestsV3 goderive would silently fail
with a partially generated equality function for access requests.

The TeleportEqual derive plugin was updated to ignore the details
field. They are manually validated in `(AccessRequestV3) IsEqual`
if all other fields match.

`resourceAccessIDEqual` manually validates the constraints fields and
relies on goderived equality functions for each concrete type.
This commit is contained in:
rosstimothy
2026-04-09 21:30:50 +00:00
committed by GitHub
parent 02a1b0b537
commit 76a5ef03a7
4 changed files with 1967 additions and 1120 deletions
+90
View File
@@ -151,6 +151,8 @@ type AccessRequest interface {
SetRequestedResourceAccessIDs([]ResourceAccessID)
// GetAllRequestedResourceIDs get all requested resources, in [ResourceAccessID]-form.
GetAllRequestedResourceIDs() []ResourceAccessID
// IsEqual determines if two access requests are equivalent to one another.
IsEqual(AccessRequest) bool
}
// NewAccessRequest assembles an AccessRequest resource.
@@ -179,6 +181,94 @@ func NewAccessRequestWithResources(name string, user string, roles []string, res
return &req, nil
}
// IsEqual determines if two access requests are equivalent to one another.
// The Revision field is ignored during comparison.
func (r *AccessRequestV3) IsEqual(other AccessRequest) bool {
if r == nil && other == nil {
return true
}
otherv3, ok := other.(*AccessRequestV3)
if !ok {
return false
}
if r == nil && otherv3 == nil {
return true
}
if !deriveTeleportEqualAccessRequestV3(r, otherv3) {
return false
}
// The derived equality function skips RequestedResourceAccessIDs entirely
// because the ResourceConstraints.Details oneof interface is not handled by
// goderive. Compare it manually here.
if !resourceAccessIDsEqual(r.Spec.RequestedResourceAccessIDs, otherv3.Spec.RequestedResourceAccessIDs) {
return false
}
return true
}
// resourceAccessIDsEqual compares two ResourceAccessID slices independent of
// ordering. This is necessary because goderive cannot handle the oneof
// Details field on ResourceConstraints.
func resourceAccessIDsEqual(a, b []ResourceAccessID) bool {
if len(a) != len(b) {
return false
}
for i := range a {
if !resourceAccessIDEqual(&a[i], &b[i]) {
return false
}
}
return true
}
// resourceAccessIDEqual compares all fields of two ResourceAccessIDs.
// goderive handles all fields except the Details oneof on ResourceConstraints
// must be checked manually.
func resourceAccessIDEqual(a, b *ResourceAccessID) bool {
if !deriveTeleportEqualResourceAccessID(a, b) {
return false
}
return resourceConstraintsDetailsEqual(a.Constraints, b.Constraints)
}
// resourceConstraintsDetailsEqual compares the Details oneof field of two
// ResourceConstraints. The oneof interface is not handled by goderive, so
// this must be done manually.
func resourceConstraintsDetailsEqual(a, b *ResourceConstraints) bool {
if a == nil || b == nil {
return a == b
}
switch av := a.Details.(type) {
case *ResourceConstraints_AwsConsole:
bv, ok := b.Details.(*ResourceConstraints_AwsConsole)
if !ok {
return false
}
if av == nil || bv == nil {
return av == bv
}
return deriveTeleportEqualAWSConsoleResourceConstraints(av.AwsConsole, bv.AwsConsole)
case *ResourceConstraints_Ssh:
bv, ok := b.Details.(*ResourceConstraints_Ssh)
if !ok {
return false
}
if av == nil || bv == nil {
return av == bv
}
return deriveTeleportEqualSSHResourceConstraints(av.Ssh, bv.Ssh)
default:
return a.Details == nil && b.Details == nil
}
}
// GetUser gets User
func (r *AccessRequestV3) GetUser() string {
return r.Spec.User
+495
View File
@@ -17,6 +17,9 @@ limitations under the License.
package types
import (
"fmt"
"reflect"
"strings"
"testing"
"time"
@@ -183,3 +186,495 @@ func Test_AccessRequest_SetExpiry(t *testing.T) {
require.NotNil(t, reqV3.Spec.ResourceExpiry)
require.Equal(t, t1, *reqV3.Spec.ResourceExpiry)
}
func TestAccessRequestV3IsEqual(t *testing.T) {
newReq := func(t *testing.T) *AccessRequestV3 {
t.Helper()
req, err := NewAccessRequest("req-1", "alice", "role-a", "role-b")
require.NoError(t, err)
return req.(*AccessRequestV3)
}
awsID := func(name string, arns ...string) ResourceAccessID {
return ResourceAccessID{
Id: ResourceID{ClusterName: "cluster", Kind: "app", Name: name},
Constraints: &ResourceConstraints{
Version: V1,
Details: &ResourceConstraints_AwsConsole{
AwsConsole: &AWSConsoleResourceConstraints{RoleArns: arns},
},
},
}
}
sshID := func(name string, logins ...string) ResourceAccessID {
return ResourceAccessID{
Id: ResourceID{ClusterName: "cluster", Kind: "node", Name: name},
Constraints: &ResourceConstraints{
Version: V1,
Details: &ResourceConstraints_Ssh{
Ssh: &SSHResourceConstraints{Logins: logins},
},
},
}
}
unconstrainedID := func(name string) ResourceAccessID {
return ResourceAccessID{
Id: ResourceID{ClusterName: "cluster", Kind: "node", Name: name},
}
}
// mockAccessRequest is a minimal implementation of the AccessRequest
// interface to exercise the non-*AccessRequestV3 type assertion path.
type mockAccessRequest struct {
AccessRequestV3
}
tests := []struct {
name string
a, b func(t *testing.T) AccessRequest
want bool
}{
{
name: "both nil interface",
a: func(t *testing.T) AccessRequest {
var r *AccessRequestV3
return r
},
b: func(t *testing.T) AccessRequest {
return nil
},
want: true,
},
{
name: "both typed nil",
a: func(t *testing.T) AccessRequest {
var r *AccessRequestV3
return r
},
b: func(t *testing.T) AccessRequest {
var r *AccessRequestV3
return r
},
want: true,
},
{
name: "typed nil vs populated",
a: func(t *testing.T) AccessRequest {
var r *AccessRequestV3
return r
},
b: func(t *testing.T) AccessRequest { return newReq(t) },
want: false,
},
{
name: "populated vs typed nil",
a: func(t *testing.T) AccessRequest { return newReq(t) },
b: func(t *testing.T) AccessRequest {
var r *AccessRequestV3
return r
},
want: false,
},
{
name: "identical requests",
a: func(t *testing.T) AccessRequest { return newReq(t) },
b: func(t *testing.T) AccessRequest { return newReq(t) },
want: true,
},
{
name: "non-v3 type returns false",
a: func(t *testing.T) AccessRequest { return newReq(t) },
b: func(t *testing.T) AccessRequest { return &mockAccessRequest{} },
want: false,
},
{
name: "different user",
a: func(t *testing.T) AccessRequest { return newReq(t) },
b: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.User = "bob"
return r
},
want: false,
},
{
name: "different roles",
a: func(t *testing.T) AccessRequest { return newReq(t) },
b: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.Roles = []string{"role-c"}
return r
},
want: false,
},
{
name: "different state",
a: func(t *testing.T) AccessRequest { return newReq(t) },
b: func(t *testing.T) AccessRequest {
r := newReq(t)
require.NoError(t, r.SetState(RequestState_APPROVED))
return r
},
want: false,
},
{
name: "different request reason",
a: func(t *testing.T) AccessRequest { return newReq(t) },
b: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestReason = "urgent"
return r
},
want: false,
},
{
name: "different metadata labels",
a: func(t *testing.T) AccessRequest { return newReq(t) },
b: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Metadata.Labels = map[string]string{"env": "prod"}
return r
},
want: false,
},
{
name: "revision difference ignored",
a: func(t *testing.T) AccessRequest { return newReq(t) },
b: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Metadata.SetRevision("different-revision")
return r
},
want: true,
},
{
name: "aws console constraints equal",
a: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{awsID("app-1", "arn:aws:iam::123:role/admin")}
return r
},
b: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{awsID("app-1", "arn:aws:iam::123:role/admin")}
return r
},
want: true,
},
{
name: "aws console constraints differ by arn",
a: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{awsID("app-1", "arn:aws:iam::123:role/admin")}
return r
},
b: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{awsID("app-1", "arn:aws:iam::123:role/readonly")}
return r
},
want: false,
},
{
name: "ssh constraints equal",
a: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{sshID("node-1", "root", "ubuntu")}
return r
},
b: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{sshID("node-1", "root", "ubuntu")}
return r
},
want: true,
},
{
name: "ssh constraints differ by login",
a: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{sshID("node-1", "root")}
return r
},
b: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{sshID("node-1", "ubuntu")}
return r
},
want: false,
},
{
name: "aws vs ssh constraint type mismatch",
a: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{awsID("res-1", "arn:aws:iam::123:role/admin")}
return r
},
b: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{sshID("res-1", "root")}
return r
},
want: false,
},
{
name: "constrained vs unconstrained",
a: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{sshID("node-1", "root")}
return r
},
b: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{unconstrainedID("node-1")}
return r
},
want: false,
},
{
name: "both unconstrained",
a: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{unconstrainedID("node-1")}
return r
},
b: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{unconstrainedID("node-1")}
return r
},
want: true,
},
{
name: "different resource access ID count",
a: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{sshID("node-1", "root")}
return r
},
b: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{sshID("node-1", "root"), sshID("node-2", "root")}
return r
},
want: false,
},
{
name: "resource access IDs reversed order",
a: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{
sshID("node-1", "root"),
awsID("app-1", "arn:aws:iam::123:role/admin"),
}
return r
},
b: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{
awsID("app-1", "arn:aws:iam::123:role/admin"),
sshID("node-1", "root"),
}
return r
},
want: false,
},
{
name: "resource access IDs shuffled mixed types",
a: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{
sshID("node-1", "root"),
awsID("app-1", "arn:aws:iam::123:role/admin"),
unconstrainedID("node-2"),
}
return r
},
b: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{
unconstrainedID("node-2"),
sshID("node-1", "root"),
awsID("app-1", "arn:aws:iam::123:role/admin"),
}
return r
},
want: false,
},
{
name: "both nil resource access IDs",
a: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = nil
return r
},
b: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = nil
return r
},
want: true,
},
{
name: "typed-nil aws console constraints does not panic",
a: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{{
Id: ResourceID{ClusterName: "cluster", Kind: "app", Name: "app-1"},
Constraints: &ResourceConstraints{Version: V1, Details: (*ResourceConstraints_AwsConsole)(nil)},
}}
return r
},
b: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{{
Id: ResourceID{ClusterName: "cluster", Kind: "app", Name: "app-1"},
Constraints: &ResourceConstraints{Version: V1, Details: (*ResourceConstraints_AwsConsole)(nil)},
}}
return r
},
want: true,
},
{
name: "typed-nil ssh constraints does not panic",
a: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{{
Id: ResourceID{ClusterName: "cluster", Kind: "node", Name: "node-1"},
Constraints: &ResourceConstraints{Version: V1, Details: (*ResourceConstraints_Ssh)(nil)},
}}
return r
},
b: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{{
Id: ResourceID{ClusterName: "cluster", Kind: "node", Name: "node-1"},
Constraints: &ResourceConstraints{Version: V1, Details: (*ResourceConstraints_Ssh)(nil)},
}}
return r
},
want: true,
},
{
name: "typed-nil aws constraints vs populated aws constraints",
a: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{{
Id: ResourceID{ClusterName: "cluster", Kind: "app", Name: "app-1"},
Constraints: &ResourceConstraints{Version: V1, Details: (*ResourceConstraints_AwsConsole)(nil)},
}}
return r
},
b: func(t *testing.T) AccessRequest {
r := newReq(t)
r.Spec.RequestedResourceAccessIDs = []ResourceAccessID{awsID("app-1", "arn:aws:iam::123:role/admin")}
return r
},
want: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
a := tt.a(t)
b := tt.b(t)
require.Equal(t, tt.want, a.IsEqual(b))
})
}
}
// TestResourceConstraintsDetailsExhaustive ensures that resourceConstraintsDetailsEqual
// handles all ResourceConstraints.Details oneof variants. If a new variant is added
// to the proto but not resourceConstraintsDetailsEqual, then the test will fail because the
// function returns false for two identical unknown and non-nil Details types.
func TestResourceConstraintsDetailsExhaustive(t *testing.T) {
var constraints *ResourceConstraints
for _, w := range constraints.XXX_OneofWrappers() {
wrapperType := reflect.TypeOf(w).Elem()
t.Run(wrapperType.Name(), func(t *testing.T) {
// Create two ResourceConstraints with identical Details for
// the current OneOf type.
rcA := populateResourceConstraintsDetails(t, wrapperType, 1)
rcB := populateResourceConstraintsDetails(t, wrapperType, 1)
// Validate that they are not nil.
require.NotNil(t, rcA.Details)
require.NotNil(t, rcB.Details)
// Validate that they are equivalent.
require.True(t, resourceConstraintsDetailsEqual(rcA, rcB))
// Validate that a populated constraint and a nil constraint are not equal.
require.False(t, resourceConstraintsDetailsEqual(rcA, &ResourceConstraints{}))
// Validate that two ResourceConstraints with the same Details type, but
// different values are not equivalent.
rcC := populateResourceConstraintsDetails(t, wrapperType, 2)
require.False(t, resourceConstraintsDetailsEqual(rcA, rcC))
})
}
}
// populateResourceConstraintsDetails returns a ResourceConstraints that has the Details
// field set to a fully populated OneOf variant based on the provided seed. The value
// returned will always be identical for the same type and seed.
func populateResourceConstraintsDetails(t *testing.T, detailsType reflect.Type, seed int) *ResourceConstraints {
t.Helper()
wrapper := reflect.New(detailsType).Elem()
for i := range detailsType.NumField() {
wf := wrapper.Field(i)
if !wf.CanSet() || strings.HasPrefix(detailsType.Field(i).Name, "XXX_") {
continue
}
require.Equal(t, reflect.Pointer, wf.Kind(), "unexpected non-pointer field %s in %s oneof", detailsType.Field(i).Name, detailsType.Name())
inner := reflect.New(wf.Type().Elem())
innerElem := inner.Elem()
for j := range innerElem.NumField() {
sf := innerElem.Type().Field(j)
f := innerElem.Field(j)
if !f.CanSet() || strings.HasPrefix(sf.Name, "XXX_") {
continue
}
switch f.Kind() {
case reflect.String:
f.SetString(fmt.Sprintf("value-%d", seed))
case reflect.Bool:
f.SetBool(seed%2 == 0)
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
f.SetInt(int64(seed))
case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64:
f.SetUint(uint64(seed))
case reflect.Float32, reflect.Float64:
f.SetFloat(float64(seed))
case reflect.Slice:
elem := reflect.New(f.Type().Elem()).Elem()
switch elem.Kind() {
case reflect.String:
elem.SetString(fmt.Sprintf("value-%d", seed))
case reflect.Bool:
elem.SetBool(seed%2 == 0)
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
elem.SetInt(int64(seed))
case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64:
elem.SetUint(uint64(seed))
case reflect.Float32, reflect.Float64:
elem.SetFloat(float64(seed))
default:
t.Fatalf("unhandled slice element type %s in field %s", elem.Kind(), sf.Name)
}
f.Set(reflect.Append(f, elem))
default:
t.Fatalf("unhandled kind %s for field %s", f.Kind(), sf.Name)
}
}
wf.Set(inner)
}
return &ResourceConstraints{Details: wrapper.Addr().Interface().(isResourceConstraints_Details)}
}
+1366 -1120
View File
File diff suppressed because it is too large Load Diff
@@ -135,6 +135,22 @@ func removeIgnoredFieldsFromStruct(name string, strct *types.Struct) types.Type
continue
}
// Ignore Details on ResourceConstraints; the Details oneof interface
// is not handled by goderive. It is up to AccessRequest.IsEqual to
// account for this manually.
if name == "ResourceConstraints" && fieldName == "Details" {
continue
}
// Ignore RequestedResourceAccessIDs on AccessRequestSpecV3; the
// ResourceConstraints.Details oneof within each element is not
// handled by goderive, and the slice requires order-independent
// comparison. It is up to AccessRequestV3.IsEqual to account for
// this manually.
if name == "AccessRequestSpecV3" && fieldName == "RequestedResourceAccessIDs" {
continue
}
filteredFields = append(filteredFields, field)
filteredTags = append(filteredTags, strct.Tag(i))