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.

Sign up for GitHub

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

Jump to bottom

trait: Add tests for route trait and read certificate from secrets #2577

Merged
merged 1 commit into from
Sep 9, 2021
Merged

trait: Add tests for route trait and read certificate from secrets #2577

merged 1 commit into from
Sep 9, 2021

Conversation

claudio4j
Copy link
Contributor

@claudio4j claudio4j commented Aug 18, 2021 •
edited
Loading

  • Add parameters to read TLS certificates from secrets
  • Add unit tests for route TLS configuration
  • Add e2e tests for route TLS configuration
  • Add example in route trait documentation

Fix #2574

Release Note

trait/route: Add support to read certificates from secrets

@claudio4j claudio4j mentioned this pull request Aug 18, 2021
Closed
squakez
squakez requested changes Aug 18, 2021
View reviewed changes
Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

I think we can take the opportunity and make this more general than just secret. We should use the path concept (ie TLSCertificatePath) that will read a generic path on the runtime integration. That path will be filled by a secret/configmap/file/volume ... by the user via --resource option.

astefanutti
astefanutti reviewed Aug 18, 2021
View reviewed changes
Copy link
Member

@astefanutti astefanutti left a comment

Choose a reason for hiding this comment

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

We need to complete the e2e tests with re-encrypt and pass-through cases. That would help making the configuration coherent.

For the edge case, the configuration is independent from the Camel K runtime, but for the other cases, it may be useful to have a consistent way to inject the certificates into the runtime.

Last but not least, it'd be great that the re-encrypt case covers the use of a certificate provided by service signing certificate secrets, see https://docs.openshift.com/container-platform/4.8/security/certificates/service-serving-certificate.html#add-service-certificate_service-serving-certificate.

e2e/common/traits/route_test.go Outdated Show resolved Hide resolved
e2e/common/traits/route_test.go Outdated Show resolved Hide resolved
squakez
squakez reviewed Aug 26, 2021
View reviewed changes
e2e/common/traits/route_test.go Outdated Show resolved Hide resolved
pkg/trait/route.go Show resolved Hide resolved
@claudio4j claudio4j force-pushed the fix_route_trait_tls branch from 9609f3d to 56e8753 Compare August 30, 2021 15:53
@claudio4j
Copy link
Contributor Author

@astefanutti more e2e tests were added, to support 5 cases: no tls, edge, reencrypt, reencrypt with service signing and passthrough
Also, added more examples in docs on how to run integrations with the different route options.

@claudio4j
Copy link
Contributor Author

I think we can take the opportunity and make this more general than just secret. We should use the path concept (ie TLSCertificatePath) that will read a generic path on the runtime integration. That path will be filled by a secret/configmap/file/volume ... by the user via --resource option.

It may not help in this case, because by using --resource the certificate files are mounted in the integration pod, but the openshift route for the edge case is created from the operator pod, so the certificate file just doesn't exist in operator pod file system.

This was referenced Aug 30, 2021
Closed
Closed
squakez
squakez requested changes Aug 31, 2021
View reviewed changes
Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

I think there are still room for certain improvements. It would benefit the e2e test performance and maintainability of code.

e2e/common/traits/route_test.go Show resolved Hide resolved
e2e/common/traits/route_test.go Outdated Show resolved Hide resolved
e2e/common/traits/route_test.go Outdated Show resolved Hide resolved
e2e/common/traits/route_test.go Outdated
return &svc, err
}

func addAnnotation(ns string, name string, annotations map[string]string) error {
Copy link
Contributor

@squakez squakez Aug 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Same comment as for getService method

pkg/trait/route.go Show resolved Hide resolved
@claudio4j claudio4j force-pushed the fix_route_trait_tls branch 5 times, most recently from f37842c to cd7671f Compare August 31, 2021 21:56
@claudio4j
Copy link
Contributor Author

There are issues running the route tests, related to openshift 3 test cluster (CI) complaining about different base domain names. I am investigating this issue.

@claudio4j
Copy link
Contributor Author

I changed the e2e test to skip TLS certificate validation for client http request when it is unable to determine the dns base domain name (from dns/cluster object). So the route tests are green, but other unrelated tests are failing due to timeouts.

@claudio4j
Copy link
Contributor Author

retest this please

@claudio4j
Copy link
Contributor Author

There is a single test "Route reencrypt (with openshift service CA) https works" failing because the route host is not ready yet, that may occur occasionally, we may increase the timeout before the route object is ready for use, but it is my view the tests are in good shape.

nicolaferraro added a commit to nicolaferraro/camel-k that referenced this pull request Sep 3, 2021
@nicolaferraro
Attempt to reduce flakyness in apache#2577
9fc22eb
nicolaferraro added a commit to nicolaferraro/camel-k that referenced this pull request Sep 3, 2021
@nicolaferraro
Attempt to reduce flakyness in apache#2577
edeb685
astefanutti
astefanutti requested changes Sep 3, 2021
View reviewed changes
Copy link
Member

@astefanutti astefanutti left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure I understand why it's necessary for users to deal with low level resource mounting and Quarkus properties. Could the user provide only the secret name and the termination type, then the trait would take care of mounting the secret and set the Quarkus properties accordingly?

astefanutti
astefanutti reviewed Sep 3, 2021
View reviewed changes
deploy/traits.yaml Outdated Show resolved Hide resolved
astefanutti
astefanutti reviewed Sep 3, 2021
View reviewed changes
deploy/traits.yaml Outdated Show resolved Hide resolved
astefanutti
astefanutti reviewed Sep 3, 2021
View reviewed changes
deploy/traits.yaml Outdated Show resolved Hide resolved
astefanutti
astefanutti reviewed Sep 3, 2021
View reviewed changes
deploy/traits.yaml Outdated Show resolved Hide resolved
@claudio4j
Copy link
Contributor Author

I'm not sure I understand why it's necessary for users to deal with low level resource mounting and Quarkus properties. Could the user provide only the secret name and the termination type, then the trait would take care of mounting the secret and set the Quarkus properties accordingly?

It just happen that using quarkus http requires these quarkus properties for SSL setup, whatever http client the user may choose it may require a different set of parameters to setup it. For example, Netty SSL requires mounting -resource file:KeyStore.jks@/etc/ssl/keystore.jks to setup sslContextParameters object. With that said, it would be difficult to look at every SSL setup to automate in route trait.

@astefanutti
Copy link
Member

I'm not sure I understand why it's necessary for users to deal with low level resource mounting and Quarkus properties. Could the user provide only the secret name and the termination type, then the trait would take care of mounting the secret and set the Quarkus properties accordingly?

It just happen that using quarkus http requires these quarkus properties for SSL setup, whatever http client the user may choose it may require a different set of parameters to setup it. For example, Netty SSL requires mounting -resource file:KeyStore.jks@/etc/ssl/keystore.jks to setup sslContextParameters object. With that said, it would be difficult to look at every SSL setup to automate in route trait.

it's not clear to me why the quarkus.http.ssl.certificate properties could not be set automatically.

@claudio4j claudio4j force-pushed the fix_route_trait_tls branch 2 times, most recently from bc168b6 to 1be24ad Compare September 3, 2021 21:57
@claudio4j
Copy link
Contributor Author

For testing I am using Openshift on IBM cloud and I noticed the operator-ingress uses a letsencrypt certificate CA to manage the serving service CA certificates, which are different than the serving certificates provided by a standard openshift 4 installation. They use different secrets to store the CA and certificate.s That said, I could not yet find a standard way to retrieve the certificate in either cloud services, to read them and set in the http client of the e2e tests. So, I disabled the https certificate verification on client side of the e2e route tests. So I ask to accept this tests as it is, and I can work on this specific certificate client side validation later on.

@claudio4j claudio4j mentioned this pull request Sep 6, 2021
Open
@claudio4j claudio4j force-pushed the fix_route_trait_tls branch 2 times, most recently from bb9347c to a0258d4 Compare September 6, 2021 21:24
astefanutti
astefanutti approved these changes Sep 9, 2021
View reviewed changes
Copy link
Member

@astefanutti astefanutti left a comment

Choose a reason for hiding this comment

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

Great work!

@astefanutti
Copy link
Member

@claudio4j could you please fix the conflicts in route_test.go and regenerate resources.go? Sorry about that.

@claudio4j
trait: Add tests for route trait and read certificate from secrets
0b239e3 
* Add parameters to read TLS certificates from secrets
* Add unit tests for route TLS configuration
* Add e2e tests for route TLS configuration
* Add example in route trait documentation
@claudio4j
Copy link
Contributor Author

@astefanutti rebase done. All checks are green.

@astefanutti astefanutti merged commit 7f60a70 into apache:main Sep 9, 2021
@astefanutti
Copy link
Member

@claudio4j thanks!

@claudio4j claudio4j deleted the fix_route_trait_tls branch September 10, 2021 17:27
@nicolaferraro nicolaferraro mentioned this pull request Nov 10, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@squakez squakez squakez requested changes

@oscerd oscerd oscerd approved these changes

@astefanutti astefanutti astefanutti approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Read certificates from secrets to setup TLS config for route trait
4 participants
@claudio4j @astefanutti @oscerd @squakez