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.

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

Fix auto-inject config when TLS is disabled #2246

Merged
merged 1 commit into from Feb 11, 2019
Merged

Conversation

klingerf
Copy link
Member

As of #2163, TLS is no longer required when installing linkerd with automatic proxy injection, but the injected linkerd configs were still referencing TLS volumes. This change updates the proxy-injector to only include those volumes when the control plane is installed with auto-inject and TLS enabled.

Fixes #2236.

Signed-off-by: Kevin Lingerfelt <kl@buoyant.io>
@klingerf klingerf self-assigned this Feb 11, 2019
@@ -1,94 +0,0 @@
{
Copy link
Member Author

Choose a reason for hiding this comment

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

This text fixture file was not being used anywhere, so I'm removing it.

}

// NewWebhookConfig returns a new instance of initiator.
func NewWebhookConfig(client kubernetes.Interface, controllerNamespace, webhookServiceName string, noInitContainer bool, rootCA *tls.CA) (*WebhookConfig, error) {
func NewWebhookConfig(client kubernetes.Interface, controllerNamespace, webhookServiceName string, rootCA *tls.CA) (*WebhookConfig, error) {
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 webhook config template does not use the value of noInitContainer when rendering the template, so it doesn't need to be passed into this func or set on the WebhookConfig struct.

Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

Looks good to me. Good catch on no-init-container non-usage 👍
FYI, I tried reproducing the failed healthcheck, but couldn't. I first thought the recently introduced change in the proxy for disabling TLS-to-plaintext fallback would make this fail, but it doesn't:
linkerd/linkerd2-proxy@3336918

Copy link
Contributor

@ihcsim ihcsim left a comment

Choose a reason for hiding this comment

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

LGTM

@klingerf klingerf added this to In progress in 2.2 via automation Feb 11, 2019
@klingerf klingerf moved this from In progress to Reviewer approved in 2.2 Feb 11, 2019
@klingerf klingerf merged commit 26aa771 into master Feb 11, 2019
2.2 automation moved this from Reviewer approved to Done Feb 11, 2019
@klingerf klingerf deleted the kl/auto-inject-tls branch February 11, 2019 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
2.2
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants