New feature: client's certificate-based authentication to Infoblox NIOS server #2841

skudriavtsev · 2022-06-22T20:29:28Z

Description

This PR contains changes for a new feature: client's certificate authentication to Infoblox NIOS server. And some bugfixes as well, including the fix for the issue #2198

Fixes #2198

Checklist

Unit tests updated
End user documentation updated

k8s-ci-robot · 2022-06-22T20:29:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: skudriavtsev
To complete the pull request process, please assign seanmalloy after the PR has been reviewed.
You can assign the PR to them by writing /assign @seanmalloy in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

skudriavtsev · 2022-06-27T07:00:40Z

By the way, I would like to put some example YAML-deployment files for Infoblox provider and I found no separate directory for that. Probably I missed something in the project repository's rules ...

To the reviewers: which place of the repository should I put the example files in?

ranjishmp · 2022-07-06T12:04:36Z

Hi @Raffo / @njuettner, Can you please review this PR and share your comments ?

* TestInfobloxRecords and TestInfobloxRecordsReverse tests performed the same actions. To reduce the amount of test-logic, merged them into one table-driven test. * Added comments for the test function and for each respective test case.

linux-foundation-easycla · 2022-07-06T15:08:30Z

The committers listed above are authorized under a signed CLA.

✅ login: skudriavtsev / name: Sergey Kudriavtsev (f124f6a, 0ad789d)
✅ login: achernevskii / name: Aleksei Chernevskii (cfda29a, ba94532)

ranjishmp · 2022-07-12T06:10:17Z

Hi @Raffo / @njuettner, it will be really great if you can provide your review comments

k8s-ci-robot · 2022-08-18T18:49:48Z

@skudriavtsev: PR needs rebase.

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.

ranjishmp · 2022-09-12T04:20:44Z

@skudriavtsev Please rebase the PR

alebedev87

Thanks for the PR, @skudriavtsev !
I left some comments/questions, hope they'll be helpful.
Didn't look at the unit test part yet, will do later after my questions are clarified.

alebedev87 · 2022-10-04T20:25:47Z

pkg/apis/externaldns/types.go

@@ -251,11 +253,13 @@ var defaultConfig = &Config{
 	AkamaiEdgercPath:            "",
 	InfobloxGridHost:            "",
 	InfobloxWapiPort:            443,
-	InfobloxWapiUsername:        "admin",


This may break the existing configurations. I agree that empty user would be more generic choice at the very beginning but admin seems to be a reasonable default.

This makes not a great sense with an empty password. But need to think of it more, this is one of the controversial points.

alebedev87 · 2022-10-04T20:34:46Z

pkg/apis/externaldns/validation/validation.go

+		if cfg.InfobloxWapiPassword == "" && cfg.InfobloxClientKey == "" && cfg.InfobloxClientCert == "" {
+			return errors.New("neither Infoblox WAPI password nor paths to public/private parts of TLS client certificate specified")


This condition allows having client certificate without the key or the other way around: client key without certificate. I think we want both parts for the client certificate auth if the password is empty.

Yes, you are right.

alebedev87 · 2022-10-04T20:48:01Z

provider/infoblox/infoblox.go

+	} else {
+		return nil, fmt.Errorf("neither client-certificate nor username and password are specified for authentication to Infoblox NIOS server")
+	}


This check is redundant, the config validation already checked this in main.go.

Agree, just one more over-insurance. I will remove it.

alebedev87 · 2022-10-04T21:28:43Z

provider/infoblox/infoblox.go

+		authMethodCert     bool
+	)
+	if strings.Trim(ibStartupCfg.View, " \a\b\t\n\r\f\v") == "" {
+		return nil, fmt.Errorf("non-empty DNS view's name is required")


Why the DNS view is now required? It was working without it even when ibclient version 2 was already in use.

This is mostly not about the default value. It is about the case when a user specifies formally non-empty string which actually contains no visible characters (ex. only spaces as a result of incorrect work of automation scripts). Treat it as over-insurance :-)

For now, let's leave the default DNS view as 'default'. Under discussion, see below.

alebedev87 · 2022-10-05T08:29:39Z

provider/infoblox/infoblox.go

