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

feat: add kubernetes discovery module #4880

Merged
merged 78 commits into from
Feb 23, 2022
Merged

feat: add kubernetes discovery module #4880

merged 78 commits into from
Feb 23, 2022

Conversation

zhixiongdu027
Copy link
Contributor

@zhixiongdu027 zhixiongdu027 commented Aug 23, 2021

Signed-off-by: adugeek root@libssl.com

What this PR does / why we need it:

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

#4388

todo:

  • add docs [done]
  • support namespace-selector [done]
  • support label-selector [done]

Signed-off-by: adugeek <root@libssl.com>
apisix/discovery/k8s.lua Outdated Show resolved Hide resolved
apisix/discovery/k8s.lua Outdated Show resolved Hide resolved
apisix/discovery/k8s.lua Outdated Show resolved Hide resolved
@tao12345666333 tao12345666333 self-assigned this Aug 24, 2021
@tao12345666333
Copy link
Member

Thanks for your contribution, I will review it today

zhixiogndu added 2 commits September 5, 2021 20:09
Signed-off-by: adugeek <root@libssl.com>
Signed-off-by: adugeek <root@libssl.com>
t/APISIX.pm Outdated Show resolved Hide resolved
Signed-off-by: adugeek <root@libssl.com>
zhixiogndu added 2 commits September 6, 2021 23:46
Signed-off-by: adugeek <root@libssl.com>
Signed-off-by: adugeek <root@libssl.com>
@tzssangglass
Copy link
Member

If you want to use environment variables in code, you can use: os.getenv()

If you want to use environment variables in test cases, you can

zhixiogndu added 4 commits September 8, 2021 08:32
Signed-off-by: adugeek <root@libssl.com>
Signed-off-by: adugeek <root@libssl.com>
Signed-off-by: adugeek <root@libssl.com>
Signed-off-by: adugeek <root@libssl.com>
adugeek added 3 commits September 19, 2021 15:08
Signed-off-by: adugeek <root@libssl.com>
Signed-off-by: adugeek <root@libssl.com>
Copy link
Contributor Author

@zhixiongdu027 zhixiongdu027 left a comment

Choose a reason for hiding this comment

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

ci file is updated for mount kubernetes token file into centos container

apisix/discovery/k8s.lua Outdated Show resolved Hide resolved
curl -Lo ./jq https://github.com/stedolan/jq/releases/download/jq-1.6/jq-linux64
chmod +x ./jq

until [[ $(curl 127.0.0.1:6445/api/v1/pods?fieldSelector=status.phase%21%3DRunning |./jq .items) == "[]" ]]; do
Copy link
Member

Choose a reason for hiding this comment

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

you can just using kubectl wait --for=condition=Ready pods --all -A instead of this.

Copy link
Contributor Author

@zhixiongdu027 zhixiongdu027 Sep 20, 2021

Choose a reason for hiding this comment

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

In my test
When kubectl wait --for=condition=Ready pods --all -A is finished,
Still have pod Pending or ContainerCreating
This will cause the test to fail

@tao12345666333

Copy link
Contributor Author

@zhixiongdu027 zhixiongdu027 Sep 20, 2021

Choose a reason for hiding this comment

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

maybe we can use like this:

echo "wait k8s start..."
sleep 10
until [[ $(./kubectl get pods -A --field-selector 'status.phase!=Running' 2>&1) =~ "No resources found" ]]; do
  echo 'still wait k8s start...'
  sleep 1
done

ci/install-ext-services-via-docker.sh Outdated Show resolved Hide resolved
Signed-off-by: adugeek <root@libssl.com>
-- TODO: maybe we can read dict name from discovery config
endpoint_dict = ngx.shared.discovery
if not endpoint_dict then
error("failed to get ngx.shared.dict discovery")
Copy link
Member

Choose a reason for hiding this comment

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

failed to get Nginx shared dict: discovery, please check your APISIX version

membphis
membphis previously approved these changes Feb 9, 2022
@zhixiongdu027
Copy link
Contributor Author

@membphis @spacewander @tao12345666333 @crazyMonkey1995
Hi, I think the current code is more in line with the apisix style.
Please help start ci and reviews.
Many THK.

@tao12345666333
Copy link
Member

ok, thanks for your contribution.


informer.continue = data.metadata.continue
if informer.continue and informer.continue ~= "" then
list(httpc, informer)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the apiserver(the second parameter of the function) is missing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, It'a mistake

@zhixiongdu027
Copy link
Contributor Author

zhixiongdu027 commented Feb 14, 2022

@crazyMonkey1995
HI, tks review verymuch .and mistake had fixed.
Please help review again.

@tzssangglass
Copy link
Member

