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

Update matcher #431

Merged
merged 9 commits into from
Dec 19, 2023
Merged

Update matcher #431

merged 9 commits into from
Dec 19, 2023

Conversation

EvanHStanton
Copy link
Contributor

@EvanHStanton EvanHStanton commented Dec 18, 2023

Context:

  • While developing our IngressRouteTCP we have found a need to update the matcher to adjust for non-HTTP traffic management, in this case HostSNI (Server Name Indication). This is related to our work with FerretDB, but also adds flexibility and security to future projects.

Update IngressRouteTCP Matcher (ingress.rs)

  • Update fn generate_ingress_tcp_routes matcher to HostSNI

Update naming convention to be

  • Update fn reconcile_app_services to define app_name

Update assertion (integration_tests.rs)

@@ -269,7 +269,7 @@ pub fn generate_ingress_tcp_routes(
continue;
}

let matcher = format!("{host_matcher} && PathPrefix(`{}`)", path);
let matcher = format!("HostSNI(`{}`)", host_matcher);
Copy link
Member

Choose a reason for hiding this comment

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

You may need to update some assertions in the functional_test_app_service integration test

) -> Result<(), kube::Error> {
let ingress_api: Api<IngressRouteTCP> = Api::namespaced(client.clone(), ns);
let name = format!("{}-apps", coredb_name);
let name = format!("{}-{}", coredb_name, app_name);
Copy link
Member

Choose a reason for hiding this comment

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

👌

ns, coredb_name, e
);
has_errors = true;
for appsvc in appsvcs.iter() {
Copy link
Member

Choose a reason for hiding this comment

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

As a follow-up, let's make sure the expected IngressRouteTCP resources are created when we have multiple apps that use IngressType: tcp

@EvanHStanton EvanHStanton marked this pull request as ready for review December 19, 2023 16:42
@ianstanton ianstanton self-requested a review December 19, 2023 16:50
@EvanHStanton
Copy link
Contributor Author

Note: we observed the app-services-integration-test fail, but pass upon running again.

@EvanHStanton EvanHStanton merged commit 32c370f into main Dec 19, 2023
9 checks passed
@EvanHStanton EvanHStanton deleted the update-matcher branch December 19, 2023 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants