From 8fac6cb9a400f2ef6da763b57710565a018a08d7 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Thu, 10 Mar 2022 15:26:22 -0500 Subject: [PATCH] Rework or remove tests that rely on the http port Signed-off-by: Monis Khan --- test/integration/supervisor_discovery_test.go | 154 +++++++++--------- test/integration/supervisor_healthz_test.go | 21 --- 2 files changed, 80 insertions(+), 95 deletions(-) diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index a8099296..4274d880 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -32,116 +32,122 @@ import ( "go.pinniped.dev/test/testlib" ) -// This test is intended to exercise the supervisor's HTTP port 8080. It can either access it directly via -// the env.SupervisorHTTPAddress setting, or it can access it indirectly through a TLS-enabled Ingress which -// uses the supervisor's port 8080 as its backend via the env.SupervisorHTTPSIngressAddress and -// env.SupervisorHTTPSIngressCABundle settings, or it can exercise it both ways when all of those -// env settings are present. -// -// Testing talking to the supervisor's port 8443 where the supervisor is terminating TLS itself is -// handled by the others tests in this file. +// TestSupervisorOIDCDiscovery_Disruptive is intended to exercise the supervisor's HTTPS port. +// It can either access it directly via the env.SupervisorHTTPSAddress setting, or it can access +// it indirectly through a TLS-enabled Ingress which uses the supervisor's HTTPS port as its backend +// (via the env.SupervisorHTTPSIngressAddress and env.SupervisorHTTPSIngressCABundle settings), +// or it can exercise it both ways when all of those env settings are present. // Never run this test in parallel since deleting all federation domains is disruptive, see main_test.go. func TestSupervisorOIDCDiscovery_Disruptive(t *testing.T) { env := testlib.IntegrationEnv(t) client := testlib.NewSupervisorClientset(t) + kubeClient := testlib.NewKubernetesClientset(t) ns := env.SupervisorNamespace ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) defer cancel() + httpsAddress := env.SupervisorHTTPSAddress + var ips []net.IP + if host, _, err := net.SplitHostPort(httpsAddress); err == nil { + httpsAddress = host + } + if ip := net.ParseIP(httpsAddress); ip != nil { + ips = append(ips, ip) + } + temporarilyRemoveAllFederationDomainsAndDefaultTLSCertSecret(ctx, t, ns, defaultTLSCertSecretName(env), client, testlib.NewKubernetesClientset(t)) + defaultCA := createTLSCertificateSecret(ctx, t, ns, httpsAddress, ips, defaultTLSCertSecretName(env), kubeClient) tests := []struct { + Name string Scheme string Address string CABundle string }{ - {Scheme: "http", Address: env.SupervisorHTTPAddress}, - {Scheme: "https", Address: env.SupervisorHTTPSIngressAddress, CABundle: env.SupervisorHTTPSIngressCABundle}, + {Name: "direct https", Scheme: "https", Address: env.SupervisorHTTPSAddress, CABundle: string(defaultCA.Bundle())}, + {Name: "ingress https", Scheme: "https", Address: env.SupervisorHTTPSIngressAddress, CABundle: env.SupervisorHTTPSIngressCABundle}, } for _, test := range tests { - scheme := test.Scheme - addr := test.Address - caBundle := test.CABundle + test := test + t.Run(test.Name, func(t *testing.T) { + scheme := test.Scheme + addr := test.Address + caBundle := test.CABundle - if addr == "" { - // Both cases are not required, so when one is empty skip it. - continue - } + if addr == "" { + // Both cases are not required, so when one is empty skip it. + t.Skip("no address defined") + } - // Test that there is no default discovery endpoint available when there are no FederationDomains. - requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, fmt.Sprintf("%s://%s", scheme, addr)) + // Test that there is no default discovery endpoint available when there are no FederationDomains. + requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, fmt.Sprintf("%s://%s", scheme, addr)) - // Define several unique issuer strings. Always use https in the issuer name even when we are accessing the http port. - issuer1 := fmt.Sprintf("https://%s/nested/issuer1", addr) - issuer2 := fmt.Sprintf("https://%s/nested/issuer2", addr) - issuer3 := fmt.Sprintf("https://%s/issuer3", addr) - issuer4 := fmt.Sprintf("https://%s/issuer4", addr) - issuer5 := fmt.Sprintf("https://%s/issuer5", addr) - issuer6 := fmt.Sprintf("https://%s/issuer6", addr) - badIssuer := fmt.Sprintf("https://%s/badIssuer?cannot-use=queries", addr) + // Define several unique issuer strings. Always use https in the issuer name even when we are accessing the http port. + issuer1 := fmt.Sprintf("https://%s/nested/issuer1", addr) + issuer2 := fmt.Sprintf("https://%s/nested/issuer2", addr) + issuer3 := fmt.Sprintf("https://%s/issuer3", addr) + issuer4 := fmt.Sprintf("https://%s/issuer4", addr) + issuer5 := fmt.Sprintf("https://%s/issuer5", addr) + issuer6 := fmt.Sprintf("https://%s/issuer6", addr) + badIssuer := fmt.Sprintf("https://%s/badIssuer?cannot-use=queries", addr) - // When FederationDomain are created in sequence they each cause a discovery endpoint to appear only for as long as the FederationDomain exists. - config1, jwks1 := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer1, client) - requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config1, client, ns, scheme, addr, caBundle, issuer1) - config2, jwks2 := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer2, client) - requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config2, client, ns, scheme, addr, caBundle, issuer2) - // The auto-created JWK's were different from each other. - require.NotEqual(t, jwks1.Keys[0]["x"], jwks2.Keys[0]["x"]) - require.NotEqual(t, jwks1.Keys[0]["y"], jwks2.Keys[0]["y"]) + // When FederationDomain are created in sequence they each cause a discovery endpoint to appear only for as long as the FederationDomain exists. + config1, jwks1 := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer1, client) + requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config1, client, ns, scheme, addr, caBundle, issuer1) + config2, jwks2 := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer2, client) + requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config2, client, ns, scheme, addr, caBundle, issuer2) + // The auto-created JWK's were different from each other. + require.NotEqual(t, jwks1.Keys[0]["x"], jwks2.Keys[0]["x"]) + require.NotEqual(t, jwks1.Keys[0]["y"], jwks2.Keys[0]["y"]) - // When multiple FederationDomains exist at the same time they each serve a unique discovery endpoint. - config3, jwks3 := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer3, client) - config4, jwks4 := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer4, client) - requireDiscoveryEndpointsAreWorking(t, scheme, addr, caBundle, issuer3, nil) // discovery for issuer3 is still working after issuer4 started working - // The auto-created JWK's were different from each other. - require.NotEqual(t, jwks3.Keys[0]["x"], jwks4.Keys[0]["x"]) - require.NotEqual(t, jwks3.Keys[0]["y"], jwks4.Keys[0]["y"]) + // When multiple FederationDomains exist at the same time they each serve a unique discovery endpoint. + config3, jwks3 := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer3, client) + config4, jwks4 := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer4, client) + requireDiscoveryEndpointsAreWorking(t, scheme, addr, caBundle, issuer3, nil) // discovery for issuer3 is still working after issuer4 started working + // The auto-created JWK's were different from each other. + require.NotEqual(t, jwks3.Keys[0]["x"], jwks4.Keys[0]["x"]) + require.NotEqual(t, jwks3.Keys[0]["y"], jwks4.Keys[0]["y"]) - // Editing a provider to change the issuer name updates the endpoints that are being served. - updatedConfig4 := editFederationDomainIssuerName(t, config4, client, ns, issuer5) - requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, issuer4) - jwks5 := requireDiscoveryEndpointsAreWorking(t, scheme, addr, caBundle, issuer5, nil) - // The JWK did not change when the issuer name was updated. - require.Equal(t, jwks4.Keys[0], jwks5.Keys[0]) + // Editing a provider to change the issuer name updates the endpoints that are being served. + updatedConfig4 := editFederationDomainIssuerName(t, config4, client, ns, issuer5) + requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, issuer4) + jwks5 := requireDiscoveryEndpointsAreWorking(t, scheme, addr, caBundle, issuer5, nil) + // The JWK did not change when the issuer name was updated. + require.Equal(t, jwks4.Keys[0], jwks5.Keys[0]) - // When they are deleted they stop serving discovery endpoints. - requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config3, client, ns, scheme, addr, caBundle, issuer3) - requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, updatedConfig4, client, ns, scheme, addr, caBundle, issuer5) + // When they are deleted they stop serving discovery endpoints. + requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config3, client, ns, scheme, addr, caBundle, issuer3) + requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, updatedConfig4, client, ns, scheme, addr, caBundle, issuer5) - // When the same issuer is added twice, both issuers are marked as duplicates, and neither provider is serving. - config6Duplicate1, _ := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer6, client) - config6Duplicate2 := testlib.CreateTestFederationDomain(ctx, t, issuer6, "", "") - requireStatus(t, client, ns, config6Duplicate1.Name, v1alpha1.DuplicateFederationDomainStatusCondition) - requireStatus(t, client, ns, config6Duplicate2.Name, v1alpha1.DuplicateFederationDomainStatusCondition) - requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, issuer6) + // When the same issuer is added twice, both issuers are marked as duplicates, and neither provider is serving. + config6Duplicate1, _ := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer6, client) + config6Duplicate2 := testlib.CreateTestFederationDomain(ctx, t, issuer6, "", "") + requireStatus(t, client, ns, config6Duplicate1.Name, v1alpha1.DuplicateFederationDomainStatusCondition) + requireStatus(t, client, ns, config6Duplicate2.Name, v1alpha1.DuplicateFederationDomainStatusCondition) + requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, issuer6) - // If we delete the first duplicate issuer, the second duplicate issuer starts serving. - requireDelete(t, client, ns, config6Duplicate1.Name) - requireWellKnownEndpointIsWorking(t, scheme, addr, caBundle, issuer6, nil) - requireStatus(t, client, ns, config6Duplicate2.Name, v1alpha1.SuccessFederationDomainStatusCondition) + // If we delete the first duplicate issuer, the second duplicate issuer starts serving. + requireDelete(t, client, ns, config6Duplicate1.Name) + requireWellKnownEndpointIsWorking(t, scheme, addr, caBundle, issuer6, nil) + requireStatus(t, client, ns, config6Duplicate2.Name, v1alpha1.SuccessFederationDomainStatusCondition) - // When we finally delete all issuers, the endpoint should be down. - requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config6Duplicate2, client, ns, scheme, addr, caBundle, issuer6) + // When we finally delete all issuers, the endpoint should be down. + requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config6Duplicate2, client, ns, scheme, addr, caBundle, issuer6) - // Only test this for http endpoints because https endpoints are going through an Ingress, - // and while it is possible to configure an Ingress to serve multiple hostnames with matching TLS certs - // for each hostname, that it not something that we felt like doing on all of our clusters that we - // run tests against. :) - if scheme == "http" { // "Host" headers can be used to send requests to discovery endpoints when the public address is different from the issuer name. issuer7 := "https://some-issuer-host-and-port-that-doesnt-match-public-supervisor-address.com:2684/issuer7" config7, _ := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer7, client) requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config7, client, ns, scheme, addr, caBundle, issuer7) - } - // When we create a provider with an invalid issuer, the status is set to invalid. - badConfig := testlib.CreateTestFederationDomain(ctx, t, badIssuer, "", "") - requireStatus(t, client, ns, badConfig.Name, v1alpha1.InvalidFederationDomainStatusCondition) - requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, badIssuer) - requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, badConfig, client, ns, scheme, addr, caBundle, badIssuer) + // When we create a provider with an invalid issuer, the status is set to invalid. + badConfig := testlib.CreateTestFederationDomain(ctx, t, badIssuer, "", "") + requireStatus(t, client, ns, badConfig.Name, v1alpha1.InvalidFederationDomainStatusCondition) + requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, badIssuer) + requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, badConfig, client, ns, scheme, addr, caBundle, badIssuer) + }) } } diff --git a/test/integration/supervisor_healthz_test.go b/test/integration/supervisor_healthz_test.go index 637dd635..8e4519af 100644 --- a/test/integration/supervisor_healthz_test.go +++ b/test/integration/supervisor_healthz_test.go @@ -17,27 +17,6 @@ import ( "go.pinniped.dev/test/testlib" ) -// The Supervisor health endpoint is public because that makes it easier -// for users to create an Ingress for the supervisor on platforms like -// GKE where the Ingress wants to perform a health check. It's somewhere -// between inconvenient and impossible to make that Ingress health check -// happen on a private container port at this time. -// This test checks that it is working and that it is public. -func TestSupervisorHealthz(t *testing.T) { - env := testlib.IntegrationEnv(t) - - if env.SupervisorHTTPAddress == "" { - t.Skip("PINNIPED_TEST_SUPERVISOR_HTTP_ADDRESS not defined") - } - - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) - defer cancel() - - httpClient := &http.Client{} - - httpGet(ctx, t, httpClient, fmt.Sprintf("http://%s/healthz", env.SupervisorHTTPAddress), http.StatusOK, "ok") -} - // Never run this test in parallel since deleting all federation domains and the default TLS secret is disruptive, see main_test.go. func TestSupervisorHealthzBootstrap_Disruptive(t *testing.T) { env := testlib.IntegrationEnv(t)