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

feat: Display supported policy exception constraints #1068

Merged
merged 2 commits into from
Dec 15, 2022
Merged

feat: Display supported policy exception constraints #1068

merged 2 commits into from
Dec 15, 2022

Conversation

rmoles
Copy link
Collaborator

@rmoles rmoles commented Dec 14, 2022

Signed-off-by: Ross [email protected]

Summary

When running a lacework policy show we should parse & display the supported policy exception constraints, as we now have this info available in the response

How did you test this change?

Tested locally

Issue

https://lacework.atlassian.net/browse/GROW-1314

@rmoles rmoles requested a review from a team December 14, 2022 12:46
Owner string `json:"owner" yaml:"-"`
LastUpdateTime string `json:"lastUpdateTime" yaml:"-"`
LastUpdateUser string `json:"lastUpdateUser" yaml:"-"`
ExceptionConfiguration map[string][]PolicyExceptionConfigurationConstraints `json:"exceptionConfiguration" yaml:"-"`
Copy link
Collaborator Author

@rmoles rmoles Dec 14, 2022

Choose a reason for hiding this comment

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

I don't like this, but I couldn't find a better way to parse the Policy Exception constraints, as the format is -
an Array of Maps in a Map.
It's not clear to me why the outer map was required 🤷

    "exceptionConfiguration": {
      "constraintFields": [
        {
          "dataType": "String",
          "fieldKey": "accountIds",
          "multiValue": true
        },
        {
          "dataType": "String",
          "fieldKey": "resourceNames",
          "multiValue": false
        },
        {
          "dataType": "KVTagPair",
          "fieldKey": "resourceTags",
          "multiValue": true
        }
      ]
    },

Copy link

@iok0 iok0 Dec 15, 2022

Choose a reason for hiding this comment

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

Yeah, doesn't look necessary right now. Maybe they've plans to add another field to exceptionConfiguration.
Looks like Go needs the structure/type to be defined up front.

Copy link
Collaborator Author

@rmoles rmoles Dec 15, 2022

Choose a reason for hiding this comment

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

Perhaps. Still annoying for parsing!
For parsing the values into structs we need to declare the structure up front. I was hoping I could tell GoLang to chain the map keys when parsing ie. json:"exceptionConfiguration.constraintFields" yaml:"-" that way we could remove the outer map[string] but that doesn't seem to be possible.

Comment on lines 597 to 600
if exceptionConstraints != "" {
entry := []string{"VALID EXCEPTION CONSTRAINTS", exceptionConstraints}
details = append(details, entry)
}
Copy link
Collaborator Author

@rmoles rmoles Dec 15, 2022

Choose a reason for hiding this comment

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

@afiune any thoughts on whether we want to just drop this field from the table, or whether we should put a message to the effect of no constraints found?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add it always and if the policy do not have constraints then show "None" - because no constraint found implies that maybe, there could be constraints.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good. I'll update!

@rmoles rmoles merged commit c6ebd68 into main Dec 15, 2022
@rmoles rmoles deleted the GROW-1314 branch December 15, 2022 16:07
@lacework-releng lacework-releng mentioned this pull request Dec 20, 2022
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