@@ -271,28 +325,38 @@ func (p *ProviderConfig) Records(ctx context.Context) (endpoints []*endpoint.End
 			// example: 10.196.38.0/24 becomes 38.196.10.in-addr.arpa
 			arpaZone, err := transform.ReverseDomainName(zone.Fqdn)
 			if err == nil {
-				var resP []ibclient.RecordPTR
+				var resPtrStatic, resPtrDynamic []ibclient.RecordPTR


Why are we filtering SYSTEM PTR records?

If we do some filtering why do we care for DYNAMIC records? can they even be created by ExternalDNS?

Why don't we do the similar filtering (by the record creator) for the other record types (A, CNAME)?

alebedev87 · 2022-10-05T10:14:07Z

provider/infoblox/infoblox.go

@@ -546,11 +631,26 @@ func (p *ProviderConfig) recordSet(ep *endpoint.Endpoint, getObject bool, target
 		obj.PtrdName = ep.DNSName
 		obj.Ipv4Addr = ep.Targets[targetIndex]
 		obj.View = p.view
+		sf := map[string]string{


What does sf stand for? qp used in ther other parts of the PR looks more intuitive to me.

qp = query parameters
sf = search fields, actually a part of QP.

alebedev87 · 2022-10-05T10:20:36Z

provider/infoblox/infoblox.go

+				expTypeA := ibclient.NewEmptyRecordA().ObjectType()
+				expTypeTXT := ibclient.NewEmptyRecordTXT().ObjectType()
+				expTypePTR := ibclient.NewEmptyRecordPTR().ObjectType()
+				expTypeCNAME := ibclient.NewEmptyRecordCNAME().ObjectType()


It doesn't make much sense to create those variable for each for loop iteration. I think that they are better to be defined in const section at the very top of the file.

It is fine (better) to move them out of loops but they cannot be easily defined as constants.

Yes true, they are computed at runtime, so they cannot be constants. var section would be ok too, my thought was to move avoid their creation at each iteration.

alebedev87 · 2022-10-05T10:22:09Z

provider/infoblox/infoblox.go

+				expTypePTR := ibclient.NewEmptyRecordPTR().ObjectType()
+				expTypeCNAME := ibclient.NewEmptyRecordCNAME().ObjectType()
+				actType := recordSet.obj.ObjectType()
+				if actType == expTypeA {


More go idiomatic would be to replace these if/elses with a switch.

alebedev87 · 2022-10-05T10:26:24Z

docs/tutorials/infoblox.md

@@ -16,6 +16,8 @@ export WAPI_USERNAME=admin
 export WAPI_PASSWORD=infoblox
 ```

+**Warning**: Grid's Cloud platform member is not supported yet as a target host to be used by eDNS plugin.


Suggested change

**Warning**: Grid's Cloud platform member is not supported yet as a target host to be used by eDNS plugin.

**Warning**: Grid's Cloud platform member is not supported yet as a target host to be used by ExternalDNS plugin.

alebedev87 · 2022-10-05T10:29:31Z

docs/tutorials/infoblox.md

 Now external-dns will manage PTR records for you.
+
+**Important note 1**: you may expect that the reverse-mapping zone must be in the
+form of a subdomain of the 'in-addr.arpa' domain, but in case of eDNS's Infoblox provider and


Suggested change

form of a subdomain of the 'in-addr.arpa' domain, but in case of eDNS's Infoblox provider and

form of a subdomain of the 'in-addr.arpa' domain, but in case of ExternalDNS's Infoblox provider and

skudriavtsev · 2022-10-06T07:45:53Z

@alebedev87, thank you for your review, I will address the comments but today I am going to split this PR into 2-3 separate PRs instead of re-basing this one. That is why I just answered 'agree' instead of just accepting your proposals. I will update this PR with exact links.

P.S.: I guess, I will add more comments to the code.

skudriavtsev · 2022-10-06T14:58:13Z

replaced by new PRs: #3067 and skudriavtsev#4 (which will be rebased to the original repo after the 1-st PR will be merged)

Merge branch master into client-cert-auth

f124f6a

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 22, 2022

k8s-ci-robot requested review from njuettner and Raffo June 22, 2022 20:29

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 22, 2022

skudriavtsev changed the title ~~Merge branch master into client-cert-auth~~ New feature: client's certificate-based authentication to InfobloxNIOS server Jun 22, 2022

skudriavtsev changed the title ~~New feature: client's certificate-based authentication to InfobloxNIOS server~~ New feature: client's certificate-based authentication to Infoblox NIOS server Jun 22, 2022

code changes to satisfy CI-linter

0ad789d

This was referenced Jun 27, 2022

Add maintainers for Infoblox provider #2675

Closed

Add infoblox maintainers #2713

Closed

achernevskii added 2 commits July 6, 2022 18:08

Created table-driven tests for the Infoblox provider ApplyChanges method

ba94532

skalpin mentioned this pull request Aug 11, 2022

Infoblox provider deletes all fetched records if any DNS record is updated #2931

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2022

alebedev87 reviewed Oct 5, 2022

View reviewed changes

skudriavtsev closed this Oct 6, 2022

ranjishmp mentioned this pull request Oct 18, 2022

Infoblox provider: bugfixes #3067

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New feature: client's certificate-based authentication to Infoblox NIOS server #2841

New feature: client's certificate-based authentication to Infoblox NIOS server #2841

skudriavtsev commented Jun 22, 2022

k8s-ci-robot commented Jun 22, 2022

skudriavtsev commented Jun 27, 2022

ranjishmp commented Jul 6, 2022

linux-foundation-easycla bot commented Jul 6, 2022 •

edited

Loading

ranjishmp commented Jul 12, 2022

k8s-ci-robot commented Aug 18, 2022

ranjishmp commented Sep 12, 2022

alebedev87 left a comment •

edited

Loading

alebedev87 Oct 4, 2022

skudriavtsev Oct 6, 2022

alebedev87 Oct 4, 2022

skudriavtsev Oct 6, 2022

alebedev87 Oct 4, 2022

skudriavtsev Oct 6, 2022

alebedev87 Oct 4, 2022

skudriavtsev Oct 6, 2022

alebedev87 Oct 5, 2022

alebedev87 Oct 5, 2022

skudriavtsev Oct 6, 2022

alebedev87 Oct 5, 2022

skudriavtsev Oct 6, 2022 •

edited

Loading

alebedev87 Oct 18, 2022

alebedev87 Oct 5, 2022

alebedev87 Oct 5, 2022

skudriavtsev Oct 6, 2022 •

edited

Loading

alebedev87 Oct 5, 2022

skudriavtsev Oct 6, 2022

skudriavtsev commented Oct 6, 2022 •

edited

Loading

skudriavtsev commented Oct 6, 2022

		if cfg.InfobloxWapiPassword == "" && cfg.InfobloxClientKey == "" && cfg.InfobloxClientCert == "" {
		return errors.New("neither Infoblox WAPI password nor paths to public/private parts of TLS client certificate specified")

	Warning: Grid's Cloud platform member is not supported yet as a target host to be used by eDNS plugin.
	Warning: Grid's Cloud platform member is not supported yet as a target host to be used by ExternalDNS plugin.

	form of a subdomain of the 'in-addr.arpa' domain, but in case of eDNS's Infoblox provider and
	form of a subdomain of the 'in-addr.arpa' domain, but in case of ExternalDNS's Infoblox provider and

New feature: client's certificate-based authentication to Infoblox NIOS server #2841

New feature: client's certificate-based authentication to Infoblox NIOS server #2841

Conversation

skudriavtsev commented Jun 22, 2022

k8s-ci-robot commented Jun 22, 2022

skudriavtsev commented Jun 27, 2022

ranjishmp commented Jul 6, 2022

linux-foundation-easycla bot commented Jul 6, 2022 • edited Loading

ranjishmp commented Jul 12, 2022

k8s-ci-robot commented Aug 18, 2022

ranjishmp commented Sep 12, 2022

alebedev87 left a comment • edited Loading

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

Choose a reason for hiding this comment

skudriavtsev Oct 6, 2022 • edited Loading

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

skudriavtsev Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skudriavtsev commented Oct 6, 2022 • edited Loading

skudriavtsev commented Oct 6, 2022

linux-foundation-easycla bot commented Jul 6, 2022 •

edited

Loading

alebedev87 left a comment •

edited

Loading

skudriavtsev Oct 6, 2022 •

edited

Loading

skudriavtsev Oct 6, 2022 •

edited

Loading

skudriavtsev commented Oct 6, 2022 •

edited

Loading