Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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 @@ -9,6 +9,7 @@ import (
"fmt"
"io"
"math/big"
"net"
"net/http"
"os"
"sort"
Expand Down Expand Up @@ -761,7 +762,9 @@ func (r *HostedControlPlaneReconciler) reconcileInfrastructureStatusCondition(ct
Reason: hyperv1.AsExpectedReason,
}
if util.HCPOAuthEnabled(hostedControlPlane) {
hostedControlPlane.Status.OAuthCallbackURLTemplate = fmt.Sprintf("https://%s:%d/oauth2callback/[identity-provider-name]", infraStatus.OAuthHost, infraStatus.OAuthPort)
// JoinHostPort brackets IPv6 literals so url.Parse accepts the callback template.

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.

nit: JoinHostPort already conveys the intent — the comment could be dropped per project conventions (no comments on self-explanatory code).

hostedControlPlane.Status.OAuthCallbackURLTemplate = fmt.Sprintf("https://%s/oauth2callback/[identity-provider-name]",
net.JoinHostPort(infraStatus.OAuthHost, strconv.Itoa(int(infraStatus.OAuthPort))))
}
} else {
message := "Cluster infrastructure is still provisioning"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ package kas
import (
"encoding/json"
"fmt"
"net"
"net/url"
"strconv"

"github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/manifests"
"github.com/openshift/hypershift/support/certs"
Expand Down Expand Up @@ -39,7 +42,11 @@ func adaptOauthMetadata(cpContext component.WorkloadContext, cfg *corev1.ConfigM
return fmt.Errorf("failed to unmarshal oauth metadata: %w", err)
}

oauthURL := fmt.Sprintf("https://%s:%d", cpContext.InfraStatus.OAuthHost, cpContext.InfraStatus.OAuthPort)
// Use net.JoinHostPort so IPv6 NodePort addresses are bracketed (RFC 3986).
oauthURL := (&url.URL{
Scheme: "https",
Host: net.JoinHostPort(cpContext.InfraStatus.OAuthHost, strconv.Itoa(int(cpContext.InfraStatus.OAuthPort))),
}).String()
oauthMetadata["issuer"] = oauthURL
oauthMetadata["authorization_endpoint"] = fmt.Sprintf("%s/oauth/authorize", oauthURL)
oauthMetadata["token_endpoint"] = fmt.Sprintf("%s/oauth/token", oauthURL)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
package kas

import (
"encoding/json"
"testing"

. "github.com/onsi/gomega"

hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
"github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/infra"
controlplanecomponent "github.com/openshift/hypershift/support/controlplane-component"

corev1 "k8s.io/api/core/v1"
Expand All @@ -16,10 +18,15 @@ import (
func TestAdaptOauthMetadata(t *testing.T) {
t.Parallel()
tests := []struct {
name string
cfg *corev1.ConfigMap
wantErr bool
errSubstr string
name string
cfg *corev1.ConfigMap
oauthHost string
oauthPort int32
wantErr bool
errSubstr string
wantIssuer string
wantAuthz string
wantToken string
}{
{
name: "When ConfigMap contains invalid JSON, it should return an unmarshal error",
Expand All @@ -31,6 +38,45 @@ func TestAdaptOauthMetadata(t *testing.T) {
wantErr: true,
errSubstr: "failed to unmarshal oauth metadata",
},
{

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.

nit: the project convention for test names is "When ..., it should ...". For example:

  • "When OAuth host is IPv6, it should bracket the address"
  • "When OAuth host is IPv4, it should leave issuer URLs unbracketed"

name: "When OAuth host is IPv4, issuer URLs should not use brackets",
cfg: &corev1.ConfigMap{
Data: map[string]string{
OauthMetadataConfigKey: `{}`,
},
},
oauthHost: "192.0.2.10",
oauthPort: 32047,
wantIssuer: "https://192.0.2.10:32047",
wantAuthz: "https://192.0.2.10:32047/oauth/authorize",
wantToken: "https://192.0.2.10:32047/oauth/token",
},
{
name: "When OAuth host is IPv6, issuer URLs should bracket the address",
cfg: &corev1.ConfigMap{
Data: map[string]string{
OauthMetadataConfigKey: `{}`,
},
},
oauthHost: "fd2e:6f44:5dd8:c956::14",
oauthPort: 32047,
wantIssuer: "https://[fd2e:6f44:5dd8:c956::14]:32047",
wantAuthz: "https://[fd2e:6f44:5dd8:c956::14]:32047/oauth/authorize",
wantToken: "https://[fd2e:6f44:5dd8:c956::14]:32047/oauth/token",
},
{
name: "When OAuth host is a hostname, issuer URLs should remain unbracketed",
cfg: &corev1.ConfigMap{
Data: map[string]string{
OauthMetadataConfigKey: `{}`,
},
},
oauthHost: "oauth.example.com",
oauthPort: 443,
wantIssuer: "https://oauth.example.com:443",
wantAuthz: "https://oauth.example.com:443/oauth/authorize",
wantToken: "https://oauth.example.com:443/oauth/token",
},
}

for _, tt := range tests {
Expand All @@ -41,15 +87,25 @@ func TestAdaptOauthMetadata(t *testing.T) {
Context: t.Context(),
HCP: &hyperv1.HostedControlPlane{},
Client: fake.NewClientBuilder().Build(),
InfraStatus: infra.InfrastructureStatus{
OAuthHost: tt.oauthHost,
OAuthPort: tt.oauthPort,
},
}

err := adaptOauthMetadata(cpContext, tt.cfg)
if tt.wantErr {
g.Expect(err).To(HaveOccurred())
g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr))
} else {
g.Expect(err).ToNot(HaveOccurred())
return
}

g.Expect(err).ToNot(HaveOccurred())
var oauthMetadata map[string]interface{}
g.Expect(json.Unmarshal([]byte(tt.cfg.Data[OauthMetadataConfigKey]), &oauthMetadata)).To(Succeed())
g.Expect(oauthMetadata["issuer"]).To(Equal(tt.wantIssuer))
g.Expect(oauthMetadata["authorization_endpoint"]).To(Equal(tt.wantAuthz))
g.Expect(oauthMetadata["token_endpoint"]).To(Equal(tt.wantToken))
})
}
}