-
Notifications
You must be signed in to change notification settings - Fork 4.7k
TLS 1.3 / Modern profile tests #29611
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
base: main
Are you sure you want to change the base?
Conversation
…sue. Add TLS modern profile test to APIServer. Add (WIP) TLS 1.3 test to etcd.
… test as it is just one additional check with otherwise entirely common test needs.
Supports OCPBUGS-37706, OCPSTRAT-1364 |
Job Failure Risk Analysis for sha: 6c00d75
|
/lgtm |
/retest-required |
Job Failure Risk Analysis for sha: ff62120
|
which jobs run the new test ? i have checked |
…s causing TLS 1.2 connections to be rejected.
Job Failure Risk Analysis for sha: b6d72fd
|
@jacobsee I think you need to run |
@p0lyn0mial Thanks! I hadn't seen that. It's in there now, and I've realized that it should probably be marked [slow] as well. To your earlier comment, do we now still need to pick jobs to run this? This is my first brush with the origin tests, so I'm not sure what all is needed to get a new one plugged in correctly. |
Job Failure Risk Analysis for sha: 39e5f22
|
it looks like you could use
xref: https://docs.ci.openshift.org/docs/release-oversight/pull-request-testing/#payload-job we just need to find a periodic job that will run your test. maybe some serial test from ? so, overall:
|
@p0lyn0mial: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/745c1ee0-0985-11f0-8a93-335e1efa7bdb-0 |
profile to be TLS 1.2 or 1.3.
Job Failure Risk Analysis for sha: f132b67
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: f132b67
New tests seen in this PR at sha: f132b67
|
skip the test on IPv6 or DualStack clusters instead.
/retest-required |
Job Failure Risk Analysis for sha: 28f893b
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 28f893b
New tests seen in this PR at sha: 28f893b
|
Job Failure Risk Analysis for sha: 28f893b
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 28f893b
New tests seen in this PR at sha: 28f893b
|
/lgtm |
going through the route. Add retry with backoff to port-forward. Fix up IP family detection.
return &config | ||
} | ||
|
||
func getIPFamilyForCluster(client exutil.CLI, namespace string) IPFamily { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the code related to getting the current IP family exactly the same as the one used in the existing network tests?
If so, maybe we could move it to a common place and reuse it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping ^ (we could open a new PR to refactor)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's mostly the same, but it adds a security context to let the check pod run in restricted contexts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That and the port forwarder are probably both reusable. Where should they live?
Job Failure Risk Analysis for sha: caeab10
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: caeab10
|
Job Failure Risk Analysis for sha: caeab10
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: caeab10
|
Job Failure Risk Analysis for sha: 59f3798
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 59f3798
|
Job Failure Risk Analysis for sha: 59f3798
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 59f3798
|
test/extended/apiserver/tls.go
Outdated
g.Skip("tls configuration is only tested on IPv4 clusters, skipping") | ||
} | ||
|
||
insecure := true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused
test/extended/apiserver/tls.go
Outdated
} | ||
|
||
insecure := true | ||
configFlags := &genericclioptions.ConfigFlags{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused, please remove.
test/extended/apiserver/tls.go
Outdated
}) | ||
}) | ||
|
||
func ForwardPortsAndExecute(serviceName string, namespace string, ports []string, maxConnectRetries int, initialBackoff time.Duration, toExecute func(int)) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this function could be simplified a bit:
func forwardPortAndExecute(serviceName string, namespace string, remotePort string, toExecute func(localPort int)) error {
var err error
for i := 0; i < 3; i++ {
if err = func() error {
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
localPort := rand.Intn(65534-1025) + 1025
args := []string{
"port-forward",
fmt.Sprintf("svc/%s", serviceName),
fmt.Sprintf("%d:%s", localPort, remotePort),
"-n", namespace,
}
cmd := exec.CommandContext(ctx, "oc", args...)
stdout, stderr, err := e2e.StartCmdAndStreamOutput(cmd)
if err != nil {
return err
}
defer stdout.Close()
defer stderr.Close()
defer e2e.TryKill(cmd)
e2e.Logf("oc port-forward output: %s", readPartialFrom(stdout, 1024))
toExecute(localPort)
return nil
}(); err != nil {
err = fmt.Errorf("failed to start oc port-forward command: %w", err)
e2e.Logf(err.Error())
time.Sleep(2 * time.Second)
}
}
return err
}
func readPartialFrom(r io.Reader, maxBytes int) string {
buf := make([]byte, maxBytes)
n, err := r.Read(buf)
if err != nil && err != io.EOF {
return fmt.Sprintf("error reading: %v", err)
}
return string(buf[:n])
}
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added, but modified to let the check return early on success instead of needing to do the port forward & function call 3 times on every invocation
test/extended/apiserver/tls.go
Outdated
200*time.Millisecond, | ||
func(port int) { | ||
conn, err := tls.Dial("tcp", fmt.Sprintf("localhost:%d", port), tlsShouldWork) | ||
o.Expect(err).NotTo(o.HaveOccurred()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could change the signature of the toExecute
function to return an err so that we could retry on errors.
test/extended/apiserver/tls.go
Outdated
err = ForwardPortsAndExecute( | ||
"apiserver", | ||
"openshift-kube-apiserver", | ||
[]string{"443"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why [] ?
test/extended/apiserver/tls.go
Outdated
} | ||
} | ||
|
||
////// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove
test/extended/apiserver/tls.go
Outdated
[]string{"443"}, | ||
3, | ||
200*time.Millisecond, | ||
func(port int) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
most of the time this function is repeated, extract to a common function and reuse.
}) | ||
return podIPs, err | ||
} | ||
|
||
var _ = g.Describe("[sig-api-machinery][Feature:APIServer]", func() { | ||
defer g.GinkgoRecover() | ||
|
||
oc := exutil.NewCLI("apiserver") | ||
|
||
g.It("TestTLSDefaults", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does this test differs from the one that has been added ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The older test goes deeper and tests acceptable cipher suites, but it only checks the API server, and only when it's running under the default configuration.
test/extended/apiserver/tls.go
Outdated
conn.Close() | ||
|
||
_, err = tls.Dial("tcp", fmt.Sprintf("localhost:%d", port), tlsShouldNotWork) | ||
o.Expect(err).To(o.HaveOccurred()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what type/kind of err do we expect here? how are we going to distinguish it from a network err ?
…ortAndExecute a single remote port, make the test callback return an error to trigger retry logic on failure, refactor repeated TLS connection logic into CheckTLSConnection, which now checks that the failure is due to TLS
@jacobsee: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Job Failure Risk Analysis for sha: 64f5a15
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 64f5a15
|
1 similar comment
Job Failure Risk Analysis for sha: 64f5a15
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 64f5a15
|
Job Failure Risk Analysis for sha: 64f5a15
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 64f5a15
|
/lgtm |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dusk125, jacobsee The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR:
cc: @dusk125