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

Gloo: Create gloo upstreams from non-discovered services #894

Merged
merged 14 commits into from
May 11, 2021

Conversation

saiskee
Copy link
Contributor

@saiskee saiskee commented Apr 27, 2021

Signed-off-by: Keerthan Ekbote keerthan.ekbote@solo.io

Signed-off-by: Keerthan Ekbote <keerthan.ekbote@solo.io>
Signed-off-by: Keerthan Ekbote <keerthan.ekbote@solo.io>
pkg/router/gloo.go Outdated Show resolved Hide resolved
Signed-off-by: Keerthan Ekbote <keerthan.ekbote@solo.io>
@saiskee saiskee force-pushed the create-non-discovered-gloo-upstreams branch from 7f07be5 to 3f1af1e Compare April 27, 2021 21:19
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

@saiskee please run make fmt and commit the changes to unblock CI.

Signed-off-by: Keerthan Ekbote <keerthan.ekbote@solo.io>
@saiskee saiskee marked this pull request as ready for review April 28, 2021 13:53
@stefanprodan
Copy link
Member

CI is failing with:

--- FAIL: TestGlooRouter_Sync (0.01s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1d01fe0]

@snuggie12
Copy link

Perhaps this should be a separate issue/PR, but this only fixes our non-istio upstreams.

To solve that I think you'd need in your Canary something like upstreamSslConfigSpec

Signed-off-by: Keerthan Ekbote <keerthan.ekbote@solo.io>
Signed-off-by: Keerthan Ekbote <keerthan.ekbote@solo.io>
@saiskee saiskee marked this pull request as draft April 29, 2021 16:48
Signed-off-by: Keerthan Ekbote <keerthan.ekbote@solo.io>
@saiskee saiskee marked this pull request as ready for review April 29, 2021 23:16
@stefanprodan
Copy link
Member

@saiskee this again:

please run make fmt and commit the changes to unblock CI.

Signed-off-by: Keerthan Ekbote <keerthan.ekbote@solo.io>
@stefanprodan
Copy link
Member

@saiskee now the unit tests are failing, can you please run make test before pushing any more changes? Thanks

--- FAIL: TestGlooRouter_Sync (0.00s)
    gloo_test.go:42: 
        	Error Trace:	gloo_test.go:42
        	Error:      	Received unexpected error:
        	            	error creating flagger primary upstream: service podinfo-primary.default get query error: services "podinfo-primary" not found
        	Test:       	TestGlooRouter_Sync
--- FAIL: TestGlooRouter_SetRoutes (0.00s)
    gloo_test.go:82: 
        	Error Trace:	gloo_test.go:82
        	Error:      	Received unexpected error:
        	            	error creating flagger primary upstream: service podinfo-primary.default get query error: services "podinfo-primary" not found
        	Test:       	TestGlooRouter_SetRoutes
--- FAIL: TestGlooRouter_GetRoutes (0.00s)
    gloo_test.go:144: 
        	Error Trace:	gloo_test.go:144
        	Error:      	Received unexpected error:
        	            	error creating flagger primary upstream: service podinfo-primary.default get query error: services "podinfo-primary" not found
        	Test:       	TestGlooRouter_GetRoutes

pkg/apis/gloo/gloo/v1/types.go Outdated Show resolved Hide resolved
pkg/apis/gloo/gloo/v1/types.go Outdated Show resolved Hide resolved
Signed-off-by: Keerthan Ekbote <keerthan.ekbote@solo.io>
@saiskee saiskee force-pushed the create-non-discovered-gloo-upstreams branch from 882c793 to f0f44c9 Compare May 5, 2021 15:20
Signed-off-by: Keerthan Ekbote <keerthan.ekbote@solo.io>
@saiskee saiskee requested a review from stefanprodan May 5, 2021 15:21
Signed-off-by: Keerthan Ekbote <keerthan.ekbote@solo.io>
@codecov-commenter
Copy link

Codecov Report

Merging #894 (fdc8dd8) into main (c9257bd) will increase coverage by 0.15%.
The diff coverage is 81.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #894      +/-   ##
==========================================
+ Coverage   56.64%   56.80%   +0.15%     
==========================================
  Files          69       69              
  Lines        5665     5704      +39     
==========================================
+ Hits         3209     3240      +31     
- Misses       1978     1981       +3     
- Partials      478      483       +5     
Impacted Files Coverage Δ
pkg/metrics/observers/gloo.go 46.66% <ø> (ø)
pkg/router/factory.go 0.00% <0.00%> (ø)
pkg/router/gloo.go 80.52% <85.71%> (-1.11%) ⬇️
pkg/canary/config_tracker.go 83.25% <0.00%> (-0.91%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9257bd...fdc8dd8. Read the comment docs.

@stefanprodan
Copy link
Member

@saiskee please update the Helm docs to tell people that discovery is no longer required: https://github.com/fluxcd/flagger/blob/main/charts/flagger/README.md

This also needs an update: https://github.com/fluxcd/flagger/blob/main/docs/gitbook/tutorials/gloo-progressive-delivery.md#bootstrap

# applied 
deployment.apps/podinfo
horizontalpodautoscaler.autoscaling/podinfo
virtualservices.gateway.solo.io/podinfo
canary.flagger.app/podinfo

# generated 
deployment.apps/podinfo-primary
horizontalpodautoscaler.autoscaling/podinfo-primary
service/podinfo
service/podinfo-canary
service/podinfo-primary
routetables.gateway.solo.io/podinfo

Signed-off-by: Keerthan Ekbote <keerthan.ekbote@solo.io>
Signed-off-by: Keerthan Ekbote <keerthan.ekbote@solo.io>
@saiskee saiskee requested a review from stefanprodan May 6, 2021 17:00
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @saiskee 🏅

@stefanprodan stefanprodan merged commit 3ad55c9 into fluxcd:main May 11, 2021
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.

5 participants