-
Notifications
You must be signed in to change notification settings - Fork 163
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
OCPBUGS-43777: certrotationcontroller: run tests which runs deployment and creates projects #1759
base: master
Are you sure you want to change the base?
Conversation
@vrutkovs: This pull request references Jira Issue OCPBUGS-43777, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vrutkovs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
896b6ba
to
1eb6081
Compare
/retest |
@@ -324,7 +324,7 @@ func newCertRotationController( | |||
Name: "service-network-serving-certkey", | |||
AdditionalAnnotations: certrotation.AdditionalAnnotations{ | |||
JiraComponent: "kube-apiserver", | |||
AutoRegenerateAfterOfflineExpiry: "https://github.com/openshift/cluster-kube-apiserver-operator/pull/1631,'operator conditions kube-apiserver'", | |||
AutoRegenerateAfterOfflineExpiry: "https://github.com/openshift/cluster-kube-apiserver-operator/pull/1631,'[sig-apps] Deployment RollingUpdateDeployment should delete old pods and create new ones [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]'", |
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 don't understand how this one tests the service network.
- test to external LB to create deployment
- kcm uses internal LB to create replicaset and pod
- scheduler uses internal LB to scheduler pod
- kubelet uses internal LB to get pods and report available status
- test uses external LB to confirm available replicas -- and FYI, this is the bit that requires the scheduler. interestingly, the deployment logic itself does not.
which hop uses the service network?
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.
@vrutkovs, if you could provide an explanation, that would be very helpful.
I also think that adding a comment with an explanation for each certificate/test pair would be greatly beneficial for future us and for easier reviewing.
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.
Ah, sorry, previously this was using
[sig-apps] Deployment RollingUpdateDeployment should delete old pods and create new ones [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]
but deployment rollout doesn't use service network, instead its
[sig-network] Services should serve a basic endpoint from pods [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]
now.
I'm not sure its worth commenting tests (we probably should have an easy to understand alias), but there will be an update for certificate descriptions: #1763 - would it be sufficient? Do you want me to add comments on what these tests do in the code (and not touch the annotations)?
@@ -646,7 +646,7 @@ func newCertRotationController( | |||
Name: "control-plane-node-admin-client-cert-key", | |||
AdditionalAnnotations: certrotation.AdditionalAnnotations{ | |||
JiraComponent: "kube-apiserver", | |||
AutoRegenerateAfterOfflineExpiry: "https://github.com/openshift/cluster-kube-apiserver-operator/pull/1631,'operator conditions kube-apiserver'", | |||
AutoRegenerateAfterOfflineExpiry: "https://github.com/openshift/cluster-kube-apiserver-operator/pull/1631,'[sig-apps] Deployment RollingUpdateDeployment should delete old pods and create new ones [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]'", |
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 don't think this test uses the kubeconfig that this certificate is embedded into. this is for being able to use oc
if you ssh to the node.
@@ -698,7 +698,7 @@ func newCertRotationController( | |||
Name: "check-endpoints-client-cert-key", | |||
AdditionalAnnotations: certrotation.AdditionalAnnotations{ | |||
JiraComponent: "kube-apiserver", | |||
AutoRegenerateAfterOfflineExpiry: "https://github.com/openshift/cluster-kube-apiserver-operator/pull/1631,'operator conditions kube-apiserver'", | |||
AutoRegenerateAfterOfflineExpiry: "https://github.com/openshift/cluster-kube-apiserver-operator/pull/1631,'[sig-apps] Deployment RollingUpdateDeployment should delete old pods and create new ones [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]'", |
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 don't think that this test confirms that the check endpoints controller (which may or may not still exist) can function.
@@ -723,7 +723,7 @@ func newCertRotationController( | |||
Name: "node-system-admin-signer", | |||
AdditionalAnnotations: certrotation.AdditionalAnnotations{ | |||
JiraComponent: "kube-apiserver", | |||
AutoRegenerateAfterOfflineExpiry: "https://github.com/openshift/cluster-kube-apiserver-operator/pull/1631,'operator conditions kube-apiserver'", | |||
AutoRegenerateAfterOfflineExpiry: "https://github.com/openshift/cluster-kube-apiserver-operator/pull/1631,'[sig-apps] Deployment RollingUpdateDeployment should delete old pods and create new ones [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]'", |
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 as client-cert comment above.
1eb6081
to
12ec413
Compare
12ec413
to
d00bec9
Compare
I can tag this PR to get it merged, as it simply updates some descriptions, but I'm not sure if that is the intended goal. I think the goal is to ensure we actually have a list of tests that can validate whether certificates have been regenerated properly. To do that, I would have to carefully check each test listed here. If you want me to confirm the tests, I would suggest changing just one item. That would make it much easier for me to check and make some progress. |
The plan here is:
|
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.
okay, let's start with the aggregation layer.
Could you remind me of the purpose of the certificates and explain how the[sig-cli] oc adm new-project [apigroup:project.openshift.io][apigroup:authorization.openshift.io] [Suite:openshift/conformance/parallel]
test is going to validate each certificate?
@@ -135,7 +135,7 @@ func newCertRotationController( | |||
Name: "aggregator-client-signer", |
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 this the signer used to sign --proxy-client-cert
?
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
@@ -150,7 +150,7 @@ func newCertRotationController( | |||
Name: "kube-apiserver-aggregator-client-ca", |
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 this the CA that is added to --requestheader-client-ca-file
?
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 (that's the entire CA btw)
@@ -162,7 +162,7 @@ func newCertRotationController( | |||
Name: "aggregator-client", |
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 this --proxy-client-cert
?
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.
Correct
4051bd1
to
fdcdee7
Compare
fdcdee7
to
fd2867a
Compare
/retest |
Tests we run after cert rotation should ensure that pod gets created from deployment, scheduled on the node and openshift-apiserver can create projects to validate that all component certificates have been regenerated. The test names are included in certificates.openshift.io/auto-regenerate-after-offline-expiry annotation
fd2867a
to
8e7d25b
Compare
/retest |
@vrutkovs: 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. |
AutoRegenerateAfterOfflineExpiry: "https://github.com/openshift/cluster-kube-apiserver-operator/pull/1631,'operator conditions openshift-apiserver'", | ||
JiraComponent: "kube-apiserver", | ||
// This test would ensure kube-apiserver request would be passed to openshift-apiserver | ||
AutoRegenerateAfterOfflineExpiry: "https://github.com/openshift/cluster-kube-apiserver-operator/pull/1631,'[sig-cli] oc adm new-project [apigroup:project.openshift.io][apigroup:authorization.openshift.io] [Suite:openshift/conformance/parallel]'", |
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.
// AutoRegenerateAfterOfflineExpiry contains a link to PR and an e2e test name which verifies
// that TLS artifact is correctly regenerated after it has expired
Would the PR not be openshift/release#58130 where we add and rehearse these tests to the cert rotation jobs?
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.
According to https://github.com/openshift/origin/blob/master/pkg/cmd/update-tls-artifacts/generate-owners/tlsmetadata/autoregenerate_after_expiry/requirement.go#L23 it needs to be the PR which added the annotation, not necessarily the one which runs these tests
Tests we run after cert rotation should ensure that pod gets created from deployment, scheduled on the node and openshift-apiserver can create projects to validate that all component certificates have been regenerated. The test names are included in
certificates.openshift.io/auto-regenerate-after-offline-expiry annotation