#522 - Policy Mutation Validation #736
Conversation
shravanshetty1
changed the title
shravanshetty1
changed the title
|
fixes #522 |
|
Adding @shivdudhani as the reviewer as he had some doubts regarding the previous PR. |
|
I'm unclear why we are using 2 clients both using The |
| for kind, rules := range kindToRules { | ||
| newPolicy := *policy.DeepCopy() | ||
| newPolicy.Spec.Rules = rules | ||
| resource, _ := generateEmptyResource(openApiGlobalState.definitions[openApiGlobalState.kindToDefinitionName[kind]]).(map[string]interface{}) |
There was a problem hiding this comment.
Does this generateEmptyResource fill in the default value for each field? As previously we encountered the issue described here, when the policy is defined with variable context, it throws error as the referred path is not present.
There was a problem hiding this comment.
Yes, if the openApi schema does have a default value for the field - it will use that over an empty value.
I have added the forcemutate function which does not check for any conditions and simply mutates the given resource.
Any substitution variables {{suchAsThese}}, will be replaced with dummy values - and does not check any conditions before mutating. Hence if there is a rule which wants to mutate resource if it has a specific name(which would fail since empty resource has only empty values) - forcemutate ensures that it will do the mutation without checking the condition.
The patched resource is then validated.
I am not sure what you mean, currently there is only 1 client being used which calls |
|
We already have a ticker which polls in the client package which can be reused that adding a new ticker and another wrapper to the client. |
OpenAPISchema is only being called once and not part of the new ticker. I implemented it this way since after testing i found out that openAPISchema from server is not being updated with CRD's |
| if mutation.Overlay != nil { | ||
| overlay := mutation.Overlay | ||
| if ctx != nil { | ||
| if overlay, err = variables.SubstituteVars(ctx, overlay); err != nil { |
There was a problem hiding this comment.
This ForceMutate function is only called once and we pass nil to ctx. Do we need to substitute variables here if ctx != nil?
The concern here is the ctx is built with the admission request, while this function validates policies that are potentially created by cluster admin, it is usually not what the policy defined for. For example, I want to substitute service account in a rule, while the serviceaccount should be the one who creates the resource, not the .one who creates the policy.
There was a problem hiding this comment.
In the current implementation ctx is never passed in - i added this code in the hopes that this can more easily be able to become a subset for engine.Mutate.
Policy validation will never need to send ctx so it will not have the above issues.
When it eventually becomes part of engine.mutate - the ctx will be valid since its generated when resource is being applied to policy
No description provided.