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.

Sign up for GitHub

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

Refactor name setting for complex cases #174

Merged
merged 1 commit into from
Aug 5, 2024
Merged

Refactor name setting for complex cases #174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/policygenerator-reference.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,8 @@ policies:
- path: ""
# Optional. This name is used when ConsolidateManifests is set to false and will serve as the ConfigurationPolicy name.
# If multiple manifests are present in the path, an index number will be appended.
# If multiple manifests are present and their names are provided, with `consolidateManifests` set to true,
# the name of the first manifest will be used for all manifest paths.
name: "my-config-name"
# Optional. (See policyDefaults.complianceType for description.)
complianceType: "musthave"
Expand Down
8 changes: 7 additions & 1 deletion internal/ordering_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,10 +293,16 @@ metadata:
policyDefaults:
orderPolicies: true
namespace: my-policies
consolidateManifests: false
consolidateManifests: true
policies:
- name: one
manifests:
- path: {{printf "%v/%v" .Dir "configmap.yaml"}}
name: tiger
- path: {{printf "%v/%v" .Dir "configmap.yaml"}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although this manifest path name has "rabbit", this name is ignored because it is consolidateManifests. The configuration name will be "tiger"

Copy link
Member

Choose a reason for hiding this comment

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

It feels like this test has a lot going on, I couldn't quite follow what it was doing. Can it be split into multiple tests? Or does it need to be all together in one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand I wanted to cover most of the edge cases. because base cases are covered in other unit tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can say this is the ULTIMATE test case.

Copy link
Contributor Author

@yiraeChristineKim yiraeChristineKim Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe it is good idea to split Let me think

name: rabbit
- path: {{printf "%v/%v" .Dir "object-templates-raw.yaml"}}
name: bird
- path: {{printf "%v/%v" .Dir "object-templates-raw.yaml"}}
- path: {{printf "%v/%v" .Dir "object-templates-raw.yaml"}}
`,
Expand Down
2 changes: 1 addition & 1 deletion internal/testdata/ordering/manifest-extradeps-configpolicy.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ spec:
apiVersion: policy.open-cluster-management.io/v1
kind: ConfigurationPolicy
metadata:
name: one2
name: one
spec:
object-templates:
- complianceType: musthave
Expand Down
77 changes: 77 additions & 0 deletions internal/testdata/ordering/manifest-level-name-raw-consolidate-true.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,42 @@ metadata:
spec:
disabled: false
policy-templates:
- objectDefinition:
apiVersion: policy.open-cluster-management.io/v1
kind: ConfigurationPolicy
metadata:
name: bird
spec:
object-templates-raw: |-
- complianceType: musthave
objectDefinition:
apiVersion: v1
kind: ConfigMap
metadata:
name: example
namespace: default
data:
extraData: data
remediationAction: inform
severity: low
- objectDefinition:
apiVersion: policy.open-cluster-management.io/v1
kind: ConfigurationPolicy
metadata:
name: bird2
spec:
object-templates-raw: |-
- complianceType: musthave
objectDefinition:
apiVersion: v1
kind: ConfigMap
metadata:
name: example
namespace: default
data:
extraData: data
remediationAction: inform
severity: low
- objectDefinition:
apiVersion: policy.open-cluster-management.io/v1
kind: ConfigurationPolicy
Expand Down Expand Up @@ -84,6 +120,47 @@ spec:
extraData: data
remediationAction: inform
severity: low
- objectDefinition:
apiVersion: policy.open-cluster-management.io/v1
kind: ConfigurationPolicy
metadata:
name: tiger
spec:
object-templates:
- complianceType: musthave
objectDefinition:
apiVersion: v1
data:
game.properties: enemies=potato
kind: ConfigMap
metadata:
name: my-configmap
- complianceType: musthave
objectDefinition:
apiVersion: v1
data:
game.properties: enemies=cabbage
kind: ConfigMap
metadata:
name: config-2
- complianceType: musthave
objectDefinition:
apiVersion: v1
data:
game.properties: enemies=potato
kind: ConfigMap
metadata:
name: my-configmap
- complianceType: musthave
objectDefinition:
apiVersion: v1
data:
game.properties: enemies=cabbage
kind: ConfigMap
metadata:
name: config-2
remediationAction: inform
severity: low
remediationAction: inform
---
apiVersion: cluster.open-cluster-management.io/v1beta1
Expand Down
47 changes: 30 additions & 17 deletions internal/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,8 @@ func getPolicyTemplates(policyConf *types.PolicyConfig) ([]map[string]interface{
objectTemplates := make([]map[string]interface{}, 0, objectTemplatesLength)
policyTemplates := make([]map[string]interface{}, 0, policyTemplatesLength)

policyNameCounter := 0

for i, manifestGroup := range manifestGroups {
complianceType := policyConf.Manifests[i].ComplianceType
metadataComplianceType := policyConf.Manifests[i].MetadataComplianceType
Expand All @@ -177,7 +179,9 @@ func getPolicyTemplates(policyConf *types.PolicyConfig) ([]map[string]interface{
ignorePending := policyConf.Manifests[i].IgnorePending
extraDeps := policyConf.Manifests[i].ExtraDependencies

for j, manifest := range manifestGroup {
manifestNameCounter := 0

for _, manifest := range manifestGroup {
err := setGatekeeperEnforcementAction(manifest,
policyConf.Manifests[i].GatekeeperEnforcementAction)
if err != nil {
Expand All @@ -204,8 +208,7 @@ func getPolicyTemplates(policyConf *types.PolicyConfig) ([]map[string]interface{
policyConf,
manifest["object-templates-raw"],
&policyConf.Manifests[i].ConfigurationPolicyOptions,
getConfigurationPolicyName(policyConf, i, len(policyTemplates),
j, !policyConf.ConsolidateManifests),
getConfigurationPolicyName(policyConf, i, &policyNameCounter, &manifestNameCounter),
)
} else {
policyTemplate = map[string]interface{}{"objectDefinition": manifest}
Expand Down Expand Up @@ -262,8 +265,8 @@ func getPolicyTemplates(policyConf *types.PolicyConfig) ([]map[string]interface{
policyConf,
[]map[string]interface{}{objTemplate},
&policyConf.Manifests[i].ConfigurationPolicyOptions,
getConfigurationPolicyName(policyConf, i, len(policyTemplates),
j, !policyConf.ConsolidateManifests),
getConfigurationPolicyName(policyConf, i,
&policyNameCounter, &manifestNameCounter),
)

setTemplateOptions(policyTemplate, ignorePending, extraDeps)
Expand All @@ -282,11 +285,13 @@ func getPolicyTemplates(policyConf *types.PolicyConfig) ([]map[string]interface{
// just build one policyTemplate by using the above non-empty consolidated objectTemplates
// ConsolidateManifests = true or there is non-policy-type manifest
if policyConf.ConsolidateManifests && len(objectTemplates) > 0 {
// If ConsolidateManifests is true and multiple manifest[].names are provided, the configuration policy
// name will be the first name of manifest[].names
policyTemplate := buildPolicyTemplate(
policyConf,
objectTemplates,
&policyConf.ConfigurationPolicyOptions,
getConfigurationPolicyName(policyConf, 0, 0, 0, false),
getConfigurationPolicyName(policyConf, 0, &policyNameCounter, nil),
)
setTemplateOptions(policyTemplate, policyConf.IgnorePending, policyConf.ExtraDependencies)
policyTemplates = append(policyTemplates, policyTemplate)
Expand Down Expand Up @@ -322,27 +327,35 @@ func getPolicyTemplates(policyConf *types.PolicyConfig) ([]map[string]interface{
return policyTemplates, nil
}

// This function returns configurationPolicyName with Index+1 based on the provided PolicyConfig.
// When index is 0, it won't attach any number. When useManifestName is true,
// it returns the policyConf.Manifests[manifestGroupIndex].Name otherwise,
// It returns policyConf.Name + index
func getConfigurationPolicyName(policyConf *types.PolicyConfig, manifestGroupIndex, policyNum,
manifestIndex int, useManifestName bool,
// getConfigurationPolicyName returns the `ConfigurationPolicy` name based on the provided PolicyConfig.
// When a name is assigned, it increments the associated counter, using it as a suffix if it's greater
// than one to create unique names. If both `manifest[].name` and `policies.name` are provided,
// `manifest[].name` takes priority as the configuration name.
func getConfigurationPolicyName(policyConf *types.PolicyConfig, manifestGroupIndex int,
policyNameCounter *int, manifestNameCounter *int,
) string {
// Do not append a number to configName when it is used for the first time.
configName := policyConf.Manifests[manifestGroupIndex].Name

if useManifestName && configName != "" {
if manifestIndex > 0 {
return fmt.Sprintf("%s%d", configName, manifestIndex+1)
// For the case where ConsolidateManifests is true and the manifest's name is provided,
if manifestNameCounter == nil && configName != "" {
return configName
}

if manifestNameCounter != nil && configName != "" {
*manifestNameCounter++
if *manifestNameCounter > 1 {
return fmt.Sprintf("%s%d", configName, *manifestNameCounter)
}

return configName
}

configName = policyConf.Name
if policyNum > 0 {
return fmt.Sprintf("%s%d", configName, policyNum+1)

*policyNameCounter++
if *policyNameCounter > 1 {
return fmt.Sprintf("%s%d", configName, *policyNameCounter)
}

return configName
Expand Down