CM-98: Add --enable-certificate-owner-ref as supported cert-manager controller arg #137
Conversation
…er arg Signed-off-by: arkadeepsen <arsen@redhat.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@arkadeepsen: This pull request references CM-98 which is a valid jira issue. 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 kubernetes/test-infra repository. |
|
/assign @thejasn |
|
/test all |
|
@arkadeepsen: 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/test-infra repository. I understand the commands that are listed here. |
Signed-off-by: arkadeepsen <arsen@redhat.com>
| @@ -375,7 +375,7 @@ func verifyValidControllerOperatorStatus(t *testing.T, client *certmanoperatorcl | |||
|
|
|||
| func addValidControlleDeploymentConfig(operator *v1alpha1.CertManager) { | |||
| operator.Spec.ControllerConfig = &v1alpha1.DeploymentConfig{ | |||
| OverrideArgs: []string{"--dns01-recursive-nameservers=10.10.10.10:53", "--dns01-recursive-nameservers-only"}, | |||
| OverrideArgs: []string{"--dns01-recursive-nameservers=10.10.10.10:53", "--dns01-recursive-nameservers-only", "--enable-certificate-owner-ref"}, | |||
There was a problem hiding this comment.
I think we should refrain from adding this flag here, as it would mean that every e2e test would end up using certificate-owner-ref which isn't something we want to test (IMO better not to change default behaviour for now) rather we add e2e(s) specific to this scenario as a follow-up PR to specifically test the effects of applying this flag (similar to discussion we had today).
"--dns01-recursive-nameservers=10.10.10.10:53", "--dns01-recursive-nameservers-only" can still remain IMHO because some certificate tests on AWS explicitly needs these flags.
There was a problem hiding this comment.
The function addValidControlleDeploymentConfig is only used once inside TestContainerOverrides e2e test and not all e2e tests. Also these flags are added once and the validity of the status of the cert-manager operator is checked. After that the flags are removed. Thus this flags do not effect any other tests.
When we modularize the e2e test, we anyway need to add the flag --enable-certificate-owner-ref to check whether it is correctly working or not. Adding it here, IMO, verifies that it is working correctly and makes the PR complete.
Let me know your thoughts.
There was a problem hiding this comment.
Cool, then we're good to keep it here for now.
|
Can PR description and subject be updated? Maybe https://hypershift-docs.netlify.app/contribute/ is of help. |
@arkadeepsen Could you please add a note regarding release note for this PR? So, when we do the 1.12 release, we can share the PR description to docs as a reference. cc: @xenolinux |
|
@arkadeepsen: This pull request references CM-98 which is a valid jira issue. 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 kubernetes/test-infra repository. |
|
/lgtm |
|
/cc @thejasn |
openshift-ci
bot
requested review from
davemulford,
thejasn,
xenolinux and
xingxingxia
|
/label px-approved |
openshift-ci
bot
added
the
px-approved
|
/label qe-approved |
openshift-ci
bot
added
the
qe-approved
|
/label docs-approved |
openshift-ci
bot
added
the
docs-approved
|
/approve @xenolinux We'll need to add a warning/note when using this field, because once this arg is added if we uninstall the operand/operator or delete the certificate this will break network connectivity since the attached secret is deleted. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arkadeepsen, swghosh, thejasn 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 |
openshift-ci
bot
added
the
approved
This PR adds the support for the flag
--enable-certificate-owner-ref. This flag can be enabled by adding it to thespec.controllerConfig.overrideArgsfield of the CertManagerclusterobject. When the flag is enabled the certificate resource is set as an owner of secret where the tls certificate is stored and the secret will be automatically removed when the certificate resource is deleted.