add customprecompare to normalize policy #75
Conversation
pkg/resource/queue/hooks.go
Outdated
| var policyDoc policy.Policy | ||
|
|
||
| decoder := json.NewDecoder(bytes.NewBufferString(jsonStr)) | ||
| decoder.DisallowUnknownFields() |
There was a problem hiding this comment.
i'm curious, why DisallowUnknownFields ? AFAIK iam-controller doesn't use this approach
There was a problem hiding this comment.
DisallowUnknownFields will catch unknown fields in the policy.
Readme:
https://github.com/micahhausler/aws-iam-policy
invalidPolicyJSON := []byte(`{
"Id": "CloudTrailBucketPolicy",
"Foo": "hypothetical new field",
"Statement": [
{
"Sid": "AWSCloudTrailWrite20150319",
"Effect": "Allow",
"Principal": {
"Service": "cloudtrail.amazonaws.com"
},
"Action": "s3:PutObject",
"Resource": "arn:aws:s3:::examplebucket/AWSLogs/123456789012/*"
}
]
}`)
var p policy.Policy
decoder := json.NewDecoder(bytes.NewBuffer(invalidPolicyJSON))
decoder.DisallowUnknownFields()
err := decoder.Decode(&p)
if err != nil {
fmt.Println(err)
}
// Output:
// json: unknown field "Foo"
There was a problem hiding this comment.
We should use this in the iam controller as well?
pkg/resource/queue/hooks.go
Outdated
| // normalizeRedrivePolicyString takes an SQS RedrivePolicy JSON string, unmarshals | ||
| // it into an interface{}, and then marshals it back into a compact JSON string. | ||
| // This normalizes whitespace and key order using a generic approach. | ||
| // https://go.dev/play/p/YtzsxG0l9ze | ||
| func normalizeRedrivePolicyString(jsonStr string) (string, error) { |
There was a problem hiding this comment.
why isn't this one using a custom decoder w/ DisallowUnknownFields
There was a problem hiding this comment.
RedrivePolicy is not an iam policy, so cant use it.
https://docs.aws.amazon.com/AWSSimpleQueueService/latest/APIReference/API_CreateQueue.html
so using an interface: https://go.dev/play/p/PSV7ZxUQykg
took the string from the issue: aws-controllers-k8s/community#2421
rushmash91
force-pushed
the
policy
branch
2 times, most recently
from
9097fad to
b60ef36
Compare
michaelhtm
left a comment
michaelhtm
left a comment
There was a problem hiding this comment.
Thank you @rushmash91 👍
left a few comments below
| decoderB.DisallowUnknownFields() | ||
| errB := decoderB.Decode(&policyB) | ||
|
|
||
| if errA != nil || errB != nil || !reflect.DeepEqual(policyA, policyB) { |
There was a problem hiding this comment.
should this be && instead?
There was a problem hiding this comment.
Depends do we want a diff if the unmarshal fails? I think it might be better for it to show up..
michaelhtm
left a comment
michaelhtm
left a comment
There was a problem hiding this comment.
Thank you @rushmash91
left a few more comments
|
@rushmash91: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
Nice 👍 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: michaelhtm, rushmash91 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
fixes aws-controllers-k8s/community#2421
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.