GH-324 pollicyaffected condition for gateway #433

maksymvavilov · 2023-08-17T11:44:33Z

closes #324

What

Reporting the state of the DNSPolicy to the Gateway affected by that policy

Verification

Get the MGC running
Create a DNSPolicy

kubectl apply -f - <<EOF
apiVersion: kuadrant.io/v1alpha1
kind: DNSPolicy
metadata:
  name: prod-web
  namespace: multi-cluster-gateways
spec:
  targetRef:
    name: prod-web
    group: gateway.networking.k8s.io
    kind: Gateway
  healthCheck:
    allowInsecureCertificates: true
    endpoint: /
    expectedResponses:
      - 200
      - 201
      - 301
    failureThreshold: 5
    port: 443
    protocol: https
EOF

Watch conditions on the gateway getting updated.

openshift-ci · 2023-08-17T11:44:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: makslion
Once this PR has been reviewed and has the lgtm label, please assign sergioifg94 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

maksymvavilov · 2023-08-17T11:53:36Z

/hold
I'd like to ensure that this is what we want and how we want.
The "what" part:

We have an annotation on the Gateway referencing policy. The conditions block looks like a glorified annotation with an error message attached in case something goes wrong. In any case, it might be worth discussing with Kuadrant people since they are the ones to implement an annotation.
There is still a discussion on the Gateway side about how this should be done. Since all this is still considered to be "experimental" and aimed to land after the GA should we wait for them to deploy conditions for the DNSPolicy itself?

The "how" part:

We are about to have more stuff that could use those conditions - such as TLS policies. Would it be worth "standardizing" the way we do conditions?
Annotations vs condition blocks - one of them or both?

Also, invite @mikenairn to add more or correct me on this.

maleck13 · 2023-08-21T07:49:33Z

pkg/_internal/conditions/conditions.go

-	ConditionTypeReady string = "Ready"
+	ConditionTypeReady ConditionType = "Ready"
+
+	DNSPolicyAffected         ConditionType   = "DNSPolicyAffected"


hmm I am wondering if this would be better in the dnspolicy package?

maleck13 · 2023-08-21T08:14:37Z

pkg/controllers/dnspolicy/dnspolicy_controller.go

@@ -225,6 +236,49 @@ func (r *DNSPolicyReconciler) readyCondition(targetNetworkObjectectKind string,
 	return cond
 }

