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

chore: add config manifests and skaffold configuration for kong admin api servie discovery #3474

Merged
merged 1 commit into from
Feb 3, 2023

Conversation

pmalek
Copy link
Member

@pmalek pmalek commented Feb 2, 2023

What this PR does / why we need it:

This PR extracts debug manifests from #3421 to minimize the mental burden when reviewing.

This provides the end user with debug_multi_gw skaffold profile which can be launched like so:

make debug.skaffold SKAFFOLD_DEBUG_PROFILE=debug_multi_gw

When merged with changes from #3421 this should spawn 2 separate Deployments: 1 for KIC and 1 for Kong (with 2 replicas by default).

The above mentioned command is expected to fail at this point because:

  • --kong-admin-svc flag is not introduced yet hence can't be used
  • --kong-admin-url flag is meant to be filled with IP(s) and with this setup we're not able to fill that in.

Hence user is expected to get the following error message:

time="2023-02-02T17:18:44Z" level=info msg="Retrying kong admin api client call after error" error="making HTTP request: Get \"https://127.0.0.1:8444/\": dial tcp 127.0.0.1:8444: connect: connection refused" logger=setup retries=29/60

@pmalek pmalek added the area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality. label Feb 2, 2023
@pmalek pmalek self-assigned this Feb 2, 2023
@pmalek pmalek marked this pull request as ready for review February 2, 2023 17:20
@pmalek pmalek requested a review from a team as a code owner February 2, 2023 17:20
@pmalek pmalek enabled auto-merge (squash) February 2, 2023 18:12
@codecov
Copy link

codecov bot commented Feb 2, 2023

Codecov Report

Base: 73.5% // Head: 73.5% // Decreases project coverage by -0.1% ⚠️

Coverage data is based on head (10d0df0) compared to base (4187223).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #3474     +/-   ##
=======================================
- Coverage   73.5%   73.5%   -0.1%     
=======================================
  Files        118     118             
  Lines      14065   14065             
=======================================
- Hits       10345   10340      -5     
- Misses      3059    3064      +5     
  Partials     661     661             
Impacted Files Coverage Δ
...nternal/controllers/gateway/tlsroute_controller.go 71.4% <0.0%> (-2.1%) ⬇️
internal/dataplane/kongstate/service.go 66.0% <0.0%> (-1.3%) ⬇️
internal/dataplane/parser/parser.go 91.5% <0.0%> (+0.8%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pmalek pmalek added this to the KIC v2.9.0 milestone Feb 3, 2023
@czeslavo
Copy link
Contributor

czeslavo commented Feb 3, 2023

I wonder if it wouldn't make sense to make the multiple_gw config one of the variants under config/variants and use it as a base for the debug one. As I understand that's going to be the supported deployment option and it could make sense to have it there. Generating an "all-in-one" manifest is also what I've done for konnect config. WDYT?

@pmalek
Copy link
Member Author

pmalek commented Feb 3, 2023

I wonder if it wouldn't make sense to make the multiple_gw config one of the variants under config/variants and use it as a base for the debug one.

It certainly would. What would be the benefit of that? 🤔 Or difference for that matter.

@czeslavo
Copy link
Contributor

czeslavo commented Feb 3, 2023

I wonder if it wouldn't make sense to make the multiple_gw config one of the variants under config/variants and use it as a base for the debug one.

It certainly would. What would be the benefit of that? 🤔 Or difference for that matter.

Well, it would be useful for running it in not-debug mode and would be consistent as we have such a config for every supported deployment variant. It would also be useful in E2E tests - we would be able to deploy the manifests to test it.

@pmalek pmalek merged commit 1abef9c into main Feb 3, 2023
@pmalek pmalek deleted the single-controller-deployments-skaffold-manifests branch February 3, 2023 13:17
@pmalek
Copy link
Member Author

pmalek commented Feb 3, 2023

I wonder if it wouldn't make sense to make the multiple_gw config one of the variants under config/variants and use it as a base for the debug one.

It certainly would. What would be the benefit of that? 🤔 Or difference for that matter.

Well, it would be useful for running it in not-debug mode and would be consistent as we have such a config for every supported deployment variant. It would also be useful in E2E tests - we would be able to deploy the manifests to test it.

As agreed in a separate discussion let's do this in a separate PR. We can track this under #3480.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants