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: add CleanupPolicy validation code to CleanupPolicyHandler #5338

Merged
merged 6 commits into from
Nov 16, 2022

Conversation

NikhilSharmaWe
Copy link
Contributor

@NikhilSharmaWe NikhilSharmaWe commented Nov 14, 2022

Signed-off-by: Nikhil Sharma nikhilsharma230303@gmail.com

Explanation

This PR add validation code for validating CleanupPolicies to CleanupPolicyHandler.

Validations added:

  1. Check whether the policy name is valid or not.
  2. Check whether the Schedule specified by the user is in proper cron format or not.
  3. Validate the MatchResources and ExcludeResources.
  4. Check if the resultant of match and exclude block is not an empty set.
  5. Check whether the CleanupPolicy have permission to delete the matching resources.

@codecov
Copy link

codecov bot commented Nov 14, 2022

Codecov Report

Merging #5338 (542de69) into main (c37e9d4) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #5338      +/-   ##
==========================================
- Coverage   36.03%   35.98%   -0.05%     
==========================================
  Files         167      169       +2     
  Lines       18913    18938      +25     
==========================================
  Hits         6815     6815              
- Misses      11309    11334      +25     
  Partials      789      789              
Impacted Files Coverage Δ
pkg/utils/admission/cleanup.go 0.00% <0.00%> (ø)
pkg/utils/admission/policy.go 0.00% <0.00%> (ø)
pkg/utils/admission/utils.go 0.00% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

os.Exit(1)
}

openApiManager, err := openapi.NewManager()
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the open api manager ?

@eddycharly
Copy link
Member

This is a good start. Can you add a kuttl test, at least for simple cases ?

@eddycharly
Copy link
Member

@NikhilSharmaWe it looks like we are not supporting namespaced policy ?

@eddycharly eddycharly added this to the Kyverno Release 1.9.0 milestone Nov 14, 2022
@eddycharly eddycharly self-assigned this Nov 14, 2022
@NikhilSharmaWe
Copy link
Contributor Author

@eddycharly We are using CleanupPolicyInterface, which includes both CleanupPolicy and ClusterCleanupPolicy.

@eddycharly
Copy link
Member

eddycharly commented Nov 14, 2022

@NikhilSharmaWe I mean in this code:

func UnmarshalCleanupPolicy(kind string, raw []byte) (kyvernov1alpha1.CleanupPolicyInterface, error) {
	var policy kyvernov1alpha1.CleanupPolicyInterface
	if kind == "CleanupPolicy" {
		if err := json.Unmarshal(raw, &policy); err != nil {
			return policy, err
		}
		return policy, nil
	}
	return policy, fmt.Errorf("admission request does not contain a cleanuppolicy")
}

How is this going to work with ClusterCleanupPolicy ?


type cleanupPolicyHandlers struct {
client dclient.Interface
openApiManager openapi.Manager
Copy link
Member

Choose a reason for hiding this comment

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

Why add openAPIManager? Do we need extra schema validation for cleanup policies?

Copy link
Member

Choose a reason for hiding this comment

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

Aren't we going to need it to validate the resource scope ?

// setup signals
signalCtx, signalCancel := setupSignals()
defer signalCancel()
metricsConfig, metricsShutdown, err := setupMetrics(logger, kubeClient)
Copy link
Member

Choose a reason for hiding this comment

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

What metrics do we want to expose for cleanup policies?

cmd/cleanup-controller/utils/utils.go Outdated Show resolved Hide resolved
@realshuting
Copy link
Member

@NikhilSharmaWe - can you be more specific about what validations are added via this PR?

Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>
Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
@eddycharly eddycharly marked this pull request as ready for review November 15, 2022 17:15
Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>
@eddycharly eddycharly enabled auto-merge (squash) November 15, 2022 20:08
@eddycharly eddycharly merged commit 0fb45ed into kyverno:main Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants