Support Pod /32 IP request via k8s annotation (ipAddrsNoIpam) #264

gunjan5 · 2017-02-07T03:49:20Z

Part of issue #258

cni.projectcalico.org/ipAddrsNoIpam: "[\"10.0.0.1\"]"

If specified, this annotation ignores all other IPAM settings and will be stored in the Calico WorkloadEndpoint without calling any IPAM plugin, no questions asked. This allows non-CNI things to be the source of truth for IPAM.

tomdee · 2017-02-07T17:31:08Z

calico_cni_k8s_test.go

+					_, _, _, _, contAddresses, _, err := CreateContainer(netconfCalicoIPAM, name)
+					Expect(err).NotTo(HaveOccurred())
+
+					podIP := contAddresses[0].IP


What about checking what IP was stored in the workload endpoint in etcd?

tomdee · 2017-02-07T17:34:48Z

k8s/k8s.go

+			if visited6 >= 1 {
+				logger.Fatal("Can not have more than one IPv6 addresses in ipAddrsNoIpam annotation")
+			} else {
+				result.IP6.IP.IP = ipAddr


Do you really need the visited stuff? Why not just checkout result.IPx.IP or whatever?

tomdee · 2017-02-07T17:35:36Z

k8s/k8s.go

+		logger.WithField("annotation", ipAddrsNoIpam).Fatal("Invalid JSON")
+	}
+
+	result := types.Result{


Do you really need all of this for creating the Result?

Not sure I understand what you mean here. Can you elaborate?

I think I get what you're saying now. Done!

tomdee · 2017-02-07T17:35:48Z

k8s/k8s.go

+	// We also make sure there is only one IPv4 and/or one IPv6 passed in,
+	// since CNI spec only supports one of each right now.
+	for _, ip := range ips {
+


Extra blank line

caseydavenport · 2017-02-07T18:06:24Z

k8s/k8s.go

+			}
+			logger.Debugf("IPAM plugin returned: %+v", result)
+		} else {
+			// ipAddrsNoIpam annotation is set so bypass IPMA, and set the IPs manually.


caseydavenport · 2017-02-07T18:06:52Z

k8s/k8s.go

+		} else {
+			// ipAddrsNoIpam annotation is set so bypass IPMA, and set the IPs manually.
+			result = overrideIPAMResult(ipAddrsNoIpam, logger)
+			logger.Debugf("Bypassing IPAM to set the IP config to: %+v", result)


IP config -> result to be consistent.

caseydavenport · 2017-02-07T18:09:43Z

k8s/k8s.go

+
+	err := json.Unmarshal([]byte(ipAddrsNoIpam), &ips)
+	if err != nil {
+		logger.WithField("annotation", ipAddrsNoIpam).Fatal("Invalid JSON")


Could you make the error here more descriptive? It's user facing, so something like "failed to parse ipAddrsNoIpam as json"

caseydavenport · 2017-02-07T18:16:39Z

utils/types.go

+		AssignIpv6    *string  `json:"assign_ipv6"`
+		IPv4Pools     []string `json:"ipv4_pools,omitempty"`
+		IPv6Pools     []string `json:"ipv6_pools,omitempty"`
+		IPAddrsNoIpam []string `json:"ip_addrs_noipam,omitempty"`


I don't think this needs to be part of the CNI config file.

caseydavenport · 2017-02-07T18:25:22Z

k8s/k8s.go

+// overrideIPAMResult generates types.Result like the one produced by IPAM plugin,
+// but sets IP field manually since IPAM is bypassed with this annotation.
+// Example annotation:
+// cni.projectcalico.org/ipAddrsNoIpam: "[\"10.0.0.1\", \"2001:db8::1\"]"


The whole annotation isn't passed to this function, right? We should document the format of the string that gets passed to this function.

caseydavenport · 2017-02-07T18:31:19Z

k8s/k8s.go

+				return nil, err
+			}
+			logger.Debugf("IPAM plugin returned: %+v", result)
+		} else {


Are we worried about the fact that we do IPAM related stuff up above even when this annotation is specified?

e.g https://github.com/projectcalico/cni-plugin/pull/264/files#diff-b396ca928d5bd0e161acd283b56ead9fR84

and https://github.com/projectcalico/cni-plugin/pull/264/files#diff-b396ca928d5bd0e161acd283b56ead9fR124?

Probably not because it's a kinda weird case, but it does mean we'll be doing more work than we need to.

(discussed this offline. Going to leave this as it is for now. Will need to refactor how we call k8s APIs at some point. )

gunjan5 · 2017-02-07T23:55:13Z

@tomdee @caseydavenport addressed your comments. PTAL

caseydavenport · 2017-02-08T01:13:54Z

k8s/k8s.go

+
+	err := json.Unmarshal([]byte(ipAddrsNoIpam), &ips)
+	if err != nil {
+		logger.WithField("annotation", ipAddrsNoIpam).Fatal("Failed to parse ipAddrsNoIpam as json")


Should we be doing a log.Fatal here? Shouldn't this function return an error so we can pass back an error over CNI?

Same below.

caseydavenport · 2017-02-08T01:17:38Z

k8s/k8s.go

+
+	// annotation value can't be empty.
+	if len(ips) == 0 {
+		logger.WithField("annotation", "cni.projectcalico.org/ipAddrsNoIpam").Fatal("No IPs specified")


I always forget... should this actually be ipAddrsNoIPAM?

I think that's what you and @lxpollitt decided, but then later you had a suggestion of using ipAddrsSkipIPAM which I personally like better

caseydavenport · 2017-02-08T02:01:10Z

k8s/k8s.go

+	err := json.Unmarshal([]byte(ipAddrsNoIpam), &ips)
+	if err != nil {
+		logger.WithField("annotation", ipAddrsNoIpam).Error("Failed to parse ipAddrsNoIpam as json")
+		return nil, err


Would be nice to wrap these errors with context before returning - remember they all end up in the user's eyes via kubectl describe pod

e.g return nil, fmt.Errorf("Failed to parse ipAddrsNoIpam: %s", err)

Same goes for other returned errors.

done! BTW, this is why I think we should be using https://github.com/pkg/errors 🙂

caseydavenport · 2017-02-09T18:44:01Z

k8s/k8s.go

+		ipAddr := net.ParseIP(ip)
+		if ipAddr == nil {
+			logger.WithField("IP", ip).Error("Invalid IP format")
+			return nil, fmt.Errorf("Invalid IP format: %s", err)


It's this line that should include the IP in the returned error so that it's clear what the misconfig is.

caseydavenport · 2017-02-09T18:44:20Z

k8s/k8s.go

+			// If/when CNI spec supports more than one IP, we can loosen this requirement.
+			if result.IP6 != nil {
+				logger.Error("Can not have more than one IPv6 addresses in ipAddrsNoIpam annotation")
+				return nil, fmt.Errorf("Can not have more than one IPv6 addresses in ipAddrsNoIpam annotation")


typo: more than one IPv6 addresses

caseydavenport · 2017-02-09T18:44:36Z

k8s/k8s.go

+			// It's an IPv4 address.
+			if result.IP4 != nil {
+				logger.Error("Can not have more than one IPv4 addresses in ipAddrsNoIpam annotation")
+				return nil, fmt.Errorf("Can not have more than one IPv4 addresses in ipAddrsNoIpam annotation")


more than one IPv4 addresses (shouldn't be plural)

gunjan5 self-assigned this Feb 7, 2017

gunjan5 added this to the Calico v2.1.0 milestone Feb 7, 2017

gunjan5 mentioned this pull request Feb 7, 2017

[WIP] Support specifying IP with ipamOverride annotation #259

Closed

gunjan5 requested review from tomdee and caseydavenport February 7, 2017 16:57

gunjan5 assigned tomdee and caseydavenport and unassigned gunjan5 Feb 7, 2017

tomdee suggested changes Feb 7, 2017

View reviewed changes

caseydavenport reviewed Feb 7, 2017

View reviewed changes

ozdanborne changed the title ~~Support for annotation ipAddrsNoIpam to assign IP to pod bypassing IPAM~~ Support Pod /32 IP request via k8s annotation (ipAddrsNoIpam) Feb 7, 2017

caseydavenport reviewed Feb 8, 2017

View reviewed changes

tomdee approved these changes Feb 8, 2017

View reviewed changes

caseydavenport reviewed Feb 8, 2017

View reviewed changes

caseydavenport reviewed Feb 9, 2017

View reviewed changes

support for annotation ipAddrsNoIpam to assign IP to pod bypassing IPAM

d5cd7e4

gunjan5 force-pushed the ipAddrsNoIpam branch from 634f54d to d5cd7e4 Compare February 9, 2017 19:03

gunjan5 merged commit 701df28 into projectcalico:master Feb 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Pod /32 IP request via k8s annotation (ipAddrsNoIpam) #264

Support Pod /32 IP request via k8s annotation (ipAddrsNoIpam) #264

gunjan5 commented Feb 7, 2017 •

edited

tomdee Feb 7, 2017

tomdee Feb 7, 2017

tomdee Feb 7, 2017

gunjan5 Feb 7, 2017

gunjan5 Feb 7, 2017

tomdee Feb 7, 2017

caseydavenport Feb 7, 2017

caseydavenport Feb 7, 2017

caseydavenport Feb 7, 2017

caseydavenport Feb 7, 2017

caseydavenport Feb 7, 2017

caseydavenport Feb 7, 2017

gunjan5 Feb 7, 2017 •

edited

gunjan5 commented Feb 7, 2017

caseydavenport Feb 8, 2017

caseydavenport Feb 8, 2017

caseydavenport Feb 8, 2017

gunjan5 Feb 8, 2017

caseydavenport Feb 8, 2017 •

edited

gunjan5 Feb 8, 2017

caseydavenport Feb 9, 2017

caseydavenport Feb 9, 2017

caseydavenport Feb 9, 2017

Support Pod /32 IP request via k8s annotation (ipAddrsNoIpam) #264

Support Pod /32 IP request via k8s annotation (ipAddrsNoIpam) #264

Conversation

gunjan5 commented Feb 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunjan5 Feb 7, 2017 • edited

Choose a reason for hiding this comment

gunjan5 commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caseydavenport Feb 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunjan5 commented Feb 7, 2017 •

edited

gunjan5 Feb 7, 2017 •

edited

caseydavenport Feb 8, 2017 •

edited