cc @tao12345666333 @membphis @spacewander @tokers @crazyMonkey1995

Looks like this PR is complete?

tzssangglass
tzssangglass previously approved these changes Feb 16, 2022
@tangzhenhuang
Copy link
Contributor

cc @tao12345666333 @membphis @spacewander @tokers @crazyMonkey1995

Looks like this PR is complete?

I think it's ok.

@tao12345666333
Copy link
Member

It's on my list. I will review it ASAP

membphis
membphis previously approved these changes Feb 16, 2022
Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@tao12345666333 tao12345666333 left a comment

Choose a reason for hiding this comment

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

LGTM

Wouldn't it be better if namespaces were not directly hardcoded in the configuration file, but filtered by labels?

docs/zh/latest/discovery/kubernetes.md Outdated Show resolved Hide resolved
schema: https #default https

# kubernetes apiserver host, options [ ipv4 | ipv6 | domain | env variable]
host: 10.0.8.95 #default ${KUBERNETES_SERVICE_HOST}
Copy link
Member

Choose a reason for hiding this comment

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

I suggest writing kubernetes.default directly here, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

In cluster

Copy link
Member

Choose a reason for hiding this comment

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

@zhixiongdu027 what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We may have two operating environments:

Inside the Pod:
Both the apiserver host and apiserver port values are stored via environment variables.
Therefore, our configuration items must support the reference environment variable value, and the user needs to recognize the referenced environment variable value at a glance.
${} is a good form

Outside the Pod:
The apiserver host and apiserver port values are manually input by the user. We need a specific format to distinguish whether the value entered by the user is a domain name or an environment variable,
${} is also a good form

If we have next config:

 kubernetes:
    service:
       host:  kubernets.default

How can we tell if "kubernets.default" is a constant or a domain name?

Copy link
Member

Choose a reason for hiding this comment

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

In the cluster we can get the address of the kubernetes service through KUBERNETES_SERVICE_HOST, but once we really need to verify the SSL certificate, most of the time we do not issue a certificate for the IP address. Domain names are far more generic than IP addresses.

Copy link
Contributor Author

@zhixiongdu027 zhixiongdu027 Feb 19, 2022

Choose a reason for hiding this comment

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

most of the time we do not issue a certificate for the IP address. Domain names are far more generic than IP addresses.

Usually this is the case.

But in the clusters I have seen, the san list of the certificates used by the apiserver all contain${KUBERNETES_SERVICE_HOST} and Host_IP,

so even if "ssl_verify=true "
I think there is no problem to use ${KUBERNETES_SERVICE_HOST} as the server address within the Pod.

maybe you know some exception?

@tao12345666333

Copy link
Member

Choose a reason for hiding this comment

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

We can keep the status for now.

@zhixiongdu027
Copy link
Contributor Author

@membphis @tao12345666333 @crazyMonkey1995 @tokers
Hi ,doc had update.
Please help start ci and review again .
Many TKS.

Copy link
Member

@tao12345666333 tao12345666333 left a comment

Choose a reason for hiding this comment

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

We can keep this implementation for now, let's move on.

@jianhaiqing
Copy link

it's a big move. We keep eyes on this feature.

@spacewander spacewander merged commit f7b50f2 into apache:master Feb 23, 2022
hongbinhsu pushed a commit to fitphp/apix that referenced this pull request Feb 23, 2022
* upstream: (52 commits)
  feat: add kubernetes discovery module (apache#4880)
  docs: fix For L7 proxy -> For L4 proxy (apache#6423)
  fix(deps): upgrade jsonschema to 0.9.8 (apache#6407)
  docs: translate Chinese to English in en clickhouse-logger (apache#6416)
  docs: add zh proxy-control.md&modify other doc error (apache#6346)
  docs: update public API relative usage (apache#6318)
  docs(cn): remove datadog from sidebar & fix doc lint conf (apache#6411)
  fix(request-validation): should not limit the urlencoded post args number (apache#6396)
  docs: fix configuration file typo (apache#6395)
  docs(extern-plugin): the implementation of runner (apache#6336)
  docs: polishing skywalking-logger plugin's docs (apache#6377)
  doc: adjust the directory structure of observability's documents (apache#6391)
  change(admin): empty nodes should be encoded as array (apache#6384)
  fix: should not limit the header number (apache#6379)
  ci: remove unnecessary tmate action (apache#6367)
  fix(opentelemetry): batch_span_processor export zero length spans (apache#6349)
  feat(graphql): support http get and post json request (apache#6343)
  feat: support for configuring the number of etcd health check retries (apache#6322)
  feat(wasm): support getting request body (apache#6325)
  fix(hmac-auth): hmac-auth plugin sort array param (apache#6314)
  ...
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.