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

Coredns integration with K8s-charm #35

Open
wants to merge 39 commits into
base: main
Choose a base branch
from

Conversation

louiseschmidtgen
Copy link
Contributor

@louiseschmidtgen louiseschmidtgen commented Feb 27, 2024

Description

This PR adds the coredns integration to the k8s charm.

Handle Integration

  • Checks for a new dns-provider relation between the k8s-charm and coredns.
  • Disables built in dns if this relation exists
  • Gets dns bits and configures coredns

Integration Tests

  • add a coredns-model fixture, this juju model gets deployed on a new k8s cloud.
  • The two tests checks for the cross-model-relation and the dns functionality.

limitation

The test_dns function can only be validated once cluster config bits are propagated properly.

Copy link
Contributor

@addyess addyess left a comment

Choose a reason for hiding this comment

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

Aight, here are some suggestions for the charm code. Looking at integration tests next

charms/worker/k8s/src/charm.py Outdated Show resolved Hide resolved
charms/worker/k8s/src/charm.py Outdated Show resolved Hide resolved
charms/worker/k8s/src/charm.py Outdated Show resolved Hide resolved
charms/worker/k8s/src/charm.py Outdated Show resolved Hide resolved
charms/worker/k8s/src/charm.py Outdated Show resolved Hide resolved
charms/worker/k8s/src/charm.py Outdated Show resolved Hide resolved
charms/worker/k8s/src/charm.py Outdated Show resolved Hide resolved
charms/worker/k8s/src/charm.py Outdated Show resolved Hide resolved
Copy link
Contributor

@addyess addyess left a comment

Choose a reason for hiding this comment

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

I've left some changes that will shrink this down a good bit. It's key to go learn about how pytest fixtures work. Not all my suggestions will be spot on -- but maybe they get close. Particularly on the model clean up

tests/integration/conftest.py Outdated Show resolved Hide resolved
tests/integration/conftest.py Outdated Show resolved Hide resolved
tests/integration/conftest.py Outdated Show resolved Hide resolved
tests/integration/conftest.py Outdated Show resolved Hide resolved
tests/integration/conftest.py Outdated Show resolved Hide resolved
tests/integration/conftest.py Outdated Show resolved Hide resolved
tests/integration/conftest.py Outdated Show resolved Hide resolved
tests/integration/conftest.py Outdated Show resolved Hide resolved
tests/integration/test_k8s.py Outdated Show resolved Hide resolved
tests/integration/test_k8s.py Outdated Show resolved Hide resolved
@louiseschmidtgen louiseschmidtgen marked this pull request as ready for review March 12, 2024 15:27
@louiseschmidtgen louiseschmidtgen requested a review from a team as a code owner March 12, 2024 15:27
Copy link
Contributor

@addyess addyess left a comment

Choose a reason for hiding this comment

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

Looking pretty smooth. I had one little nit about using a command rather than a charm action

Comment on lines 235 to 240
action = await k8s.run("k8s config")
result = await action.wait()
assert result.results["return-code"] == 0, "Failed to get kubeconfig with kubectl"

kubeconfig_path = ops_test.tmp_path / "kubeconfig"
kubeconfig_path.write_text(result.results["stdout"])
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use the action on the charm now? Please check my logic -- this might yield a json kubeconfig file too...

Suggested change
action = await k8s.run("k8s config")
result = await action.wait()
assert result.results["return-code"] == 0, "Failed to get kubeconfig with kubectl"
kubeconfig_path = ops_test.tmp_path / "kubeconfig"
kubeconfig_path.write_text(result.results["stdout"])
action = await k8s.run_action("get-kubeconfig")
result = await action.wait()
assert result.results["return-code"] == 0, "Failed to get kubeconfig with kubectl"
kubeconfig_path = ops_test.tmp_path / "kubeconfig"
kubeconfig_path.write_text(result.results["kubeconfig"])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't seem to work just like that. I can take another look at this in the follow-up ticket. I'd like to get what we have merged for now.

tests/integration/cos_substrate.py Outdated Show resolved Hide resolved
Copy link
Member

@mateoflorido mateoflorido left a comment

Choose a reason for hiding this comment

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

Some inquires

charms/worker/k8s/lib/charms/k8s/v0/k8sd_api_manager.py Outdated Show resolved Hide resolved
Comment on lines 247 to 251
action = await k8s.run("k8s config")
result = await action.wait()
assert result.results["return-code"] == 0, "Failed to get kubeconfig with kubectl"
kubeconfig_path = ops_test.tmp_path / "kubeconfig"
kubeconfig_path.write_text(result.results["stdout"])
Copy link
Contributor

Choose a reason for hiding this comment

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

so, i wish this used the juju action instead of running through the snap CLI in order to make use of the public address of the unit

Suggested change
action = await k8s.run("k8s config")
result = await action.wait()
assert result.results["return-code"] == 0, "Failed to get kubeconfig with kubectl"
kubeconfig_path = ops_test.tmp_path / "kubeconfig"
kubeconfig_path.write_text(result.results["stdout"])
action = await k8s.run_action("get-kubeconfig")
result = await action.wait()
kubeconfig_path = ops_test.tmp_path / "kubeconfig"
kubeconfig_path.write_text(result.results["kubeconfig"])

Copy link
Contributor

@addyess addyess left a comment

Choose a reason for hiding this comment

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

only request is that we use the charm action

tests/integration/test_k8s.py Dismissed Show dismissed Hide dismissed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants