Skip to content
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.

Sign up for GitHub

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

Jump to bottom

CM-98: Add --enable-certificate-owner-ref as supported cert-manager controller arg #137

Merged
merged 2 commits into from
Jul 26, 2023
Merged

CM-98: Add --enable-certificate-owner-ref as supported cert-manager controller arg #137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
24188ee
Add --enable-certificate-owner-ref as supported cert-manager controll…
arkadeepsen Jul 11, 2023
1c71be3
Added e2e test
arkadeepsen Jul 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/controller/deployment/deployment_overrides_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ func withContainerArgsValidateHook(certmanagerinformer certmanagerinformer.CertM
// Application Default Credentials (ADC) per
// https://cloud.google.com/docs/authentication/application-default-credentials#search_order
"--issuer-ambient-credentials",
// Whether to set the certificate resource as an owner of secret where the tls certificate
// is stored. When this flag is enabled, the secret will be automatically removed when the
// certificate resource is deleted.
"--enable-certificate-owner-ref",
}
supportedCertManagerWebhookArgs := []string{
// Log Level
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/cert_manager_deployment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, then we're good to keep it here for now.

OverrideEnv: []corev1.EnvVar{
{
Name: "HTTP_PROXY",
Expand Down