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

feat: add csr-domain-name config option (#381) #386

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

DnPlas
Copy link
Contributor

@DnPlas DnPlas commented Mar 1, 2024

feat: enable csr-domain-name config option so istio-pilot can use it on CSRs

The istio-pilot charm already has a mechanism in place to discover the ingress gateway address from the Service, but it is limited to only returning IP addresses, which not all TLS certificate providers accept as a valid cert subject. Having the domain-name config option will allow users to specify the domain name they'd like to use when integrating with TLS certificate operators. This feature expands the support for integrating with TLS certificate providers that cannot issue signed certificates on a CSR that only contains an IP address (like we used to do). This commit also adds some test coverage to test the recently added code.

Fixes #379

NOTE: CI may fail because of #385 and #384

Testing instructions

Assuming you've got a microk8s deployment with
Ensure the upgrade is done correctly:

  1. Deploy istio-operators
juju deploy istio-gateway istio-ingressgateway --config kind=ingress --channel 1.17/stable --trust
juju deploy istio-pilot --channel 1.17/stable --trust
juju relate istio-pilot istio-ingressgateway
  1. Wait for them to be active and idle
  2. Refresh istio-pilot to the version in this PR juju refresh istio-pilot --channel latest/edge/pr-386 --revision <check-revision-in-CI>
  3. Ensure no errors are logged

Ensure the certificate has the correct domain name

  1. Deploy juju deploy self-signed-certificates
  2. Relate juju relate self-signed-certificates istio-pilot
  3. Change CSR domain name juju config istio-pilot csr-domain-name="istio-test.com"
  4. Check the certificate subject, it should be the same as the csr-domain-name that was set in (3)
# Get the issues certificate, look for it in the unit
$ juju show-unit istio-pilot/0
...
  - relation-id: 0
    endpoint: peers
    related-endpoint: peers
    application-data: {}
    local-unit:
      in-scope: true
      data:
        certificate: |-
          -----BEGIN CERTIFICATE-----
         ...
          -----END CERTIFICATE-----

# Check the subject name
$ openssl x509 -noout -subject -in ca.cert <--- you can save the cert in a file and pass it here
subject=CN = istio-test.com, x500UniqueIdentifier = abfd61dc-629d-4eaa-9b19-9613908e0313

@DnPlas DnPlas added the backport Backport a change from one track to another. label Mar 1, 2024
@DnPlas DnPlas requested a review from a team as a code owner March 1, 2024 09:36
@DnPlas DnPlas force-pushed the dnplas-cherry-pick-6ad839d branch from 3a30030 to 876b27b Compare March 1, 2024 12:01
@DnPlas
Copy link
Contributor Author

DnPlas commented Mar 1, 2024

It seems like the CI issue is related to the charmcraft version and the charmcraft.yaml we have in place. Please refer to #387 for more details. That PR should be merged for the CI in this one to pass.

The CI issues are caused by:

  1. The kubeflow-volumes pointing at latest/edge which now need to be trusted. Please refer to tests: pin kubeflow-volumes to its 1.8/stable channel #388 for details and a fix.
  2. charmcraft latest/edge introducing a check for the length of the summary charmcraft pack fails with a false positive on the summary check charmcraft#1568. Please refer to fix: shorten the summary as a workaround for canonical/charmcraft#1568 #389 for details and a fix.

Both PRs should be merged before the CI in this one passes.

feat: enable csr-domain-name config option so istio-pilot can use it on CSRs

The istio-pilot charm already has a mechanism in place to discover the ingress gateway address from the `Service`, but it is limited to only returning IP addresses, which not all TLS certificate providers accept as a valid cert subject. Having the domain-name config option will allow users to specify the domain name they'd like to use when integrating with TLS certificate operators. This feature expands the support for integrating with TLS certificate providers that cannot issue signed certificates on a CSR that only contains an IP address (like we used to do).
This commit also adds some test coverage to test the recently added code.

Fixes #379
@DnPlas DnPlas force-pushed the dnplas-cherry-pick-6ad839d branch from 1cc0a99 to 0ce0a6d Compare March 4, 2024 10:40
Copy link
Contributor

@NohaIhab NohaIhab left a comment

Choose a reason for hiding this comment

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

followed testing instructions successfully,
LGTM!

@DnPlas DnPlas merged commit b5353cc into track/1.17 Mar 4, 2024
17 checks passed
@DnPlas DnPlas deleted the dnplas-cherry-pick-6ad839d branch March 4, 2024 12:02
@Barteus
Copy link
Contributor

Barteus commented Mar 21, 2024

I deployed the charms with CKF 1.8, as described above and got this error in oidc-gatekeeper:

2024-03-21T15:59:37.856Z [oidc-authservice] time="2024-03-21T15:59:37Z" level=error msg="OIDC provider setup failed, retrying in 10 seconds: Get \"https://10.64.140.43.nip.io//dex/.well-known/openid-configuration\": tls: failed to verify certificate: x509: certificate is valid for istio-pilot-0.istio-pilot-endpoints.kubeflow.svc.cluster.local, not 10.64.140.43.nip.io"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Backport a change from one track to another. Libraries: Out of sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants