Create "raw" managed policy using existing class props#2822
Create "raw" managed policy using existing class props#2822
Conversation
🦋 Changeset detectedLatest commit: 29190f3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
||
| export class GuDeveloperPolicy extends ManagedPolicy { | ||
| constructor(scope: GuStack, id: string, props: GuDeveloperPolicyProps) { | ||
| super(scope, id, { path: `/developer-policy/${props.permission}/`, ...props }); |
There was a problem hiding this comment.
Spreading props last would mean any values contained within wins. That is, if we have:
const myProps: GuDeveloperPolicyProps = {
path: 'my/own/value'
}
new GuDeveloperPolicy(myProps);Then /developer-policy/??? will be overridden to my/own/value.
I think there are a couple of options to resolve:
- Spreading
propsfirst and settingpathsecond - Have the
GuDeveloperPolicyPropsinterfaceOmitthepathproperty fromManagedPolicyProps
There was a problem hiding this comment.
I actually chose this hierarchy on purpose when this was intended as a generic managed policy object then we decided to make it dedicated. I think I'll just remove the facility to set for yourself.
There was a problem hiding this comment.
I've gone back and pruned the props much harder - this class shouldn't be used to grant policies to roles, user, or groups, so I've enforced that by Omit'ing them.
| export type GuDeveloperPolicyProps = Omit<ManagedPolicyProps, "statements"> & { | ||
| allow: GuAllowPolicyProps[]; | ||
| deny?: GuDenyPolicyProps[]; | ||
| permission: string; | ||
| description?: string; | ||
| }; |
There was a problem hiding this comment.
Could we add some doc strings to these properties?
There was a problem hiding this comment.
Might name be more descriptive, replacing permission?
There was a problem hiding this comment.
Permission is the deliberate choice for the UI. So I think we'd prefer to keep the nomenclature the same. It's the permission you're getting.
| description?: string; | ||
| }; | ||
|
|
||
| export class GuDeveloperPolicy extends ManagedPolicy { |
There was a problem hiding this comment.
Could we add a doc string to this class to describe when/how/why to use it, please?
.changeset/angry-parrots-confess.md
Outdated
| "@guardian/cdk": minor | ||
| --- | ||
|
|
||
| Create "raw" managed policy using existing class props |
There was a problem hiding this comment.
These notes make their way into the CHANGELOG and the release notes. Would they benefit from more detail? For example, covering when to use the new construct, the name of the new construct, and maybe a concrete example1?
Footnotes
-
Whilst the tests provide example implementations, they don't really cover real-life situations (assuming a
permissionshould be more descriptive thantest123?) ↩
| export type GuDeveloperPolicyProps = Omit<ManagedPolicyProps, "users" | "roles" | "groups" | "statements" | "path"> & { | ||
| allow: GuAllowPolicyProps[]; | ||
| deny?: GuDenyPolicyProps[]; | ||
| permission: string; | ||
| description?: string; | ||
| }; |
There was a problem hiding this comment.
IIUC the remaining properties from ManagedPolicyProps are: document and managedPolicyName. We're defaulting the latter to permission. Are we expecting document to be set? If not, it might not be worth extending ManagedPolicyProps at all.
There was a problem hiding this comment.
It's getting a bit thin, isn't it. I'm allowing statements and document to be set, in case people prefer to use really raw content, but it's a bit weak. Name and path are defaulted, not forced, so could be set.
But the main reason I would expect to do this is because these properties are then passed into the super call. If a parameter were to be added to ManagedPolicy, this should flow through.
There was a problem hiding this comment.
Changed my mind. The friendly name restriction is too dangerous.
| allow: GuAllowPolicyProps[]; | ||
| deny?: GuDenyPolicyProps[]; | ||
| permission: string; | ||
| description?: string; |
There was a problem hiding this comment.
Could we document these properties please? TypeDoc strings make their way to the documentation site.
aug24
force-pushed
the
raw-managed-policies
branch
2 times, most recently
from
fbeca4a to
9bce0a1
Compare
Create a new managed policy for 'raw' using existing class properties.
Co-authored-by: kelvin-chappell <1722550+kelvin-chappell@users.noreply.github.com>
Co-authored-by: kelvin-chappell <1722550+kelvin-chappell@users.noreply.github.com>
aug24
force-pushed
the
raw-managed-policies
branch
from
8e86a42 to
29190f3
Compare
3 participants
What does this change?
This PR provides a base class for provision of an AWS Managed Policy with specific, discoverable, characteristics. These should be used to create Developer Policies which can be granted as needed with temporary credentials for short term workloads.
How to test
How can we measure success?
Have we considered potential risks?
Checklist
Footnotes
Consider whether this is something that will mean changes to projects that have already been migrated, or to the CDK CLI tool. If changes are required, consider adding a checklist here and/or linking to related PRs. ↩
If you are adding a new construct or pattern, has new documentation been added? If you are amending defaults or changing behaviour, are the existing docs still valid? ↩