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

mutual-auth: Support spire k8s service dns resolution #26031

Merged
merged 3 commits into from Jun 15, 2023

Conversation

sayboras
Copy link
Member

@sayboras sayboras commented Jun 8, 2023

Description

Please refer to individual commits for more details.

Testing

Testing with the temp commit, the conformance-e2e is running successfully.

https://github.com/cilium/cilium/actions/runs/5220420030/jobs/9423492566?pr=26031

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 8, 2023
@sayboras sayboras added release-blocker/1.14 This issue will prevent the release of the next version of Cilium. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/servicemesh GH issues or PRs regarding servicemesh labels Jun 8, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jun 8, 2023
@sayboras
Copy link
Member Author

sayboras commented Jun 8, 2023

/test

@sayboras sayboras added the dont-merge/preview-only Only for preview or testing, don't merge it. label Jun 8, 2023
@sayboras sayboras force-pushed the tam/mutual-auth-clean-up branch 4 times, most recently from 7e85201 to 363050c Compare June 9, 2023 10:05
@sayboras sayboras changed the title mutual-auth: Support spire service dns resolution mutual-auth: Support spire k8s service dns resolution Jun 9, 2023
@sayboras sayboras removed the dont-merge/preview-only Only for preview or testing, don't merge it. label Jun 9, 2023
@sayboras
Copy link
Member Author

sayboras commented Jun 9, 2023

/test

@sayboras sayboras marked this pull request as ready for review June 9, 2023 10:13
@sayboras sayboras requested review from a team as code owners June 9, 2023 10:13
@squeed
Copy link
Contributor

squeed commented Jun 9, 2023

If the operator needs to run on the same node as the SPIRE agent, then that should be expressed with PodAffinity. Or, we should include the spire container in the operator pod.

@squeed
Copy link
Contributor

squeed commented Jun 9, 2023

One point of confusion -- the PR seems to suggest that we need ClusterFirstWithHostNet if we wish to reach SPIRE via the service hostname. That also seems to be the default. So, do we need to set ClusterFirstWithHostNet automatically?

operator/auth/spire/client.go Outdated Show resolved Hide resolved
@sayboras
Copy link
Member Author

If the operator needs to run on the same node as the SPIRE agent, then that should be expressed with PodAffinity. Or, we should include the spire container in the operator pod.

This was one of the approaches that we considered, however, cilium operator is running in HA mode with the leader election, so it's hard to control the node for both cilium operator and spire server.

@sayboras
Copy link
Member Author

One point of confusion -- the PR seems to suggest that we need ClusterFirstWithHostNet if we wish to reach SPIRE via the service hostname. That also seems to be the default. So, do we need to set ClusterFirstWithHostNet automatically?

Sorry for the confusion, ideally, we would love to set ClusterFirstWithHostNet by default, however, such a configuration was not working in EKS due to #24774. Hence, the goal of this PR is to avoid setting dnsPolicy as ClusterFirstWithHostNet by default if mutual auth is enabled. Users can still have the option to set dnsPolicy in their deployment.

Let me update the commit message to make it clearer.

@sayboras sayboras force-pushed the tam/mutual-auth-clean-up branch 2 times, most recently from 03b2159 to 84e0c4a Compare June 13, 2023 13:19
@sayboras sayboras requested a review from a team as a code owner June 13, 2023 13:19
operator/auth/spire/client.go Outdated Show resolved Hide resolved
Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

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

LGTM for docs

@joestringer joestringer added the kind/enhancement This would improve or streamline existing functionality. label Jun 14, 2023
@sayboras
Copy link
Member Author

Force push to address the log comment

Diff
diff --git a/operator/auth/spire/client.go b/operator/auth/spire/client.go
index 6af073b4c6..6d59e299e3 100644
--- a/operator/auth/spire/client.go
+++ b/operator/auth/spire/client.go
@@ -173,18 +173,18 @@ func (c *Client) connect(ctx context.Context) (*grpc.ClientConn, error) {
 
 	tlsConfig := tlsconfig.MTLSClientConfig(source, source, tlsconfig.AuthorizeMemberOf(trustedDomain))
 
-	c.log.WithFields(map[string]interface{}{
-		logfields.URL:    c.cfg.SpireServerAddress,
-		logfields.IPAddr: resolvedTarget,
+	c.log.WithFields(logrus.Fields{
+		logfields.Address: c.cfg.SpireServerAddress,
+		logfields.IPAddr:  resolvedTarget,
 	}).Info("Trying to connect to SPIRE server")
 	conn, err := grpc.Dial(*resolvedTarget, grpc.WithTransportCredentials(credentials.NewTLS(tlsConfig)))
 	if err != nil {
 		return nil, fmt.Errorf("failed to create connection to SPIRE server: %w", err)
 	}
 
-	c.log.WithFields(map[string]interface{}{
-		logfields.URL:    c.cfg.SpireServerAddress,
-		logfields.IPAddr: resolvedTarget,
+	c.log.WithFields(logrus.Fields{
+		logfields.Address: c.cfg.SpireServerAddress,
+		logfields.IPAddr:  resolvedTarget,
 	}).Info("Connected to SPIRE server")
 	return conn, nil
 }
diff --git a/pkg/logging/logfields/logfields.go b/pkg/logging/logfields/logfields.go
index c4796bf951..3dcd56051d 100644
--- a/pkg/logging/logfields/logfields.go
+++ b/pkg/logging/logfields/logfields.go
@@ -121,6 +121,9 @@ const (
 	// NextHop is an IPV4 or IPv6 address for the next hop
 	NextHop = "nextHop"
 
+	// Address is an IPV4, IPv6 or FQDN address
+	Address = "address"
+
 	// IPAddr is an IPV4 or IPv6 address
 	IPAddr = "ipAddr"
 

@sayboras sayboras requested a review from a team as a code owner June 14, 2023 15:22
@sayboras
Copy link
Member Author

/test

@sayboras
Copy link
Member Author

/test

The description must be connection timeout instead of endpoint.

Signed-off-by: Tam Mach <tam.mach@cilium.io>
DNS Policy as ClusterFirstWithHostNet is not strictly required if spire
components are installed with Cilium helm chart. For external spire
installation, user can choose to set server address accordingly:

- If a routable IP address is used, no further setting is required.
- If a k8s Service FQDN is used, user can set dnsPolicy via existing
  helm attribute (e.g. operator.dnsPolicy)

Fixes: #25860

Signed-off-by: Tam Mach <tam.mach@cilium.io>
This is to make sure that we have the coverage for different data path
modes (e.g. kernel version, ipsec, wireguard, node encryption, etc).

Signed-off-by: Tam Mach <tam.mach@cilium.io>
@sayboras
Copy link
Member Author

Force pushed to rebase with main, and drop the last commit as cilium-cli version got upgraded in main branch now.

@sayboras
Copy link
Member Author

/test

@sayboras
Copy link
Member Author

Reviews from all required teams are in, all tests passed, merging.

@sayboras sayboras added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 15, 2023
@sayboras sayboras merged commit 0c9ccd0 into main Jun 15, 2023
61 checks passed
@sayboras sayboras deleted the tam/mutual-auth-clean-up branch June 15, 2023 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/servicemesh GH issues or PRs regarding servicemesh kind/enhancement This would improve or streamline existing functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-blocker/1.14 This issue will prevent the release of the next version of Cilium. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants