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

Adding ability to define a floating ip for a kubernetes workload via annotation #419

Merged
merged 1 commit into from Sep 18, 2018

Conversation

Projects
None yet
3 participants
@bradbeam
Contributor

bradbeam commented Nov 14, 2017

Description

This PR adds the ability to define DNAT/floating IP for a kubernetes workload via annotation. Most of the plumbing was already in place, this adds the missing link.

Todos

  • Tests
  • Documentation
  • Release note

Release Note

You can now define a floating IP ( DNAT ) via container annotation `cni.projectcalico.org/floatingIPs`
@fasaxc

Thanks for the submission and for adding a unit test. I spotted a couple of minor issues. Please can you fix those up?

Ideally, we'd like to have an end-to-end test of this function that test that packet actually flow as expected. Is that something you'd be able to add (in the calico repo).

Context("using floatingIPs annotation to assign a DNAT", func() {
It("successfully assigns a DNAT IP address from the annotated floatingIP", func() {
netconfCalicoIPAM := fmt.Sprintf(`
{

This comment has been minimized.

@fasaxc

fasaxc Feb 5, 2018

Member

The formatting looks a bit odd here, please can you fix up and run goimports on the code?

This comment has been minimized.

@bradbeam

bradbeam Feb 7, 2018

Contributor

Looks like it gets left unformatted since it's part of a string. I'll fix it up.

k8s/k8s.go Outdated

for _, ip := range ips {
endpoint.Spec.IPNATs = append(endpoint.Spec.IPNATs, api.IPNAT{
InternalIP: result.IPs[0].Address.String(),

This comment has been minimized.

@fasaxc

fasaxc Feb 5, 2018

Member

I think the internal IP should be a bare IP address, not a /32 or /128. IIRC, it might be enough to do IPs[0].Address.IP.String()

This comment has been minimized.

@bradbeam

bradbeam Feb 7, 2018

Contributor

Yeah, looks like libcalico spec changed since I initially put this up. Updated.

@fasaxc fasaxc self-assigned this Feb 5, 2018

@fasaxc

This comment has been minimized.

Member

fasaxc commented Feb 6, 2018

@caseydavenport reminded me that this change will only cover our etcd mode, please can you add support for the kubernetes datastore driver too (where we directly map Pods to WorkloadEndpoints in the datastore layer)?

That means adding similar logic to this function to copy the annotation into the WorkloadEndpoint: https://github.com/projectcalico/libcalico-go/blob/master/lib/backend/k8s/conversion/conversion.go#L171

@bradbeam

This comment has been minimized.

Contributor

bradbeam commented May 15, 2018

Bump

@bradbeam bradbeam force-pushed the bradbeam:fipannot branch from 5b141b7 to 5ef7f34 Jun 14, 2018

@bradbeam bradbeam force-pushed the bradbeam:fipannot branch from 5ef7f34 to fd20be8 Sep 13, 2018

@bradbeam bradbeam force-pushed the bradbeam:fipannot branch from fd20be8 to dfecba9 Sep 13, 2018

@bradbeam

This comment has been minimized.

Contributor

bradbeam commented Sep 13, 2018

@fasaxc think we're ready ( finally ), along with projectcalico/libcalico-go#781

@caseydavenport caseydavenport added this to the Calico v3.3.0 milestone Sep 18, 2018

@caseydavenport caseydavenport merged commit 0e52590 into projectcalico:master Sep 18, 2018

2 checks passed

license/cla Contributor License Agreement is signed.
Details
semaphoreci The build passed on Semaphore.
Details

@bradbeam bradbeam deleted the bradbeam:fipannot branch Sep 18, 2018

@caseydavenport caseydavenport referenced this pull request Sep 27, 2018

Merged

Disable floating IPs in k8s by default #605

0 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment