Skip to content

[CIAM-2473] Update validation to check all namespaces#1531

Merged
Lucy Fan (lucy-fan) merged 7 commits intomainfrom
lfan/ciam-2473
Nov 21, 2022
Merged

[CIAM-2473] Update validation to check all namespaces#1531
Lucy Fan (lucy-fan) merged 7 commits intomainfrom
lfan/ciam-2473

Conversation

@lucy-fan
Copy link
Member

Checklist

  1. [CRUCIAL] Is the change for CP or CCloud functionalities that are already live in prod?
    • yes: ok

What

SR role binding creation was failing when including a resource type:

> XX_DATAPLANE_3_ENABLE=1 ./confluent iam rbac role-binding create --principal User:u-d0zkjy --role DeveloperRead --environment env-vny780 --schema-registry-cluster-id lsrc-95vkmy --resource Subject:test
Error: invalid resource type "Subject"

Suggestions:
    The available resource types are: CloudCluster, OwnClusterApiKey, Cluster, OwnKafkaClusterApiKey, TransactionalId, Group, Topic, Environment, Organization, NotificationSubscription, SupportPlan, CustomConnectorPlugin, NotificationIntegration.

It turns out that one of the validation steps involved seeing if a resource type was in the access policy of the role it was trying to create, but it would only check the dataplane and public namespace, whereas the SR roles are in datagovernance. In addition, the validation used an API that got a role only based off of a role name (and optional namespace), so in the instance where we tried to create a DeveloperRead, it would see the DeveloperRead in the dataplane namespace (ie. The one for Kafka) and fail the validation since Subject is not a valid resource type for this particular DeveloperRead. So this PR updates the validation by getting all the roles that have the role name in all the namespaces and checking each one to see if the resource type is valid.

References

https://confluentinc.atlassian.net/browse/CIAM-2473

Test & Review

Successfully created SR DeveloperRead and Kafka DeveloperRead

@lucy-fan Lucy Fan (lucy-fan) changed the title Update validation to check all namespaces [CIAM-2473] Update validation to check all namespaces Nov 18, 2022
if err != nil {
notFoundErr = err
}
if role.Name != "" { // Check if resource type is supported in this particular role
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember that the role api will return the internal name of this role (like ClusterOperator) instead cli only works with display name (Operator) so this validation might fail as well..? We're migrating to new role endpoints in Q1, will that resolve this issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

The role API should be able to get a role by either name or display name (since this PR) so I think it should be okay. I've tested it with creating a DeveloperRead with SR scope + Subject resource and the validation passed.

Choose a reason for hiding this comment

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

Do you mind explaining why this check is being made? I don't think I'm following the logic here.

Copy link
Member Author

@lucy-fan Lucy Fan (lucy-fan) Nov 18, 2022

Choose a reason for hiding this comment

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

If there is a role with the specified rolename in a particular namespace, then we want to check if the resource is valid or not for this role. Since there are now roles with the same name but different resource types in the access policies, we need to check all the possible roles.

Ie. There is a DeveloperRead in dataplane, public and datagovernance namespace. The DeveloperRead in dataplane namespace has access policies with Topic, etc. whereas the DeveloperRead in datagovernance has access policies with Subject, etc. Since this function only takes in a role name (DeveloperRead) and the resourceType(something like Subject or Topic), then there is no way of knowing exactly which DeveloperRead we are trying to check. Hence we need to check all of them. Otherwise we can run into issues where we are trying to make a DeveloperRead with the datagovernance namespace and it fails because it looks at the DeveloperRead in dataplane and sees there are no Subject resourcetypes, and does not look at the DeveloperRead in datagovernance at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

This validation is being removed in v3 and will be added back when v2 role api becomes available

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a test for this. Also were you able to build and test locally

Lucy Fan (lucy-fan) and others added 3 commits November 18, 2022 14:06
Co-authored-by: Brian Strauch <bstrauch@confluent.io>
Co-authored-by: MuweiHe <83249359+MuweiHe@users.noreply.github.com>
@lucy-fan Lucy Fan (lucy-fan) marked this pull request as ready for review November 18, 2022 23:25
@lucy-fan Lucy Fan (lucy-fan) requested a review from a team as a code owner November 18, 2022 23:25
Lucy Fan (lucy-fan) and others added 2 commits November 18, 2022 15:48
Co-authored-by: Brian Strauch <bstrauch@confluent.io>
Comment on lines 423 to 426
var uniqueResourceTypes []string
for rt := range allResourceTypes {
uniqueResourceTypes = append(uniqueResourceTypes, rt)
for resourceType := range allResourceTypes {
uniqueResourceTypes = append(uniqueResourceTypes, resourceType)
}

Choose a reason for hiding this comment

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

Oops, forgot to mention, we have a set package with a Slice() function!

Copy link
Member Author

Choose a reason for hiding this comment

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

This is also currently done in validateResourceTypeV2, so we would probably need to update both of those to use the set package. I can do this in another PR later so that the content of this one isn't too confusing.

Copy link

@brianstrauch Brian Strauch (brianstrauch) Nov 21, 2022

Choose a reason for hiding this comment

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

It would probably just be a 4-5 line diff, so feel free to add it to this PR!

Copy link
Member Author

Choose a reason for hiding this comment

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

If it's not urgent, then if it's alright with you, I would rather do it in a separate PR next week, as I am currently on PTO for this week 😅

Co-authored-by: Brian Strauch <bstrauch@confluent.io>
@lucy-fan Lucy Fan (lucy-fan) merged commit fc93b5d into main Nov 21, 2022
@lucy-fan Lucy Fan (lucy-fan) deleted the lfan/ciam-2473 branch November 21, 2022 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants