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

Helm chart updated for ExternalDNS, rfc2136 provider, in order to support GSS-TSIG authentication configuration. #1064

Merged
merged 13 commits into from
Dec 18, 2023

Conversation

v-esteves
Copy link
Contributor

@v-esteves v-esteves commented Feb 13, 2023

Changed helm template for external-dns as well as the values.yaml and values.schema.json to support several authentication types in the provider RFC2136.

The previous version only supported RFC2136 configuration for TSIG, which doesn't work with Windows DNS.

Fixes #1061
Fixes #929

Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

hi @v-esteves , thanks a lot for this contribution!

Could you please extend documentation briefly describing this functionality?

Please follow the steps described in https://github.com/k8gb-io/k8gb/blob/master/CONTRIBUTING.md#documentation

Was this change tested end-to-end? If yes, please describe how in the commit message

Please also comply with DCO by signing your commits with git commit -s

Thanks a ton!

name: kerberos-config-volume
subPath: krb5.conf
{{- end }}
{{- if .Values.rfc2136.rfc2136auth.gssTsig.enabled }}
Copy link
Member

@jkremser jkremser Feb 17, 2023

Choose a reason for hiding this comment

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

can't this be simplified to one block? The condition is the same as the previous one, is there an issue with indentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. I was leaving room for future updates, that are not only related with the required configuration for GSS-TSIG, but for the moment we can simplify and set everything in one block.
I'll do that right away.

Copy link
Member

@jkremser jkremser left a comment

Choose a reason for hiding this comment

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

these changes are also part of your other pr #1065 right?

Please name the pull request better. 1061 fix is very cryptic :D

@v-esteves v-esteves changed the title 1061 fix 1061 - Helm chart updated for ExternalDNS, rfc2126 provider, in order to support GSS-TSIG authentication configuration. Mar 23, 2023
Vitor Esteves and others added 3 commits March 27, 2023 12:27
…ation for ExternalDNS

Signed-off-by: vestevesaws@gmail.com <x191116@MacBook-Pro-de-Vitor.local>
… for kerberos configuration file from config-map

Signed-off-by: vestevesaws@gmail.com <x191116@MacBook-Pro-de-Vitor.local>
…he ConfigMap with a krb5.conf configuration for Kerberos authentication (GSS-TSIG). Also updated the values.schema.json and values.yaml for this new input. README.md was updated with the correct description of the values.yaml structure with the new inputs for the rfc2136 provider configuration

Signed-off-by: vestevesaws@gmail.com <x191116@MBP-de-Vitor.home>
Signed-off-by: vestevesaws@gmail.com <x191116@MacBook-Pro-de-Vitor.local>
Changed to allow the configuration of the provider RFC2136 with GSS-TSIG

Fixes k8gb-io#1061

Signed-off-by: vestevesaws@gmail.com <x191116@MacBook-Pro-de-Vitor.local>
@v-esteves v-esteves requested review from ytsarev and jkremser and removed request for somaritane, k0da, donovanmuller and kuritka March 27, 2023 13:50
@netlify
Copy link

netlify bot commented Mar 27, 2023

Deploy Preview for k8gb-preview ready!

Name Link
🔨 Latest commit ff6939e
🔍 Latest deploy log https://app.netlify.com/sites/k8gb-preview/deploys/657b05ad02568400084ed41d
😎 Deploy Preview https://deploy-preview-1064--k8gb-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-advanced-security
Copy link

You have successfully added a new terrascan configuration .github/workflows/terrascan.yaml:terrascan. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

left minor doc comments, thanks a lot for the contribution

docs/provider_rfc2136.md Show resolved Hide resolved
| rfc2136.rfc2136auth.gssTsig.kerberosConfigMap | string | `"kerberos-configmap"` | When using GSS-TSIG, a ConfigMap with a valid krb5.conf configuration should be provided |
| rfc2136.rfc2136auth.gssTsig.gssTsigCreds[0].kerberos-username | string | `"ad-user-account"` | AD user account with permissions for DNS updates |
| rfc2136.rfc2136auth.gssTsig.gssTsigCreds[1].kerberos-password | string | `"ad-user-account-password"` | Passowrd of the AD user account |
| rfc2136.rfc2136auth.gssTsig.gssTsigCreds[2].kerberos-realm | string | `"REALM.DOMAIN"` | Kerberos REALM that should be used for authentication |
Copy link
Member

Choose a reason for hiding this comment

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

Looks like helm values are duplicated with the main README https://github.com/k8gb-io/k8gb/pull/1064/files#diff-a9751a1d587556fed5161a73d263258bfeb7b5c4632430d9ec0d853529539aa9R98. ? Should we avoid duplication to keep it at the same place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed those duplicated values and added a reference to the readme with the Helm values description. This way there isn't duplicated text and updates are only in one place.

@ytsarev ytsarev changed the title 1061 - Helm chart updated for ExternalDNS, rfc2126 provider, in order to support GSS-TSIG authentication configuration. 1061 - Helm chart updated for ExternalDNS, rfc2136 provider, in order to support GSS-TSIG authentication configuration. Apr 12, 2023
…ed duplicaded Helm Values from RFC2136 provider tutorial.

Signed-off-by: vestevesaws@gmail.com <x191116@MacBook-Pro-de-Vitor.local>
@v-esteves v-esteves requested a review from ytsarev August 9, 2023 15:48
@ytsarev
Copy link
Member

ytsarev commented Aug 10, 2023

@v-esteves thanks a lot for the consolidation, it really helps! Can you please resolve the conflict in README.md?

@ytsarev ytsarev changed the title 1061 - Helm chart updated for ExternalDNS, rfc2136 provider, in order to support GSS-TSIG authentication configuration. Helm chart updated for ExternalDNS, rfc2136 provider, in order to support GSS-TSIG authentication configuration. Aug 10, 2023
vestevesaws@gmail.com and others added 2 commits August 10, 2023 10:41
…lues.

Signed-off-by: vestevesaws@gmail.com <x191116@MBP-de-Vitor.lan>
Signed-off-by: Vitor Esteves <64093608+v-esteves@users.noreply.github.com>
Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

@v-esteves last bit before the merge I believe :)

docs/deploy_windowsdns.md Show resolved Hide resolved
@ytsarev
Copy link
Member

ytsarev commented Aug 10, 2023

Signed-off-by: Yury Tsarev <yury@upbound.io>
vestevesaws@gmail.com and others added 2 commits September 10, 2023 13:57
Signed-off-by: vestevesaws@gmail.com <x191116@MBP-de-Vitor.home>
Signed-off-by: Vitor Esteves <64093608+v-esteves@users.noreply.github.com>
@v-esteves
Copy link
Contributor Author

@ytsarev I've added the link in main Readme.md has requested.

Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

unfortunately all terratest pipelines are still failing :( we need to investigate

@v-esteves
Copy link
Contributor Author

@ytsarev can I help with something regarding those falling tests? Let me know what can I do from our end.

@ytsarev
Copy link
Member

ytsarev commented Oct 6, 2023

@v-esteves it looks like terratest is failing for the reason, we need to debug the test pipeline. If you can try to validate the local terratest run it will be great.

Signed-off-by: Vitor Esteves <64093608+v-esteves@users.noreply.github.com>
@ytsarev
Copy link
Member

ytsarev commented Dec 14, 2023

image https://github.com/k8gb-io/k8gb/actions/runs/7209689707?pr=1064 I believe it is actual valid test failure we are facing here.
panic: test timed out after 15m0s

goroutine 2511 [running]:
testing.(*M).startAlarm.func1()
/opt/hostedtoolcache/go/1.19.1/x64/src/testing/testing.go:2036 +0x8e
created by time.goFunc
/opt/hostedtoolcache/go/1.19.1/x64/src/time/sleep.go:176 +0x32

Something is blocking the test run

…cted. This will ensure that current running workloads using TSIG authentication for BIND will not break due to change on the helm values list for provider 2136. Added validations to _helpers template, in order to ensure that provider2136 options are not added to the external DNS deployment configuration, when provider2136 isn't enabled.

Signed-off-by: Vitor Esteves <x191116@MacBook-Pro-de-Vitor.local>
@v-esteves
Copy link
Contributor Author

@ytsarev like we have discussed before, the problem was regarding the new values that should be added to the helm statement, in order to deploy correctly External DNS.
To bypass this issue and ensure that there aren't breaking changes, I've added the default value to true for TSIG authentication when provider2136 is active. This will allow that running workloads don't have any issue when upgrading the version. For new installations, the desired configuration should be added through correct values definition.

@ytsarev
Copy link
Member

ytsarev commented Dec 18, 2023

@v-esteves great solution with the backward compatibility 👍 Thank you so much!
Please consider creating the issue to track the old values support removal after a couple of project releases

@ytsarev ytsarev merged commit 77fd09b into k8gb-io:master Dec 18, 2023
12 checks passed
ytsarev added a commit to ytsarev/k8gb that referenced this pull request Dec 18, 2023
* Release `v0.12.0` with
  Cloudflare(k8gb-io#1278) and GSS-TSIG
  support(k8gb-io#1064)
* Inline maintainer list update to be aligned with https://github.com/k8gb-io/k8gb/blob/master/CODEOWNERS

Signed-off-by: Yury Tsarev <yury@upbound.io>
@ytsarev ytsarev mentioned this pull request Dec 18, 2023
ytsarev added a commit to ytsarev/k8gb that referenced this pull request Dec 19, 2023
* Release `v0.12.0` with
  Cloudflare(k8gb-io#1278) and GSS-TSIG
  support(k8gb-io#1064)
* Inline maintainer list update to be aligned with https://github.com/k8gb-io/k8gb/blob/master/CODEOWNERS

Signed-off-by: Yury Tsarev <yury@upbound.io>
ytsarev added a commit that referenced this pull request Dec 19, 2023
* Release `v0.12.0` with
  Cloudflare(#1278) and GSS-TSIG
  support(#1064)
* Inline maintainer list update to be aligned with https://github.com/k8gb-io/k8gb/blob/master/CODEOWNERS

Signed-off-by: Yury Tsarev <yury@upbound.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants