Skip to content

Commit

Permalink
Initial changes per PR review
Browse files Browse the repository at this point in the history 
Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
  • Loading branch information
@JustinKuli
JustinKuli committed Jan 29, 2024
1 parent 2185f7b commit 6454bb7
Show file tree
Hide file tree
Showing 3 changed files with 159 additions and 150 deletions.
209 changes: 107 additions & 102 deletions controllers/operatorpolicy_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/validation"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/builder"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -172,78 +173,74 @@ func (r *OperatorPolicyReconciler) handleOpGroup(ctx context.Context, policy *po
case 1:
opGroup := foundOpGroups[0]

if policy.Spec.OperatorGroup != nil {
emptyNameMatch := desiredOpGroup.Name == "" && opGroup.GetGenerateName() == desiredOpGroup.GenerateName

if opGroup.GetName() == desiredOpGroup.Name || emptyNameMatch {
// check whether the specs match
desiredUnstruct, err := runtime.DefaultUnstructuredConverter.ToUnstructured(desiredOpGroup)
if err != nil {
return fmt.Errorf("error converting desired OperatorGroup to an Unstructured: %w", err)
}

if reflect.DeepEqual(desiredUnstruct["spec"], opGroup.Object["spec"]) {
// Everything relevant matches! This is a "happy path".
// Only update the condition if it reflects a NonCompliant state because emitting a
// Compliant -> Compliant 'change' is noisy and would hide *why* this part of the
// policy is currently compliant (eg that the policy previously updated the object).
idx, existingCond := policy.Status.GetCondition(opGroupConditionType)

if idx == -1 || existingCond.Status == metav1.ConditionFalse {
err := r.updateStatus(ctx, policy, matchesCond("OperatorGroup"), matchedObj(&opGroup))
if err != nil {
return fmt.Errorf("error updating the status for an OperatorGroup that matches: %w", err)
}
}
} else {
// The names match, but the specs don't: report NonCompliance
err := r.updateStatus(ctx, policy, mismatchCond("OperatorGroup"), mismatchedObj(&opGroup))
if err != nil {
return fmt.Errorf("error updating the status for an OperatorGroup that does not match: %w", err)
}

if policy.Spec.RemediationAction.IsEnforce() {
desiredOpGroup.ResourceVersion = opGroup.GetResourceVersion()

err := r.Update(ctx, desiredOpGroup)
if err != nil {
return fmt.Errorf("error updating the OperatorGroup: %w", err)
}
desiredOpGroup.SetGroupVersionKind(operatorGroupGVK) // Update stripped this information

// It was updated and should match now, so report Compliance
err = r.updateStatus(ctx, policy, updatedCond("OperatorGroup"), updatedObj(desiredOpGroup))
if err != nil {
return fmt.Errorf("error updating the status after updating the OperatorGroup: %w", err)
}
}
}
} else {
// There is an OperatorGroup in the namespace that does not match the name of what is in the policy.
// Just creating a new one would cause the "TooManyOperatorGroups" failure.
// So, just report a NonCompliant status.
missing := missingWantedObj(desiredOpGroup)
badExisting := mismatchedObj(&opGroup)

err := r.updateStatus(ctx, policy, mismatchCond("OperatorGroup"), missing, badExisting)
if err != nil {
return fmt.Errorf("error updating the status for an OperatorGroup with the wrong name: %w", err)
}
}
} else {
if policy.Spec.OperatorGroup == nil {
// There is one operator group in the namespace, but the policy doesn't specify what it should look like.

// FUTURE: check if the one operator group is compatible with the desired subscription.

// For an initial implementation, assume if an OperatorGroup already exists, then it's a good one.
// Only update the condition if it reflects a NonCompliant state, to prevent repeats.
idx, existingCond := policy.Status.GetCondition(opGroupConditionType)

if idx == -1 || existingCond.Status == metav1.ConditionFalse {
err := r.updateStatus(ctx, policy, matchesCond("OperatorGroup"), matchedObj(&opGroup))
if err != nil {
return fmt.Errorf("error updating the status for an OperatorGroup that matches: %w", err)
}
err := r.updateStatus(ctx, policy, opGroupPreexistingCond, matchedObj(&opGroup))
if err != nil {
return fmt.Errorf("error updating the status for an OperatorGroup that matches: %w", err)
}

return nil
}

// OperatorGroup is specified in the policy, check if what's on the cluster matches.
emptyNameMatch := desiredOpGroup.Name == "" && opGroup.GetGenerateName() == desiredOpGroup.GenerateName

if !(opGroup.GetName() == desiredOpGroup.Name || emptyNameMatch) {
// There is an OperatorGroup in the namespace that does not match the name of what is in the policy.
// Just creating a new one would cause the "TooManyOperatorGroups" failure.
// So, just report a NonCompliant status.
missing := missingWantedObj(desiredOpGroup)
badExisting := mismatchedObj(&opGroup)

err := r.updateStatus(ctx, policy, mismatchCond("OperatorGroup"), missing, badExisting)
if err != nil {
return fmt.Errorf("error updating the status for an OperatorGroup with the wrong name: %w", err)
}

return nil
}

// check whether the specs match
desiredUnstruct, err := runtime.DefaultUnstructuredConverter.ToUnstructured(desiredOpGroup)
if err != nil {
return fmt.Errorf("error converting desired OperatorGroup to an Unstructured: %w", err)
}

// NOTE: this is more like mustonlyhave than musthave, and won't quite work.
// something from config-policy will be needed to check if the specs match.
if reflect.DeepEqual(desiredUnstruct["spec"], opGroup.Object["spec"]) {
// Everything relevant matches!
err := r.updateStatus(ctx, policy, matchesCond("OperatorGroup"), matchedObj(&opGroup))
if err != nil {
return fmt.Errorf("error updating the status for an OperatorGroup that matches: %w", err)
}

return nil
}

// The names match, but the specs don't: report NonCompliance
err = r.updateStatus(ctx, policy, mismatchCond("OperatorGroup"), mismatchedObj(&opGroup))
if err != nil {
return fmt.Errorf("error updating the status for an OperatorGroup that does not match: %w", err)
}

if policy.Spec.RemediationAction.IsEnforce() {
desiredOpGroup.ResourceVersion = opGroup.GetResourceVersion()

err := r.Update(ctx, desiredOpGroup)
if err != nil {
return fmt.Errorf("error updating the OperatorGroup: %w", err)
}

desiredOpGroup.SetGroupVersionKind(operatorGroupGVK) // Update stripped this information

// It was updated and should match now, so report Compliance
err = r.updateStatus(ctx, policy, updatedCond("OperatorGroup"), updatedObj(desiredOpGroup))
if err != nil {
return fmt.Errorf("error updating the status after updating the OperatorGroup: %w", err)
}
}
default:
Expand Down Expand Up @@ -280,6 +277,10 @@ func buildOperatorGroup(
return nil, fmt.Errorf("namespace is required in spec.subscription")
}

if validationErrs := validation.IsDNS1123Label(subNamespace); len(validationErrs) != 0 {
return nil, fmt.Errorf("the namespace specified in spec.subscription is not a valid namespace identifier")
}

// Create a default OperatorGroup if one wasn't specified in the policy
if policy.Spec.OperatorGroup == nil {
operatorGroup.ObjectMeta.SetNamespace(subNamespace)
Expand All @@ -299,7 +300,7 @@ func buildOperatorGroup(
// Fallback to the Subscription namespace if the OperatorGroup namespace is not specified in the policy.
ogNamespace := subNamespace

if specifiedNS, ok := opGroup["namespace"].(string); ok {
if specifiedNS, ok := opGroup["namespace"].(string); ok || specifiedNS == "" {
ogNamespace = specifiedNS
}

Expand Down Expand Up @@ -332,7 +333,7 @@ func (r *OperatorPolicyReconciler) handleSubscription(ctx context.Context, polic

foundSub, err := r.DynamicWatcher.Get(watcher, subscriptionGVK, desiredSub.Namespace, desiredSub.Name)
if err != nil {
return fmt.Errorf("error listing OperatorGroups: %w", err)
return fmt.Errorf("error getting the Subscription: %w", err)
}

if foundSub == nil {
Expand All @@ -356,44 +357,48 @@ func (r *OperatorPolicyReconciler) handleSubscription(ctx context.Context, polic
return fmt.Errorf("error updating the status for a created Subscription: %w", err)
}
}
} else {
// Subscription found; check if specs match
desiredUnstruct, err := runtime.DefaultUnstructuredConverter.ToUnstructured(desiredSub)

return nil
}

// Subscription found; check if specs match
desiredUnstruct, err := runtime.DefaultUnstructuredConverter.ToUnstructured(desiredSub)
if err != nil {
return fmt.Errorf("error converting desired Subscription to an Unstructured: %w", err)
}

// NOTE: this is more like mustonlyhave than musthave, and won't quite work.
// something from config-policy will be needed to check if the specs match.
if reflect.DeepEqual(desiredUnstruct["spec"], foundSub.Object["spec"]) {
// FUTURE: Check more details about the *status* of the Subscription
// For now, just mark it as compliant
err := r.updateStatus(ctx, policy, matchesCond("Subscription"), matchedObj(foundSub))
if err != nil {
return fmt.Errorf("error converting desired Subscription to an Unstructured: %w", err)
return fmt.Errorf("error updating the status for an OperatorGroup that matches: %w", err)
}

if reflect.DeepEqual(desiredUnstruct["spec"], foundSub.Object["spec"]) {
// FUTURE: Check more details about the *status* of the Subscription
// For now, (conditionally) mark it as compliant
idx, existingCond := policy.Status.GetCondition(subConditionType)
return nil
}

if idx == -1 || existingCond.Status == metav1.ConditionFalse {
err := r.updateStatus(ctx, policy, matchesCond("Subscription"), matchedObj(foundSub))
if err != nil {
return fmt.Errorf("error updating the status for an OperatorGroup that matches: %w", err)
}
}
} else {
err := r.updateStatus(ctx, policy, mismatchCond("Subscription"), mismatchedObj(foundSub))
if err != nil {
return fmt.Errorf("error updating status for a mismatched Subscription: %w", err)
}
// Specs don't match.
err = r.updateStatus(ctx, policy, mismatchCond("Subscription"), mismatchedObj(foundSub))
if err != nil {
return fmt.Errorf("error updating status for a mismatched Subscription: %w", err)
}

if policy.Spec.RemediationAction.IsEnforce() {
desiredSub.ResourceVersion = foundSub.GetResourceVersion()

if policy.Spec.RemediationAction.IsEnforce() {
desiredSub.ResourceVersion = foundSub.GetResourceVersion()
err := r.Update(ctx, desiredSub)
if err != nil {
return fmt.Errorf("error updating the Subscription: %w", err)
}

err := r.Update(ctx, desiredSub)
if err != nil {
return fmt.Errorf("error updating the Subscription: %w", err)
}
desiredSub.SetGroupVersionKind(subscriptionGVK) // Update stripped this information
desiredSub.SetGroupVersionKind(subscriptionGVK) // Update stripped this information

err = r.updateStatus(ctx, policy, updatedCond("Subscription"), updatedObj(desiredSub))
if err != nil {
return fmt.Errorf("error updating status after updating the Subscription: %w", err)
}
}
err = r.updateStatus(ctx, policy, updatedCond("Subscription"), updatedObj(desiredSub))
if err != nil {
return fmt.Errorf("error updating status after updating the Subscription: %w", err)
}
}

Expand Down
65 changes: 39 additions & 26 deletions controllers/operatorpolicy_status.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (
// condition has changed. If not provided, the `lastTransitionTime` will use "now". It also handles
// preserving the `CreatedByPolicy` property on relatedObjects.
//
// This function assumes that all given related objects are of the same kind.
// This function requires that all given related objects are of the same kind.
//
// Note that only changing the related objects will not emit a new compliance event, but will update
// the status.
Expand Down Expand Up @@ -81,37 +81,42 @@ func (r *OperatorPolicyReconciler) updateStatus(

relObjsChanged := false

prevRelObjs := policy.Status.RelatedObjsOfKind(updatedRelatedObjs[0].Object.Kind)
if len(prevRelObjs) == len(updatedRelatedObjs) {
for _, prevObj := range prevRelObjs {
nameFound := false

for i, updatedObj := range updatedRelatedObjs {
if prevObj.Object.Metadata.Name == updatedObj.Object.Metadata.Name {
nameFound = true

if updatedObj.Properties != nil && prevObj.Properties != nil {
if updatedObj.Properties.UID != prevObj.Properties.UID {
relObjsChanged = true
} else if prevObj.Properties.CreatedByPolicy != nil {
// There is an assumption here that it will never need to transition to false.
updatedRelatedObjs[i].Properties.CreatedByPolicy = prevObj.Properties.CreatedByPolicy
}
}

if prevObj.Compliant != updatedObj.Compliant {
relObjsChanged = true
} else if prevObj.Reason != updatedObj.Reason {
relObjsChanged = true
}
prevRelObjs := make(map[int]policyv1.RelatedObject)
if len(updatedRelatedObjs) != 0 {
prevRelObjs = policy.Status.RelatedObjsOfKind(updatedRelatedObjs[0].Object.Kind)
}

for _, prevObj := range prevRelObjs {
nameFound := false

for i, updatedObj := range updatedRelatedObjs {
if prevObj.Object.Metadata.Name != updatedObj.Object.Metadata.Name {
continue
}

nameFound = true

if updatedObj.Properties != nil && prevObj.Properties != nil {
if updatedObj.Properties.UID != prevObj.Properties.UID {
relObjsChanged = true
} else if prevObj.Properties.CreatedByPolicy != nil {
// There is an assumption here that it will never need to transition to false.
updatedRelatedObjs[i].Properties.CreatedByPolicy = prevObj.Properties.CreatedByPolicy
}
}

if !nameFound {
if prevObj.Compliant != updatedObj.Compliant || prevObj.Reason != updatedObj.Reason {
relObjsChanged = true
}
}
} else {

if !nameFound {
relObjsChanged = true
}
}

// Catch the case where there is a new object in updatedRelatedObjs
if len(prevRelObjs) != len(updatedRelatedObjs) {
relObjsChanged = true
}

Expand Down Expand Up @@ -340,6 +345,14 @@ func updatedCond(kind string) metav1.Condition {
}
}

var opGroupPreexistingCond = metav1.Condition{
Type: opGroupConditionType,
Status: metav1.ConditionTrue,
Reason: "PreexistingOperatorGroupFound",
Message: "the policy does not specify an OperatorGroup but one already exists in the namespace - " +
"assuming that OperatorGroup is correct",
}

// opGroupTooManyCond is a NonCompliant condition with a Reason like 'TooManyOperatorGroups',
// and a Message like 'there is more than one OperatorGroup in the namespace'
var opGroupTooManyCond = metav1.Condition{
Expand Down
Loading

0 comments on commit 6454bb7

Please sign in to comment.