Skip to content

Service to service troubleshooting#1851

Merged
curtbushko merged 11 commits intomainfrom
curtbushko/svc-to-svc-cli
Feb 13, 2023
Merged

Service to service troubleshooting#1851
curtbushko merged 11 commits intomainfrom
curtbushko/svc-to-svc-cli

Conversation

@curtbushko
Copy link
Copy Markdown
Contributor

@curtbushko curtbushko commented Jan 25, 2023

Things to still do:

  • Clean up text strings based on designer input
  • Add acceptance test when troubleshoot package is working with validation
  • Remove hardcoded replace in go.mod when branches are merged into the consul repo

Changes proposed in this PR:

How I've tested this PR:

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...)

@curtbushko curtbushko self-assigned this Jan 25, 2023
@curtbushko curtbushko force-pushed the curtbushko/svc-to-svc-cli branch from 1353726 to f53dc25 Compare February 9, 2023 17:05
@curtbushko curtbushko changed the title Service to service troubleshooting (WIP) Service to service troubleshooting Feb 10, 2023
@curtbushko curtbushko marked this pull request as ready for review February 10, 2023 18:12
Copy link
Copy Markdown
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

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

Amazing!! Left a few comments!

This could definitely come in a future PR, but could we add a case for testing with a config entry (deploy like static-server1 and static-server2 and do a splitter between them) to the acceptance test?

Comment thread acceptance/tests/cli/cli_install_test.go Outdated
Comment thread acceptance/tests/cli/cli_install_test.go Outdated
Comment thread acceptance/tests/cli/cli_install_test.go Outdated

if c.flagUpstreamEnvoyID == "" && c.flagUpstreamIP == "" {
return fmt.Errorf("-upstream-envoy-id OR -upstream-ip is required.\n Please run `consul troubleshoot upstreams` to find the corresponding upstream.")
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we validate that exactly one or the other is provided also? That way it's known that both aren't required early on. cc @malizz this would be good to add the the consul CLI as well if it's not already there!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I have it:

None:

$ ./cli/bin/consul-k8s troubleshoot proxy -pod static-client-5644c6f9c9-rl8kf

! Invalid argument: -upstream-envoy-id OR -upstream-ip is required.
Please run consul troubleshoot upstreams to find the corresponding upstream.

Both:

$ ./cli/bin/consul-k8s troubleshoot proxy -pod static-client-5644c6f9c9-rl8kf -upstream-ip 10.96.217.21 -upstream-envoy-id 1234
! Invalid argument: -upstream-envoy-id OR -upstream-ip is required.
Please run consul troubleshoot upstreams to find the corresponding upstream.

Comment thread cli/cmd/troubleshoot/proxy/proxy.go Outdated
Comment thread cli/cmd/troubleshoot/proxy/proxy_test.go
Copy link
Copy Markdown
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

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

Amazing!!!

@curtbushko curtbushko merged commit e71a71c into main Feb 13, 2023
@curtbushko curtbushko deleted the curtbushko/svc-to-svc-cli branch February 13, 2023 03:06
david-yu pushed a commit that referenced this pull request Feb 14, 2023
- Add troubleshooting commands for 'upstream' and 'proxy' to allow troubleshooting of envoy config.
ishustava pushed a commit that referenced this pull request Feb 18, 2023
- Add troubleshooting commands for 'upstream' and 'proxy' to allow troubleshooting of envoy config.
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.

2 participants