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

Tproxy split init containers #441

Merged
merged 1 commit into from
Feb 24, 2021
Merged

Tproxy split init containers #441

merged 1 commit into from
Feb 24, 2021

Conversation

kschoche
Copy link
Contributor

@kschoche kschoche commented Feb 19, 2021 •
edited
Loading

Changes proposed in this PR:

  • split the existing connect init container into two init containers: 1 does the consul binary copy and the other does the existing leg-work for registration. This is part of the tproxy refactor which sees the existing init container functionality replaced by a new consul-k8s command (incoming PR).

How I've tested this PR:
manually deployed and confirmed connect inject still works with the correct init containers.
new and existing unit tests work.

How I expect reviewers to test this PR:
deploy kschoche/consul-k8s-dev and a sample connect injected app to confirm it still mutates and now has 2 containers instead of 1.

Events:
  Type     Reason     Age   From               Message
  ----     ------     ----  ----               -------
  Normal   Scheduled  22s   default-scheduler  Successfully assigned default/counting to [redacted]
  Normal   Pulled     21s   kubelet            Container image "hashicorp/consul:1.9.3" already present on machine
  Normal   Created    21s   kubelet            Created container consul-connect-inject-init-copy-container
  Normal   Started    21s   kubelet            Started container consul-connect-inject-init-copy-container
  Normal   Pulled     17s   kubelet            Container image "hashicorp/consul:1.9.3" already present on machine
  Normal   Created    17s   kubelet            Created container consul-connect-inject-init
  Normal   Started    16s   kubelet            Started container consul-connect-inject-init
  Normal   Pulled     12s   kubelet            Container image "hashicorp/counting-service:0.0.2" already present on machine
  Normal   Started    11s   kubelet            Started container counting

Checklist:

  • Tests added
  • CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

@kschoche kschoche added type/enhancement New feature or request theme/tproxy Items related to transparent proxy labels Feb 19, 2021
@kschoche kschoche marked this pull request as ready for review February 23, 2021 14:30
kschoche
kschoche commented Feb 23, 2021
View reviewed changes
subcommand/consul-sidecar/command.go Outdated
@@ -1,4 +1,4 @@
package subcommand
package consulsidecar
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything past this in the PR is a merge from feature-metrics which is the base of feature-tproxy.
I've rebased feature-tproxy off feature-metrics and pushed it but for some reason it's not showing up here.
Will try to sort it out before merging.

@kschoche kschoche requested review from a team, ndhanushkodi and ishustava and removed request for a team February 23, 2021 14:38
@kschoche kschoche self-assigned this Feb 23, 2021
ndhanushkodi
ndhanushkodi approved these changes Feb 23, 2021
View reviewed changes
Copy link
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.

Looks good! Had a thought on the name and was curious about the JSON patches.

connect-inject/container_init.go Outdated Show resolved Hide resolved
connect-inject/handler_test.go Show resolved Hide resolved
ishustava
ishustava reviewed Feb 24, 2021
View reviewed changes
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

Looking pretty good! I've left a couple of comments and chimed in on the naming conversation 😄 Once we decide on those, it's good to go. I've also left a couple of minor edits just to follow the godoc format.

I've run connect acceptance tests with an image from this branch, and they pass!

CHANGELOG.md Outdated Show resolved Hide resolved
connect-inject/container_init.go Outdated Show resolved Hide resolved
connect-inject/container_init.go Outdated Show resolved Hide resolved
connect-inject/container_init_test.go Outdated Show resolved Hide resolved
connect-inject/container_init.go Outdated Show resolved Hide resolved
connect-inject/handler.go Outdated Show resolved Hide resolved
connect-inject/container_init.go Outdated Show resolved Hide resolved
ishustava
ishustava approved these changes Feb 24, 2021
View reviewed changes
Copy link
Contributor

@ishustava ishustava 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! (assuming we solve the diff before merging)

@kschoche
Copy link
Contributor Author

kschoche commented Feb 24, 2021 •
edited
Loading

Looks good! (assuming we solve the diff before merging)

Oddly enough if I pull both from origin and diff the two (feature-tproxy && tproxy-split-init-containers) the metrics bits dont show up in git diff, I wonder if this is a strange quirk of github? both branches have commit a756490c0bb3cd835a06ec580daa1adfb9cf7653 :-/

@kschoche
Split the init container into two init containers. One will cp the co…
f87a6a8 
…nsul bin and the other will retain all of the existing behaviours

Co-authored-by: Iryna Shustava <[email protected]>
@kschoche kschoche merged commit 9d33955 into feature-tproxy Feb 24, 2021
@kschoche kschoche deleted the tproxy-split-init-containers branch February 24, 2021 18:08
ndhanushkodi pushed a commit that referenced this pull request Mar 9, 2021
@kschoche @ishustava @ndhanushkodi
Split the init container into two init containers. One will cp the co…
cd47a7b 
…nsul bin and the other will retain all of the existing behaviours (#441)

Co-authored-by: Iryna Shustava <[email protected]>
thisisnotashwin pushed a commit that referenced this pull request Mar 26, 2021
@kschoche @ishustava @thisisnotashwin
Split the init container into two init containers. One will cp the co…
2296990 
…nsul bin and the other will retain all of the existing behaviours (#441)

Co-authored-by: Iryna Shustava <[email protected]>
thisisnotashwin pushed a commit that referenced this pull request Mar 26, 2021
@kschoche @ishustava @thisisnotashwin
Split the init container into two init containers. One will cp the co…
d133d0f 
…nsul bin and the other will retain all of the existing behaviours (#441)

Co-authored-by: Iryna Shustava <[email protected]>
ishustava added a commit that referenced this pull request Apr 14, 2021
@kschoche @ishustava
Split the init container into two init containers. One will cp the co…
b6e7557 
…nsul bin and the other will retain all of the existing behaviours (#441)

Co-authored-by: Iryna Shustava <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndhanushkodi ndhanushkodi ndhanushkodi approved these changes

@ishustava ishustava ishustava approved these changes

Assignees

@kschoche kschoche

Labels
theme/tproxy Items related to transparent proxy type/enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kschoche @ndhanushkodi @ishustava