From b0c36c663356b60e7dc89d6e5bf0fe910d80e6a8 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 15 Feb 2022 11:19:49 -0800 Subject: [PATCH 1/4] Fix int test that was failing on MacOS, and some small doc changes --- CONTRIBUTING.md | 14 +++++++++++--- GOVERNANCE.md | 4 ++-- hack/prepare-for-integration-tests.sh | 3 ++- test/integration/e2e_test.go | 12 +++++------- 4 files changed, 20 insertions(+), 13 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 39bd6e94..1e0d9aa7 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -72,7 +72,7 @@ Please follow the procedure described in [SECURITY.md](SECURITY.md). ## CLA -We welcome contributions from everyone but we can only accept them if you sign +We welcome contributions from everyone but, we can only accept them if you sign our Contributor License Agreement (CLA). If you would like to contribute and you have not signed it, our CLA-bot will walk you through the process when you open a Pull Request. For questions about the CLA process, see the @@ -82,13 +82,21 @@ tracker. ## Building The [Dockerfile](Dockerfile) at the root of the repo can be used to build and -package the code. After making a change to the code, rebuild the docker image with the following command. +package the server-side code. After making a change to the code, rebuild the +docker image with the following command. ```bash # From the root directory of the repo... docker build . ``` +The Pinniped CLI client can be built for local use with the following command. + +```bash +# From the root directory of the repo... +go build -o pinniped ./cmd/pinniped +``` + ## Testing ### Running Lint @@ -122,7 +130,7 @@ docker build . brew install kind k14s/tap/ytt k14s/tap/kapp kubectl chromedriver nmap && brew cask install docker ``` -1. Create a kind cluster, compile, create container images, and install Pinniped and supporting dependencies using: +1. Create a kind cluster, compile, create container images, and install Pinniped and supporting test dependencies using: ```bash ./hack/prepare-for-integration-tests.sh diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 679648f6..8de08d8c 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -31,7 +31,7 @@ A supermajority is defined as two-thirds of members in the group. A supermajorit Ideally, all project decisions are resolved by consensus. If impossible, any maintainer may call a vote. Unless otherwise specified in this document, any vote will be decided by a supermajority of maintainers. --- -# Proposal Process +# Proposal Process The proposal process is currently being worked on. No formal process is available at this time. You may reach out to the maintainers in the Kubernetes Slack Workspace within the [#pinniped](https://kubernetes.slack.com/archives/C01BW364RJA) channel or on the [Pinniped mailing list](project-pinniped@googlegroups.com) with any questions you may have or to send us your proposals. --- @@ -48,4 +48,4 @@ Lazy consensus does not apply to the process of: --- # Updating Governance -All substantive changes in Governance require a supermajority agreement by all maintainers. \ No newline at end of file +All substantive changes in Governance require a supermajority agreement by all maintainers. diff --git a/hack/prepare-for-integration-tests.sh b/hack/prepare-for-integration-tests.sh index 2b251899..6fb4e63c 100755 --- a/hack/prepare-for-integration-tests.sh +++ b/hack/prepare-for-integration-tests.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash -# Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +# Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 # @@ -420,6 +420,7 @@ EOF log_note log_note "🚀 Ready to run integration tests! For example..." log_note " cd $pinniped_path" +log_note " ulimit -n 512" log_note ' source /tmp/integration-test-env && go test -v -race -count 1 -timeout 0 ./test/integration' log_note log_note "Using GoLand? Paste the result of this command into GoLand's run configuration \"Environment\"." diff --git a/test/integration/e2e_test.go b/test/integration/e2e_test.go index 2af62cc2..88c189ba 100644 --- a/test/integration/e2e_test.go +++ b/test/integration/e2e_test.go @@ -449,8 +449,6 @@ func TestE2EFullIntegration(t *testing.T) { // nolint:gocyclo start := time.Now() kubectlCmd := exec.CommandContext(ctx, "kubectl", "get", "namespace", "--kubeconfig", kubeconfigPath) kubectlCmd.Env = append(os.Environ(), env.ProxyEnv()...) - stdoutPipe, err := kubectlCmd.StdoutPipe() - require.NoError(t, err) ptyFile, err := pty.Start(kubectlCmd) require.NoError(t, err) @@ -492,10 +490,10 @@ func TestE2EFullIntegration(t *testing.T) { // nolint:gocyclo t.Logf("waiting for kubectl to output namespace list") // Read all output from the subprocess until EOF. // Ignore any errors returned because there is always an error on linux. - kubectlStdOutOutputBytes, _ := ioutil.ReadAll(stdoutPipe) - kubectlStdErrOutputBytes, _ := ioutil.ReadAll(ptyFile) - requireKubectlGetNamespaceOutput(t, env, string(kubectlStdOutOutputBytes)) - require.Contains(t, string(kubectlStdErrOutputBytes), "Access token from identity provider has lifetime of less than 3 hours. Expect frequent prompts to log in.") + kubectlOutputBytes, _ := ioutil.ReadAll(ptyFile) + requireKubectlGetNamespaceOutput(t, env, string(kubectlOutputBytes)) + // This warning should be on stderr, but with pty on MacOS it's hard to assert that specifically. + require.Contains(t, string(kubectlOutputBytes), "Access token from identity provider has lifetime of less than 3 hours. Expect frequent prompts to log in.") t.Logf("first kubectl command took %s", time.Since(start).String()) @@ -1013,7 +1011,7 @@ func readFromFileUntilStringIsSeen(t *testing.T, f *os.File, until string) strin return true, nil // found it! finished. } if foundEOF { - return false, fmt.Errorf("reached EOF of subcommand's output without seeing expected string %q", until) + return false, fmt.Errorf("reached EOF of subcommand's output without seeing expected string %q. Output read so far was:\n%s", until, readFromFile) } return false, nil // keep waiting and reading }, 1*time.Minute, 1*time.Second) From 1aa17bd84de348539e633489389722f5f8d7d56d Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 15 Feb 2022 13:45:04 -0800 Subject: [PATCH 2/4] Check for darwin before relaxing stderr vs stdout assertion in e2e test --- test/integration/e2e_test.go | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/test/integration/e2e_test.go b/test/integration/e2e_test.go index 88c189ba..ccf8d114 100644 --- a/test/integration/e2e_test.go +++ b/test/integration/e2e_test.go @@ -17,6 +17,7 @@ import ( "os/exec" "path/filepath" "regexp" + "runtime" "sort" "strings" "sync/atomic" @@ -449,6 +450,13 @@ func TestE2EFullIntegration(t *testing.T) { // nolint:gocyclo start := time.Now() kubectlCmd := exec.CommandContext(ctx, "kubectl", "get", "namespace", "--kubeconfig", kubeconfigPath) kubectlCmd.Env = append(os.Environ(), env.ProxyEnv()...) + var kubectlStdoutPipe io.ReadCloser + if runtime.GOOS != "darwin" { + // For some unknown reason this breaks the pty library on some MacOS machines. + // The problem doesn't reproduce for everyone, so this is just a workaround. + kubectlStdoutPipe, err = kubectlCmd.StdoutPipe() + require.NoError(t, err) + } ptyFile, err := pty.Start(kubectlCmd) require.NoError(t, err) @@ -490,10 +498,19 @@ func TestE2EFullIntegration(t *testing.T) { // nolint:gocyclo t.Logf("waiting for kubectl to output namespace list") // Read all output from the subprocess until EOF. // Ignore any errors returned because there is always an error on linux. - kubectlOutputBytes, _ := ioutil.ReadAll(ptyFile) - requireKubectlGetNamespaceOutput(t, env, string(kubectlOutputBytes)) - // This warning should be on stderr, but with pty on MacOS it's hard to assert that specifically. - require.Contains(t, string(kubectlOutputBytes), "Access token from identity provider has lifetime of less than 3 hours. Expect frequent prompts to log in.") + kubectlPtyOutputBytes, _ := ioutil.ReadAll(ptyFile) + if kubectlStdoutPipe != nil { + // On non-MacOS check that stdout of the CLI contains the expected output. + kubectlStdOutOutputBytes, _ := ioutil.ReadAll(kubectlStdoutPipe) + requireKubectlGetNamespaceOutput(t, env, string(kubectlStdOutOutputBytes)) + } else { + // On MacOS check that the pty (stdout+stderr+stdin) of the CLI contains the expected output. + requireKubectlGetNamespaceOutput(t, env, string(kubectlPtyOutputBytes)) + } + // Due to the GOOS check in the code above, on MacOS the pty will include stdout, and other platforms it will not. + // This warning message is supposed to be printed by the CLI on stderr. + require.Contains(t, string(kubectlPtyOutputBytes), + "Access token from identity provider has lifetime of less than 3 hours. Expect frequent prompts to log in.") t.Logf("first kubectl command took %s", time.Since(start).String()) From 3f4e6cf3677dac08f234c104dd3d421a1962e7e1 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 15 Feb 2022 16:45:49 -0800 Subject: [PATCH 3/4] Fix a typo in CONTRIBUTING.md from a recent commit: comma in wrong place --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1e0d9aa7..ddef26d0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -72,7 +72,7 @@ Please follow the procedure described in [SECURITY.md](SECURITY.md). ## CLA -We welcome contributions from everyone but, we can only accept them if you sign +We welcome contributions from everyone, but we can only accept them if you sign our Contributor License Agreement (CLA). If you would like to contribute and you have not signed it, our CLA-bot will walk you through the process when you open a Pull Request. For questions about the CLA process, see the From e5a60a8c841e180b4d421f9ee50082ce0c048dae Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 16 Feb 2022 11:09:05 -0800 Subject: [PATCH 4/4] Update a comment --- internal/oidc/provider/formposthtml/form_post.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/oidc/provider/formposthtml/form_post.js b/internal/oidc/provider/formposthtml/form_post.js index 9e4b0203..cb73c8cd 100644 --- a/internal/oidc/provider/formposthtml/form_post.js +++ b/internal/oidc/provider/formposthtml/form_post.js @@ -53,8 +53,8 @@ window.onload = () => { // Requests made using "no-cors" mode will hide the real response.status by making it 0 // and the real response.ok by making it false. // If the real response was success, then we would like to show the success state. - // If the real response was an error, then we wish we could show the manual - // state, but we have no way to know that, as long as we are making "no-cors" requests. + // If the real response was an error, then we wish we could do something else (maybe show the error?), + // but we have no way to know the real response as long as we are making "no-cors" requests. // For now, show the success status for all responses. // In the future, we could make this request in "cors" mode once old versions of our CLI // which did not handle CORS are upgraded out by our users. That would allow us to use