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

Implement Cilium Ingress #15795

Merged
merged 4 commits into from
Sep 10, 2023
Merged

Implement Cilium Ingress #15795

merged 4 commits into from
Sep 10, 2023

Conversation

zadjadr
Copy link
Contributor

@zadjadr zadjadr commented Aug 17, 2023

Fixes #15784

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 17, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @zadjadr. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@zadjadr zadjadr force-pushed the feature/cilium-ingress branch 2 times, most recently from 14408d0 to f4d5a8d Compare August 17, 2023 23:08
@hakman
Copy link
Member

hakman commented Aug 18, 2023

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 18, 2023
olemarkus
olemarkus requested changes Aug 21, 2023
View reviewed changes
pkg/apis/kops/validation/validation.go Show resolved Hide resolved
pkg/model/components/cilium.go Outdated Show resolved Hide resolved
pkg/apis/kops/v1alpha3/networking.go Outdated Show resolved Hide resolved
pkg/model/components/cilium.go Outdated Show resolved Hide resolved
@olemarkus
Copy link
Member

/cc @johngmyers

@zadjadr
Copy link
Contributor Author

zadjadr commented Aug 23, 2023 •
edited
Loading

@olemarkus I have added all your suggestions here (except for the possible check on shared lb name): 16a7b9b

I'll squash the commits if you're happy with the changes.

hakman
hakman requested changes Aug 23, 2023
View reviewed changes
pkg/model/components/cilium.go Outdated Show resolved Hide resolved
@zadjadr
Copy link
Contributor Author

zadjadr commented Aug 23, 2023

/retest

olemarkus
olemarkus requested changes Aug 25, 2023
View reviewed changes
pkg/model/components/cilium.go Outdated
c.Ingress.EnableSecretsSync = fi.PtrTo(true)
}
if c.Ingress.LoadBalancerAnnotationPrefixes == "" {
c.Ingress.LoadBalancerAnnotationPrefixes = "service.beta.kubernetes.io service.kubernetes.io cloud.google.com"
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't cilium have a default here if we leave it blank?
I think it would be good to just leave this one unset unless configured. That way we don't have to e.g remove the beta annotation if cilium changes default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where can I read up on that? I know the helm chart sets a default value in the configmap, but no idea if cilium will set it if not set in the configmap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay found the default options here: https://pkg.go.dev/github.com/cilium/cilium/operator/option

I'll remove the stuff that can be left blank.

pkg/model/components/cilium.go Outdated
c.Ingress.DefaultLoadBalancerMode = "dedicated"
}
if c.Ingress.SharedLoadBalancerServiceName == "" {
c.Ingress.SharedLoadBalancerServiceName = "cilium-ingress"
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, if cilium provides a default value, we don't have to explicitly set a value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where did you find the default value set by cilium? I can't find it anywhere in https://pkg.go.dev/github.com/cilium/cilium/operator/option or in https://github.com/cilium/cilium/tree/main/pkg/defaults

Copy link
Member

Choose a reason for hiding this comment

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

I think these are the ones: https://github.com/cilium/cilium/blob/main/operator/pkg/ingress/ingress_options.go#L21-L31.

upup/models/cloudup/resources/addons/networking.cilium.io/k8s-1.16-v1.13.yaml.template
name: "cilium-operator"
namespace: kube-system
---
# Source: cilium/templates/cilium-ingress-service.yaml
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this one only be created if one is using the shared loadbalancer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Atleast the helm chart always creates these if ingress is enabled: https://github.com/cilium/cilium/blob/v1.14.1/install/kubernetes/cilium/templates/cilium-ingress-service.yaml

But it makes sense to disable this behavior in kops. Should we go against the helm chart here?

By the way, just curious about this: why do we not create a "values.yaml" and generate the entire cilium k8s-1.16-v1.13.yaml.template via helms go library?

Copy link
Member

Choose a reason for hiding this comment

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

That may be an interesting approach for the future.

upup/models/cloudup/resources/addons/networking.cilium.io/k8s-1.16-v1.13.yaml.template
nodePort:
type: LoadBalancer
---
# Source: cilium/templates/cilium-ingress-service.yaml
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 10, 2023
zadjadr and others added 2 commits September 10, 2023 09:05
@zadjadr
Populate ingress spec values only when needed
592e84d 
Co-authored-by: Ciprian Hacman <ciprian@hakman.dev>

fmt
@zadjadr
hack/update-expected.sh
47919e5
hakman
hakman approved these changes Sep 10, 2023
View reviewed changes
Copy link
Member

@hakman hakman left a comment

Choose a reason for hiding this comment

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

Thanks @zadjadr, LGTM!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 10, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 10, 2023
@k8s-ci-robot k8s-ci-robot merged commit 0b432e4 into kubernetes:master Sep 10, 2023
8 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.29 milestone Sep 10, 2023
@hakman hakman modified the milestones: v1.29, v1.28 Sep 10, 2023
@hakman hakman mentioned this pull request Sep 10, 2023
Closed
@zadjadr zadjadr deleted the feature/cilium-ingress branch September 10, 2023 21:55
@BrewTestBot BrewTestBot mentioned this pull request May 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olemarkus olemarkus olemarkus requested changes

@hakman hakman hakman approved these changes

@johngmyers johngmyers Awaiting requested review from johngmyers

Assignees

@olemarkus olemarkus

@hakman hakman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/addons area/api cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.28
Development

Successfully merging this pull request may close these issues.

Add Cilium Ingress suport
4 participants
@zadjadr @k8s-ci-robot @hakman @olemarkus