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

agentless: support updating health checks on consul clients during an upgrade to agentless #1690

Merged
merged 5 commits into from
Nov 10, 2022

Conversation

ishustava
Copy link
Contributor

@ishustava ishustava commented Nov 9, 2022

Changes proposed in this PR:
We will need to handle the case when a user has many applications on the service mesh and needs to take time to re-inject all those applications. If that is the case, there may be situations where the application changes its health status, but because it still hasn’t been re-started (re-injected), its health status needs to be synced into Consul clients rather than servers the way we used to do before. To do this, we will add a new annotation to all pods pointing to the current consul-k8s version: consul.hashicorp.com/consul-k8s-version: 1.0.0. If this annotation is not there or if the version is less than the version that supports “agentless”, we will continue to sync health checks to Consul clients for that particular pod

How I've tested this PR:
manual upgrade testing in multiple setups (not-secure, tls, tls +acls, tls+auto-encrypt+acls)

How I expect reviewers to test this PR:
👀

Checklist:

  • Tests added
  • CHANGELOG entry added

    HashiCorp engineers only, community PRs should not add a changelog entry.
    Entries should use present tense (e.g. Add support for...)

@ishustava ishustava force-pushed the ishustava/agentless-upgrades branch 2 times, most recently from a945604 to dedcc3f Compare November 9, 2022 20:11
@@ -236,24 +236,12 @@ spec:
{{- end }}
{{- end }}

{{- if .Values.global.consulSidecarContainer }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These flags are no longer used

Copy link
Contributor

@curtbushko curtbushko Nov 10, 2022

Choose a reason for hiding this comment

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

The removal of these are actually in the PR I am working on right now as I missed them last week. I will remove them from my commits and we'll leave them in yours.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hopefully the rebase of main onto your PR should automatically clear some of that from your commits.

Comment on lines +303 to +308
items:
- key: {{ default "tls.crt" .Values.global.tls.caCert.secretKey }}
path: tls.crt
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My IDE decided to format these

Copy link
Contributor

@thisisnotashwin thisisnotashwin Nov 10, 2022

Choose a reason for hiding this comment

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

clearly our IDEs disagree on how YAML slices should be indented 😭

@ishustava ishustava force-pushed the ishustava/refactor-connect-inject-package branch 2 times, most recently from d5e5ebd to 35ea3e0 Compare November 9, 2022 20:55
Base automatically changed from ishustava/refactor-connect-inject-package to main November 9, 2022 22:11
@ishustava ishustava force-pushed the ishustava/agentless-upgrades branch 2 times, most recently from 500aff3 to d11b712 Compare November 9, 2022 23:16
@ishustava ishustava marked this pull request as ready for review November 10, 2022 01:27
Copy link
Contributor

@curtbushko curtbushko left a comment

Choose a reason for hiding this comment

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

Nice job!

Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

Excellent work, as usual 😄

Comment on lines +51 to +74
if r.EnableAutoEncrypt {
// Get Connect CA.
caRoots, _, err := serverClient.Agent().ConnectCARoots(nil)
if err != nil {
return nil, err
}
if caRoots == nil {
return nil, fmt.Errorf("ca root list is nil")
}
if caRoots.Roots == nil {
return nil, fmt.Errorf("ca roots is nil")
}
if len(caRoots.Roots) == 0 {
return nil, fmt.Errorf("the list of root CAs is empty")
}

for _, root := range caRoots.Roots {
if root.Active {
ccCfg.TLSConfig.CAFile = ""
ccCfg.TLSConfig.CAPem = []byte(root.RootCertPEM)
break
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

:chefkiss:

}
}

func TestConsulClientForNodeAgent(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@ishustava ishustava merged commit 6dc218d into main Nov 10, 2022
@ishustava ishustava deleted the ishustava/agentless-upgrades branch November 10, 2022 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants