-
Notifications
You must be signed in to change notification settings - Fork 86
test: Addition of ginkgo framework dependencies and TLS propagation test #409
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: master
Are you sure you want to change the base?
test: Addition of ginkgo framework dependencies and TLS propagation test #409
Conversation
48a0740 to
5501ab7
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kaleemsiddiqu The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Local run is successful ... |
|
@ricardomaraschini @ingvagabund @gangwgr please review this. |
cmd/cluster-openshift-controller-manager-operator-tests-ext/main.go
Outdated
Show resolved
Hide resolved
cmd/cluster-openshift-controller-manager-operator-tests-ext/main.go
Outdated
Show resolved
Hide resolved
cmd/cluster-openshift-controller-manager-operator-tests-ext/main.go
Outdated
Show resolved
Hide resolved
cmd/cluster-openshift-controller-manager-operator-tests-ext/main.go
Outdated
Show resolved
Hide resolved
|
make 2 commits, vendor changes should be in different commit |
Add vendor dependencies for the Ginkgo testing framework, openshift-tests-extension framework, and testify assertion library Signed-off-by: Kaleemullah Siddiqui <ksiddiqu@redhat.com>
5501ab7 to
8ffbd4c
Compare
Implement end-to-end test to verify that TLS security profile changes propagate from the APIServer to the OpenShift Controller Manager. Signed-off-by: Kaleemullah Siddiqui <ksiddiqu@redhat.com>
8ffbd4c to
dd531db
Compare
| @@ -0,0 +1,8 @@ | |||
| // This file imports test packages to ensure they are included in the build. | |||
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.
file name not correct
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 should be filename here? dependencymagnet.go ?
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.
yes
|
|
||
| // Now verify the TLS config was propagated to the observed config | ||
| t.Log("Verifying TLS config in observed config") | ||
| err = wait.PollUntilContextTimeout(ctx, 5*time.Second, 2*time.Minute, true, func(ctx context.Context) (bool, 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.
better to create func for verification of tls and move to library-go repo, so it can be use other components
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.
so it can reuse in both restoration and observation for verification
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 is specific to controller-manager operator with couple of checks for TLS 1.3 version only.
I do not know what could be format of a generic TLS config check as this may involve a lot input and combination of checks depending upon the use case for TLS config check.
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.
are you planning similar case in other namespaces? if not then we can keep your change
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.
If you are planning for other namespaces better to keep common func in which we only pass tls values and compare. it will reduce duplicate work for other namespaces
|
|
||
| // Wait for the operator to finish progressing (reconciliation complete) | ||
| // This typically takes 12-15 minutes for TLS changes to propagate | ||
| t.Log("Waiting for operator to complete reconciliation (may take up to 15 minutes)") |
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.
same for this better to create func and move to library-go repo, so it can be use other components
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 have created this openshift/library-go#2050
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 is expected here? should i used changes done by you in library-go ? I see that change still in review.
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 can use that function here because it will reduce code size
| } | ||
|
|
||
| // Modern profile should have exactly these TLS 1.3 cipher suites | ||
| expectedCiphers := []string{ |
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.
move this in constant out side of case
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 will do the needful after our above discussion resolution.
|
@kaleemsiddiqu: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
Test for changes done in #407