Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ spec:
args:
- --service-account-namespace=openshift-cloud-network-config-controller
- --service-account-name=cloud-network-config-controller
- --token-audience=openshift
- --token-file=/var/run/secrets/openshift/serviceaccount/token
- --kubeconfig=/etc/kubernetes/kubeconfig
resources:
Expand Down Expand Up @@ -189,6 +190,10 @@ spec:
{{- if not (eq .AzureManagedSecretProviderClass "")}}
- name: "ARO_HCP_CLIENT_CREDENTIALS_PATH"
value: "{{ .AzureManagedCredsPath}}"
{{ end }}
{{- if not (eq .GCPCredentialsPath "")}}
- name: "GOOGLE_APPLICATION_CREDENTIALS"
value: "{{ .GCPCredentialsPath}}"
{{ end }}
resources:
requests:
Expand Down
9 changes: 9 additions & 0 deletions pkg/network/cloud_network.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,15 @@ func renderCloudNetworkConfigController(conf *operv1.NetworkSpec, bootstrapResul
data.Data["AzureManagedCertDirectory"] = azureCertPath
data.Data["AzureManagedCredsPath"] = filepath.Join(azureCertPath, os.Getenv("MANAGED_AZURE_HCP_CREDENTIALS_FILE_PATH"))
data.Data["AzureManagedSecretProviderClass"] = os.Getenv("ARO_HCP_SECRET_PROVIDER_CLASS")
// GCP WIF credential path for HCP deployments.
gcpCredsFile := os.Getenv("GCP_CNCC_CREDENTIALS_FILE")
Comment thread
apahim marked this conversation as resolved.
if gcpCredsFile == "" {
data.Data["GCPCredentialsPath"] = ""
Comment thread
coderabbitai[bot] marked this conversation as resolved.
} else if filepath.Base(gcpCredsFile) != gcpCredsFile {
return nil, errors.Errorf("invalid GCP_CNCC_CREDENTIALS_FILE %q: must be a filename", gcpCredsFile)
} else {
data.Data["GCPCredentialsPath"] = filepath.Join("/etc/secret/cloudprovider", gcpCredsFile)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So... if the feature is configured, hypershift passes the literal string "application_default_credentials.json" to CNO, which then prepends the literal string "/etc/secret/cloudprovider/" and passes it to CNCC, which uses it as a filename.

This would make sense if hypershift knew the credentials filename but not the directory it was in, and CNO knew the directory but not the filename. But I'm guessing that's not the case.

Where do the creds come from? Who writes them there?

If HyperShift actually knows for sure what the credentials filename is (and nobody else does know for sure) then it should pass the whole path to CNO, which should pass it to CNCC.

If HyperShift doesn't know where the creds are, but only knows whether or not it wants CNCC to use them, then it should just pass a true/false value to CNO, and then likewise, CNO can either pass the full pathname or a true/false value to CNCC.

Also, it seems like it would be better to configure CNCC via a command-line argument...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hypershift creates the secret cloud-network-config-controller-creds containing the WIF credential JSON under the key application_default_credentials.json

Here specifically:

But that's becuase the secret name is hardcoded in CNO:

Then CNO mounts it in /etc/secret/cloudprovider:

Then we have to create the env var for CNO with the filename:

The filename-only approach follows the existing Azure pattern at line 110, where MANAGED_AZURE_HCP_CREDENTIALS_FILE_PATH similarly passes a filename that CNO combines with a mount path (/var/run/secrets/azure).

Each component owns its part of the path: HyperShift controls the secret key name, CNO controls the mount point.

The env var also doubles as a feature flag - empty means GCP WIF is not configured.

Passing the full path would couple HyperShift to CNO's mount point; passing a boolean would require hardcoding the filename in CNO, coupling it to HyperShift's secret key choice. The current split keeps both sides independent.

Regarding configuring CNCC: the env var set on the CNCC container is GOOGLE_APPLICATION_CREDENTIALS, which is a standard GCP SDK convention. The client libraries read it automatically to locate credentials... It's not an arbitrary env var, so a CLI argument would add an extra layer that just sets this env var anyway.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

huh... ok

caOverride.ObjectMeta = metav1.ObjectMeta{
Namespace: hcpCfg.Namespace,
Name: "cloud-network-config-controller-kube-cloud-config",
Expand Down
173 changes: 173 additions & 0 deletions pkg/network/cloud_network_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
package network

import (
"testing"

"github.com/openshift/cluster-network-operator/pkg/render"
uns "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
)

func makeManagedControllerRenderData() render.RenderData {
data := render.MakeRenderData()
data.Data["ReleaseVersion"] = "4.18.0"
data.Data["PlatformType"] = "GCP"
data.Data["PlatformRegion"] = "us-central1"
data.Data["PlatformTypeAWS"] = "AWS"
data.Data["PlatformTypeAzure"] = "Azure"
data.Data["PlatformTypeGCP"] = "GCP"
data.Data["CloudNetworkConfigControllerImage"] = "test-image"
data.Data["KubernetesServiceURL"] = "https://localhost:6443"
data.Data["ExternalControlPlane"] = true
data.Data["PlatformAzureEnvironment"] = ""
data.Data["PlatformAWSCAPath"] = ""
data.Data["PlatformAPIURL"] = ""
data.Data["CLIImage"] = "cli-image"
data.Data["TokenMinterImage"] = "token-minter-image"
data.Data["TokenAudience"] = "https://issuer.example.com"
data.Data["ManagementClusterName"] = "test-cluster"
data.Data["HostedClusterNamespace"] = "test-ns"
data.Data["ReleaseImage"] = "release-image"
data.Data["HCPNodeSelector"] = map[string]string{}
data.Data["HCPLabels"] = map[string]string{}
data.Data["HCPTolerations"] = []string{}
data.Data["RunAsUser"] = ""
data.Data["PriorityClass"] = ""
data.Data["HTTP_PROXY"] = ""
data.Data["HTTPS_PROXY"] = ""
data.Data["NO_PROXY"] = ""
data.Data["AzureManagedCertDirectory"] = ""
data.Data["AzureManagedCredsPath"] = ""
data.Data["AzureManagedSecretProviderClass"] = ""
data.Data["GCPCredentialsPath"] = ""
return data
}

// getEnvVar looks up an env var by name from a container map and returns its value.
func getEnvVar(t *testing.T, container map[string]interface{}, name string) (string, bool) {
t.Helper()
envSlice, found, err := uns.NestedSlice(container, "env")
if err != nil || !found {
return "", false
}
for _, e := range envSlice {
em := e.(map[string]interface{})
n, _, _ := uns.NestedString(em, "name")
if n == name {
v, _, _ := uns.NestedString(em, "value")
return v, true
}
}
return "", false
}

// findUnstructuredContainer finds a container by name from a deployment's unstructured object.
func findUnstructuredContainer(t *testing.T, obj map[string]interface{}, containerName string) (map[string]interface{}, bool) {
t.Helper()
containers, found, err := uns.NestedSlice(obj, "spec", "template", "spec", "containers")
if err != nil || !found {
return nil, false
}
for _, c := range containers {
cm := c.(map[string]interface{})
name, _, _ := uns.NestedString(cm, "name")
if name == containerName {
return cm, true
}
}
return nil, false
}

// TestGCPCredentialsPathTemplateRendering tests that the managed controller.yaml template
// correctly renders GOOGLE_APPLICATION_CREDENTIALS when GCPCredentialsPath is set.
func TestGCPCredentialsPathTemplateRendering(t *testing.T) {
tests := []struct {
name string
gcpCredentialsPath string
expectGoogleAppCredentials bool
expectedValue string
}{
{
name: "GCPCredentialsPath set renders GOOGLE_APPLICATION_CREDENTIALS",
gcpCredentialsPath: "/etc/secret/cloudprovider/application_default_credentials.json",
expectGoogleAppCredentials: true,
expectedValue: "/etc/secret/cloudprovider/application_default_credentials.json",
},
{
name: "GCPCredentialsPath empty omits GOOGLE_APPLICATION_CREDENTIALS",
gcpCredentialsPath: "",
expectGoogleAppCredentials: false,
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
data := makeManagedControllerRenderData()
data.Data["GCPCredentialsPath"] = tc.gcpCredentialsPath

objs, err := render.RenderDir("../../bindata/cloud-network-config-controller/managed", &data)
if err != nil {
t.Fatalf("failed to render managed controller: %v", err)
}

for _, obj := range objs {
if obj.GetKind() != "Deployment" {
continue
}

container, found := findUnstructuredContainer(t, obj.Object, "controller")
if !found {
t.Fatal("controller container not found in Deployment")
}

val, found := getEnvVar(t, container, "GOOGLE_APPLICATION_CREDENTIALS")
if tc.expectGoogleAppCredentials && !found {
t.Errorf("expected GOOGLE_APPLICATION_CREDENTIALS in deployment, but not found")
}
if !tc.expectGoogleAppCredentials && found {
t.Errorf("expected GOOGLE_APPLICATION_CREDENTIALS to be absent, but found")
}
if tc.expectGoogleAppCredentials && val != tc.expectedValue {
t.Errorf("expected GOOGLE_APPLICATION_CREDENTIALS value %q, got %q", tc.expectedValue, val)
}
return
}
t.Fatal("Deployment object not found in rendered output")
})
}
}

// TestCloudTokenMinterHasTokenAudience verifies that the cloud-token minter container
// has --token-audience=openshift in its args.
func TestCloudTokenMinterHasTokenAudience(t *testing.T) {
data := makeManagedControllerRenderData()

objs, err := render.RenderDir("../../bindata/cloud-network-config-controller/managed", &data)
if err != nil {
t.Fatalf("failed to render managed controller: %v", err)
}

for _, obj := range objs {
if obj.GetKind() != "Deployment" {
continue
}

container, found := findUnstructuredContainer(t, obj.Object, "cloud-token")
if !found {
t.Fatal("cloud-token container not found in Deployment")
}

args, found, err := uns.NestedStringSlice(container, "args")
if err != nil || !found {
t.Fatal("args not found in cloud-token-minter container")
}

for _, arg := range args {
if arg == "--token-audience=openshift" {
return
}
}
t.Error("expected cloud-token minter to have --token-audience=openshift arg")
return
}
t.Fatal("Deployment object not found in rendered output")
}