-
Notifications
You must be signed in to change notification settings - Fork 140
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
PolicyRecommendation controller overwrites tigera-ca bundle per tenant #3191
PolicyRecommendation controller overwrites tigera-ca bundle per tenant #3191
Conversation
pkg/controller/policyrecommendation/policyrecommendation_controller.go
Outdated
Show resolved
Hide resolved
pkg/controller/policyrecommendation/policyrecommendation_controller_test.go
Outdated
Show resolved
Hide resolved
pkg/controller/policyrecommendation/policyrecommendation_controller_test.go
Outdated
Show resolved
Hide resolved
pkg/controller/policyrecommendation/policyrecommendation_controller_test.go
Outdated
Show resolved
Hide resolved
KeyPairOptions: []rcertificatemanagement.KeyPairOption{ | ||
rcertificatemanagement.NewKeyPairOption(policyRecommendationKeyPair, true, true), | ||
}, | ||
TrustedBundle: nil, |
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 nil the right value here? I'm not sure what the downstream result of this value being nil would be when setting these environment variables: https://github.com/tigera/operator/blob/master/pkg/render/policyrecommendation.go#L303
I think the difference you're intending is that in multi-tenant mode we simply load the existing trusted bundle created by the tenant controller, while in single or zero-tenant mode we need to create a trusted bundle, but in both cases I think we still need to populate the TrustedBundle
field no?
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.
For multi-tenant, we should leave this nil because we want to create the secrets that contain the x509 certificates, but not the actual config map tigera-ca-bundle
. For zero-tenant/single-tenant we want both the secrets and the config map created, because we install in tigera-policy-recommendation namespace.
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.
Oh so when those environment variables are blank it will trigger creation of the x509 certificates?
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.
Actually, the secrets that hold the certificates.
return reconcile.Result{}, err | ||
} | ||
|
||
components = append(components, component) |
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.
By adding the component after the other components, doesn't the operator try to render tls assets before the namespace exists on the first ever iteration? I think that would create an unnecessary error in the logs / tigerastatus.
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.
lgtm
/merge-when-ready |
OK, I will merge the pull request when it's ready, leave the commits as is when I merge it, and leave the branch after I've merged it. |
Description
Tigera-ca-bundle is created by the tenant controller for a multi-tenant setup (https://github.com/tigera/operator/blob/master/pkg/controller/secrets/tenant_controller.go#L187-L212). This gets overwritten by PolicyRecommendation Controller. This causes Linseed to reject connection with Elastic.
For PR author
make gen-files
make gen-versions
For PR reviewers
A note for code reviewers - all pull requests must have the following:
kind/bug
if this is a bugfix.kind/enhancement
if this is a a new feature.enterprise
if this PR applies to Calico Enterprise only.