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.

Sign up for GitHub

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

Jump to bottom

OCPBUGSM-30853: Change default telemeter server of installed clusters to prod. #1988

Merged
merged 1 commit into from
Jun 14, 2021
Merged

OCPBUGSM-30853: Change default telemeter server of installed clusters to prod. #1988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 11 additions & 10 deletions internal/network/manifests_generator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,10 +295,10 @@ data:
telemeterServerURL: {{.TELEMETER_SERVER_URL}}
`

prodServiceBaseURL = "https://api.openshift.com"
stageServiceBaseURL = "https://api.stage.openshift.com"
stageTelemeterURL = "https://infogw.api.stage.openshift.com"
dummyURL = "https://dummy.com"
stageServiceBaseURL = "https://api.stage.openshift.com"
integrationServiceBaseURL = "https://api.integration.openshift.com"
stageTelemeterURL = "https://infogw.api.stage.openshift.com"
dummyURL = "https://dummy.com"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not blocking, but how about using stuff from .invalid TLD instead of domain that is registered to someone somewhere? It should not matter much until we decide to send there (accidentally or not) some data that we should not

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 don't see any reason why not to use https://dummy.invalid.
Anyway, I want to keep this PR minimal as possible - only the bug fix.
What you see is only the indention change but this wasn't inserted in this PR.
@mkowalski

Thanks for your review BTW, I have learned something new :)

Copy link
Member

Choose a reason for hiding this comment

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

@mkowalski @ybettan can one of you open an issue to track this? Or a PR to fix it?
I think this is a great point.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have opened #1993 for that

Copy link
Member

Choose a reason for hiding this comment

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

👍 thanks

)

// Default Telemeter server is prod.
Expand All @@ -308,20 +308,21 @@ func (m *ManifestsGenerator) AddTelemeterManifest(ctx context.Context, log logru

manifestParams := map[string]string{}

if m.Config.ServiceBaseURL == prodServiceBaseURL {
return nil
}
switch m.Config.ServiceBaseURL {

if m.Config.ServiceBaseURL == stageServiceBaseURL {
case stageServiceBaseURL:

log.Infof("Creating manifest to redirect metrics from installed cluster to telemeter-stage")
manifestParams["TELEMETER_SERVER_URL"] = stageTelemeterURL

} else {
case integrationServiceBaseURL:

log.Infof("Creating manifest to redirect metrics from installed cluster to a dummy URL")
log.Infof("Creating manifest to redirect metrics from installed cluster to dummy URL")
manifestParams["TELEMETER_SERVER_URL"] = dummyURL

default:
return nil

}

content, err := fillTemplate(manifestParams, redirectTelemeterStageManifest, log)
Expand Down
15 changes: 7 additions & 8 deletions internal/network/manifests_generator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,17 +345,16 @@ var _ = Describe("telemeter manifest", func() {
envName string
serviceBaseURL string
}{
{
envName: "Prod env",
serviceBaseURL: prodServiceBaseURL,
},
{
envName: "Stage env",
serviceBaseURL: stageServiceBaseURL,
},
{
envName: "Other envs",
serviceBaseURL: dummyURL,
envName: "Integration env",
serviceBaseURL: integrationServiceBaseURL,
},
{
envName: "Other envs",
},
} {
test := test
Expand All @@ -366,15 +365,15 @@ var _ = Describe("telemeter manifest", func() {
})

It("happy flow", func() {
if test.envName != "Prod env" {
if test.envName == "Stage env" || test.envName == "Integration env" {
mockManifestsApi.EXPECT().CreateClusterManifest(ctx, gomock.Any()).Return(operations.NewCreateClusterManifestCreated())
}
err := manifestsGeneratorApi.AddTelemeterManifest(ctx, log, &cluster)
Expect(err).ShouldNot(HaveOccurred())
})

It("AddTelemeterManifest failure", func() {
if test.envName == "Prod env" {
if test.envName != "Stage env" && test.envName != "Integration env" {
Skip("We don't create any additional manifest in prod")
}
mockManifestsApi.EXPECT().CreateClusterManifest(ctx, gomock.Any()).Return(common.GenerateErrorResponder(errors.Errorf("failed to upload to s3")))
Expand Down
2 changes: 1 addition & 1 deletion subsystem/manifests_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ spec:
It("check installation telemeter manifests", func() {

isProdDeployment := func() bool {
return Options.InventoryHost == "api.openshift.com"
return Options.InventoryHost != "api.stage.openshift.com" && Options.InventoryHost != "api.integration.openshift.com"
}

if isProdDeployment() {
Expand Down