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

Make deploy/ manifests use KIC 2.0 #1500

Closed
wants to merge 3 commits into from
Closed

Conversation

mflendrich
Copy link
Contributor

Part of #1256

This PR:

  • makes manifests under deploy/ use KIC 2.0. (The 2.0 tag is not pushed yet, so for testing it needs to be substituted with next-railgun).
  • adds the UDPIngress CRD to the existing CRDs in deploy/
  • makes kustomize manifests use the readyz endpoint, new in KIC 2.0

This PR does not:

@mflendrich mflendrich requested a review from a team as a code owner July 7, 2021 14:14
@mflendrich mflendrich temporarily deployed to Configure ci July 7, 2021 14:15 Inactive
@codecov
Copy link

codecov bot commented Jul 7, 2021

Codecov Report

Merging #1500 (e184668) into next (2c11ced) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             next    #1500   +/-   ##
=======================================
  Coverage   53.29%   53.29%           
=======================================
  Files          47       47           
  Lines        3916     3916           
=======================================
  Hits         2087     2087           
  Misses       1680     1680           
  Partials      149      149           

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 2c11ced...e184668. Read the comment docs.

@@ -92,6 +96,8 @@ spec:
value: "true"
- name: CONTROLLER_PUBLISH_SERVICE
value: "kong/kong-proxy"
- name: CONTROLLER_CONTROLLER_UDPINGRESS
value: "false"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it mean : UDPIngress is not supported by default ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Line 94: The hardcode 127.0.0.1:8444 is resolved in PR
#1487

Copy link
Contributor

@ccfishk ccfishk Jul 8, 2021

Choose a reason for hiding this comment

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

I don't think we need change this line of code when we deploying both controller and proxy into the same node. either 127.0.0.1:8444 or localhost:8444 would work fine.

@@ -120,7 +126,7 @@ spec:
failureThreshold: 3
readinessProbe:
httpGet:
path: /healthz
path: /readyz
Copy link
Contributor

Choose a reason for hiding this comment

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

Right. this is good. Do we have an issue adding the readiness check for controller ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no specific issue - this has apparently been added as a part of https:/Kong/kubernetes-ingress-controller/pull/1314/files

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha.
we need follow k8s readiness probes and define the function reporting the readiness. https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/#define-readiness-probes. The good thing is we already having the path defined in the pod.

@rainest
Copy link
Contributor

rainest commented Jul 8, 2021

Do we indeed want to change the existing all-in-one YAMLs and deploy/manifests/base/ content for this? Per prior discussion with Shane C., we intend to continue support for 1.x for some period of time, and will continue to need manifests that deploy it. We will presumably change the default manifests to 2.x once that's available, but that should be a GA task rather than a beta one.

My expectation here was that we'd create a new version of hack/build-single-manifests.sh that composes manifests from railgun/config. The contents of the existing deploy/manifests Deployment templates are largely still appropriate for 2.x (aside from the readiness endpoint, which we'd need a new kustomize patch for), but the end results would need to be separate (as we can only have a single version and the new readiness endpoint is not compatible with 1.x).

2.x needs updated RBAC as well, for both UDPIngress permissions and the addition of finalizer edit permissions to most other resources.

I think we can try to have a central place for building 2.x manifests as well though need some engineering work under railgun/config to sort it out.

And, our controller is an extension manager, which watches resources from different extensions (CRD), an appropriate RBAC would be as following
https:/Kong/kubernetes-ingress-controller/pull/1487/files#diff-e35bff7662f401f7147347e04d81e9a731e69c626ab6fb0130b037606ab7e397

@ccfishk
Copy link
Contributor

ccfishk commented Jul 8, 2021

Do we indeed want to change the existing all-in-one YAMLs and deploy/manifests/base/ content for this? Per prior discussion with Shane C., we intend to continue support for 1.x for some period of time, and will continue to need manifests that deploy it. We will presumably change the default manifests to 2.x once that's available, but that should be a GA task rather than a beta one.

My expectation here was that we'd create a new version of hack/build-single-manifests.sh that composes manifests from railgun/config. The contents of the existing deploy/manifests Deployment templates are largely still appropriate for 2.x (aside from the readiness endpoint, which we'd need a new kustomize patch for), but the end results would need to be separate (as we can only have a single version and the new readiness endpoint is not compatible with 1.x).

2.x needs updated RBAC as well, for both UDPIngress permissions and the addition of finalizer edit permissions to most other resources.

I think we can try to have a central place for building 2.x manifests as well though need some engineering work under railgun/config to sort it out.

And, our controller is an extension manager, which watches resources from different extensions (CRD), an appropriate RBAC would be as following https:/Kong/kubernetes-ingress-controller/pull/1487/files#diff-e35bff7662f401f7147347e04d81e9a731e69c626ab6fb0130b037606ab7e397

@mflendrich
Copy link
Contributor Author

This is being reworked based on the considerations above.

@mflendrich mflendrich closed this Jul 9, 2021
@shaneutt shaneutt deleted the cleanup/railgun-manifests branch October 27, 2021 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants