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: adding openshift routes #1890

Closed
wants to merge 6 commits into from
Closed

feat: adding openshift routes #1890

wants to merge 6 commits into from

Conversation

hamza-m-masood
Copy link
Contributor

Which problem does the PR fix?

What's in this PR?

Rleated: https://github.com/camunda/distribution/issues/254

Checklist

Please make sure to follow our Contributing Guide.

Before opening the PR:

  • In the repo's root dir, run make go.update-golden-only.
  • There is no other open pull request for the same update/change.
  • Tests for charts are added (if needed).
  • In-repo documentation are updated (if needed).

After opening the PR:

  • Did you sign our CLA (Contributor License Agreement)? It will show once you open the PR.
  • Did all checks/tests pass in the PR?

@hamza-m-masood hamza-m-masood self-assigned this Jun 1, 2024
@hamza-m-masood hamza-m-masood added platform/openshift Issues related to OpenShift kind/enhancement New feature or request kind/task labels Jun 1, 2024
@hamza-m-masood
Copy link
Contributor Author

hamza-m-masood commented Jun 1, 2024

To start out, I will add a global flag to enable routes for each components.
The first route object I will create will be for identity.

@hamza-m-masood
Copy link
Contributor Author

Not sure if ingress.kubernetes.io/rewrite-target: / is needed as an annotation.

@hamza-m-masood
Copy link
Contributor Author

I am now able to get identity working with it's route object and no tls

@hamza-m-masood
Copy link
Contributor Author

hamza-m-masood commented Jun 1, 2024

@aabouzaid and @camunda/distribution
How should I implement the tls secret functionality?
openshift routes defines the tls certificate and key inside the route itself and does not reference any secret.
Knowing this, how should I structure the values.yaml?

For example, picking the identity component, the easiest way would be as follows:

identity:
  routes:
    tls:
      enabled: false
      insecureEdgeTerminationPolicy: Redirect
      termination: edge
      certificate: ""
      key: ""

Give the customer the option to specify the raw certificate and key in the values.yaml.

Another option (but a little tricky to implement) would be to reference an already existing k8s secret in the values.yaml:

identity:
  routes:
    tls:
      enabled: false
      insecureEdgeTerminationPolicy: Redirect
      termination: edge
      existingSecret: testSecret

The route object template would have to extract the information from the existing k8s secret. I am not completely sure how this would be done but I can work my way through this.

The final option I see is to not give the option to specify a tls cert and key in the values.yaml. If the customer really want a tls in their routes then we can tell them to generate the routes through the ingress because the ingress generated routes automatically includes the tls contents from the secret that the ingress references.

The customer could also manually add in the tls contents in each route themselves.

@leiicamundi
Copy link
Contributor

Hello @hamza-m-masood,

Regarding the Routes, I'd suggest looking at the way they are handled in this example: Confluence Route Template.

@hamza-m-masood
Copy link
Contributor Author

Thank you @leiicamundi that really helped. So I will go with the option of the customer defining the existingSecret key in the values.yaml.

@hamza-m-masood
Copy link
Contributor Author

hamza-m-masood commented Jun 3, 2024

I wanted to stay away from using the lookup function because of the dependency on the kubernetes API. This dependency can introduce potential points of failure, especially if the API server is experiencing issues or is not accessible. This would also requires appropriate permissions to access resources in the cluster. Maybe some potential or existing customers would not be able to use this feature.

Error handling with the lookup function can be challenging. If a resource is not found or an API request fails, it can be difficult to handle these errors gracefully within the Helm templates. This can lead to incomplete or failed deployments.

Also using helm commands like helm template won't give the correct output if the lookup function is used.

I will still implement it since you guys approve of it, and I will test it out myself to see how reliable it is.

@hamza-m-masood
Copy link
Contributor Author

@jessesimpson36's reponse to the above:
I'm personally fine with making users specify their certificate in values.yaml, since users have to do that anyway in the Route api. I dislike lookUp for the reasons you gave in github. But I think your judgement will be most valuable here.
If you wanted to implement your own lookUp, I can show you how I added a keyword into a custom build of helm. it could help you debug or further understand the existing implementation of lookUp.

@hamza-m-masood
Copy link
Contributor Author

I hope it is clear from my comment but I agree with Jesse. I think it's okay to specify the tls cert and key in the values.yaml.
I'm just a bit hesitant because I don't know what our customers would think about that.
Maybe we can have both options?

  1. Defining the tls crt and key in the values.yaml
  2. Defining an existingSecret

@jessesimpson36
Copy link
Contributor

jessesimpson36 commented Jun 4, 2024

I hope it is clear from my comment but I agree with Jesse. I think it's okay to specify the tls cert and key in the values.yaml. I'm just a bit hesitant because I don't know what our customers would think about that. Maybe we can have both options?

  1. Defining the tls crt and key in the values.yaml
  2. Defining an existingSecret

My response to a customer with these concerns is that they can make use of helm template to render a manifest of a valid openshift route, but disable then it in values.yaml.

helm template --set identity.route.enabled=true camunda camunda/camunda-platform --show-only templates/identity/route.yaml  > route.yaml

Then they can manually modify the route.yaml to remove the managed-by headers so that helm will not provide any updates to this file. apply it to the cluster. The benefit is that customers may see what annotations are necessary for having a working installation, so it takes them further than having to define their own Route from scratch.

There are other recommendations we could give too. Perhaps they can use a tool like SOPS or git-crypt to encrypt/decrypt the TLS certificate, and then use some CD platform to deploy pre-rendered manifests rather than using helm install.

A customer could also use a proxied load-balancing service, such as a cloudflare HTTPS proxy or AWS ALB.

@jessesimpson36
Copy link
Contributor

But if you can make existingSecret work, that'd be pretty sweet too!

@hamza-m-masood
Copy link
Contributor Author

hamza-m-masood commented Jun 4, 2024

@jessesimpson36
I personally am not very comfortable in telling the customer to manually modify their helm installation after successful deployment because of multiple reasons: configuration drift, loss of idempotency (meaning running a helm install on the same values.yaml is not going to give the same result, making future helm operations unpredictable), upgrade and rollback problems, inconsistencies across environments, problems with CI/CD (as you mentioned, maybe customers will have to modify their pipelines to add this extra manual step)

@leiicamundi
Copy link
Contributor

After re-reading the Red Hat charts (e.g. https://github.com/redhat-cop/helm-charts/blob/main/charts/jenkins/templates/route.yaml), it seems that the definition of certificates is always omitted.

Red Hat appears to be addressing this in the next version of OpenShift:
redhat-developer/gitops-operator#629 (comment)

ctrought commented May 17, 2024

OCP 4.16 is supposed to have a new field added to Route API "externalCertificate". I don't know the exact design details, but I assume the intention is to allow referencing a secret natively. Again assuming this is implemented, ArgoCD should be able to easily expose the field in the spec.server.route.tls.externalCertificate,

https://docs.openshift.com/container-platform/4.16/rest_api/network_apis/route-route-openshift-io-v1.html#spec-tls-externalcertificate

For now we've been overcoming this by leveraging either cert-manager combined with cert-manager routes and configuring the cert via annotations.
https://github.com/cert-manager/openshift-routes

The other option, and probably more stable would be simply using the ingress in ArgoCD CR as a means for configuring the route which does let you specify the TLS secret and OCP will generate the route + embed the certificate automatically.

Alternatively, we could use Ingress objects and let OpenShift generate the associated routes. This workaround is suggested in openshift/origin#2162 (comment).

The only downside is that it is not possible to define the outgoing CA in the Ingress object, but this seems to be a specific case.

@hamza-m-masood
Copy link
Contributor Author

Thanks @leiicamundi that really helps.

@aabouzaid aabouzaid force-pushed the main branch 6 times, most recently from 283abfc to f0da3dc Compare June 6, 2024 13:47
@hamza-m-masood
Copy link
Contributor Author

Closing this PR since we agreed to generate the routes from ingress objects.

@hamza-m-masood hamza-m-masood deleted the routes branch July 3, 2024 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request kind/task platform/openshift Issues related to OpenShift
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants