-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Policy pack configuration using policy enable cmd #4127
Conversation
07e78cf
to
d2e3291
Compare
026f4b2
to
5a5a90f
Compare
CHANGELOG.md
Outdated
@@ -8,6 +8,8 @@ CHANGELOG | |||
[#4141](https://github.com/pulumi/pulumi/pull/4141) | |||
- Add missing builtin `MapArray` to Go SDK. | |||
[#4144](https://github.com/pulumi/pulumi/pull/4144) | |||
- Add support for policy pack configuration using CLI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the changelog is actually used for our releases, we may want to clarify this. "Add support for enabling Policy Packs with configuration" ... or something to that effect.
pkg/cmd/util.go
Outdated
@@ -702,3 +703,20 @@ func updateFlagsToOptions(interactive, skipPreview, yes bool) (backend.UpdateOpt | |||
SkipPreview: skipPreview, | |||
}, nil | |||
} | |||
|
|||
// loadJSONConfigFile loads file with json configuration data and unmarshals into a map[string]*json.RawMessage object. | |||
func loadJSONConfigFile(filePath string) (map[string]*json.RawMessage, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than having this function here, I am wondering if we should call the same function we use to load local config specified to the --policy-pack-config
flag of pulumi up/preview/watch
.
pulumi/pkg/resource/analyzer/config.go
Lines 31 to 37 in 61928f0
func LoadPolicyPackConfigFromFile(file string) (map[string]plugin.AnalyzerPolicyConfig, error) { | |
b, err := ioutil.ReadFile(file) | |
if err != nil { | |
return nil, err | |
} | |
return parsePolicyPackConfig(b) | |
} |
That way we have consistent behavior of loading local policy config from a file.
This existing function does some validation of the EnforcementLevel
values, and also, as a convenience, allows specifying enforcement level for a policy like:
{
"some-policy": "advisory"
}
Rather than having to specify it like:
{
"some-policy": {
"enforcementLevel": "advisory"
}
}
This would require adding some code to convert map[string]plugin.AnalyzerPolicyConfig
to map[string]*json.RawMessage
. Probably the easiest way to do that is to loop through the map and json.Marshal
each plugin.AnalyzerPolicyConfig
value to bytes. The bytes can then be cast to json.RawMessage
. Similar to what's done here:
pulumi/pkg/backend/httpstate/client/client.go
Lines 633 to 641 in c28c602
properties := map[string]*json.RawMessage{} | |
for k, v := range schema.Properties { | |
bytes, err := json.Marshal(v) | |
if err != nil { | |
return nil, err | |
} | |
raw := json.RawMessage(bytes) | |
properties[k] = &raw | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I will leverage that function so we can use the validations it has and then just convert the types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments.. ALSO thinking we should definitely add a pulumi policy validate-config
command to allow people to validate config before enabling. You can open another issue if you dont want to do it as part of this?
"@pulumi/policy": "^0.2.0" | ||
"@pulumi/pulumi": "1.13.0", | ||
"@pulumi/aws": "1.27.0", | ||
"@pulumi/policy": "0.4.1-dev.1584625475" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we talked about have one Policy Pack using an older version of the policy repo to ensure backwards compatibility.. is there a reason you changed your mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, was actually just a miss on my part. the only way I could get this to work was just to add another test policy pack directory with the old version of the policy sdk. Not sure how far back in versions you want to test, so I just used the version that we were using for the tests before I made these changes. I also just made a completely different test case with all the old existing tests, since the set of commands we were using in the test I believe are only backwards compatible not forwards compatible with the changes, so I had to keep a lot of the old commands that we were testing with previously as well, for example the way we were specifying pp versions.
configSchema: { | ||
properties: { | ||
message: { | ||
"type": "string", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think type, minLength and maxLength do not need to be quotes & may make this look cleaner.
pkg/cmd/policy_enable.go
Outdated
}), | ||
} | ||
|
||
cmd.PersistentFlags().StringVar( | ||
&args.policyGroup, "policy-group", "", | ||
"The Policy Group for which the Policy Pack will be enabled; if not specified, the default Policy Group is used") | ||
|
||
cmd.PersistentFlags().StringVar( | ||
&args.configFile, "config-file", "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think when running a policy-pack locally there is a different flag name used for the config file. Should we use the same here for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For pulumi preview/up/watch
, the flag is --policy-pack-config
. Since this command is already about policies, I think we can leave off the policy-pack
prefix, and just use --config
.
pkg/cmd/util.go
Outdated
@@ -702,3 +703,20 @@ func updateFlagsToOptions(interactive, skipPreview, yes bool) (backend.UpdateOpt | |||
SkipPreview: skipPreview, | |||
}, nil | |||
} | |||
|
|||
// loadJSONConfigFile loads file with json configuration data and unmarshals into a map[string]*json.RawMessage object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is in a generic util file, maybe be more specific about its use in the comment? Or just move it to the pkg/cmd/policy_enable.go file. I dont think its used anywhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I went ahead and relocated to policy_enable.co and then leveraged the function justin is suggesting since it does some validations.
@@ -0,0 +1,6 @@ | |||
{ | |||
"s3-no-public-read":{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
"s3-no-public-read":{ | |
"s3-no-public-read": { |
@@ -0,0 +1,6 @@ | |||
{ | |||
"s3-no-public-read":{ | |||
"enforcementLevel":"mandatory", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
"enforcementLevel":"mandatory", | |
"enforcementLevel": "mandatory", |
{ | ||
"s3-no-public-read":{ | ||
"enforcementLevel":"mandatory", | ||
"message":"this message is invalid because it is way more than 10 characters......" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
"message":"this message is invalid because it is way more than 10 characters......" | |
"message": "this message is invalid because it is way more than 10 characters......" |
@@ -0,0 +1,10 @@ | |||
{ | |||
"all":{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
"all":{ | |
"all": { |
@@ -0,0 +1,10 @@ | |||
{ | |||
"all":{ | |||
"enforcementLevel":"advisory" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
"enforcementLevel":"advisory" | |
"enforcementLevel": "advisory" |
}, | ||
"s3-no-public-read": | ||
{ | ||
"enforcementLevel":"mandatory", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
"enforcementLevel":"mandatory", | |
"enforcementLevel": "mandatory", |
"s3-no-public-read": | ||
{ | ||
"enforcementLevel":"mandatory", | ||
"message":"test msg" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
"message":"test msg" | |
"message": "test msg" |
329475c
to
9cd2504
Compare
pkg/cmd/policy_enable.go
Outdated
// Convert type map[string]plugin.AnalyzerPolicyConfig to map[string]*json.RawMessage. | ||
config := map[string]*json.RawMessage{} | ||
for k, v := range analyzerPolicyConfig { | ||
bytes, err := json.Marshal(v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can just marshal v
directly.
Doing so results in JSON like:
{"EnforcementLevel":"mandatory","Properties":{"foo":"bar"}}
The value needs to be converted into the format expected by the service. Something like:
func marshalAnalyzerPolicyConfig(c AnalyzerPolicyConfig) (*json.RawMessage, error) {
m := make(map[string]interface{})
for k, v := range c.Properties {
m[k] = v
}
if c.EnforcementLevel != "" {
m["enforcementLevel"] = c.EnforcementLevel
}
bytes, err := json.Marshal(m)
if err != nil {
return nil, err
}
raw := json.RawMessage(bytes)
return &raw, nil
}
Which will result in:
{"enforcementLevel":"mandatory","foo":"bar"}
I created a Go Playground to demonstrate: https://play.golang.org/p/L8GYTzTAsOR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, I noticed it structured the configurable values under the Properties key. Thanks for adding that code snippet btw. I updated to convert the values.
aeb3c92
to
ac8fa40
Compare
OK, this is ready for a re-review. I made a few changes since last review:
|
9fdf93c
to
8daef9b
Compare
@@ -0,0 +1,14 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if it makes more sense to call this dir policy_pack_w_config
since the version is probably meaningless to most people?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, that's a more meaningful name. will update
@@ -14,9 +14,61 @@ import ( | |||
) | |||
|
|||
// TestPolicy tests policy related commands work. | |||
func TestPolicy(t *testing.T) { | |||
t.Skip("Temporarily skipping test that is causing unrelated tests to fail - pulumi/pulumi#4149") | |||
func TestPolicyVersion0_4_1_dev(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with naming here.
e.RunCommand("pulumi", "policy", "rm", fmt.Sprintf("%s/%s", orgName, policyPackName), "all") | ||
} | ||
|
||
func TestPolicyVersion0_2_0(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets change naming here too
pkg/cmd/policy_enable.go
Outdated
}), | ||
} | ||
|
||
cmd.PersistentFlags().StringVar( | ||
&args.policyGroup, "policy-group", "", | ||
"The Policy Group for which the Policy Pack will be enabled; if not specified, the default Policy Group is used") | ||
|
||
cmd.PersistentFlags().StringVar( | ||
&args.config, "config", "", | ||
"The file path for the policy pack configuration file") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Policy Pack
pkg/cmd/policy_enable.go
Outdated
} | ||
|
||
// Convert type map[string]plugin.AnalyzerPolicyConfig to map[string]*json.RawMessage. | ||
config := map[string]*json.RawMessage{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm not sure how this is working, but don't you normally have to use make
for a map? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is equivalent to initalizing an empty map using make. Only difference is not including the curly braces at the end when using the make function. make(map[string]*json.RawMessage)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using make
seems to be the more consistent pattern for us from a quick glance in our code base. So went and just changed to using make while I was in there. 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion about deleting a commented-out line, and aside from Erin's feedback, LGTM.
pkg/cmd/policy_enable.go
Outdated
// Load the configuration from the user-specified JSON file into config object. | ||
var config map[string]*json.RawMessage | ||
if args.config != "" { | ||
// config, err = loadJSONConfigFile(args.configFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// config, err = loadJSONConfigFile(args.configFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol... sorry. completely missed that. will make change.
8daef9b
to
2677fbc
Compare
Issue #3756
Support adding policy pack configuration through CLI by specifying a policy configuration json file.
Example cmd:
Right now all policy pack config validation is being done server side. Maybe if we feel there would be an advantage to doing config validation on the client we can move to that at some point, but I believe we would have to request the schema from the server anyway, so either way we would need to make an additional api call. I feel we can revisit this at some point if need be. We can always do a follow up PR if we feel doing validation client side is necessary.