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

validate the presence of specified issuer or clusterisssuer in tlspolicy #594

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

laurafitzgerald
Copy link
Contributor

@laurafitzgerald laurafitzgerald commented Sep 21, 2023

What

Closes #581

Verification
Follow https://docs.kuadrant.io/getting-started/
Follow https://docs.kuadrant.io/multicluster-gateway-controller/docs/how-to/multicluster-gateways-walkthrough/ until the creation of the TLSPolicy

Error should be reported in status for ClusterIssuer that doesn't exist in cluster

Create TLSPolicy pointing to ClusterIssuer with a name that doesn't exist in the cluster
Validate that error is present in the status of the TLSPolicy

 - lastTransitionTime: "2023-09-21T10:13:13Z"
      message: ClusterIssuer.cert-manager.io "blah" not found
      reason: ReconciliationError
      status: "False"
      type: Ready

Error should be reported in status for Issuer that doesn't exist in the namespace of the tlspolicy

Create an issuer in a namespace that is different to the TLSPolicy - steps here to do that https://github.com/Kuadrant/multicluster-gateway-controller/blob/main/docs/tlspolicy/tls-policy.md#lets-encrypt-issuer-for-route53-hosted-domain - update the namespace in the issuer in the steps to another namespace in the cluster

Validate that error is present in the status of the TLSPolicy

- lastTransitionTime: "2023-09-21T10:13:13Z"
      message: Issuer.cert-manager.io "le-staging" not found
      reason: ReconciliationError
      status: "False"
      type: Ready

Error should NOT be reported in status for Issuer that does exist in the same namespace as the tlspolicy

Create the same issuer but in the same namespace as the tlspolicy e.g. multi-cluster-gateways
Validate the status is good

    - lastTransitionTime: "2023-09-21T10:21:11Z"
      message: Gateway is TLS Enabled
      reason: GatewayTLSEnabled
      status: "True"
      type: Ready

Error should NOT be reported in status for ClusterIssuer that does exist in the same namespace as the tlspolicy

Update the policy to have a valid ClusterIssuer glbc
e.g.

issuerRef:
      group: cert-manager.io
      kind: ClusterIssuer
      name: glbc-ca

Validate the status is good

    - lastTransitionTime: "2023-09-21T10:21:11Z"
      message: Gateway is TLS Enabled
      reason: GatewayTLSEnabled
      status: "True"
      type: Ready

Error should NOT be reported in status for Issuer that does not specify kind

Update the policy to have the following issuerRef - create the issuer in the same namespace if needed
e.g.

issuerRef:
      group: cert-manager.io
      name: le-staging

Validate the status is good

    - lastTransitionTime: "2023-09-21T10:21:11Z"
      message: Gateway is TLS Enabled
      reason: GatewayTLSEnabled
      status: "True"
      type: Ready

@laurafitzgerald laurafitzgerald changed the title validate the presence of specified cluster or clusterisssuer in tlspolicy validate the presence of specified issuer or clusterisssuer in tlspolicy Sep 21, 2023
pkg/controllers/tlspolicy/tlspolicy_controller.go Outdated Show resolved Hide resolved
test/e2e/gateway_single_spoke_test.go Outdated Show resolved Hide resolved
test/integration/tlspolicy_controller_test.go Outdated Show resolved Hide resolved
test/util/suite_config.go Outdated Show resolved Hide resolved
test/util/test_types.go Outdated Show resolved Hide resolved
@laurafitzgerald
Copy link
Contributor Author

the e2e tests are passing locally for me. so i'm looking into that.

@laurafitzgerald laurafitzgerald temporarily deployed to e2e-internal September 21, 2023 16:24 — with GitHub Actions Inactive
@mikenairn
Copy link
Member

Couple of comments, but looks fine i think.

It did make me wonder if this is the best way to do this kind of thing, ideally we wouldn't be doing validation that is expected to be done by cert manager (the thing this policy is wrapping), but rather rely on aggregating the status of created cert manager resources (Certificates), or expected generated resources (CertificateRequests), since this would be a truer reflection of the state of the policy.

Example CertificateRequests status(cert manager 1.13.0), that we could get this info from:

kubectl get certificaterequests mn-hcpapps-net-tls-1 -n single-cluster-gateways -o json | jq '.status.conditions[] | select(.type == "Ready")'
{
  "lastTransitionTime": "2023-09-22T07:42:03Z",
  "message": "Referenced \"ClusterIssuer\" not found: clusterissuer.cert-manager.io \"glbc-caaaaa\" not found",
  "reason": "Pending",
  "status": "False",
  "type": "Ready"
}

Since we aren't going to do that aggregation now, and will likely be done in line with the proposed kuadrant policy status/conditions, a basic check like this is probably fine to avoid unnecessary confusion in the meantime.

@mikenairn
Copy link
Member

/lgtm
/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 22, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: laurafitzgerald, mikenairn

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@maleck13 maleck13 added this pull request to the merge queue Sep 22, 2023
Merged via the queue into Kuadrant:main with commit 7bd2464 Sep 22, 2023
10 checks passed
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.

Incorrect status reported in tlspolicy when issuerRef points to an issuer that doesn't exist
4 participants