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.

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

Add option to list (import) roles from provider #55

Open
rahmatrhd opened this issue Jul 6, 2023 · 6 comments
Open

Add option to list (import) roles from provider #55

rahmatrhd opened this issue Jul 6, 2023 · 6 comments

Comments

@rahmatrhd
Copy link
Member

Summary
Project-level (or org-level) access in GCP already comes with many predefined roles plus the custom role that the user can create themselves, and it would be tedious to list all of those manually in ResourceConfig.Roles

Roles []*Role `json:"roles" yaml:"roles" validate:"required"`

Would be easier if this can be automatically populated from the provider (similar way with resources being imported from provider)

Proposed solution

  1. Update provider config
// domain/provider.go
+ type ImportRoles struct {
+ 	Enable bool
+ 	Filter string // expression
+ }

  type ResourceConfig struct {
	Type        string        `json:"type"
	Filter      string        `json:"filter"
	Policy      *PolicyConfig `json:"policy"
	Roles       []*Role       `json:"roles"
+ 	ImportRoles ImportRoles   `json:"import_roles"
  }

A new option ImportRoles is introduced within the ResourceConfig to indicate that the roles listing will be coming from the provider instead of the old Roles list

  1. Handle new config during provider create and edit
    During create/edit, the following logic applies:

    1. Validation
      1. Roles is required if ImportRoles is empty/not specified vice versa
      2. Return error if both values exist (should only specify one)
    2. Imported roles won't be stored in the DB, after fetching from the provider the list will be stored in an in-memory cache to keep the roles up to date
  2. Provider roles listing
    The following logic will be updated to support retrieving the roles from the cached data instead of from ResourceConfig.Roles in the case of providers with ImportRoles option enabled

    func (p *Provider) GetRoles(pc *domain.ProviderConfig, resourceType string) ([]*domain.Role, error) {
    if resourceType != ResourceTypeProject && resourceType != ResourceTypeOrganization {
    return nil, ErrInvalidResourceType
    }
    return provider.GetRoles(pc, resourceType)
    }

@bsushmith
Copy link
Collaborator

Imported roles won't be stored in the DB, after fetching from the provider the list will be stored in an in-memory cache to keep the roles up to date

How would this work without external cache if guardian is running in multiple pods on k8s ? How are you planning to keep the cache updated in all the pods on a provider roles fetch call?

@rahmatrhd
Copy link
Member Author

@bsushmith adding TTL to the cached data might help keep the roles synced between pods and with the provider (with some delay based on the TTL).

one reason for not storing it in DB was to avoid updating the provider config without the user's consent/input.

@ravisuhag
Copy link

ravisuhag commented Jul 7, 2023

@rahmatrhd why not use just * in roles to import all roles. Instead of complicating config with import roles etc. A combination of include and exclude can also be used in case user want to omit very specific roles.

Also even if we are going towards toggle etc. better would be to restructure roles key to something like this.

roles: {
 import: bool
 use : []Role
 exclude: 
 include: 
}

@rahmatrhd
Copy link
Member Author

@ravisuhag Need to separate imported roles and custom roles (existing []Role config) as the structures are slightly different. A custom role consists of multiple "provider permissions" (gcp GCP, provider permission = GCP Role), while for imported roles, at least for GCP, each GCP role will be treated as one Guardian role

# custom role
roles:
  - id: my-role-id
    permissions:
      - roles/viewer
      - roles/owner
      - roles/x
      - ...

# mapped imported role
  - id: roles/viewer
    permissions:
      - roles/viewer
  - id: roles/owner
    permissions:
      - roles/owner
  ...     

additionally, filter (include & exclude) will only relevant for imported roles

Also even if we are going towards toggle etc. better would be to restructure roles key to something like this.

roles: {
 import: bool
 use : []Role
 exclude: 
 include: 
}

agree with this, existing role config better be restructured to accommodate import, but IMO below example is more preferred if breaking changes wants to be avoided

resource_config:
  ...
  roles: []Role # keep this field for backward compatibility 
  role_config: # new roles config
    import: true
    filter: Expression
    custom_options: []Role

@ravisuhag
Copy link

How about maybe providing a migration script/command from old resource config to new to handle breaking change, but atleast thinking this appropriately would make config more readable.

@rahmatrhd
Copy link
Member Author

@ravisuhag migration script can always be added, but replacing the existing ResouceConfig.Role is still be a breaking change for the guardian API. I would still prefer to have at least two releases for this, the first one is for adding the new ResourceConfig.RoleConfig while still keeping the old ResourceConfig.Role (marked as deprecated), and then the next release is for completely removing the old config

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants