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

Mw/net 5679 explicit upstreams mesh webhook injects upstream env variable #2995

Merged
merged 8 commits into from
Sep 25, 2023
Merged

Mw/net 5679 explicit upstreams mesh webhook injects upstream env variable #2995

merged 8 commits into from
Sep 25, 2023

Conversation

wilkermichael
Copy link
Contributor

Changes proposed in this PR:

  • This introduces injection of the port/ip address environment variables

How I've tested this PR:

  • unit tests

How I expect reviewers to test this PR:
👀

Checklist:

@wilkermichael wilkermichael mentioned this pull request Sep 21, 2023
Merged
2 tasks
@wilkermichael wilkermichael force-pushed the mw/NET-5679-explicit-upstreams-mesh-webhook-injects-upstream-env-variable branch from d61e083 to 3264528 Compare September 21, 2023 21:07
@wilkermichael wilkermichael force-pushed the mw/NET-5679-refactor-upstream-processor branch from b39b194 to 1276430 Compare September 21, 2023 21:14
@wilkermichael wilkermichael force-pushed the mw/NET-5679-explicit-upstreams-mesh-webhook-injects-upstream-env-variable branch from 3264528 to 72638d7 Compare September 21, 2023 21:15
@wilkermichael wilkermichael added pr/no-changelog PR does not need a corresponding .changelog entry pr/no-backport signals that a PR will not contain a backport label labels Sep 21, 2023
@wilkermichael wilkermichael force-pushed the mw/NET-5679-explicit-upstreams-mesh-webhook-injects-upstream-env-variable branch from 48aa3da to 1026401 Compare September 22, 2023 14:04
@wilkermichael wilkermichael reopened this Sep 22, 2023
@wilkermichael wilkermichael force-pushed the mw/NET-5679-explicit-upstreams-mesh-webhook-injects-upstream-env-variable branch from 1026401 to ff506eb Compare September 22, 2023 14:24
@wilkermichael wilkermichael force-pushed the mw/NET-5679-refactor-upstream-processor branch from 747f9f2 to 9c4fb95 Compare September 22, 2023 14:25
@wilkermichael wilkermichael force-pushed the mw/NET-5679-explicit-upstreams-mesh-webhook-injects-upstream-env-variable branch from ff506eb to e586c40 Compare September 22, 2023 14:25
@wilkermichael wilkermichael force-pushed the mw/NET-5679-explicit-upstreams-mesh-webhook-injects-upstream-env-variable branch from e586c40 to 559356d Compare September 22, 2023 18:15
@wilkermichael wilkermichael force-pushed the mw/NET-5679-refactor-upstream-processor branch from 1acf15c to 91edc8f Compare September 22, 2023 19:31
@wilkermichael wilkermichael force-pushed the mw/NET-5679-explicit-upstreams-mesh-webhook-injects-upstream-env-variable branch from 559356d to 253063f Compare September 22, 2023 20:00
DanStough
DanStough suggested changes Sep 22, 2023
View reviewed changes
Copy link
Contributor

@DanStough DanStough left a comment

Choose a reason for hiding this comment

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

Looks great! My only dealbreaker was the naming of the ENV variable. Open to discussion, though.

control-plane/connect-inject/webhook_v2/container_env.go Outdated Show resolved Hide resolved
control-plane/connect-inject/webhook_v2/container_env.go Outdated Show resolved Hide resolved
control-plane/connect-inject/webhook_v2/container_env.go Outdated Show resolved Hide resolved
control-plane/connect-inject/webhook_v2/container_env.go Outdated Show resolved Hide resolved
control-plane/connect-inject/constants/constants_test.go Outdated Show resolved Hide resolved
control-plane/connect-inject/common/annotation_processor.go Outdated Show resolved Hide resolved
@wilkermichael wilkermichael force-pushed the mw/NET-5679-explicit-upstreams-mesh-webhook-injects-upstream-env-variable branch from 253063f to d801d15 Compare September 22, 2023 20:34
@wilkermichael wilkermichael force-pushed the mw/NET-5679-refactor-upstream-processor branch from 813cd76 to b0dda3e Compare September 22, 2023 22:43
@wilkermichael wilkermichael force-pushed the mw/NET-5679-explicit-upstreams-mesh-webhook-injects-upstream-env-variable branch 2 times, most recently from 5efefd6 to 48e3820 Compare September 22, 2023 23:11
@wilkermichael wilkermichael force-pushed the mw/NET-5679-refactor-upstream-processor branch from d827503 to 01331ba Compare September 23, 2023 04:31
@wilkermichael wilkermichael force-pushed the mw/NET-5679-explicit-upstreams-mesh-webhook-injects-upstream-env-variable branch from 48e3820 to ad8aea8 Compare September 23, 2023 04:32
Base automatically changed from mw/NET-5679-refactor-upstream-processor to main September 23, 2023 05:02
@wilkermichael
first commit
cb1d8cf
@wilkermichael
fixed a bug in annotation processor
ae1da90 
- nil pointer exception because we weren't checking for a length of two in the unlabeled case
- added a test to make sure it's covered
@wilkermichael
added container env logic for injecting env variables
c3ce59e 
- due to refactoring can support same labeled/unlabeld parsing logic as when pod controller processes the annotations
@wilkermichael
added an error check to the containerEnvVars(pod) call
3babb5e 
- added test cases
@wilkermichael wilkermichael force-pushed the mw/NET-5679-explicit-upstreams-mesh-webhook-injects-upstream-env-variable branch from ad8aea8 to d46dece Compare September 23, 2023 05:09
@wilkermichael wilkermichael enabled auto-merge (squash) September 23, 2023 05:10
@wilkermichael
Copy link
Contributor Author

@DanStough can you take a look at this now that I've addressed your comments? Thanks!

thisisnotashwin
thisisnotashwin reviewed Sep 25, 2023
View reviewed changes
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.

This looks really good. Have one question that is a bit of a blocker but great work otherwise.

control-plane/connect-inject/webhookv2/container_env.go Outdated
// }
//}
func (w *MeshWebhook) containerEnvVars(pod corev1.Pod) ([]corev1.EnvVar, error) {
upstreams, err := common.ProcessPodUpstreams(pod, true, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to update the MeshWebhook struct to have an EnablePartitions field and pass in w.enabledNamespaces and w.enablePartitions here right? Is there a reason we default Process Pod Upstreams to have true for both those values?

@thisisnotashwin thisisnotashwin removed the request for review from DanStough September 25, 2023 15:30
@wilkermichael wilkermichael merged commit 756c37f into main Sep 25, 2023
22 of 32 checks passed
@wilkermichael wilkermichael deleted the mw/NET-5679-explicit-upstreams-mesh-webhook-injects-upstream-env-variable branch September 25, 2023 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanStough DanStough DanStough approved these changes

@thisisnotashwin thisisnotashwin thisisnotashwin approved these changes

@zalimeni zalimeni Awaiting requested review from zalimeni

Assignees
No one assigned
Labels
pr/no-backport signals that a PR will not contain a backport label pr/no-changelog PR does not need a corresponding .changelog entry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wilkermichael @DanStough @thisisnotashwin