+func buildGatewayCondition(reason conditions.ConditionReason, err error) *metav1.Condition {


prob should name this buildDNSPolicyAffectedCondition ?

or we could make it a bit more generic and add it to the conditions package

func buildPolicyAffectedCondition(conditionType conditions.ConditionType, policy metav1.Object, reason conditions.ConditionReason, err error) *metav1.Condition { condition := &metav1.Condition{ Type: string(conditionType), Status: metav1.ConditionTrue, Reason: string(reason), Message: fmt.Sprintf("%s policy affected. policy namespace %s policy name", conditionType, policy.Namespace, policy.Name), } if err != nil { condition.Status = metav1.ConditionFalse condition.Message = err.Error() } return condition }

maleck13 · 2023-08-21T08:38:42Z

pkg/controllers/dnspolicy/dnspolicy_controller.go

+	return condition
+}
+
+func (r *DNSPolicyReconciler) updateGatewayCondition(ctx context.Context, condition *metav1.Condition, gatewayDiff *reconcilers.GatewayDiff) error {


These seem like potentially generic for all policy controllers?

maleck13 · 2023-08-21T08:39:17Z

pkg/controllers/dnspolicy/dnspolicy_controller.go

-	if err := r.reconcileDNSRecords(ctx, dnsPolicy, gatewayDiffObj); err != nil {
+	if err = r.reconcileDNSRecords(ctx, dnsPolicy, gatewayDiffObj); err != nil {
+		gatewayCondition = buildGatewayCondition(conditions.DNSPolicyReasonInvalid, err)
+		_ = r.updateGatewayCondition(ctx, gatewayCondition, gatewayDiffObj)


not sure why we are ignoring the error here?

maleck13 · 2023-08-21T08:39:41Z

pkg/controllers/dnspolicy/dnspolicy_controller.go

-	if err := r.reconcileHealthChecks(ctx, dnsPolicy, gatewayDiffObj); err != nil {
+	if err = r.reconcileHealthChecks(ctx, dnsPolicy, gatewayDiffObj); err != nil {
+		gatewayCondition = buildGatewayCondition(conditions.DNSPolicyReasonInvalid, err)
+		_ = r.updateGatewayCondition(ctx, gatewayCondition, gatewayDiffObj)


same here and below. Not much point in returning an error if all we do is ignore it

maleck13 · 2023-08-21T09:14:23Z

test/integration/dnspolicy_controller_test.go

@@ -357,7 +357,7 @@ var _ = Describe("DNSPolicy", Ordered, func() {
 						return false
 					}

-					return meta.IsStatusConditionTrue(dnsPolicy.Status.Conditions, conditions.ConditionTypeReady)
+					return meta.IsStatusConditionTrue(dnsPolicy.Status.Conditions, string(conditions.ConditionTypeReady))


why did we change the types here out of curiosity

maleck13 · 2023-08-21T15:21:38Z

#436

maksymvavilov had a problem deploying to e2e-internal August 17, 2023 11:44 — with GitHub Actions Failure

openshift-ci bot requested review from jasonmadigan and mikenairn August 17, 2023 11:44

maksymvavilov linked an issue Aug 17, 2023 that may be closed by this pull request

Add PolicyAffected status condition for gateway #324

Closed

openshift-ci bot added the do-not-merge/hold label Aug 17, 2023

maksymvavilov force-pushed the GH-324 branch from f540886 to 729a026 Compare August 17, 2023 12:06

maksymvavilov had a problem deploying to e2e-internal August 17, 2023 12:06 — with GitHub Actions Failure

maksymvavilov requested a review from maleck13 August 17, 2023 12:07

maksymvavilov force-pushed the GH-324 branch from 729a026 to 026b971 Compare August 17, 2023 12:14

maksymvavilov had a problem deploying to e2e-internal August 17, 2023 12:14 — with GitHub Actions Failure

KuadrantGH-324 pollicyaffected condition for gateway

c877a64

maksymvavilov force-pushed the GH-324 branch from 026b971 to c877a64 Compare August 17, 2023 12:30

maksymvavilov temporarily deployed to e2e-internal August 17, 2023 12:30 — with GitHub Actions Inactive

maleck13 reviewed Aug 21, 2023

View reviewed changes

maleck13 mentioned this pull request Aug 21, 2023

Gh 324 policy affected condition #436

Merged

maleck13 closed this Aug 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-324 pollicyaffected condition for gateway #433

GH-324 pollicyaffected condition for gateway #433

maksymvavilov commented Aug 17, 2023 •

edited by maleck13

Loading

openshift-ci bot commented Aug 17, 2023

maksymvavilov commented Aug 17, 2023 •

edited

Loading

maleck13 Aug 21, 2023

maleck13 Aug 21, 2023 •

edited

Loading

maleck13 Aug 21, 2023 •

edited

Loading

maleck13 Aug 21, 2023

maleck13 Aug 21, 2023

maleck13 Aug 21, 2023

maleck13 Aug 21, 2023

maleck13 commented Aug 21, 2023

GH-324 pollicyaffected condition for gateway #433

GH-324 pollicyaffected condition for gateway #433

Conversation

maksymvavilov commented Aug 17, 2023 • edited by maleck13 Loading

What

Verification

openshift-ci bot commented Aug 17, 2023

maksymvavilov commented Aug 17, 2023 • edited Loading

maleck13 Aug 21, 2023

Choose a reason for hiding this comment

maleck13 Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

maleck13 Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

maleck13 Aug 21, 2023

Choose a reason for hiding this comment

maleck13 Aug 21, 2023

Choose a reason for hiding this comment

maleck13 Aug 21, 2023

Choose a reason for hiding this comment

maleck13 Aug 21, 2023

Choose a reason for hiding this comment

maleck13 commented Aug 21, 2023

maksymvavilov commented Aug 17, 2023 •

edited by maleck13

Loading

maksymvavilov commented Aug 17, 2023 •

edited

Loading

maleck13 Aug 21, 2023 •

edited

Loading

maleck13 Aug 21, 2023 •

edited

Loading