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

add target group policy controller and status updates #509

Merged
merged 6 commits into from
Nov 17, 2023

Conversation

mikhail-aws
Copy link
Contributor

@mikhail-aws mikhail-aws commented Nov 16, 2023

Note:
Add TargetGroupPolicy controller. Controller validate targetRef and update status. TargetGroup builder applies only Accepted and TargetRefNotFound policies. TargetRefNotFound is safe to apply, it means policy was created before TG.
I have to update CRD because Status wasnt there fully. Users need to re-apply CRD after this change.

Tested manually:

  • Create policy before TG, after TG was created checked policy applied
  • Create multiple conflicting policies and checked none of them applied
  • Created policy and TG, then created conflicting policy. Checked second conflicting policy didnt override first one

We should refactor our PolicyAttachment code, right now every Policy CRD implements same validation and status update logic. Need to create general purpose PolicyAttachment handler for validation and status update. #508

close #440

@solmonk
Copy link
Contributor

solmonk commented Nov 16, 2023

I have to update CRD because Status wasnt there fully. Users need to re-apply CRD after this change.

So when the controller upgrades without CRD upgrade, it will disregard all TGPs and turn HTTP2 TGs into HTTP1 TGs, it seems. It is a bit worrying behavior to me actually. Should we add a fallback, like consider TGPs with no status as valid? Or, should we increment TGP version so that only old TGPs with no status are valid?

@@ -20,7 +22,11 @@ type policyInfo struct {
kind gwv1beta1.Kind
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like that would be a better policyInfo naming:

type policyInfo struct {
	policyList core.PolicyList
	targetRefGroup      gwv1beta1.Group
	targetRefKind       gwv1beta1.Kind
}

Reasons []gwv1alpha2.PolicyConditionReason
}

func GetAttachedPoliciesConditionFilter[T core.Policy](ctx context.Context, k8sClient client.Client, searchTargetRef types.NamespacedName, policy T, filter PolicyConditionFilter) ([]T, error) {
Copy link
Contributor

@zijun726911 zijun726911 Nov 16, 2023

Choose a reason for hiding this comment

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

Seems this function actually do: GetAttachedPoliciesFilterByAcceptedConditionReasons , would be good if we can add comments or change function name to make people more easily know this function's behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed and refactored for conflict resolution logic

@zijun726911
Copy link
Contributor

I have to update CRD because Status wasnt there fully. Users need to re-apply CRD after this change.

So when the controller upgrades without CRD upgrade, it will disregard all TGPs and turn HTTP2 TGs into HTTP1 TGs, it seems. It is a bit worrying behavior to me actually. Should we add a fallback, like consider TGPs with no status as valid? Or, should we increment TGP version so that only old TGPs with no status are valid?

Just provide upgrade instructions and do this breaking change before v1.0.0 looks fine for me

@mikhail-aws mikhail-aws force-pushed the target-group-policy-controller branch from 2864f21 to 6236a9f Compare November 17, 2023 00:04
Copy link
Contributor

@solmonk solmonk left a comment

Choose a reason for hiding this comment

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

The approach overall LGTM.

@solmonk solmonk merged commit ed90166 into aws:main Nov 17, 2023
5 checks passed
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.

TargetGroupPolicy and VpcAssociationPolicy CRD status Update
3 participants