OCPBUGS-9037: Use cluster wildcard certificate for ingress canary #1155
Conversation
openshift-ci-robot
added
jira/severity-important
|
@rfredette: This pull request references Jira Issue OCPBUGS-9037, which is invalid:
Comment 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 NOT APPROVED This pull-request has been approved by: 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 |
rfredette
force-pushed
the
ocpbugs-9037-use-cluster-wildcard
branch
from
4f7f29d to
a7792fa
Compare
|
test failures appear unrelated. |
Miciah
added
priority/backlog
|
/assign @Miciah |
|
/retest-required |
|
@rfredette: The following test 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. |
There was a problem hiding this comment.
@rfredette I've had these comments pending for some time. Please let me know if I've misunderstood the assignment here, but in some places it looks like we are just reusing the default cert. Was that the plan?
| @@ -2,10 +2,8 @@ | |||
| # Specific values are applied at runtime | |||
| kind: Service | |||
| apiVersion: v1 | |||
| metadata: | |||
| metadata: {} | |||
| # name and namespace are set at runtime. | |||
There was a problem hiding this comment.
nit:
| # name and namespace are set at runtime. | |
| # metadata values are set at runtime. |
| secretName, err := r.canarySecretName(controller.CanaryDaemonSetName().Namespace) | ||
| if err != nil { | ||
| return false, nil, err | ||
| } |
There was a problem hiding this comment.
Why not get the secretName within desiredCanaryDaemonSet? Then you don't need to pass it in as a parameter.
| @@ -97,6 +103,8 @@ func desiredCanaryDaemonSet(canaryImage string) *appsv1.DaemonSet { | |||
| daemonset.Spec.Template.Spec.Containers[0].Image = canaryImage | |||
| daemonset.Spec.Template.Spec.Containers[0].Command = []string{"ingress-operator", CanaryHealthcheckCommand} | |||
|
|
|||
| daemonset.Spec.Template.Spec.Volumes[0].Secret.SecretName = secretName | |||
There was a problem hiding this comment.
It would be better to call canarySecretName right here instead of in the ensure function, and then passing it in here.
| daemonset.Spec.Template.Spec.Volumes[0].Secret.SecretName = secretName | |
| daemonset.Spec.Template.Spec.Volumes[0].Secret.SecretName, err := canarySecretName(name.Namespace) |
There was a problem hiding this comment.
And then you could test the secret generation in Test_desiredCanaryDaemonSet instead of writing a new test for ensureCanaryDaemonSet.
| UID: daemonset.UID, | ||
| Controller: &trueVar, | ||
| } | ||
| if _, err := r.ensureDefaultCertificateForIngress(ca, "openshift-ingress-canary", canaryRef, ingress); err != nil { |
There was a problem hiding this comment.
Doesn't there need to be a different function to ensure a canary cert rather than ensure a default cert? Does this ensure the correct cert?
Utilize the existing ingress controller certificate management controller to also manage the certificate for the ingress canary, and use that certificate when serving the canary endpoint.